terraform icon indicating copy to clipboard operation
terraform copied to clipboard

Document the details around sensitive value handling through various expressions

Open neiser opened this issue 2 years ago • 3 comments

I was trying to extend the Gitlab terraform provider to support "optional" Gitlab project variables. I came up with the code below, using a for...if expression to filter out entries with the value property having null.

Terraform Version

$ terraform version
Terraform v1.2.6
on linux_amd64
+ provider registry.terraform.io/gitlabhq/gitlab v3.16.1

Terraform Configuration Files

main.tf:

terraform {
  required_providers {
    gitlab = {
      # https://github.com/gitlabhq/terraform-provider-gitlab/blob/master/CHANGELOG.md
      source  = "gitlabhq/gitlab"
      version = ">= 3.7.0"
    }
  }
}

resource "gitlab_project_variable" "this" {
  # explicit nonsensitive here needed only when there's a filtering if clause
  for_each = nonsensitive({ for k, v in var.variables : k => v.value if v.value != null })

  project           = var.project_id
  key               = each.key
  value             = each.value
  variable_type     = lookup(var.variables[each.key], "type", "env_var")
  protected         = lookup(var.variables[each.key], "protected", false)
  masked            = lookup(var.variables[each.key], "masked", false)
  environment_scope = var.environment
}

variables.tf:

variable "project_id" {
  type        = string
  description = "The id or name of the gitlab project to which the variable is to be set."
}

variable "environment" {
  type        = string
  description = "The gitlab environment to which the variables are added."
  default     = "*"
}

variable "variables" {
  # TODO Once Terraform 1.3.x is in use, we can use optional() object attributes!
  type        = map(any)
  description = <<-EOT
  The project variables (key is name) to be set with their properties as map value.
  The map value is itself a map with the following keys:
  `value` (if `null`, variable is not set)
  `type` (default `"env_var"`)
  `protected` (default `false`)
  `masked` (default `false`)
  EOT
}

Expected Behavior

terraform plan should run without complaining about sensitive values, even if the explicit nonsensitive call above is removed in the for...if expression.

Actual Behavior

If the nonsensitive call is removed, terraform plan complains as follows:

╷
│ Error: Invalid for_each argument
│ 
│   on .terraform/modules/gl_project_vars_gitops/main.tf line 12, in resource "gitlab_project_variable" "this":
│   12:   for_each = { for k, v in var.variables : k => v.value if v.value != null }
│     ├────────────────
│     │ var.variables is map of map of string with 20 elements
│ 
│ Sensitive values, or values derived from sensitive values, cannot be used as for_each arguments. If used, the sensitive value could be exposed as a resource instance key.
╵

If you remove the if v.value != null, it also works without the explicit nonsensitive call. As v.value is part of a object and not marked as sensitive, I really wonder what's going on here.

Let me know if I can provide any further details.

neiser avatar Aug 10 '22 06:08 neiser

Hi @neiser,

Thanks for filing the issue. The actual inputs are not shown here, but I assume that v.value is receiving a sensitive value in this case?

The reasoning here is that because you are basing the existence of map key values on a value which is marked sensitive, then the map itself must also be marked sensitive. Because it's possible that the comparison of the keys with the sensitive value could divulge information about that value, the keys must also be marked. Without the condition, Terraform can always tell that the v.value marks never have any influence over the keys.

Since this is working as designed, I'm going to close the issue. If you have more questions it's better to use the community forum where there are more people ready to help.

Thanks!

jbardin avatar Aug 10 '22 13:08 jbardin

@jbardin Thanks for clarifying this for me. The possibility that additional filtering could expose sensitive values and thus render the whole generated map expression sensitive is quite subtle though. Indeed, I was so surprised that I immediately created a bug report instead of asking the community beforehand, sorry for that!

Do you think I should create a pull request which mentions this bevahior? I think this might be a good place. At least, I was reading that page before I submitted this issue.

neiser avatar Aug 10 '22 17:08 neiser

I'm not sure if scattering pieces of the behavior around the docs is the most useful way to document this type of thing, and often makes the documentation harder to follow as numerous examples start to build up. There are countless ways various combinations of features can interact which may be surprising when you don't expect it, and trying to document all those combinations at the points of intersection isn't really feasible. Maybe there's a place for expanding the sensitive value documentation itself, or breaking that out into another page?

It's probably better if I re-open this as a documentation request, and have the documentation team decide how best to proceed.

jbardin avatar Aug 10 '22 18:08 jbardin

Hi both!

I agree with @jbardin that we probably should not just put a note in a section of the docs where folks may not see it. I wonder though if there is a case where we could have an entire page dedicated to sensitive values and the way that Terraform handles them. This would be helpful if there are other gotchas that we could document/other nuances that folks should consider when writing Terraform configuration. I think this is worth discussion with the team.

I will make a ticket on the documentation board to consider this issue and we will circle back to this ticket when we have the bandwidth over the next few quarters. Thanks for bringing this to our attention, @neiser! :)

laurapacilio avatar Aug 15 '22 15:08 laurapacilio

@jbardin @laurapacilio This also happens when we use AWS KMS encrypted sensitive data. Please Let me know if i can contribute to the documentation.

krishnaduttPanchagnula avatar Aug 18 '22 19:08 krishnaduttPanchagnula

This is a serious deficiency in the docs. I wasted hours trying to figure out why this error was only thrown when using a conditional filter inside the for_each.

@jbardin adding nonsensitve to the map doesn't even work. I get the error:

Invalid value for "value" parameter: the given value is not sensitive, so this call is redundant.

et304383 avatar Apr 26 '23 16:04 et304383