fix(iam): throw error when statement id is invalid (#34819)

[camerondurham]

Resolves #34819 by throwing error when PolicyStatement Statement id contains characters not allowed by the API.

Reference: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_sid.html

The Sid element supports ASCII uppercase letters (A-Z), lowercase letters (a-z), and numbers (0-9).

Issue # (if applicable)

Closes #34819

Reason for this change

CDK build allows users to write invalid statement ids containing dashes when building policy statements, then failed on deployment. To reduce time to a successful deployment, I believe the CDK should throw an exception at build time to help user make a fix locally before a failed deployment.

Description of changes

Validate that statement id matches requirements specified in https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_sid.html in the constructor.

Add tests that verify only valid statement ids are set in constructor.

Describe any new or updated permissions being added

n/a

Description of how you validated changes

yes, added unit tests

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

(This review is outdated)

[camerondurham]

Looks like there are existing invalid SIDs defined (DefaultSid-1, etc). Will go and fix them and revise this PR.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[camerondurham]

Current failure is due to replacing a policy within statements:

@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1576825816/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/integ.cluster-encrypt-ephemeral-storage.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1576825816/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.eventbus-cross-account-grants.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1576825816/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.eventbus.js
@aws-cdk-testing/framework-integ: !!! This test contains destructive changes !!!
@aws-cdk-testing/framework-integ:     Stack: Stack - Resource: BuscdkStatement1D7D87B9D - Impact: WILL_DESTROY
@aws-cdk-testing/framework-integ:     Stack: Stack - Resource: BuscdkStatement2341A5B58 - Impact: WILL_DESTROY
@aws-cdk-testing/framework-integ: !!! If these destructive changes are necessary, please indicate this on the PR !!!
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1576825816/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-iam/test/integ.policy-statement-sid.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src1576825816/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-logs/test/integ.log-group.js
@aws-cdk-testing/framework-integ: Error: Some changes were destructive!
@aws-cdk-testing/framework-integ:     at main (/codebuild/output/src1576825816/src/github.com/aws/aws-cdk/node_modules/@aws-cdk/integ-runner/lib/index.js:10274:11)
@aws-cdk-testing/framework-integ: Error: integ-runner exited with error code 1

However, the current CFN stack seems invalid now, with the current Sid containing invalid characters that would be rejected on deployment (https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_sid.html)

The Sid element supports ASCII uppercase letters (A-Z), lowercase letters (a-z), and numbers (0-9).

@aws-cdk-testing/framework-integ: [~] AWS::Logs::LogGroup LogGroupLambdaAC756C5B
@aws-cdk-testing/framework-integ:  └─ [~] DataProtectionPolicy
@aws-cdk-testing/framework-integ:      └─ [~] .Statement:
@aws-cdk-testing/framework-integ:          └─ @@ -1,6 +1,6 @@
@aws-cdk-testing/framework-integ:             [ ] [
@aws-cdk-testing/framework-integ:             [ ]   {
@aws-cdk-testing/framework-integ:             [-]     "Sid": "audit-statement-cdk",
@aws-cdk-testing/framework-integ:             [+]     "Sid": "auditStatementCdk",
@aws-cdk-testing/framework-integ:             [ ]     "DataIdentifier": [

Next Steps

Plan to fix this build error

• add skipValidation to PolicyStatement and update integ tests to avoid renaming existing Sid's, and add comment explaining why this had to be done

There appear to be other examples of using this approach in the CDK repo (see)

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[camerondurham]

I'd prefer to not make these changes and use the "default" skipValidation:false when using the PolicyStatement constructor. However, when I don't it causes build to fail due to destructive CDK changes during the integ test. So this is a workaround until I can get some confirmation whether the destructive integ test changes are "acceptable" or not.

For reference what I'm talking about, see: #34828 (comment)

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 411bbd2
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[pahud]

Hi,

This PR has conflicts that need to be resolved before it can be reviewed.

[aws-cdk-automation]

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

• Resume work on the PR
• OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
• OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 14 days if no action is taken.

[camerondurham]

I've adjusted this PR in #36150 to instead use a feature flag to gate the changes since they broke several snapshot tests. The linked CR does still need approval for gating.

Open to feedback on which approach is preferred.

[vishaalmehrishi]

I've adjusted this PR in #36150 to instead use a feature flag to gate the changes since they broke several snapshot tests. The linked CR does still need approval for gating.

Open to feedback on which approach is preferred.

@camerondurham Thank you for your contribution! #36150 is the better approach. I have provided some feedback on that one. Closing this PR.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.