Bind secret files to random temp paths#534
Merged
Merged
Conversation
6 tasks
Kevin-CB
approved these changes
Jul 9, 2026
Kevin-CB
left a comment
Kevin-CB
left a comment
Contributor
There was a problem hiding this comment.
Yeah, using random temp is a better approach, there is no reason to involve user input in the ondisk filename at all.. it closes the vector more cleanly that sanitizing.
I confiormed locally that was working properly.
Closing #533 in favor of this one.
Thanks Jesse!
jtnord
reviewed
Jul 9, 2026
| if (workspace != null) { | ||
| final UnbindableDir secrets = UnbindableDir.create(workspace); | ||
| final FilePath secret = secrets.getDirPath().child("keystore-" + keystoreVariable); | ||
| final FilePath secret = secrets.getDirPath().createTempFile("file", null); |
Member
There was a problem hiding this comment.
to aid debugging, use the type there (its a Certificate so will be a keystore, but could also be called cert)
Suggested change
| final FilePath secret = secrets.getDirPath().createTempFile("file", null); | |
| final FilePath secret = secrets.getDirPath().createTempFile("keystore", null); |
Member
Author
There was a problem hiding this comment.
I thought about it but settled on just a generic filename in all cases. As noted in #533, the actual filename is concealed from the logs anyway, so there is no real advantage to picking a particular prefix.
jtnord
reviewed
Jul 9, 2026
| if (workspace != null) { | ||
| final UnbindableDir secrets = UnbindableDir.create(workspace); | ||
| final FilePath secret = secrets.getDirPath().child("keystore-" + keystoreVariable); | ||
| final FilePath secret = secrets.getDirPath().createTempFile("file", null); |
Member
There was a problem hiding this comment.
alternative to above
Suggested change
| final FilePath secret = secrets.getDirPath().createTempFile("file", null); | |
| final FilePath secret = secrets.getDirPath().createTempFile("cert", null); |
jtnord
reviewed
Jul 9, 2026
| SSHUserPrivateKey sshKey = getCredentials(build); | ||
| UnbindableDir keyDir = UnbindableDir.create(workspace); | ||
| FilePath keyFile = keyDir.getDirPath().child("ssh-key-" + keyFileVariable); | ||
| FilePath keyFile = keyDir.getDirPath().createTempFile("file", null); |
Member
There was a problem hiding this comment.
Suggested change
| FilePath keyFile = keyDir.getDirPath().createTempFile("file", null); | |
| FilePath keyFile = keyDir.getDirPath().createTempFile("id", null); |
jtnord
approved these changes
Jul 9, 2026
| String baseName = new FilePath(new File(credentials.getFileName())).getName(); | ||
|
|
||
| FilePath secret = dir.child(baseName); | ||
| FilePath secret = dir.createTempDir("file", null); |
Member
There was a problem hiding this comment.
use an extension for zips?
Suggested change
| FilePath secret = dir.createTempDir("file", null); | |
| FilePath secret = dir.createTempDir("file", "zip"); |
Member
Author
There was a problem hiding this comment.
No because this is a dir not a ZIP file.
|
|
||
| # check it was where we would expect it to be | ||
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/keystore-MY_KEYSTORE$' | ||
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/file.\+[.]tmp$' |
Member
There was a problem hiding this comment.
depending on accepting previous suggestions:
Suggested change
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/file.\+[.]tmp$' | |
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/keystore.\+[.]tmp$' |
|
|
||
| # check it was where we would expect it to be | ||
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/keystore-MY_KEYSTORE$' | ||
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/file.\+[.]tmp$' |
Member
There was a problem hiding this comment.
or depending on accepting previous suggestions:
Suggested change
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/file.\+[.]tmp$' | |
| echo "$keystore_path" | grep -q '/workspace/p@tmp/secretFiles/[-0-9a-f]\{36\}/cert.\+[.]tmp$' |
jglick
enabled auto-merge
July 9, 2026 19:24
Member
|
User report of issue in #539 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #532. Alternative to #533.