-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Fixes for sensitive values used as input to provisioners #26611
Conversation
If provisioner configuration or connection info includes sensitive values, we need to unmark them before calling the provisioner. Failing to do so causes serialization to error. Unlike resources, we do not need to capture marked paths here, so we just discard the marks.
If the provisioner configuration includes sensitive values, it's a reasonable assumption that we should suppress its log output. Obvious examples where this makes sense include echoing a secret to a file using local-exec or remote-exec. This commit adds tests for both logging output from provisioners with non-sensitive configuration, and suppressing logs for provisioners with sensitive values in configuration. Note that we do not suppress logs if connection info contains sensitive information, as provisioners should not be logging connection information under any circumstances.
Codecov Report
|
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 think this looks great, including the UI, and the implementations is a nice pattern for future uses (I have vague recollection of a feature request to optionally suppress provisioner output)
// Marks on the config might result in leaking sensitive values through | ||
// provisioner logging, so we conservatively suppress all output in | ||
// this case. This should not apply to connection info values, which | ||
// provisioners ought not to be logging anyway. |
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 provisioners ought not to be logging anyway.
😂 ah you're funny, this is funny 😉
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 like it!
IMO this is a very nice user-facing change and I appreciate you tackling it. The language in the console also makes it really easy to understand what's going on even if you weren't aware of this behaviour. |
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'm marking this specifically in relation to the user-facing changes and will leave the important coding bits to the team. Thanks for taking the time to solve this one Alisdair.
A flag/reminder that we should probably add docs about this -- esp since this will impact sensitive outputs used in provisioners (so not just limited to people using new sensitive input variable functionality) |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Two separate commits in this pull request, which I'd suggest reviewing separately. The first is a bug fix which I'm pretty confident about, and the second is a UI change that I'd really appreciate feedback on.
Unmark provisioner arguments
If provisioner configuration or connection info includes sensitive values, we need to unmark them before calling the provisioner. Failing to do so causes serialization to error.
Unlike resources, we do not need to capture marked paths here, so we just discard the marks.
Hide maybe-sensitive provisioner output
If the provisioner configuration includes sensitive values, it's a reasonable assumption that we should suppress its log output. Obvious examples where this makes sense include echoing a secret to a file using local-exec or remote-exec.
This commit adds tests for both logging output from provisioners with non-sensitive configuration, and suppressing logs for provisioners with sensitive values in configuration.
Note that we do not suppress logs if connection info contains sensitive information, as provisioners should not be logging connection information under any circumstances.
Screenshot