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

Ability to Conditionally Mark Schema Attributes Sensitive #736

Open
bflad opened this issue Apr 9, 2021 · 6 comments
Open

Ability to Conditionally Mark Schema Attributes Sensitive #736

bflad opened this issue Apr 9, 2021 · 6 comments
Labels
enhancement New feature or request upstream-protocol Requires change of protocol specification, i.e. can't be done under the current protocol

Comments

@bflad
Copy link
Contributor

bflad commented Apr 9, 2021

Please Note: This issue is being submitted in the absence of an existing one I could find and meant more for gathering other use cases and/or showing an explicit decision on the subject. Especially in Terraform CLI 0.15 and later, most practitioners can and probably should use sensitive = true variable blocks or the sensitive() function to explicitly enable sensitive value handling where it is not present in providers. Conversely, the nonsensitive() function can be used to workaround provider-based sensitive values in downstream references as necessary. There is potential for this issue to be moot for code enhancements (except maybe comments around the helper/schema.Schema Sensitive field), but maybe left as tracking towards documentation and education efforts.

SDK version

v2.5.0

Use-cases

Currently, the Terraform Plugin SDK does not provide functionality to conditionally mark or unmark sensitive attributes. In numerous cases, provider developers have had to compromise by marking certain attributes with Sensitive: true in their schema while the value may not be always sensitive based on other resource configuration. When a provider attribute, especially in Terraform CLI 0.15 and later, is marked sensitive, but does not actually have a sensitive value, it has a lot of user experience consequences, including:

  • Hiding potentially helpful plan difference output, especially:
    • Maps, where keys are hidden
    • Blocks, where all values for the attribute are hidden
  • In Terraform CLI 0.15 and later: requires configuration changes to update output blocks to include sensitive = true or wrap sensitive values with the nonsensitive() function. This may not be intuitive since it previously worked fine for practitioners.

For a canonical AWS Provider example, see the aws_ssm_parameter resource, which accepts a String type or SecureString type for the parameter. It is currently set as Sensitive: true. I'll reference other issues/resources below where we have seen this request for conditional sensitivity in the AWS Provider as well.

Attempted Solutions

In the AWS Provider example, debating on whether to create a separate "sensitive"/SecureString resource:

  • If the new resource was the "sensitive" one, this requires a lot of practitioner burden though as they must know that a new resource exists and must migrate. It also would mean the developers would need to choose when to remove the sensitivity from the existing resource, which likely would not be welcomed by the community until a major version release in a long timeframe.
  • If the new resource is the "non-sensitive" one, there still is a lot of practitioner burden to learn about and migrate to the new resource.

Proposal

Enhance the SDK (presumably after Terraform CLI and Terraform Plugin Protocol changes) to support conditional sensitivity on attributes, similar to how ForceNew can be applied conditionally.

References

@bflad bflad added the enhancement New feature or request label Apr 9, 2021
@paddycarver paddycarver added the upstream-protocol Requires change of protocol specification, i.e. can't be done under the current protocol label Apr 12, 2021
@paddycarver
Copy link
Contributor

Thanks for the very detailed write-up, @bflad! I just wanted to chime in and note that your suspicions are correct, we would need an upstream Terraform change for this, as the protocol decides whether a field is Sensitive when retrieving the schema, and therefore that information is supposed to be static. To enable it to be dynamic, we'd need to figure out either how to make schemas dynamic (hard) or figure out a new way to communicate sensitivity to Terraform that allowed for it to be dynamic (open-ended). I think it's a really reasonable request, I just wanted to be clear on why it's blocked at the moment. I've labeled it accordingly, and will make sure the feedback gets to the Terraform team, to get it on their radar.

@bflad
Copy link
Contributor Author

bflad commented Apr 13, 2021

Some additionally good news here is that the upstream issue with provider-based sensitive values not being able to be unmarked via the nonsensitive() function has been fixed and will release with Terraform CLI 0.15.0. While this does not help with the plan for the affected value within the resource itself, it can be used to fix downstream attribute/output references in the plan.

@apparentlymart
Copy link
Contributor

Modern Terraform versions now treat sensitivity as a dynamic characteristic that can flow between expressions in the same sense that "unknown" is, so the good news here is that I think the overall Terraform language is ready to support something like this.

The main challenge here then, I believe, is defining a backward-compatible way for sensitivity to traverse the provider plugin wire protocol, both into and out of operations like PlanResourceChange and ApplyResourceChange (and probably any other that returns resource instance attributes, I guess?)

Related to that, although then more the concern of the SDK itself rather than Terraform, is to think about to what extent provider code should interact with sensitive values. The most ideal thing would be for providers to preserve transitive sensitivity in the same way that the Terraform language does, so that e.g. if I set the name attribute to a sensitive value and the provider uses that name as part of the arn attribute then the ARN would be reflected as sensitive too.

That's a pretty disruptive, cross-cutting requirement though... provider code typically wants to convert Terraform language values into more conventional Go types, act on them, and then convert back to the Terraform type system on the way out, but that runs into similar challenges as we have with unknown values during PlanResourceChange: there's no way to retain the "unknownness" or "sensitiveness" of a value as it passes through a string or int value, and so we'd need to handle that explicitly for all values just in case a sensitive value shows up.

It was for these reasons that our first iteration of sensitivity left the provider-side model of sensitivity still static and based on schema: that avoided imposing any new burdens on the SDK or on individual provider codebases, while still getting a reasonable compromise behavior. I expect we can find reasonable answers/compromises for these questions, and so I'm not intending this comment as rejecting the proposal but rather as some additional context that I hope will help inform the next level of design.

@paddycarver
Copy link
Contributor

I think that we probably won't ever get to perfect support here if it were dynamically addressable via providers--as @apparentlymart pointed out, provider values too frequently become Go values that lose this information. But I think we could support it in the same way we do unknown values in the framework at the very least, making it part of the tftypes.Value and then the attr.Value interface. That would allow provider developers to address it whackamole fashion at least, responding to practitioner feedback. We could even retain the current handling of Sensitive as a static modifier, marking the value as sensitive on its way out under all circumstances.

@apparentlymart
Copy link
Contributor

I expect that for backward compatibility with existing providers Terraform Core would need to continue being responsible for handling the static Sensitive flag in schema, or at least providers would need to explicitly opt out of Terraform Core doing that, but neither of those paths seem particularly troublesome.

If we don't have that opt-out then I guess Terraform Core would mark the value as sensitive if the schema says so OR if the dynamic provider response says so, with the logical meaning of OR such that the static declaration being true would effectively disable the dynamic declaration entirely, but static being false would allow dynamic to take either value.

Because there's no standard representation of this idea of "sensitive" in either JSON or msgpack, thus far Terraform Core has always serialized sensitivity marks as a sidecar data structure using attribute paths. That works in most cases, but it does have some rough edges in the same places that our other uses of attribute paths do, such as there being no way to represent traversing into a set and selecting a specific element. (In practice that's not really important for sensitive in particular, because individual set elements can't be sensitive anyway.)

With that said then, perhaps we could prototype what it would look like to add some optional new fields in the protobuf specification to carry the sensitivity information alongside the values. I don't know yet whether it would make more sense to add them to each of the relevant Response messages for particular service functions or if we'd generalize it as a new field in DynamicValue, or something else entirely; hopefully we can weigh these options by seeing what the framework-side code to handle them might look like.

@YakDriver
Copy link
Member

YakDriver commented Jul 6, 2022

I haven't seen all the possible places where this can be an issue. However, I can give the context of one place. A list (or set) of name/value pairs. If name (or type) is SecureString, then the value is sensitive. The problems appear legion with dynamically marking some element as sensitive and not others. It seems like a better, simpler approach is to just add a separate argument to bifurcate the values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request upstream-protocol Requires change of protocol specification, i.e. can't be done under the current protocol
Projects
None yet
Development

No branches or pull requests

4 participants