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

feature request: tfe_variable - sensitive=true should not store value in state as plain text #189

Closed
JacobLey opened this issue Jul 9, 2020 · 6 comments

Comments

@JacobLey
Copy link

JacobLey commented Jul 9, 2020

To preface I understand that the current functionality of tfe_variable is (to my knowledge) exactly as designed. My hope is to point out a potential flaw and discuss a workaround.

Problem

I am trying to use Terraform to manage Terraform Cloud, which gives me easy programatic ways to spin up new workspaces, add all relevant environment variables, and apply consistent settings.

The environment variables are mostly sensitive=true, such as my AWS_SECRET_ACCESS_KEY.
So when I create the environment variable via

resource "tfe_variable" "aws_secret_key" {
  key          = "AWS_SECRET_ACCESS_KEY"
  value        = "<my-super-secret-value>"
  category     = "env"
  hcl          = false
  sensitive    = true
  workspace_id = "<workspace-id>
}

I would hope that the sensitive variable works like it does in normal Terraform Cloud. That is, it is write-only. It should be impossible for me to read the value again and even the target workspace's templates itself do not have access to the variable (unless it is prepended with TF_VAR). Hence making it sensitive.

However, the value of the variable is stored in the state file in plain text! I understand the purpose of this is to check if a re-write is necessary, but it potentially exposes very important values. Given the API doesn't even expose the value, the state file can't even guarantee it is in sync with the workspace.
From https://www.terraform.io/docs/state/sensitive-data.html

Terraform Cloud always encrypts state at rest and protects it with TLS in transit. Terraform Cloud also knows the identity of the user requesting state and maintains a history of state changes. This can be used to control access and track activity. Terraform Enterprise also supports detailed audit logging.

I know the state file is "secure" and I could potentially lock it down further, but at some point it is best to prevent the source of the leak rather than requiring every user to follow access-control best practices.

Proposal

My proposal is that when sensitive=true, the state file should instead receive a hash of the variable. That way it can check against changes to state, without exposing the raw value.

An alternative solution could be that no part value of the value is stored in state (e.g. store null). That way it would actually line up with API responses, but would result in re-writing the variable every time (as it is impossible to detect drift), or perhaps never re-write and require manual tainting.

Higher Level Proposal (out of scope)

It seems like there is a need for potential higher level solution in Terraform's state functionality to store hashes/ignore values that may include sensitive info. Such as the master password on AWS RDS https://www.terraform.io/docs/providers/aws/r/db_instance.html#password in the form of a lifecycle variable.
e.g.

lifecycle {
    sensitive: ["value", "password", "field_not_to_store_in_state"]
}

although that would be out of scope for this repo/particular case.

@bendrucker
Copy link
Contributor

Hashing the value is viable, but a lot of providers opt not do it. I'm not sure how it affects a reference like tfe_variable.foo.value, which may seem a bit contrived, but might prevent something like aws_db_instance.foo.password from using hashing.

Configuring this sort of behavior in a provider-agnostic way is not viable. This pattern works when an attribute is used in Create but not Read or Update. Discarding data that the provider expects is likely to cause odd behavior if not crashes.

@JacobLey
Copy link
Author

Configuring this sort of behavior in a provider-agnostic way is not viable

Agreed probably more wishful thinking on my part than a realistic solution.

affects a reference like tfe_variable.foo.value, which may seem a bit contrived

In my opinion, a sensitive variable should not have its value accessible. If so, it should either return null, or perhaps the value passed to the resource in that particular execution, either way not providing much value (null is useless, and re-calculating the value is just as feasible)

Maybe the safest solution is a new tfe_sensitive_variable resource... Mostly forking from the existing variable logic, adding that value is not stored/exported, and either going with the hash/taint for updates.

@bendrucker
Copy link
Contributor

It's certainly doable, and should probably just happen in place as a breaking change. Mentioning it because it's a reason you don't see this pattern more often.

@jspiro
Copy link

jspiro commented Dec 23, 2020

I can't even see a sensible way to use sensitive. You'd always have to have a value available. The whole point is write once. A simpler interface that is more terrafriendly that would work would be to DECLARE the sensitive value, and enter it in the UI directly.

@scotttyso
Copy link

I wanted to comment on this issue as well. What is most deceiving about this module is that when a plan is run it states the values are sensitive when displayed but then stores the information in plain text. Entering all the information in the UI is not scalable and even in the UI the following is stated "When setting many variables at once, the Terraform Cloud Provider
or the variables API can often save time.

This is something that needs to be addressed.

  + resource "tfe_variable" "variable" {
      + category     = "terraform"
      + description  = "SSH Key for Kubernetes Cluster Authentication."
      + hcl          = false
      + id           = (known after apply)
      + key          = "ssh_key"
      + sensitive    = true
      + value        = (sensitive value)
      + workspace_id = (known after apply)
    }
  # module.tfc_variables_IKS.tfe_variable.variable[8] will be created

@barrettclark
Copy link
Contributor

Sorry for the extremely delayed response. Our current stance is that state is, in itself, sensitive. You can see this issue in terraform where this was discussed. I'm closing this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants