hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.67k stars 9.55k forks source link

Terraform outputs sensitive information when using import block as comment #33544

Open kderck opened 1 year ago

kderck commented 1 year ago

Terraform Version

Terraform v1.5.2

Terraform Configuration Files

import {
  to = aws_iam_access_key.access_key_1
  id = "AK..." # Key value omitted
}

Will output

# generated_resources.tf
# __generated__ by Terraform from "AK..."
resource "aws_iam_access_key" "access_key" {
  pgp_key = null
  status  = "Active"
  user    = "..."
}

Debug Output

N/A

Expected Behavior

Terraform maybe should hash the value instead of plain text. While having only the AWS Access Key ID without the Secret Access Key is not as dangerous as having both, but it is still essential to treat the Access Key ID as sensitive information and take measures to protect it from unauthorized access.

Actual Behavior

terraform creates the resources and leaks the access key ids while the import blocks can be moved.

Steps to Reproduce

terraform plan -generate-config-out=generated_resources.tf

Additional Context

No response

References

https://developer.hashicorp.com/terraform/language/import/generating-configuration

kmoe commented 1 year ago

Thanks, I agree we should be conservative here and not write the import id to the comment in case it's sensitive.

apparentlymart commented 1 year ago

This does seem to be a rather different idea of "sensitive" than how we usually think of it.

Knowing an access key ID alone does not give access to anything new, but knowing it might be useful for cross-referencing with other information outside of this scope. But by that argument, all ids are sensitive because they can be used for cross-referencing.

I don't mean to say that we shouldn't just delete this comment from the generated code, since the need for it seems pretty marginal anyway. But I think we should be careful to be clear about what we mean when we say "sensitive"; elsewhere in Terraform it typically means a credential granting some access to something. IAM distinguishes between the access key ID and its secret specifically so that it's possible to talk about a specific access key without disclosing its secret token.

If we broaden "sensitive" to mean "anything that any individual might prefer not to disclose" then Terraform would not be able to generate any configuration at all, and so I think as part of this change (along with removing the unnecessary comment as suggested) we might wish to make sure the docs about config generation spend some words discussing the possibility that the generated files might contain information that should be edited out by the user before committing the changes, since Terraform doesn't have enough information to reliably guess what might be problematic to include in any specific situation.

kmoe commented 1 year ago

Yes, "sensitive" is not accurate here. It is sufficient that the comment is unnecessary.

kmoe commented 1 year ago

That said, this becomes a bigger problem if #33228 is implemented, as then it may be possible for an import ID to be the result of interpolating a sensitive value, in which case it is definitely inappropriate to write the literal value to the generated file.

In the case described in this issue, the value written to the generated file is, by definition, already in configuration, since it must be a literal string inside the import block.

apparentlymart commented 1 year ago

We currently prohibit using a sensitive map (in the sense defined by the language runtime) in resource and module for_each so that we can avoid having resource instance addresses that we can't show in the UI, since that would make it very hard to properly review the proposed plan or updated state.

That might be sufficient precedent to also prohibit using a sensitive string as the id for import, although if we can avoid actually showing the ID string in the UI anywhere (aside from showing the real resource attributes that just happen to be derived from it, where our UI code already respects sensitivity) then that's less of a concern.

brental commented 8 months ago

The import block also outputs the import ID in output as where the resource is being imported from and as the Id of the resource in the plan before apply. This means for cases like the venafi_certificate resource where the import ID contains the password for the private key that sensitive information gets exposed. This occurs even if the sensitive function is used on the value assigned to the id field of the import block.

The terraform import command outputs the import ID as well.

This means that there is no way to import resources such as venafi_certificate without exposing sensitive information. It would good if the ID of the import block respected the sensitive function.

Here is the doco on the import ID venafi_certificate: https://registry.terraform.io/providers/Venafi/venafi/latest/docs/resources/venafi_certificate#import