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

Fixes for sensitive values used as input to provisioners #26611

Merged
merged 3 commits into from
Oct 19, 2020

Conversation

alisdair
Copy link
Member

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

after

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.
@alisdair alisdair requested review from pselle and a team October 16, 2020 19:49
@alisdair alisdair self-assigned this Oct 16, 2020
@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

Merging #26611 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
terraform/eval_apply.go 74.62% <100.00%> (+2.40%) ⬆️
terraform/eval_diff.go 67.52% <0.00%> (-0.94%) ⬇️
terraform/node_resource_apply_instance.go 75.00% <0.00%> (-0.80%) ⬇️
backend/remote/backend_common.go 51.98% <0.00%> (-0.73%) ⬇️
internal/providercache/dir.go 71.42% <0.00%> (+6.12%) ⬆️
terraform/ui_output_callback.go 100.00% <0.00%> (+100.00%) ⬆️

Copy link
Contributor

@mildwonkey mildwonkey left a 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.
Copy link
Contributor

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 😉

Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

I like it!

@pkolyvas
Copy link
Contributor

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.

Copy link
Contributor

@pkolyvas pkolyvas left a 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.

@pselle
Copy link
Contributor

pselle commented Oct 19, 2020

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)

@ghost
Copy link

ghost commented Nov 19, 2020

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.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants