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

[Bug]: SID validation at plan time is invalidly applied to ALL policies #40639

Closed
JacobAllenAbkes opened this issue Dec 19, 2024 · 16 comments · Fixed by #40640
Closed

[Bug]: SID validation at plan time is invalidly applied to ALL policies #40639

JacobAllenAbkes opened this issue Dec 19, 2024 · 16 comments · Fixed by #40640
Labels
bug Addresses a defect in current functionality. regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. service/iam Issues and PRs that pertain to the iam service. service/kms Issues and PRs that pertain to the kms service.
Milestone

Comments

@JacobAllenAbkes
Copy link

JacobAllenAbkes commented Dec 19, 2024

Terraform Core Version

1.7

AWS Provider Version

5.82

Affected Resource(s)

aws_iam_policy_document

Expected Behavior

We should not blanket apply plan time validation against the SID because each service may have it's own policies on allowed characters. This is a breaking change.

Actual Behavior

We blanket apply the plan time validation against the SID.

Relevant Error/Panic Output Snippet

No response

Terraform Configuration Files

resource "aws_kms_key" "name" {
  description         = "something"
  policy              = data.aws_iam_policy_document.name.json
}


data "aws_iam_policy_document" "name" {
  version                 = "2012-10-17"

  statement {
    sid    = "Allow spaces"
    effect = "Allow"
    actions = [
      "kms:Decrypt"
    ]
    resources = ["*"]
  }
}

Steps to Reproduce

Create an IAM policy document resource, have the SID include spaces

Attach the policy as a KMS policy

Debug Output

│ with aws_iam_policy_document.name,
│ on file line X, in data "aws_iam_policy_document" "name":
│ line#: sid = "Allow spaces"

Panic Output

No response

Important Factoids

No response

References

https://github.com/hashicorp/terraform-provider-aws/blob/main/CHANGELOG.md

https://docs.aws.amazon.com/kms/latest/developerguide/key-policy-overview.html

Would you like to implement a fix?

No

@JacobAllenAbkes JacobAllenAbkes added the bug Addresses a defect in current functionality. label Dec 19, 2024
Copy link

Community Note

Voting for Prioritization

  • Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
  • Please see our prioritization guide for information on how we prioritize.
  • 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.

Volunteering to Work on This Issue

  • If you are interested in working on this issue, please leave a comment.
  • If this would be your first contribution, please review the contribution guide.

@github-actions github-actions bot added service/iam Issues and PRs that pertain to the iam service. service/kms Issues and PRs that pertain to the kms service. needs-triage Waiting for first response or review from a maintainer. labels Dec 19, 2024
@faleon
Copy link

faleon commented Dec 19, 2024

Who let the intern push to production?

@asvinours
Copy link
Contributor

  │ Error: invalid value for statement.0.sid (must only include alphanumeric characters)
  │ 
  │   with data.aws_iam_policy_document.elb_access_logs_delivery,
  │   on load_balancer.tf line 70, in data "aws_iam_policy_document" "elb_access_logs_delivery":
  │   70:     sid       = "ELBNewerRegions"

Not too sure why this is considered invalid 🤔

@faleon
Copy link

faleon commented Dec 19, 2024

@asvinours react with a thumbs up to OP please to help get this escalated - #40639 (comment)

@ewbankkit
Copy link
Contributor

Relates #40562.

@ewbankkit ewbankkit added regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. and removed needs-triage Waiting for first response or review from a maintainer. labels Dec 19, 2024
@tobiasbueschel
Copy link

tobiasbueschel commented Dec 19, 2024

We've encountered the same issue just now, which blocked our deployments as we have some older SIDs with non-alphanumeric names.

For now, we're using the following workaround, hope this helps anyone who might face the same issue:

terraform {
  required_version = ">= 1.0"

  required_providers {
    aws = {
      source = "hashicorp/aws"
      # TODO: this is temporary to fix the following error, we should eventually fix our SID to make them alphanumeric 
      # https://github.com/hashicorp/terraform-provider-aws/issues/40639
      # https://github.com/hashicorp/terraform-provider-aws/pull/40562
      version = "5.81.0"
    }
  }
}

@terraform-aws-provider terraform-aws-provider bot added the prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. label Dec 19, 2024
@DrStrangepork
Copy link

DrStrangepork commented Dec 19, 2024

AWS Key Management Service documentation, Creating a key policy, Key policy format:

A key policy document must conform to the following rules:

  • Up to 32 kilobytes (32,768 bytes)
  • The Sid element in a key policy statement can include spaces. (Spaces are prohibited in the Sid element of an IAM policy document.)

KMS policy Sids can include spaces, and there are dozens of examples of KMS policy Sids with spaces throughout AWS documentation.

@JacobAllenAbkes
Copy link
Author

We've encountered the same issue just now, which blocked our deployments as we have some older SIDs with non-alphanumeric names.

For now, we're using the following workaround, hope this helps anyone who might face the same issue:

terraform {
  required_version = ">= 1.0"

  required_providers {
    aws = {
      source = "hashicorp/aws"
      # TODO: this is temporary to fix the following error, we should eventually fix our SID to make them alphanumeric 
      # https://github.com/hashicorp/terraform-provider-aws/issues/40639
      # https://github.com/hashicorp/terraform-provider-aws/pull/40562
      version = "5.81.0"
    }
  }
}

This is a temporary workaround for active releases, this issue will continue to affect clients which deploy tagged old releases with ~>5.x.

@YakDriver
Copy link
Member

My apologies for the inconvenience with this. We used the regex AWS provides for the SID but that does not seem to match with the reality of usage. We will work on a fix for this right away!

@DerekTBrown
Copy link
Contributor

I put together a PR to revert this change: #40640

Landing + tagging a new minor version should mitigate issues users pinned to a major version are seeing.

@allenschein
Copy link

I already started renaming stuff, only to realize how many more I have to change, and the stress that this causes on what was supposed to be a chill morning before the holidays... should I just wait for the provider to get updated? My team is waiting to hear back on this hot fix going out...

@JacobAllenAbkes
Copy link
Author

JacobAllenAbkes commented Dec 19, 2024

I already started renaming stuff, only to realize how many more I have to change, and the stress that this causes on what was supposed to be a chill morning before the holidays... should I just wait for the provider to get updated? My team is waiting to hear back on this hot fix going out...

Just set the AWS provider version to exactly 5.81.0. See the above comment #40639 (comment)

@allenschein
Copy link

thanks, that makes sense... sorry for the vent, this was so unexpected

@YakDriver
Copy link
Member

We used the regex that AWS provides for valid IAM Sids but, obviously, we're seeing how much this is used outside of IAM. Again our apologies for the problems this is causing! We will have a fix soon.

Copy link

Warning

This issue has been closed, meaning that any additional comments are hard for our team to see. Please assume that the maintainers will not see them.

Ongoing conversations amongst community members are welcome, however, the issue will be locked after 30 days. Moving conversations to another venue, such as the AWS Provider forum, is recommended. If you have additional concerns, please open a new issue, referencing this one where needed.

@github-actions github-actions bot added this to the v5.82.1 milestone Dec 19, 2024
@github-actions github-actions bot removed the prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. label Dec 20, 2024
Copy link

This functionality has been released in v5.82.1 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. service/iam Issues and PRs that pertain to the iam service. service/kms Issues and PRs that pertain to the kms service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants