Fix redaction leak from ssh_secret within checkout#4088
Conversation
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any code issues to raise. I'm leaving this as a comment rather than an approval because the change touches secret redaction/private-key handling, so it needs human review.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 5305, then answer my questions about the findings.
There was a problem hiding this comment.
I didn't find any code issues to raise. I'm leaving this as a comment rather than an approval because the PR changes secret redaction/private-key handling, so it needs human review.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 5308, then answer my questions about the findings.
There was a problem hiding this comment.
These changes under clicommand (to meta-data set-batch) seem totally unrelated?
|
|
||
| var receivedBatch api.MetaDataBatch | ||
| server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { | ||
| client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) { |
There was a problem hiding this comment.
Please don't do this. Using httptest and having an actual network round-trip (even if it's the same host) makes the test more faithful than stubbing out the client.
|
I've taken a stab at an alternative in #4091 |
2 participants
Description
When a checkout uses
ssh_secret, the fetched key is set asBUILDKITE_GIT_SSH_KEYin the job environment. That var is also an executor config var, so after any hook runs the executor logs it as a changed config value. That log line formats values with%q, which escapes the key's newlines into literal backslash n sequences.Context
Linear
Reported via support. A customer's build log showed their SSH deploy key in plaintext after a global environment hook ran.
Changes
%qformatting inapplyEnvironmentChanges, via a newredactedValuehelperMux.Needlestointernal/replacer, returning the deduplicated needle set across replacersBUILDKITE_GIT_SSH_KEYwith an environment hook present, plus unit tests forMux.NeedlesTesting
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Affiliation (optional, external contributors)
Disclosures / Credits
Fable 5 traced the leak path for me and explained what was occurring, also wrote the secrets integration test so I could validate.