Add sensitive attribute to variables #26183

pselle · 2020-09-09T15:45:12Z

This PR allows sensitive to be added to a variable declaration, after enabling the sensitive_variables experiment:

terraform {
  experiments = [sensitive_variables]
}

variable "foo" {
  sensitive = true
}

resource "some_resource" "bar" {
  some_val = var.foo
}

At this current stage of the project, this works on some very basic configurations. When sensitivity is defined, when Terraform outputs the plan to the CLI, attributes using this variable will not be displayed:

  # some_resource.bar will be created
  + resource "some_resource" "bar" {
      + id          = (known after apply)
      + some_val    = (sensitive)
    }

This PR depends on changes in https://github.com/zclconf/go-cty -- at the moment, go.mod points to master, while this feature is in progress. Once the methods have stabilized and there is a new release to point at, go.mod will point to that release.

I see that there's not strong covered diff coverage in this PR, which is semi-intentional, the idea to add more tests in a following PR. Saying "that's better to do here, now" would be completely valid -- my intention is that this PR be some reviewable amount of work where Terraform as-is functions, and hard edges are only discovered when using the sensitive attribute.

codecov · 2020-09-09T15:45:21Z

Codecov Report

Merging #26183 into master will decrease coverage by 0.02%.
The diff coverage is 45.55%.

Impacted Files	Coverage Δ
command/format/diagnostic.go	`44.72% <0.00%> (-0.46%)`	⬇️
plans/changes.go	`0.00% <0.00%> (ø)`
plans/changes_src.go	`0.00% <ø> (ø)`
states/instance_object.go	`0.00% <0.00%> (ø)`
terraform/eval_for_each.go	`87.67% <0.00%> (-9.30%)`	⬇️
configs/experiments.go	`70.66% <22.22%> (-6.61%)`	⬇️
plans/objchange/compatible.go	`67.71% <42.85%> (-0.96%)`	⬇️
terraform/eval_apply.go	`70.58% <50.00%> (-0.71%)`	⬇️
command/format/diff.go	`90.08% <69.56%> (-0.60%)`	⬇️
configs/named_values.go	`67.58% <100.00%> (+0.38%)`	⬆️
... and 3 more

pselle · 2020-09-09T16:08:11Z

terraform/testdata/plan-ignore-changes/main.tf

@@ -1,9 +1,9 @@
 variable "foo" {}

 resource "aws_instance" "foo" {
-  ami = "${var.foo}"
+  ami = var.foo


This was updated to remove interpolation warnings when I was in the neighborhood while fixing apply tests

mildwonkey · 2020-09-09T17:41:45Z

command/format/diff.go

@@ -518,6 +526,12 @@ func (p *blockBodyDiffPrinter) writeNestedBlockDiff(name string, label *string,
 }

 func (p *blockBodyDiffPrinter) writeValue(val cty.Value, action plans.Action, indent int) {
+	// Could check specifically for the sensitivity marker
+	if val.IsMarked() {
+		p.buf.WriteString("(sensitive)")


I don't know anything about marks, so please excuse the question if it's nonsense:

Is there any risk that not checking for the specific sensitive marker here could cause problems in the future? Is there any chance that a provider could start using marks, and then someone using terraform 0.14 and that provider would run into a problem?

There's not a risk of that. Values can't be encoded with marks, so a provider cannot send marked values, and that's also why we have to unmark/remark values sending them through providers.

On the methods themselves, IsMarked is a very simple check, with HasMark being the more specific one. https://github.com/zclconf/go-cty/blob/master/cty/marks.go#L90-L102 Using HasMark would require creating a type specific to the mark. Since there's only one (naive) mark presently ("sensitive"), this is avoiding that at the moment.

Mark sensitivity on a value. However, when the value is encoded to send to the provider to produce a changeset we must remove the marks, so unmark the value and remark it with the saved path afterwards

Updates existing code to use the new Value methods for unmarking/marking and removes panics/workarounds in cty marshall methods

The hack approach appears consistent, as we can remove marks before calling the value validation

Apply, at this moment, appears that it does not require the remarking strategy, as the plan has already been printed

Using markedPlannedNewVal caused many test failures with ignoreChanges, and I noted plannedNewVal itself is modified in the eval_diff. plannedNewVal is now marked closer to the change where it needs it. There is also a test fixture update to remove interpolation warnings.

alisdair

Neat! I found this surprisingly easy to follow for a change that touches so many files, in large part I think to excellent comments. Thank you!

I'd really like to see a test of the current functionality if you think that's feasible before merging. Something as simple as the example in the PR description would be perfect. Since this feature is behind an experiment, I'm also comfortable with that coming in a follow-up PR if it's a big ask, so ✅ from me.

plans/changes_src.go

terraform/eval_diff.go

pselle · 2020-09-10T19:36:59Z

@alisdair Thank you for the review! I've been thinking on that codecov diff coverage stat myself a lot, and I think adding a Context2Plan test (at minimum) to at least exercise the current known-working would make me feel more comfortable merging this.

This also unearthed that the marking must happen earlier in the eval_diff in order to produce a valid plan (so that the planned marked value matches the marked config value)

Adds a diff test for a changed value, and modifies the diff file to cover variable diffs on sensitive values

robcxyz · 2020-09-22T00:58:42Z

Will this address #16114 and is this already released? Did not see it in the last release notes.

pselle · 2020-09-22T14:43:39Z

@robcxyz The goal is that it will! And it is not released. The master branch is currently targeting the 0.14 release cycle, the v0.13 branch is where you can see things that have been released (or unreleased) for 0.13.

liuyangc3 · 2020-09-23T06:22:48Z

Hi @pselle thanks for this PR, I am wondering how to mark a map item sensitive, for example

variable "ssh_connection" {
  type = object({
    user        = string
    port        = number
    private_key = string # sensitive
  })
}

pselle · 2020-09-23T16:11:37Z

@liuyangc3 that's something we're exploring now! We are thinking to add support for complex types such that every member of the value would be sensitive (doesn't yet work). For your example though, you could populate values of private_key with another variable that's a string, or a separate variable that is only the private_key, in the case where you want the user and port to remain visible, but obscure private_key.

ghost · 2020-10-12T01:50:09Z

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.

pselle requested a review from a team September 9, 2020 16:01

pselle commented Sep 9, 2020

View reviewed changes

mildwonkey reviewed Sep 9, 2020

View reviewed changes

pselle added 17 commits September 10, 2020 11:02

Add a sensitive attribute

862ddf7

Track sensitivity through evaluation

84d118e

Mark sensitivity on a value. However, when the value is encoded to send to the provider to produce a changeset we must remove the marks, so unmark the value and remark it with the saved path afterwards

If the path is empty, we should not be marking the path

896d277

Unmark/remark in apply process to allow apply

6c129a9

Add sensitive variable configs test coverage

7fef1db

Use UnmarkDeepWithPaths and MarkWithPaths

bc55b6a

Updates existing code to use the new Value methods for unmarking/marking and removes panics/workarounds in cty marshall methods

Modifications to eval_diff

e9d9205

Update object compatible check to unmark

5b0b1a1

The hack approach appears consistent, as we can remove marks before calling the value validation

Apply does not need remarking

3e8b125

Apply, at this moment, appears that it does not require the remarking strategy, as the plan has already been printed

Add case to compactValueStr not to expose sensitive vals in diagnostics

61c78fd

Disallow sensitive values as for_each arguments

b03d5df

Update vendored code

4089b77

Point go module at master go-cty

3723594

Don't save PathValueMarks on instance_object

843ed89

Create experiment for sensitive attribute

02c1bdd

Rebase fix

e4e16cc

pselle force-pushed the pselle/sensitive-values branch from 30dbdec to e4e16cc Compare September 10, 2020 15:06

alisdair approved these changes Sep 10, 2020

View reviewed changes

plans/changes_src.go Outdated Show resolved Hide resolved

terraform/eval_diff.go Outdated Show resolved Hide resolved

pselle added 3 commits September 10, 2020 16:06

Add basic plan test coverage

4034cf9

This also unearthed that the marking must happen earlier in the eval_diff in order to produce a valid plan (so that the planned marked value matches the marked config value)

Add diff test with a sensitive change

8d8389d

Adds a diff test for a changed value, and modifies the diff file to cover variable diffs on sensitive values

Updates and improvements to comments

20ee878

pselle merged commit 6a126df into master Sep 11, 2020

robcxyz mentioned this pull request Sep 22, 2020

Flagging input variables are sensitive #16114

Closed

lornasong mentioned this pull request Sep 25, 2020

Look into using Terraform sensitive attribute hashicorp/consul-terraform-sync#85

Closed

ghost locked as resolved and limited conversation to collaborators Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sensitive attribute to variables #26183

Add sensitive attribute to variables #26183

pselle commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading

pselle Sep 9, 2020 •

edited

Loading

mildwonkey Sep 9, 2020

pselle Sep 9, 2020

alisdair left a comment

pselle commented Sep 10, 2020

robcxyz commented Sep 22, 2020

pselle commented Sep 22, 2020

liuyangc3 commented Sep 23, 2020

pselle commented Sep 23, 2020

ghost commented Oct 12, 2020

Add sensitive attribute to variables #26183

Add sensitive attribute to variables #26183

Conversation

pselle commented Sep 9, 2020 • edited Loading

codecov bot commented Sep 9, 2020 • edited Loading

Codecov Report

pselle Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

mildwonkey Sep 9, 2020

Choose a reason for hiding this comment

pselle Sep 9, 2020

Choose a reason for hiding this comment

alisdair left a comment

Choose a reason for hiding this comment

pselle commented Sep 10, 2020

robcxyz commented Sep 22, 2020

pselle commented Sep 22, 2020

liuyangc3 commented Sep 23, 2020

pselle commented Sep 23, 2020

ghost commented Oct 12, 2020

pselle commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading

pselle Sep 9, 2020 •

edited

Loading