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

Data aws_kms_secrets always changing on plan and exposing plaintext secrets #14564

Closed
tcondeixa opened this issue Aug 11, 2020 · 11 comments
Closed
Labels
bug Addresses a defect in current functionality. service/kms Issues and PRs that pertain to the kms service.

Comments

@tcondeixa
Copy link

tcondeixa commented Aug 11, 2020

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.

@ghost ghost added the service/kms Issues and PRs that pertain to the kms service. label Aug 11, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Aug 11, 2020
@ewbankkit
Copy link
Contributor

ewbankkit commented Aug 11, 2020

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

@jbardin
Copy link
Member

jbardin commented Aug 12, 2020

@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
Copy link
Contributor

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

@bflad bflad added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 13, 2020
@nigelellis
Copy link

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
Copy link

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
Copy link

pwilczynskiclearcode commented Sep 7, 2020

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
Copy link

@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
Copy link
Contributor

anGie44 commented Sep 16, 2020

This has been partially addressed with the merge of #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
Copy link

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
Copy link
Contributor

anGie44 commented Oct 30, 2020

Hi @gibsonje et al. 👋 , the ID behavior you're experiencing should be fixed with the recent merge and release of #15725 . If the behavior persists after upgrading to v3.13.0 of the AWS Provider, please let us know 😃

@ghost
Copy link

ghost commented Nov 29, 2020

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!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/kms Issues and PRs that pertain to the kms service.
Projects
None yet
Development

No branches or pull requests

8 participants