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.

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

Replacing #34827 since I didn't follow contributing guidelines in last PR.

Issue # (if applicable)

Closes #34819

Reason for this change

CDK build allowed me to write invalid statement ids containing dashes when building policy statements, then failed on deployment. I'd like the CDK to throw an exception on build/synth time to make that feedback cycle faster.

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: 4729599
• Result: FAILED
• Build Logs (available for 30 days)

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