Use wildcard in KMS key policy instead of referencing Distribution to avoid circular dependency #31227
Conversation
…sControl plus unit tests
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.
| @@ -84,15 +110,8 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { | |||
| } | |||
|
|
|||
| if (bucket.encryptionKey) { | |||
| let bucketName = bucket.bucketName; | |||
| if (Token.isUnresolved(bucket.bucketName)) { | |||
There was a problem hiding this comment.
Removed this because turns out bucket.bucketName is always unresolved as it returns CFN reference to the bucket (i.e. !Ref bucket). It is set here.
| } | ||
|
|
||
| if (!bucketName) { | ||
| throw new Error(`Cannot assemble static DomainName as bucket ${bucket.node.id} has no bucketName set.`); |
There was a problem hiding this comment.
I see it's getting the bucket name using
${(bucket.node.defaultChild as CfnBucket).bucketName}
at Line 132. Do we still need to check Token.isUnresolved(bucket.bucketName)?
There was a problem hiding this comment.
Good catch! We do. Thank you.
I will cache the bucketName in the class and reuse it at Line 132.
I will change this PR to Draft as well since this approach is pending security discussion.
| @@ -165,6 +166,221 @@ describe('S3BucketOrigin', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('when using bucket with KMS Customer Managed key', () => { | |||
| it('should match expected template resource', () => { | |||
There was a problem hiding this comment.
One more unit test is needed to test escape hatch. PR to come
samson-keung
changed the title
assembleDomainName option in S3BucketOrigin.withOriginAccessControl
| }, | ||
| }, | ||
| }, | ||
| ); | ||
| Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', |
There was a problem hiding this comment.
| Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', | |
| Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicyForOac', |
| Annotations.of(scope).addInfo( | ||
| `Granting OAC permissions to access KMS key for S3 bucket origin ${bucketName} may cause a circular dependency error when this stack deploys.\n` + | ||
| `Granting OAC permissions to access KMS key for S3 bucket origin ${bucket.node.id} may cause a circular dependency error when this stack deploys.\n` + | ||
| 'The key policy references the distribution\'s id, the distribution references the bucket, and the bucket references the key.\n'+ | ||
| 'See the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README for more details.\n', | ||
| ); |
There was a problem hiding this comment.
Do we still need this info message or can we combine it with the warning about wildcard? Since with the wildcard policy they most likely will not run into the circular dependency error.
There was a problem hiding this comment.
Suggested a warning message below which combines the two.
There was a problem hiding this comment.
Agree this message should not be needed anymore. I have removed it in the latest commit. Also updated the other warning to capture the info in your suggest warning message.
| }, | ||
| }, | ||
| }, | ||
| ); | ||
| Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', | ||
| 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + |
There was a problem hiding this comment.
| 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + | |
| `A wildcard is used to grant OAC permissions for the KMS key access to the S3 bucket origin ${bucket.node.id} to resolve a circular dependency between the key policy, distribution, and bucket.\n` + | |
| 'To follow best security practices and avoid unintended access, do not rely on this default.\n' + | |
| 'Instead, you scope down the policy condition to the specific distribution. Refer to the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README for guidance.' |
|
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 |
|
Comments on closed issues and PRs are hard for our team to see. |
Making the KMS key policy to use wildcard instead on the distribution id part instead of referencing the Distribution resource to get the exact distribution ID. This breaks the circular dependency in the template.