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

Order is lost for data aws_iam_policy_document when applied to S3 buckets, iam roles, kms keys, etc #11801

Closed
nitrocode opened this issue Jan 29, 2020 · 45 comments · Fixed by #22259
Assignees
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/iam Issues and PRs that pertain to the iam service. service/kms Issues and PRs that pertain to the kms service. service/sts Issues and PRs that pertain to the sts service.
Milestone

Comments

@nitrocode
Copy link
Contributor

nitrocode commented Jan 29, 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

See also #21968

Terraform Version

✗ terraform --version
Terraform v0.12.20
+ provider.aws v2.46.0

Affected Resource(s)

  • aws_ecr_repository_policy
  • aws_kms_key
  • aws_s3_bucket
  • aws_launch_configuration
  • aws_lb_listener
  • aws_codeartifact_domain_pernission_policy

Terraform Configuration Files

If running locally, please add your own real roles to local.secrets_roles otherwise the kms policy will fail.

provider "aws" {
  region = "us-east-1"
}

data "aws_caller_identity" "current" {}

locals {
  id = data.aws_caller_identity.current.account_id
  secrets_roles = [
    "asnip",
    "esnip",
    "lsnip",
    "psnip",
    "wsnip",
    "csnip",
#    "default",
  ]

  secrets_roles_arns = sort([
    for role in local.secrets_roles :
    "arn:aws:iam::${local.id}:role/${role}"
  ])

  secrets_admin = [
    "arn:aws:iam::${local.id}:role/sre"
  ]
}

data "aws_iam_policy_document" "secrets" {
  policy_id = "secrets-policy"

  statement {
    sid       = "Enable IAM User Permissions"
    effect    = "Allow"
    resources = ["*"]
    actions = [
      "kms:Create*",
      "kms:Describe*",
      "kms:Enable*",
      "kms:List*",
      "kms:Put*",
      "kms:Update*",
      "kms:Revoke*",
      "kms:Disable*",
      "kms:Get*",
      "kms:Delete*",
      "kms:ScheduleKeyDeletion",
      "kms:CancelKeyDeletion",
    ]

    principals {
      type        = "AWS"
      identifiers = local.secrets_admin
    }
  }

  statement {
    sid       = "Allow use of the key"
    effect    = "Allow"
    resources = ["*"]

    actions = [
      "kms:Encrypt",
      "kms:Decrypt",
      "kms:ReEncrypt*",
      "kms:GenerateDataKey*",
      "kms:DescribeKey",
      "kms:CreateGrant",
      "kms:ListGrants",
      "kms:RevokeGrant"
    ]

    principals {
      type = "AWS"

      identifiers = local.secrets_roles_arns
    }
  }
}

resource "aws_kms_key" "secrets_test" {
  description         = "test Default key for secrets encryption."
  key_usage           = "ENCRYPT_DECRYPT"
  is_enabled          = true
  enable_key_rotation = false

  policy = data.aws_iam_policy_document.secrets.json
}

resource "aws_kms_alias" "secrets_test" {
  name          = "alias/secrets-test"
  target_key_id = aws_kms_key.secrets_test.key_id
}

output "role_arns" {
  value = local.secrets_roles_arns
}

output "policy_json" {
  value = data.aws_iam_policy_document.secrets.json
}

output "key_policy_json" {
  value = aws_kms_key.secrets_test.policy
}

In terraform console

local.secrets_roles_arns is alphabetical

[
  "arn:aws:iam::1234567890:role/asnip",
  "arn:aws:iam::1234567890:role/csnip",
  "arn:aws:iam::1234567890:role/esnip",
  "arn:aws:iam::1234567890:role/lsnip",
  "arn:aws:iam::1234567890:role/psnip",
  "arn:aws:iam::1234567890:role/wsnip",
]

data.aws_iam_policy_document.secrets.json is reverse alphabetical which is expected (#6107)

...
      "Principal": {
        "AWS": [
          "arn:aws:iam::1234567890:role/wsnip",
          "arn:aws:iam::1234567890:role/psnip",
          "arn:aws:iam::1234567890:role/lsnip",
          "arn:aws:iam::1234567890:role/esnip",
          "arn:aws:iam::1234567890:role/csnip",
          "arn:aws:iam::1234567890:role/asnip"
        ]
      }
...

aws_kms_key.secrets_test.policy returns the same output which is perfect

      "Principal": {
        "AWS": [
          "arn:aws:iam::1234567890:role/wsnip",
          "arn:aws:iam::1234567890:role/psnip",
          "arn:aws:iam::1234567890:role/lsnip",
          "arn:aws:iam::1234567890:role/esnip",
          "arn:aws:iam::1234567890:role/csnip",
          "arn:aws:iam::1234567890:role/asnip"
        ]
      }

After applying the above, I added a default role to local.secrets_roles and I see this in the plan

...
                      ~ Principal = {
                          ~ AWS = [
                              - "arn:aws:iam::1234567890:role/asnip",
                              - "arn:aws:iam::1234567890:role/psnip",
                                "arn:aws:iam::1234567890:role/wsnip",
                              - "arn:aws:iam::1234567890:role/csnip",
                              - "arn:aws:iam::1234567890:role/esnip",
                              + "arn:aws:iam::1234567890:role/psnip",
                                "arn:aws:iam::1234567890:role/lsnip",
                              + "arn:aws:iam::1234567890:role/esnip",
                              + "arn:aws:iam::1234567890:role/default",
                              + "arn:aws:iam::1234567890:role/csnip",
                              + "arn:aws:iam::1234567890:role/asnip",
                            ]
                        }
...

Debug Output

Expected Behavior

Order should be retained. I wouldn't mind the reverse alphabetical order at the very least.

Actual Behavior

Random order

Steps to Reproduce

  1. Create a few roles (or reuse them)
  2. Set the local.secrets_roles with actual roles
  3. terraform apply
  4. Change local.secrets_roles by adding another existing role
  5. terraform plan and you will see random order

Important Factoids

I edited a policy in AWS console, changed the order of principal aws to alphabetical, saved it, and also saw a random order. Perhaps it is the AWS API that is reordering them?

References

Workaround

After upgrading to Terraform 0.14.x, my most recent workaround is by supplying an output

output "key_policy_json" {
  value = data.aws_iam_policy_document.secrets.json
}

Then in the terraform plan or terraform apply, it will hide all the unchanged and only show the few that changed.

              ~ {
                  ~ Principal = {
                      ~ AWS = [
                            # (250 unchanged elements hidden)
                            "arn:aws:iam::1234567890:role/csnip",
                          + "arn:aws:iam::1234567890:role/default",
                            "arn:aws:iam::1234567890:role/esnip",
                            # (125 unchanged elements hidden)
                        ]
                    }
                    # (4 unchanged elements hidden)
                },
@ghost ghost added service/iam Issues and PRs that pertain to the iam service. service/kms Issues and PRs that pertain to the kms service. service/sts Issues and PRs that pertain to the sts service. labels Jan 29, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 29, 2020
@joshmyers
Copy link
Contributor

joshmyers commented Mar 18, 2021

After looking into this issue and looking into the jen20 library for awspolicyequivalency and https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/resource_aws_kms_key.go#L205 nothing in Terraform was causing the policy re ordering and the Terraform outputs looked as I'd expect.

As it turns out, KMS just saves in a random order. This can be replicated with a plain old aws_kms_key (read: not related to the loops/lists as above) passing in a policy, and later appending a new principal.

Can also replicate this in the AWS console:

  • Create a key with a policy
  • Edit the policy
  • Save the policy
  • The ordering has randomly/non deterministically changed.

@chroju
Copy link
Contributor

chroju commented May 21, 2021

I'm experiencing a similar problem with Terraform 0.15.4 .

Terraform 0.15.4 will now report the changes made outside of Terraform as part of the plan result. Some list orders is not retained, so Terraform always reports list orders have been changed like below.

This problem has no real impact, but the plan results are unnecessarily long, and visibility is degraded.

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the
last "terraform apply":

  # aws_iam_role.example has been changed
  ~ resource "aws_iam_role" "example" {
      ~ assume_role_policy    = jsonencode(
          ~ {
              ~ Statement = [
                  ~ {
                      ~ Principal = {
                          ~ AWS = [
                              - "arn:aws:iam::XXXXXXXXXXXX:user/aaaaa",
                              + "arn:aws:iam::XXXXXXXXXXXX:user/ccccc",
                                "arn:aws:iam::XXXXXXXXXXXX:user/bbbbb",
                              + "arn:aws:iam::XXXXXXXXXXXX:user/aaaaa",
                                "arn:aws:iam::XXXXXXXXXXXX:user/ddddd",
                              - "arn:aws:iam::XXXXXXXXXXXX:user/ccccc",
                            ]
                        }
                        # (3 unchanged elements hidden)
                    },
                ]
                # (1 unchanged element hidden)
            }
        )
        id                    = "example"
        name                  = "example"
        tags                  = {}
        # (8 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

@nitrocode
Copy link
Contributor Author

nitrocode commented May 23, 2021

Even though the api returns it in a random order, couldn't the apply save the policy in alphabetical order in the tfstate and then have the plan retrieve it from the aws api and then alphabetize it again to diff it thereby avoiding the reorder shown in the current diff?

@zpallin
Copy link

zpallin commented May 25, 2021

I'm experiencing a similar problem with Terraform 0.15.4 .

Same.

Using Terraform 0.15.4 and applying for a directory with 30+ ecr repository resources with policies. Those policies are having their Principals section randomly sorted by AWS IAM, but neither lifecycle ignore_changes nor terraform refresh get rid of the changes on subsequent applies, which is somewhat difficult since the output stream is very verbose and makes it harder to grok the updated changes.

@brightshine1111
Copy link

brightshine1111 commented May 25, 2021

+1 for this, seeing it in both KMS resource-based-policies and IAM resources with terraform 0.15.4.

@nitrocode nitrocode changed the title Order is lost for data aws_iam_policy_document when applied to resource aws_kms_key Order is lost for data aws_iam_policy_document when applied to resource aws_kms_key May 26, 2021
@ghost
Copy link

ghost commented May 26, 2021

This issue does not appear to be limited to aws_kms_key, it may be for assume_role_policy attributes in general. As of Terraform 0.15.4 this also occurs for the assume_role_policy attribute of aws_iam_role resources if there is >1 federated principal defined. Running terraform plan over and over on an unchanging AWS resource randomly calculates different diffs in the "Objects have changed outside of Terraform" section.

  # aws_iam_role.foo has been changed
  ~ resource "aws_iam_role" "foo" {
      ~ assume_role_policy    = jsonencode(
          ~ {
              ~ Statement = [
                  ~ {
                      ~ Principal = {
                          ~ Federated = [
                              - "arn:aws:iam::ACCOUNTID:saml-provider/saml-provider-a",
                                "arn:aws:iam::ACCOUNTID:saml-provider/saml-provider-b",
                              + "arn:aws:iam::ACCOUNTID:saml-provider/saml-provider-a",
                            ]
                        }
                        # (3 unchanged elements hidden)
                    },
                ]
                # (1 unchanged element hidden)
            }
        )
        id                    = "foo"
        name                  = "foo"
        # (7 unchanged attributes hidden)
    }

@stephank
Copy link

May not even be limited to assume_role_policy. I'm also seeing this on the policy attribute of an aws_s3_bucket. (I'm also using the jsonencode approach.)

@junaid-ali
Copy link

junaid-ali commented May 31, 2021

Same issue here with 0.15.4 for aws_launch_configuration and aws_lb_listener

@tzahari
Copy link
Contributor

tzahari commented Jun 2, 2021

Same issue here with 0.14.11and aws_codeartifact_domain_permissions_policy

@pavelpichrt
Copy link

pavelpichrt commented Jun 7, 2021

Same issue here, this makes the idea of checking the plan nearly impossible without some programmatic filtering on the client side, as this bug generates thousands of lines of redundant plan code (in our case) and everything else gets lost in the noise.

Terraform 15.5

@acook-id11
Copy link

acook-id11 commented Jun 7, 2021

Same problem here with an S3 Bucket Policy. Terraform 15.5, hashicorp/aws v3.44.0

@mjreed-wbd
Copy link

Just another "me, too". This is tehncially AWS's fault if it's reordering lists inside JSON; that's supposed to be a no-no. But since these particular lists are semantically treated as sets, probably Terraform should special-case the comparison so that it doesn't care about order.

@RLuckom
Copy link

RLuckom commented Jun 11, 2021

This issue affects 1.0 as well.

@coltaan
Copy link

coltaan commented Jul 3, 2021

It still affects 1.0.1 as well.
For me all resources (usually policies) which has any ARN list like the example below are affected:

...
        Principal = {
            AWS = [
                "arn:aws:iam::123456789012:user/user-0",
                "arn:aws:iam::123456789012:user/user-1",                            ]
          }
...

@ChristophShyper
Copy link
Contributor

Confirming it works like that for ANY policy. Suggestion about sorting principals before storing and comparing should do the trick here.

@nitrocode can you change the title so it's not misleading?

@nitrocode nitrocode changed the title Order is lost for data aws_iam_policy_document when applied to resource aws_kms_key Order is lost for data aws_iam_policy_document when applied to S3 buckets, iam roles, kms keys Jul 7, 2021
@nitrocode nitrocode changed the title Order is lost for data aws_iam_policy_document when applied to S3 buckets, iam roles, kms keys Order is lost for data aws_iam_policy_document when applied to S3 buckets, iam roles, kms keys, etc Jul 7, 2021
@Raunok87
Copy link

can confirm this same issue exists in 1.0.2 as well. Ordering is not respected and subsequent runs will throw warnings for differing "changes" each time, even though no actual changes have been made.

@jonathanio
Copy link

I don't think this is an issue with the version of Terraform, but the version of the AWS provider itself, as this is wholly within the aws_iam_policy_document. Until this is fixed here, upgrading Terraform will not help?

@eddycharly
Copy link

eddycharly commented Jul 27, 2021

Strangely enough terraform 0.14.11 shows no diff but 0.15.5 shows a diff, so it might be related to the terraform version somewhat (with the same version of the aws provider).

@jw-maynard
Copy link

We skipped 0.15 and went to 1.0 but I can confirm that on 0.14 I did not see this issue and after upgrading to 1.0 I now see this in a number of places. Sorting the lists in the terraform code does not help.

@eddycharly
Copy link

IMO the way terraform core compares sets has changed. Even though it shows a diff, in the end it says the resource is up to date, weird.

@elkh510
Copy link

elkh510 commented Nov 16, 2021

hi
any updates here?

@stawii
Copy link

stawii commented Nov 16, 2021

Wouldn't automagically sorting lists in AWS API response AND expected principals before comparing solve this? This should be easy ;)

@YakDriver
Copy link
Member

Ordering is easy but...

"Order" in the context of this issue has two caveats:

  1. We're talking about the order of a JSON array within JSON data. With aws_iam_policy_document, we have some say in the order because we're generating the JSON. But, with the plain policy argument set to jsonencode() or a heredoc (🙀), we don't do anything with the order. That's not to say we couldn't but so far we haven't crossed that bridge. Hopefully, we won't have to.
  2. AWS is the data store for the JSON data and does not preserve order. We can preserve order in state but as far as how things show up in the AWS console, we have no control. Even after an import, currently we would simply use the order as provided by AWS.

What are we solving?

I see there being two problems:

  1. avoiding perpetual diffs
  2. providing a minimal diff in the case of an actual out-of-band drift detection

Our approach will be to fix # 1 and then see where we're at with # 2. Currently the two problems are intertwined in the issues to the point they are convulting each other. Once pain points are alleviated (hopefully) with #21968, we'll need your input to determine what problems still remain.

@anthonyAdhese
Copy link

Could it be that v3.69 doesn't fix all of the issues for this issue? I still have issues with aws_s3_bucket_policy showing a diff due to ordering.

@ewbankkit
Copy link
Contributor

@anthonyAdhese Could you paste in an example of the diff that is shown? Thanks.

@anthonyAdhese
Copy link

@anthonyAdhese Could you paste in an example of the diff that is shown? Thanks.

# aws_s3_bucket_policy.customer_cloud_resources["xxx"] will be updated in-place
  ~ resource "aws_s3_bucket_policy" "customer_cloud_resources" {
        id     = "doggybites-cloud-resources-xxx"
      ~ policy = jsonencode(
            {
              - Statement = [
                  - {
                      - Action    = "s3:GetObject"
                      - Effect    = "Allow"
                      - Principal = {
                          - AWS = "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity XXX"
                        }
                      - Resource  = "arn:aws:s3:::doggybites-cloud-resources-xxx/public_html/*"
                      - Sid       = ""
                    },
                  - {
                      - Action    = "s3:ListBucket"
                      - Condition = {
                          - StringLike = {
                              - s3:prefix = "public_html/*"
                            }
                        }
                      - Effect    = "Allow"
                      - Principal = {
                          - AWS = "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity XXX"
                        }
                      - Resource  = "arn:aws:s3:::doggybites-cloud-resources-xxx"
                      - Sid       = ""
                    },
                ]
              - Version   = "2012-10-17"
            }
        ) -> (known after apply)
        # (1 unchanged attribute hidden)
    }

Never had an issue with it in the past but we've upgraded from v3.23 a few weeks ago and this is the first run since then I believe.

Debug output was this:
Expected:

{
    "Version": "2012-10-17",
    "Statement": [{
        "Sid": "",
        "Effect": "Allow",
        "Principal": {
            "AWS": "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity XXX"
        },
        "Action": "s3:GetObject",
        "Resource": "arn:aws:s3:::doggybites-cloud-resources-xxx/public_html/*"
    }, {
        "Sid": "",
        "Effect": "Allow",
        "Principal": {
            "AWS": "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity XXX"
        },
        "Action": "s3:ListBucket",
        "Resource": "arn:aws:s3:::doggybites-cloud-resources-xxx",
        "Condition": {
            "StringLike": {
                "s3:prefix": "public_html/*"
            }
        }
    }]
}

Received:

{
    "Statement": [{
        "Action": "s3:GetObject",
        "Effect": "Allow",
        "Principal": {
            "AWS": "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity XXX"
        },
        "Resource": "arn:aws:s3:::doggybites-cloud-resources-xxx/public_html/*",
        "Sid": ""
    }, {
        "Action": "s3:ListBucket",
        "Condition": {
            "StringLike": {
                "s3:prefix": "public_html/*"
            }
        },
        "Effect": "Allow",
        "Principal": {
            "AWS": "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity XXX"
        },
        "Resource": "arn:aws:s3:::doggybites-cloud-resources-xxx",
        "Sid": ""
    }],
    "Version": "2012-10-17"
}

We do generate this policy with a jsonencode operation but I gave it a go with just a normal call like this:
policy = data.aws_iam_policy_document.s3_public_html_access_policy[each.value].json
and was still the same.

@YakDriver
Copy link
Member

@anthonyAdhese We hoped to alleviate the majority of a pain points with this push. I ask that you give 3.70.0 a try when it is available. If you're still seeing issues, please open a new issue! We will need a complete config to be able to reproduce the issue.

@github-actions
Copy link

This functionality has been released in v3.70.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@ghost
Copy link

ghost commented Dec 17, 2021

The form of the defect described here #11801 (comment) is still occurring.

$ terraform --version
Terraform v1.1.1
on darwin_amd64
+ provider registry.terraform.io/hashicorp/aws v3.70.0

@yongzhang
Copy link

The form of the defect described here #11801 (comment) is still occurring.

$ terraform --version
Terraform v1.1.1
on darwin_amd64
+ provider registry.terraform.io/hashicorp/aws v3.70.0

I think if you plan the second time, you won't see the diffs.

@ghost
Copy link

ghost commented Dec 20, 2021

It still occurs on every plan for a random selection of resources even after applying changes but I'll be switching to one of the newly reported defects that cover my specific remaining symptoms as suggested by YakDriver.

Thanks all for the great work on the larger issue group, it's clearly been a lot of work!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/iam Issues and PRs that pertain to the iam service. service/kms Issues and PRs that pertain to the kms service. service/sts Issues and PRs that pertain to the sts service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.