random_string leaking sensitive information on rotate

[mikebrooks-ovo]

The random_string resource leaks the current value when planning with a changed keeper value

Terraform Version

Terraform v0.11.7

• provider.random v2.0.0

Affected Resource(s)

• random_string

Terraform Configuration Files

provider "random" {
  version = "2.0"
}

variable "rotator" {
  default = 1
}

resource "random_string" "password" {
  keepers = {
    rotator = "${var.rotator}"
  }

  length  = 30
  special = true
}

Output of apply:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  + random_string.password
      id:              <computed>
      keepers.%:       "1"
      keepers.rotator: "1"
      length:          "30"
      lower:           "true"
      min_lower:       "0"
      min_numeric:     "0"
      min_special:     "0"
      min_upper:       "0"
      number:          "true"
      result:          <computed>
      special:         "true"
      upper:           "true"


Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.password: Creating...
  keepers.%:       "" => "1"
  keepers.rotator: "" => "1"
  length:          "" => "30"
  lower:           "" => "true"
  min_lower:       "" => "0"
  min_numeric:     "" => "0"
  min_special:     "" => "0"
  min_upper:       "" => "0"
  number:          "" => "true"
  result:          "" => "<computed>"
  special:         "" => "true"
  upper:           "" => "true"
random_string.password: Creation complete after 0s (ID: none)

Up until this point the value is safe. However if you then change the keeper e.g.

variable "rotator" {
  default = 2
}

And run a plan it exposes the current value in the diff:

terraform plan
random_string.password: Refreshing state... (ID: none)

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

-/+ random_string.password (new resource required)
      id:              "none" => <computed> (forces new resource)
      keepers.%:       "1" => "1"
      keepers.rotator: "1" => "2" (forces new resource)
      length:          "30" => "30"
      lower:           "true" => "true"
      min_lower:       "0" => "0"
      min_numeric:     "0" => "0"
      min_special:     "0" => "0"
      min_upper:       "0" => "0"
      number:          "true" => "true"
      result:          "3kktL*zGlEPZcx(Z5=CwlA%1D8y%]G" => <computed>
      special:         "true" => "true"
      upper:           "true" => "true"

Expected Behavior

The diff should say:

result: <sensitive> => <computed>

Actual Behavior

The sensitive value is leaked in a plain text.

Steps to Reproduce

1. Create a random string with a keeper value
2. Change the keeper value and replan

References

#17

[jcarrothers-sap]

@mildwonkey Is there any timeline for addressing this? Do you think proceeding with #48 is the best plan here, or would it make sense to revisit the idea of a separate random_password resource, that reuses most of the existing random_string code?

[mildwonkey]

I prefer a separate resource - random_string was never intended to be used for passwords, and it could be confusing for users of older versions of terraform. random_password could easily call random_string (moving the bulk of the random_string code into a function if necessary) and just not set the resource_id. I don't believe it is necessary to duplicate the code.

As for the timeline, the core team is still entirely focused on terraform 0.12 and will continue to be focused on it for the next several weeks. I know it's been a frustrating, long wait and appreciate your patience!

[mikebrooks-ovo]

I agree a separate resource would be useful however at the minute the documentation does seem to suggest that the random_string is suitable for passwords, on this page https://www.terraform.io/docs/providers/random/r/string.html

It says that:

This resource does use a cryptographic random number generator.

And the example uses the resource id password which is passed in as the password to an RDS instance. This led myself and I'm sure many others to use it in this way but then later find that values were being leaked in build log files without warning.

[jcarrothers-sap]

I did seriously consider updating the random_string example to correct exactly this problem as part of #52, but I decided to leave it alone unless specifically asked to change it as I had limited time to work on the PR.

[radeksimko]

As discussed above random_string was never intended to be used for passwords and a dedicated random_password resource was implemented in #52 and released as part of v2.2.0.

Thanks to everyone involved.

I am going to close this as there is nothing more to be done.
Feel free to report bugs/requests about either of these two resources as separate issues.

[essjayhch]

The problem is that the proposed fix in #52 doesn't actually address the broader problem highlighted in this issue, namely that keepers in the random provider leak their data regardless of what resource you use:

Terraform will perform the following actions:

  # random_password.password must be replaced
-/+ resource "random_password" "password" {
      ~ bcrypt_hash      = (sensitive value)
      ~ id               = "none" -> (known after apply)
      ~ keepers          = { # forces replacement
          ~ "resource_group" = "foobb" -> "foobbb"
        }
      ~ result           = (sensitive value)
        # (11 unchanged attributes hidden)
    }

Plan: 1 to add, 0 to change, 1 to destroy.

If you are legitimately using random strings/integers/passwords/ that need to be triggered to change when other sensitive data changes, the random_password resource isn't going to fit that use-case either as it will still display the contents of the keepers regardless of if they are sensitive or otherwise, as demonstrated above.

[radeksimko]

@essjayhch To my best knowledge, rendering of each field should reflect sensitivity.

For example,

resource "random_password" "password" {
  length           = 16
  special          = true
  keepers = {
    resource_group = azurerm_resource_group.example.id
  }
}

the resource_group value above is not treated as sensitive because the corresponding id field isn't sensitive.

You can however force it to be treated as sensitive using the sensitive function, if your interpretation of that particular field is different from the default (i.e. if for any reason you wish to treat resource group ID as sensitive).

resource "random_password" "password" {
  length           = 16
  special          = true
  keepers = {
    resource_group = sensitive(azurerm_resource_group.example.id)
  }
}

There are fields other than azurerm_resource_group id which are treated as sensitive by default, e.g. https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/postgresql_server#administrator_login_password

I don't see a reason why keepers should always be treated as sensitive, as there are different use cases, only in some of which you'd refer to a sensitive field - and when you do, then it should already be rendered as sensitive.

If it isn't then I would treat that as a bug and would ask you kindly to describe the details and attach repro case in a new issue here: https://github.com/hashicorp/terraform/issues/new/choose

FYI we also have a discourse forum available for questions: https://discuss.hashicorp.com/c/terraform-core/27

[github-actions]

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