-
Notifications
You must be signed in to change notification settings - Fork 303
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
Reject pipeline uploads containing redacted vars #1523
Conversation
Co-Authored-By: Eleanor <[email protected]> Co-Authored-By: Paula <[email protected]>
Co-Authored-By: Eleanor <[email protected]> Co-Authored-By: Paula <[email protected]>
Co-Authored-By: Eleanor <[email protected]> Co-Authored-By: Paula <[email protected]>
Co-Authored-By: Eleanor <[email protected]> Co-Authored-By: Paula <[email protected]>
a4dd9e5
to
01ef8d9
Compare
01ef8d9
to
6da0424
Compare
6da0424
to
7e10854
Compare
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.
@eleanorakh I’ve taken another look at this and had some ideas about error messages before we merge, let me know what you think 🙇 😃
clicommand/pipeline_upload.go
Outdated
serialisedPipeline, err := result.MarshalJSON() | ||
|
||
if err != nil { | ||
l.Fatal("Pipeline serialization of \"%s\" failed (%s)", src, err) |
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.
What do you think about tweaking this error to say something like:
"Couldn’t scan the %q pipeline for redacted variables. This parsed pipeline could not be serialized, ensure the pipeline YAML is valid, or disable secret redaction for this upload by passing --redacted-vars=''. (%s)", src, err
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.
Yea, I like that. It's hitting the three points of what makes a good error message. I'll update! :)
clicommand/pipeline_upload.go
Outdated
|
||
for _, needle := range needles { | ||
if strings.Contains(stringifiedserialisedPipeline, needle) { | ||
l.Fatal("Couldn't upload %q pipeline. Refusing to upload pipeline containing redacted vars. Ensure your pipeline does not include secret values or interpolated secret values", src) |
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.
I did wonder about whether GetValuesToRedact
could return tuples of (env_key, env_value)
so that this error could say "Whoops your pipeline interpolated the value of the $env_key" and point folks closer to the answer. What do you think?
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.
Yea, a bit more specificity never hurts!
How about this?
Couldn't upload %q pipeline. Refusing to upload pipeline containing interpolated values of the $env_key. Ensure your pipeline does not include secret values or interpolated secret values
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 I think that would be great 😄
Here’s how this looks when your pipeline interpolates one of the environment variables matched by the
|
Work in progress with @eleanorakh and @pzeballos to catch and prevent pipeline uploads from including interpolated redacted vars.
Fixes #1469