-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Use wildcard in KMS key policy instead of referencing Distribution to avoid circular dependency #31227
Use wildcard in KMS key policy instead of referencing Distribution to avoid circular dependency #31227
Changes from 5 commits
b08605e
f5c308c
626a8b3
4a8f9c8
9449c3c
dd11f45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,7 +4,7 @@ import { AccessLevel } from '../../aws-cloudfront'; | |||||||||
import * as iam from '../../aws-iam'; | ||||||||||
import { IKey } from '../../aws-kms'; | ||||||||||
import { IBucket } from '../../aws-s3'; | ||||||||||
import { Annotations, Aws, DefaultTokenResolver, Names, Stack, StringConcat, Token, Tokenization } from '../../core'; | ||||||||||
import { Annotations, Aws, Names, Stack } from '../../core'; | ||||||||||
|
||||||||||
const BUCKET_ACTIONS: Record<string, string[]> = { | ||||||||||
READ: ['s3:GetObject'], | ||||||||||
|
@@ -84,21 +84,14 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { | |||||||||
} | ||||||||||
|
||||||||||
if (bucket.encryptionKey) { | ||||||||||
let bucketName = bucket.bucketName; | ||||||||||
if (Token.isUnresolved(bucket.bucketName)) { | ||||||||||
bucketName = JSON.stringify(Tokenization.resolve(bucket.bucketName, { | ||||||||||
scope, | ||||||||||
resolver: new DefaultTokenResolver(new StringConcat()), | ||||||||||
})); | ||||||||||
} | ||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggested a warning message below which combines the two. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||
|
||||||||||
const keyPolicyActions = this.getKeyPolicyActions(props?.originAccessLevels ?? [cloudfront.AccessLevel.READ]); | ||||||||||
const keyPolicyResult = this.grantDistributionAccessToKey(distributionId!, keyPolicyActions, bucket.encryptionKey); | ||||||||||
const keyPolicyResult = this.grantDistributionAccessToKey(keyPolicyActions, bucket.encryptionKey); | ||||||||||
// Failed to update key policy, assume using imported key | ||||||||||
if (!keyPolicyResult.statementAdded) { | ||||||||||
Annotations.of(scope).addWarningV2('@aws-cdk/aws-cloudfront-origins:updateImportedKeyPolicy', | ||||||||||
|
@@ -154,20 +147,23 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { | |||||||||
return result; | ||||||||||
} | ||||||||||
|
||||||||||
grantDistributionAccessToKey(distributionId: string, actions: string[], key: IKey): iam.AddToResourcePolicyResult { | ||||||||||
grantDistributionAccessToKey(actions: string[], key: IKey): iam.AddToResourcePolicyResult { | ||||||||||
const oacKeyPolicyStatement = new iam.PolicyStatement( | ||||||||||
{ | ||||||||||
effect: iam.Effect.ALLOW, | ||||||||||
principals: [new iam.ServicePrincipal('cloudfront.amazonaws.com')], | ||||||||||
actions, | ||||||||||
resources: ['*'], | ||||||||||
conditions: { | ||||||||||
StringEquals: { | ||||||||||
'AWS:SourceArn': `arn:${Aws.PARTITION}:cloudfront::${Aws.ACCOUNT_ID}:distribution/${distributionId}`, | ||||||||||
ArnLike: { | ||||||||||
'AWS:SourceArn': `arn:${Aws.PARTITION}:cloudfront::${Aws.ACCOUNT_ID}:distribution/*`, | ||||||||||
}, | ||||||||||
}, | ||||||||||
}, | ||||||||||
); | ||||||||||
Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
'Using wildcard to match all Distribution IDs in Key policy condition.\n' + | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
'To further scope down the policy, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README.'); | ||||||||||
const result = key.addToResourcePolicy(oacKeyPolicyStatement); | ||||||||||
return result; | ||||||||||
} | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.