feat(iam): validate AccountPrincipal argument to be exactly 12 digits #31324
Conversation
github-actions
bot
added
the
beginning-contributor
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Exemption Request |
aws-cdk-automation
added
the
pr-linter/exemption-requested
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
aws-cdk-automation
added
the
pr/needs-community-review
duranbe
force-pushed
the
fix-iam-account-number
branch
from
626cd74
to
abdac1b
Compare
duranbe
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
rix0rrr
changed the title
| AcmCertificateArn: 'arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012', | ||
| AcmCertificateArn: 'arn:aws:acm:us-east-1:123456789012:certificate/12345678-123456789012-123456789012-123456789012-123456789012', |
There was a problem hiding this comment.
The 1234 here was not an account ID, so I'm not sure why it is file is changed.
Did you do a mass search-replace over the entire codebase?
| expect(_params).toEqual({ GroupIds: ['sg-1234'] }); | ||
| expect(_params).toEqual({ GroupIds: ['sg-123456789012'] }); |
There was a problem hiding this comment.
Security group IDs are also not account IDs.
| if (!cdk.Token.isUnresolved(accountId) && !this.accountIdRegExp.test(accountId)) { | ||
| throw new Error('accountId should be composed of 12 digits'); | ||
| } |
There was a problem hiding this comment.
I wonder if there might be use cases where people are passing valid strings that aren't exactly 12 digits in length.
I'd feel more comfortable making this a warning, rather than an exception.
rix0rrr
changed the title
aws-cdk-automation
removed
the
pr/needs-community-review
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing ✅ A exemption request has been requested. Please wait for a maintainer's review. |
Reason for this change
There is no validation and test that the AWS Account Id when creating AccountPrincipal Object. Thus, according to cdk
1234is a valid AWS Account id.Context :
In my case I missed a digit when copy pasting an account id and the build still passed, the typo has been caught only during the code review process.
Description of changes
Adding simple regex to check that AWS Id is 12 digits long & update error message
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license