Skip to content
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

Merged
merged 11 commits into from
Dec 7, 2021

Conversation

keithduncan
Copy link
Contributor

Work in progress with @eleanorakh and @pzeballos to catch and prevent pipeline uploads from including interpolated redacted vars.

Fixes #1469

@keithduncan keithduncan force-pushed the keithduncan/redact-pipline-uploads branch from a4dd9e5 to 01ef8d9 Compare October 5, 2021 01:16
@eleanorakh eleanorakh force-pushed the keithduncan/redact-pipline-uploads branch from 01ef8d9 to 6da0424 Compare October 19, 2021 00:02
@eleanorakh eleanorakh force-pushed the keithduncan/redact-pipline-uploads branch from 6da0424 to 7e10854 Compare November 30, 2021 00:58
@eleanorakh eleanorakh marked this pull request as ready for review November 30, 2021 01:04
Copy link
Contributor Author

@keithduncan keithduncan left a 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 🙇 😃

serialisedPipeline, err := result.MarshalJSON()

if err != nil {
l.Fatal("Pipeline serialization of \"%s\" failed (%s)", src, err)
Copy link
Contributor Author

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

Copy link
Contributor

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! :)


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)
Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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 😄

@keithduncan
Copy link
Contributor Author

Here’s how this looks when your pipeline interpolates one of the environment variables matched by the --redacted-vars flag:

keithduncan@Keiths-Mac-mini agent % FROG_API_TOKEN=secretcats ./buildkite-agent pipeline upload .buildkite/pipeline.yml --agent-access-token foo
2021-12-07 12:59:04 INFO   Reading pipeline config from ".buildkite/pipeline.yml"
2021-12-07 12:59:04 FATAL  Couldn’t upload the "pipeline.yml" pipeline. Refusing to upload pipeline containing the value interpolated from the "FROG_API_TOKEN" environment variable. Ensure your pipeline does not include secret values or interpolated secret values.

@keithduncan keithduncan merged commit 6d052d2 into main Dec 7, 2021
@keithduncan keithduncan deleted the keithduncan/redact-pipline-uploads branch December 7, 2021 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pipeline upload should refuse to upload pipelines containing redacted-vars
3 participants