-
Notifications
You must be signed in to change notification settings - Fork 327
Fix the pipeline upload --reject-secrets flag not rejecting secrets #3580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
wolfeidau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one query
| ".spx": "audio/ogg", | ||
| ".sql": "application/x-sql", | ||
| ".sqlite": "application/vnd.sqlite3", | ||
| ".sqlite3": "application/vnd.sqlite3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this another change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, although it also got merged in as part of #3579, so it should be gone when i rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(which i have now done)
e542975 to
f654bc5
Compare
zhming0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I am increasingly worried about the lack of integration test like we have in k8s stack 😅 .
|
@moskyb I've just updated my
I have also added some debug statements to validate the value of Note In practice those buildkite agents are EC2 instances so the update I made was to update our CloudFormation stack from the version |
|
Ah, ended up finding the issue: that was because I since I set I changed it to That means though that now every mention of the word I'm still surprised to see that the log redactor would consider the env var name |

Description
Long, long ago (#1523, #1589), we made it so that customers could opt into rejecting pipeline uploads that contained secret values -- eg, if you accidentally committed a pipeline like:
the agent would recognise that there's a cleartext secret in there (
DATABASE_PASSWORD) and refuse to upload it to buildkite.However, much later on we refactored the redactor (#2277), and extracted the secrets detection into a function. That function would return an error if it thought the pipeline shouldn't be uploaded.
... and then we didn't check the error return of the function, meaning that secrets that probably shouldn't get uploaded were uploaded anyway.
This PR adds an error check to that function call, reinstating the behaviour that if a user adds
--reject-secretsto their pipeline upload call, it'll refuse to upload sensitive values.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...)Disclosures / Credits
I didn't use AI.