redact sensitive output values in run logs

[Uk1288]

1.4.x

CHANGELOG entry draft

• Previously, logs generated from running terraform apply -json contained plaintext value of sensitive outputs. With this fix, all sensitive output values will be omitted in the json logs when terraform apply -json is run. To retrieve machine-readable output values, including those which are sensitive use terraform output -json instead. (#31813)

Description

Currently, sensitive output values are displayed as plain text in the run logs. This PR omits sensitive output values when logging.

The fix targets both CLI and TFC workflows. In CLI, when running terraform plan -json and terraform apply -json, sensitive values should be omitted. Similarly, when downloading raw log for both plan and apply in TFC, sensitive values should be omitted as well.

Before, json logs:

				"secret": {
					"sensitive": true,
					"value":     "plain-text-secret",
					"type":      "string"
				}

After these changes:

                              "secret": {
					"sensitive": true,
					"type":      "string"
				}

[apparentlymart]

Our position so far has been that "sensitivity" only affects human-oriented UI, and that any output intended for machine consumption will include the values in cleartext because it's the responsibility of the consumer of that output to decide what is an appropriate way to show (or not show) the sensitive values.

I'm not against changing it, but I think we should aim to have a principled rule for what the expectations are, ideally derived from a threat model, so that when we add new features in future we can ensure that we're being consistent with that rule. I worry that if we just change this particular case as an exception without a clear understanding of what makes it exceptional then there's nothing except institutional memory to help us decide whether a future feature that does something similar ought to include or not include a particular sensitive value.

I'm particularly interested in what the updated general rule would be for other integration points that export output values, such as terraform output -json and terraform show -json. Is the expectation here that sensitive values be hidden consistently in all of those, or are we making a rule specifically for the streaming JSON event output? If we're planning to withhold sensitive output values consistently in all integration points, are sensitive output values even useful anymore?

(My understanding is that their primary purpose is to be reviewed by human operators, so a possible UI I would've expected would be to hide them by default but to allow an operator to elect to either reveal the secret on-screen or to copy it to the clipboard, similar to the UI in common password managers. Withholding the data from the integration point that our UI uses means that the UI could not possibly offer such a UI, and that seems to raise the question of how exactly an operator could make use of a sensitive output value in that case.)

[brandonc]

@apparentlymart I understood the distinction between human oriented and machine oriented UI, and feel this is a good principle... But felt that the content of log output in particular was human-oriented, regardless of the fact that it was in a structured wrapper. I felt this was, if not a clear example of casual sensitive exposure, then definitely one worth discussing. Does your human UI analogy extend to structured log output? We thought that there wasn't a compelling reason to allow log message contents to be machine-consumable.

@alisdair similarly, I would question if changing the log contents constitutes an API version bump.

[alisdair]

I would question if changing the log contents constitutes an API version bump

If changing the semantics of the JSON logs doesn't constitute a version number increment, I don't know what would. Note that there's a separate version number exclusively for the JSON streamed log output; this isn't going to affect any other JSON output format version numbers.

[Uk1288]

Instead of post-processing the outputs in the JSONView methods, it would make more sense to me if we kept that logic in the json.Outputs type itself.

One way to do this would be adding a MarshalJSON method on the json.Outputs type. Another option is modifying the OutputsFromMap and OutputsFromChanges constructors.

Good point, I had considered this initially, however it seems like once we redact the outputs during initialization then the output values is always redacted, and cannot be used by other packages in the library. This is not a problem of course if marshalling of outputs only exist for purely logging purpose.

[Uk1288]

Instead of removing the value totally when an attribute is sensitive, I have changes it from "(sensitive value)" to "".

Reason being that another service depends on the presence of a value to make certain decisions. This service stops working as expected when the value of an output is represented as null when the value actually exist.

[apparentlymart]

Since I raised the question about what the "new rule" is here, I just wanted to close that out by documenting an interim conclusion that arose in an internal discussion:

We're planning to consider terraform apply -json and terraform plan -json as being JSON serializations of the UI, which is different than most of our other JSON integration points which are intended to be the data source for a UI to use. For the purpose of this PR that this means that henceforth we are committing to never include sensitive values in either of those specific commands, but this does not apply to any other command with JSON output and those will continue to expose the sensitive values directly and consider it the consumer's responsibility to obscure sensitive values in whatever way is appropriate for the use-case.

I imagine we'll continue to refine that definition so that we can classify any new JSON-based integration point as being in either one of these categories and allow future maintainers to understand which classification applies to each existing command. That doesn't need to block progress here though, since any documentation or similar which we'd produce from that discussion would be independent of this change.

[github-actions[bot]]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[github-actions[bot]]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.