Restrict file location for SSH and certificates credentials by Kevin-CB · Pull Request #533 · jenkinsci/credentials-binding-plugin

Kevin-CB · 2026-07-08T14:45:23Z

Testing done

Confirmed locally the file is not created outside the expected directory with the following pipeline

node {
  withCredentials([sshUserPrivateKey(credentialsId:'demo-ssh-key',
    keyFileVariable:'../../../../myFile')]) { }
}

did the same for certificate creds.

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests that demonstrate the feature works or the issue is fixed

daniel-beck

Fix looks correct, but the tests could be improved.

daniel-beck · 2026-07-08T15:10:00Z

+		FilePath secretsDir = UnbindableDir.create(workspace).getDirPath();
+		FilePath resolvedPath = secretsDir.child("keystore-" + payload);
+
+		assertFalse(resolvedPath.exists(), "Keystore file should not exist outside secure directory");


I find this lone assertion unconvincing. Didn't we do better in 3f6dece already with the #assertLogContains positive assertion? Same in e52b232 followup. Why not add something similar here?

daniel-beck · 2026-07-08T15:11:50Z

 		if (workspace != null) {
 			final UnbindableDir secrets = UnbindableDir.create(workspace);
-			final FilePath secret = secrets.getDirPath().child("keystore-" + keystoreVariable);
+			String safeKeystoreVariable = new FilePath(new File(keystoreVariable)).getName();


Looks familiar 👍 Also looks correctly adapted for the prefix unique to this case.

jtnord

the variable should not even be used for file names, create a unique file in the directory.

jtnord · 2026-07-08T15:14:19Z

+			String safeKeystoreVariable = new FilePath(new File(keystoreVariable)).getName();
+			final FilePath secret = secrets.getDirPath().child("keystore-" + safeKeystoreVariable);


keystore variable is supposed to be the name of an environment variable not the temporary file!?

this should be a temporary file and not take in user specified paths.

e.g.

withCredentials([certificate(credentialsId: 'id1', keystoreVariable: 'hello')]) { sh 'echo path is $hello and should exist' withCredentials([certificate(credentialsId: 'id2', keystoreVariable: 'hello')]) { sh 'echo path is $hello and should exist but be different to before' } sh 'echo path is $hello and should still exist (but the path for the previous id2 should be deleted' }

So rather than attempting to sanitize somthing, this should surely just create a unique file without any user supplied being used.
(additionally - this does not sanitize against max filename lengths!)

I tend to agree. But:

credentials-binding-plugin/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/FileBinding.java

Line 54 in f9d34fd

String baseName = new FilePath(new File(credentials.getFileName())).getName();

Also

credentials-binding-plugin/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java

Lines 98 to 99 in f9d34fd

this.secretValues = new LinkedHashMap<>(secretValues);

this.publicValues = new LinkedHashMap<>(publicValues);

should probably block anything that does not look like a valid variable name. (Too late for this attack.)

I think the original intent of using specific file names was to simplify diagnosis from build logs. But the bound file path is treated as a secret and masked anyway. So we might as well use arbitrary filenames (like random UUIDs).

jtnord · 2026-07-08T15:28:37Z

+			String safeKeystoreVariable = new FilePath(new File(keystoreVariable)).getName();
+			final FilePath secret = secrets.getDirPath().child("keystore-" + safeKeystoreVariable);


Suggested change

String safeKeystoreVariable = new FilePath(new File(keystoreVariable)).getName();

final FilePath secret = secrets.getDirPath().child("keystore-" + safeKeystoreVariable);

final FilePath secret = secrets.getDirPath().createTempFile("keystore-", ".ks");

createTempFile perhaps overkill given that each binding calls

credentials-binding-plugin/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/UnbindableDir.java

Lines 52 to 53 in f9d34fd

final String dirName = UUID.randomUUID().toString();

final FilePath dir = secrets.child(dirName);

so even a fixed filename would work, but sure

Kevin-CB · 2026-07-09T07:56:16Z

Closed in favor of #534

Restrict file creation for SSH and certificates credentials

9c6426a

Kevin-CB requested a review from a team as a code owner July 8, 2026 14:45

Kevin-CB requested a review from daniel-beck July 8, 2026 14:45

daniel-beck reviewed Jul 8, 2026

View reviewed changes

jtnord reviewed Jul 8, 2026

View reviewed changes

jglick mentioned this pull request Jul 8, 2026

Bind secret files to random temp paths #534

Merged

Kevin-CB closed this Jul 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Restrict file location for SSH and certificates credentials#533

Restrict file location for SSH and certificates credentials#533
Kevin-CB wants to merge 1 commit into
jenkinsci:masterfrom
Kevin-CB:restrict-file-creation

Kevin-CB commented Jul 8, 2026

Uh oh!

daniel-beck left a comment

Uh oh!

daniel-beck Jul 8, 2026

Uh oh!

daniel-beck Jul 8, 2026

Uh oh!

jtnord left a comment

Uh oh!

jtnord Jul 8, 2026 •

edited

Loading

Uh oh!

jglick Jul 8, 2026

Uh oh!

jglick Jul 8, 2026

Uh oh!

jglick Jul 8, 2026

Uh oh!

jtnord Jul 8, 2026

Uh oh!

jglick Jul 8, 2026

Uh oh!

Kevin-CB commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		String safeKeystoreVariable = new FilePath(new File(keystoreVariable)).getName();
		final FilePath secret = secrets.getDirPath().child("keystore-" + safeKeystoreVariable);

	this.secretValues = new LinkedHashMap<>(secretValues);
	this.publicValues = new LinkedHashMap<>(publicValues);

	final String dirName = UUID.randomUUID().toString();
	final FilePath dir = secrets.child(dirName);

Uh oh!

Uh oh!

Conversation

Kevin-CB commented Jul 8, 2026

Testing done

Submitter checklist

Uh oh!

daniel-beck left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-beck Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

daniel-beck Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

jtnord left a comment

Choose a reason for hiding this comment

Uh oh!

jtnord Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

jglick Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

jglick Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

jtnord Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

jglick Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Kevin-CB commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtnord Jul 8, 2026 •

edited

Loading