Data aws_kms_secrets always changing on plan and exposing plaintext secrets

tcondeixa commented 4 years ago

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform CLI and Terraform AWS Provider Version

terraform v0.13.0 aws provider v3.1.0

Affected Resource(s)

data aws_kms_secrets

Terraform Configuration Files

This is just one example, where we use several kms encrypted secrets to fill our kubernetes secrets

data "aws_kms_secrets" "new_secret" {
  dynamic "secret" {
    for_each = var.subkeys
    content {
      name = secret.key
      payload = secret.value
    }
  }
}

Expected Behavior

when nothing changed it will not appear in the terraform plan
I expect the plaintext secrets to be an output and not printed in the plan phase

Actual Behavior

the data output is printed every time there is a plan, because the ID is a date time ("2020-08-11 15:34:53.668567 +0000 UTC")
the data aws_kms_secrets is also printing in the output the plaintext map with keys and values

fake plan example:

 <= data "aws_kms_secrets" "user"  {
      ~ id        = "2020-08-11 15:34:32.42599 +0000 UTC" -> "2020-08-11 15:34:53.675945 +0000 UTC"
        plaintext = {
            "password" = "null"
        }

        secret {
            context      = {}
            grant_tokens = []
            name         = "password"
            payload      = "AQICAHjtNu+odQZxxCLPGracuxTOT90qf1WZo/T+XQ7mo6FYawFwux0/+ItR/1Dyj/hvy9F6AAAAYjBgBgkqhkiG9w0BBwagUzBRAgEAMEwGCSqGSIb3DQEHATAeBglghkgBZQMEAS4wEQQMdCQFS2yzbco9ez2FAgEQgB9BLgytWzZ5fMNB0zuyel4QHRtDAcjJibnEXFJAYhn6"
        }
    }

Steps to Reproduce

create a resource aws_kms_secrets and use the data aws_kms_secrets after to test.

ewbankkit commented 4 years ago

Similar (data source id attribute = current timestamp causes continual diff):

https://github.com/terraform-providers/terraform-provider-aws/issues/14546

jbardin commented 4 years ago

@ewbankkit,

While the changing id exacerbates the problem, the underlying issue with this resource is that none of the attributes are Computed, so the provider can't correctly set any values which will always cause a diff when the resource evaluated during plan.

ewbankkit commented 4 years ago

@jbardin Indeed, and I think running acceptance tests with TF 0.13 will flush out many more such cases.

nigelellis commented 4 years ago

I'm also hitting this issue - we have a CI run that validates no drift in configuration. This currently runs:

  terraform plan -input=false -detailed-exitcode

We now see output:

Terraform will perform the following actions:

  # data.aws_kms_secrets.secrets will be read during apply
  # (config refers to values not yet known)
 <= data "aws_kms_secrets" "secrets"  {
...

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

Terraform then returns exit code 2 indicating there is a diff even though there is nothing to apply. This completely breaks our validation pipeline.

nigelellis commented 4 years ago

Also curious why the plaintext collection output from aws_kms_secrets is not marked as sensitive per Terraform specs - see https://godoc.org/github.com/hashicorp/terraform-plugin-sdk/helper/schema#Schema.Sensitive. If it was I think this would address the leakage concerns.

pwilczynskiclearcode commented 4 years ago

My temporary hack is to filter the output with

terraform ..... | perl -p0e 's/plaintext(.*?)=.*?}\n/\n        ---removed-secret----\n/s'

so a plaintext secret from aws_kms_secrets module does not leak to the logs.

nigelellis commented 4 years ago

@pwilczynskiclearcode I took a similar approach but built a wrapper with support to filter out other secrets from non-conforming providers. For example, we get leaks from AWS CF objects with DB connection strings. When you find other patterns to filter, just drop them in the the .sed patterns file.

Just call terraform.sh as a drop-in replacement for terraform:-

[terraform.sh]

#!/usr/bin/env bash
set -euo pipefail
BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"

# Wrapper around terraform which filters the output from terraform apply / plan to redact secrets and sensitive tokens.
# Primarily called from CI validation runs to ensure secrets are not logged in job output.
if [[ $@ != *'plan'* ]] && [[ $@ != *'apply'* ]]; then
  terraform $@ || exit $?
  return
fi

# Fall through to execute filtered "apply" or "plan" action.
TEMP_LOG=$(mktemp)

cleanup_on_exit() {
  EXIT_CODE=$?
  rm -f ${TEMP_LOG}
  exit ${EXIT_CODE}
}

trap cleanup_on_exit EXIT SIGHUP SIGINT SIGTERM

# Ensure -no-color flag is present so we can filter pure text content with grep and sed
if [[ $@ != *'-no-color'* ]]; then
  echo "Error: required \"-no-color\" flag is missing in args: $@"
  exit 1
fi

# Apply verb (plan or apply) and filter output to redact sensitive data
terraform $@ 2>&1 | \
  sed -f "${BASEDIR}/terraform.sed" | \
  tee "${TEMP_LOG}"

# Validate output has zero diffs.    Note that we cannot currently make use of -detailed-exitcode due to the
# https://github.com/terraform-providers/terraform-provider-aws/issues/14564 which shows perpetual data diffs
# on aws_kms_secrets.
grep -qE '^Plan: 0 to add, 0 to change, 0 to destroy\.$|^No changes. Infrastructure is up-to-date.$' ${TEMP_LOG} || exit 2

[terraform.sed]

# post-process the output of `terraform plan` in order to redact sensitive secrets and tokens.

# Remove data-diff output for "aws_kms_secret"
# <= data "aws_kms_secrets" ... {
#       ...
#    }
#
/ <= data "aws_kms_secrets" .*{/,/^    }/d

# Redact Datapipeline password
#   Key: '*password'
#   StringValue: <password>
#
/Key: '\*password'$/{
  N
  s/StringValue: .*$/StringValue: ***REDACTED***/
}

anGie44 commented 4 years ago

This has been partially addressed with the merge of https://github.com/terraform-providers/terraform-provider-aws/pull/15169 (to release with v3.7.0 of the Terraform AWS Provider) -- plaintext values should no longer be exposed when using Terraform 0.13

gibsonje commented 4 years ago

Upgrading to AWS Provider 3.12.0 from 2.x fixed the secrets leak.

Now I see:

plaintext = (sensitive value)

But the ID still changes every apply. I've seen other providers pushing fixes for that part of the problem, but it doesn't look fixed on the AWS provider.

anGie44 commented 4 years ago

Hi @gibsonje et al. 👋 , the ID behavior you're experiencing should be fixed with the recent merge and release of https://github.com/terraform-providers/terraform-provider-aws/pull/15725 . If the behavior persists after upgrading to v3.13.0 of the AWS Provider, please let us know 😃

ghost commented 3 years ago

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

hashicorp / terraform-provider-aws