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

Removes the risk of sending decrypted EJSON secrets to output. #431

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

stantona
Copy link
Contributor

@stantona stantona commented Feb 27, 2019

NB: This addresses a security issue with application secrets

What are you trying to accomplish with this PR?
Presently, if a 'kubectl apply' fails when updating EJSON secrets, it's
possible that the decrypted secrets payload can be output as part of the
error message.

This should be avoided since it can expose all secrets of your application.

For e.g, GKE had an incident on February 26th which caused
intermittent errors when calling kubectl. We happened to see a failure
at the step when secrets are updated. Because kubectl apply failed, the
exception was bubbled up and logged to output.

How is this accomplished?
This change takes a simple approach by not including the error message in
commands where it is possible to display the payload. The 'Kubectl' class
does not log messages if the sensitive flag is set to true, however the
EjsonSecretProvisioner class raises EjsonSecretError which is logged
further up the stack.

What could go wrong?
The error that is displayed is generic and will not show specific error information
returned from kubectl. This might be problematic.

We would like some feedback on this PR specifically whether this is the best approach.
Either way, this addresses a significant security risk that should be looked at ASAP.

Pinging @dturn on the advice of a colleague who formerly worked at Shopify.

Presently, if a 'kubectl apply' fails when updating EJSON secrets, it's
possible that the decrypted secrets payload can be output as part of the
error message.

This should be avoided since it can expose all secrets of your application.

This change takes a simple approach by not including the error message in
commands where it is possible to display the payload. The 'Kubectl' class
does not log messages if the sensitive flag is set to true, however the
EjsonSecretProvisioner class raises EjsonSecretError which is logged
further up the stack.
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that losing the error message from kubectl isn't great, but I don't think we want to try and parse these messages. #424 would have eliminated a bit of this risk but not all of it.

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to make sure @DazWorrall PR isn't also affected by this, since it moves secret creation and pruning out of this class. I'm ok to merge this in the meantime though.

@benlangfeld
Copy link
Contributor

benlangfeld commented Feb 27, 2019

@KnVerey Since #424 is done, would it be possible to merge that first? It's had significant effort put into several rounds of review already and is blocking #411

@dturn
Copy link
Contributor

dturn commented Feb 27, 2019

@benlangfeld I feel your pain. Unfortunately, I think it's important that we release security issues quickly. And as @KnVerey pointed out in a different pr, there are versioning reasons why it makes sense to cut a release without 424

@dturn dturn merged commit ee10605 into Shopify:master Feb 27, 2019
@stantona stantona deleted the hide-potential-application-secrets branch February 27, 2019 19:58
@timothysmith0609 timothysmith0609 temporarily deployed to rubygems February 27, 2019 20:41 Inactive
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.

None yet

5 participants