From b08605e9caa3cd00fa0a4c6bbdd8ecb4f2a4a00c Mon Sep 17 00:00:00 2001 From: Samson Keung Date: Mon, 26 Aug 2024 23:08:37 -0700 Subject: [PATCH 1/6] introduce assembleDomainName option in S3BucketOrigin.withOriginAccessControl plus unit tests --- .../lib/s3-bucket-origin.ts | 43 +- .../lib/s3-static-website-origin.ts | 3 +- .../test/s3-bucket-origin.test.ts | 470 +++++++++++++++++- 3 files changed, 505 insertions(+), 11 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts index d179da96f408f..013c2a3899874 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts @@ -3,7 +3,7 @@ import * as cloudfront from '../../aws-cloudfront'; import { AccessLevel } from '../../aws-cloudfront'; import * as iam from '../../aws-iam'; import { IKey } from '../../aws-kms'; -import { IBucket } from '../../aws-s3'; +import { CfnBucket, IBucket } from '../../aws-s3'; import { Annotations, Aws, DefaultTokenResolver, Names, Stack, StringConcat, Token, Tokenization } from '../../core'; const BUCKET_ACTIONS: Record = { @@ -38,6 +38,15 @@ export interface S3BucketOriginWithOACProps extends S3BucketOriginBaseProps { * @default AccessLevel.READ */ readonly originAccessLevels?: AccessLevel[]; + + /** + * This flag only works with bucket with the bucketName set. + * Setting this flag to `true` will cause the code to assemble the origin DomainName using static values (i.e. without + * referencing the bucket resource.). This provides a workaround to circular dependency error when bucket is encrypted + * with a KMS key. + * @default false + */ + readonly assembleDomainName?: boolean; } /** @@ -61,10 +70,27 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { public static withOriginAccessControl(bucket: IBucket, props?: S3BucketOriginWithOACProps): cloudfront.IOrigin { return new class extends S3BucketOrigin { private originAccessControl?: cloudfront.IOriginAccessControl; + private assembleDomainName?: boolean constructor() { super(bucket, { ...props }); this.originAccessControl = props?.originAccessControl; + this.assembleDomainName = props?.assembleDomainName ?? false; + + if (this.assembleDomainName) { + let bucketName: string | undefined = undefined; + if (!Token.isUnresolved(bucket.bucketName)) { + // this is the case when bucket is from Bucket.fromBucketName + bucketName = bucket.bucketName; + } else if (!Token.isUnresolved((bucket.node.defaultChild as CfnBucket)?.bucketName)) { + // this is the case when bucket is a L2 bucket with bucketName set + bucketName = (bucket.node.defaultChild as CfnBucket)?.bucketName; + } + + if (!bucketName) { + throw new Error(`Cannot assemble static DomainName as bucket ${bucket.node.id} has no bucketName set.`); + } + } } public bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig { @@ -84,15 +110,8 @@ 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', ); @@ -108,12 +127,18 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { const originBindConfig = this._bind(scope, options); + const bucketStack = Stack.of(bucket); + const domainName = this.assembleDomainName ? + `${(bucket.node.defaultChild as CfnBucket).bucketName}.s3.${bucketStack.region}.${bucketStack.urlSuffix}` : + bucket.bucketRegionalDomainName; + // Update configuration to set OriginControlAccessId property return { ...originBindConfig, originProperty: { ...originBindConfig.originProperty!, originAccessControlId: this.originAccessControl.originAccessControlId, + domainName, }, }; } diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-static-website-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-static-website-origin.ts index 39fa326ff97d0..a3285131d4b2f 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-static-website-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-static-website-origin.ts @@ -18,7 +18,8 @@ export interface S3StaticWebsiteOriginProps extends HttpOriginProps { export class S3StaticWebsiteOrigin extends HttpOrigin { constructor(props: S3StaticWebsiteOriginProps) { super(props.bucket.bucketWebsiteDomainName, { - protocolPolicy: cloudfront.OriginProtocolPolicy.HTTP_ONLY, // S3 only supports HTTP for website buckets + // S3 only supports HTTP for website buckets. See https://docs.aws.amazon.com/AmazonS3/latest/userguide/WebsiteEndpoints.html + protocolPolicy: cloudfront.OriginProtocolPolicy.HTTP_ONLY, ...props, }); } diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts index 088ac86511b66..a0c7b26de219c 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts @@ -1,8 +1,9 @@ import { Annotations, Template } from '../../assertions'; import * as cloudfront from '../../aws-cloudfront/index'; import * as origins from '../../aws-cloudfront-origins'; +import * as kms from '../../aws-kms'; import * as s3 from '../../aws-s3/index'; -import { App, Duration, Stack } from '../../core'; +import { App, Duration, Fn, Stack } from '../../core'; describe('S3BucketOrigin', () => { describe('withOriginAccessControl', () => { @@ -165,6 +166,473 @@ describe('S3BucketOrigin', () => { }); }); + describe('when using bucket with KMS Customer Managed key', () => { + it('should match expected template resource (template contains circular dependency)', () => { + const stack = new Stack(); + const kmsKey = new kms.Key(stack, 'myKey'); + const bucket = new s3.Bucket(stack, 'myEncryptedBucket', { + encryption: s3.BucketEncryption.KMS, + encryptionKey: kmsKey, + objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED, + }); + new cloudfront.Distribution(stack, 'MyDistributionA', { + defaultBehavior: { origin: origins.S3BucketOrigin.withOriginAccessControl(bucket) }, + }); + const template = Template.fromStack(stack, { skipCyclicalDependenciesCheck: true }); + + expect(template.toJSON().Resources).toEqual({ + myKey441A1E73: { + Type: 'AWS::KMS::Key', + Properties: { + KeyPolicy: { + Statement: [ + { + Action: 'kms:*', + Effect: 'Allow', + Principal: { + AWS: { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':iam::', + { + Ref: 'AWS::AccountId', + }, + ':root', + ], + ], + }, + }, + Resource: '*', + }, + { + Action: 'kms:Decrypt', + Condition: { + StringEquals: { + 'AWS:SourceArn': { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':cloudfront::', + { + Ref: 'AWS::AccountId', + }, + ':distribution/', + { + Ref: 'MyDistributionA2150CE0F', + }, + ], + ], + }, + }, + }, + Effect: 'Allow', + Principal: { + Service: 'cloudfront.amazonaws.com', + }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }, + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + }, + myEncryptedBucket939A51C0: { + Type: 'AWS::S3::Bucket', + Properties: { + BucketEncryption: { + ServerSideEncryptionConfiguration: [ + { + ServerSideEncryptionByDefault: { + KMSMasterKeyID: { + 'Fn::GetAtt': [ + 'myKey441A1E73', + 'Arn', + ], + }, + SSEAlgorithm: 'aws:kms', + }, + }, + ], + }, + OwnershipControls: { + Rules: [ + { + ObjectOwnership: 'BucketOwnerEnforced', + }, + ], + }, + }, + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + }, + myEncryptedBucketPolicyF516140B: { + Type: 'AWS::S3::BucketPolicy', + Properties: { + Bucket: { + Ref: 'myEncryptedBucket939A51C0', + }, + PolicyDocument: { + Statement: [ + { + Action: 's3:GetObject', + Condition: { + StringEquals: { + 'AWS:SourceArn': { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':cloudfront::', + { + Ref: 'AWS::AccountId', + }, + ':distribution/', + { + Ref: 'MyDistributionA2150CE0F', + }, + ], + ], + }, + }, + }, + Effect: 'Allow', + Principal: { + Service: 'cloudfront.amazonaws.com', + }, + Resource: { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'myEncryptedBucket939A51C0', + 'Arn', + ], + }, + '/*', + ], + ], + }, + }, + ], + Version: '2012-10-17', + }, + }, + }, + MyDistributionAOrigin1S3OriginAccessControlE2649D73: { + Type: 'AWS::CloudFront::OriginAccessControl', + Properties: { + OriginAccessControlConfig: { + Name: 'MyDistributionAOrigin1S3OriginAccessControl2859DD54', + OriginAccessControlOriginType: 's3', + SigningBehavior: 'always', + SigningProtocol: 'sigv4', + }, + }, + }, + MyDistributionA2150CE0F: { + Type: 'AWS::CloudFront::Distribution', + Properties: { + DistributionConfig: { + DefaultCacheBehavior: { + CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6', + Compress: true, + TargetOriginId: 'MyDistributionAOrigin11BE8FF8C', + ViewerProtocolPolicy: 'allow-all', + }, + Enabled: true, + HttpVersion: 'http2', + IPV6Enabled: true, + Origins: [ + { + DomainName: { + 'Fn::GetAtt': [ + 'myEncryptedBucket939A51C0', + 'RegionalDomainName', + ], + }, + Id: 'MyDistributionAOrigin11BE8FF8C', + OriginAccessControlId: { + 'Fn::GetAtt': [ + 'MyDistributionAOrigin1S3OriginAccessControlE2649D73', + 'Id', + ], + }, + S3OriginConfig: { + OriginAccessIdentity: '', + }, + }, + ], + }, + }, + }, + }); + }); + + describe('when setting assembleDomainName to true', () => { + it('should use bucketName to assemble a static DomainName for the origin', () => { + const stack = new Stack(); + const kmsKey = new kms.Key(stack, 'myKey'); + const bucketName = '7e036c5f-9bdc-43cc-a1e7-95756401dc20'; + const bucket = new s3.Bucket(stack, 'myEncryptedBucket', { + bucketName, + encryption: s3.BucketEncryption.KMS, + encryptionKey: kmsKey, + objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED, + }); + new cloudfront.Distribution(stack, 'MyDistributionA', { + defaultBehavior: { + origin: origins.S3BucketOrigin.withOriginAccessControl(bucket, { assembleDomainName: true }), + }, + }); + const template = Template.fromStack(stack); + expect(template.toJSON().Resources).toEqual({ + myKey441A1E73: { + Type: 'AWS::KMS::Key', + Properties: { + KeyPolicy: { + Statement: [ + { + Action: 'kms:*', + Effect: 'Allow', + Principal: { + AWS: { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':iam::', + { + Ref: 'AWS::AccountId', + }, + ':root', + ], + ], + }, + }, + Resource: '*', + }, + { + Action: 'kms:Decrypt', + Condition: { + StringEquals: { + 'AWS:SourceArn': { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':cloudfront::', + { + Ref: 'AWS::AccountId', + }, + ':distribution/', + { + Ref: 'MyDistributionA2150CE0F', + }, + ], + ], + }, + }, + }, + Effect: 'Allow', + Principal: { + Service: 'cloudfront.amazonaws.com', + }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }, + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + }, + myEncryptedBucket939A51C0: { + Type: 'AWS::S3::Bucket', + Properties: { + BucketEncryption: { + ServerSideEncryptionConfiguration: [ + { + ServerSideEncryptionByDefault: { + KMSMasterKeyID: { + 'Fn::GetAtt': [ + 'myKey441A1E73', + 'Arn', + ], + }, + SSEAlgorithm: 'aws:kms', + }, + }, + ], + }, + BucketName: '7e036c5f-9bdc-43cc-a1e7-95756401dc20', + OwnershipControls: { + Rules: [ + { + ObjectOwnership: 'BucketOwnerEnforced', + }, + ], + }, + }, + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + }, + myEncryptedBucketPolicyF516140B: { + Type: 'AWS::S3::BucketPolicy', + Properties: { + Bucket: { + Ref: 'myEncryptedBucket939A51C0', + }, + PolicyDocument: { + Statement: [ + { + Action: 's3:GetObject', + Condition: { + StringEquals: { + 'AWS:SourceArn': { + 'Fn::Join': [ + '', + [ + 'arn:', + { + Ref: 'AWS::Partition', + }, + ':cloudfront::', + { + Ref: 'AWS::AccountId', + }, + ':distribution/', + { + Ref: 'MyDistributionA2150CE0F', + }, + ], + ], + }, + }, + }, + Effect: 'Allow', + Principal: { + Service: 'cloudfront.amazonaws.com', + }, + Resource: { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'myEncryptedBucket939A51C0', + 'Arn', + ], + }, + '/*', + ], + ], + }, + }, + ], + Version: '2012-10-17', + }, + }, + }, + MyDistributionAOrigin1S3OriginAccessControlE2649D73: { + Type: 'AWS::CloudFront::OriginAccessControl', + Properties: { + OriginAccessControlConfig: { + Name: 'MyDistributionAOrigin1S3OriginAccessControl2859DD54', + OriginAccessControlOriginType: 's3', + SigningBehavior: 'always', + SigningProtocol: 'sigv4', + }, + }, + }, + MyDistributionA2150CE0F: { + Type: 'AWS::CloudFront::Distribution', + Properties: { + DistributionConfig: { + DefaultCacheBehavior: { + CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6', + Compress: true, + TargetOriginId: 'MyDistributionAOrigin11BE8FF8C', + ViewerProtocolPolicy: 'allow-all', + }, + Enabled: true, + HttpVersion: 'http2', + IPV6Enabled: true, + Origins: [ + { + DomainName: { + 'Fn::Join': [ + '', + [ + '7e036c5f-9bdc-43cc-a1e7-95756401dc20.s3.', + { + Ref: 'AWS::Region', + }, + '.', + { + Ref: 'AWS::URLSuffix', + }, + ], + ], + }, + Id: 'MyDistributionAOrigin11BE8FF8C', + OriginAccessControlId: { + 'Fn::GetAtt': [ + 'MyDistributionAOrigin1S3OriginAccessControlE2649D73', + 'Id', + ], + }, + S3OriginConfig: { + OriginAccessIdentity: '', + }, + }, + ], + }, + }, + }, + }); + }); + + it('should throw error if the bucket does not have a bucketName set', () => { + const bucketConstructId = 'SomeBucket'; + expect(() => { + const stack = new Stack(); + const kmsKey = new kms.Key(stack, 'myKey'); + const bucket = new s3.Bucket(stack, bucketConstructId, { + encryption: s3.BucketEncryption.KMS, + encryptionKey: kmsKey, + objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED, + }); + new cloudfront.Distribution(stack, 'MyDistributionA', { + defaultBehavior: { + origin: origins.S3BucketOrigin.withOriginAccessControl(bucket, { assembleDomainName: true }), + }, + }); + }).toThrow(`Cannot assemble static DomainName as bucket ${bucketConstructId} has no bucketName set.`); + }); + }); + }); + describe('when attaching to a multiple distribution', () => { let stack: Stack; let bucket: s3.Bucket; From f5c308c5f46c3ab313c37c71abc1ef36484447a6 Mon Sep 17 00:00:00 2001 From: Samson Keung Date: Tue, 27 Aug 2024 16:30:24 -0700 Subject: [PATCH 2/6] fix using imported bucket with assembleDomainName to true --- .../lib/s3-bucket-origin.ts | 12 ++-- .../test/s3-bucket-origin.test.ts | 71 +++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts index 013c2a3899874..cbb4ae0eca259 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts @@ -70,7 +70,8 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { public static withOriginAccessControl(bucket: IBucket, props?: S3BucketOriginWithOACProps): cloudfront.IOrigin { return new class extends S3BucketOrigin { private originAccessControl?: cloudfront.IOriginAccessControl; - private assembleDomainName?: boolean + private readonly assembleDomainName?: boolean + private readonly bucketName?: string constructor() { super(bucket, { ...props }); @@ -78,16 +79,15 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { this.assembleDomainName = props?.assembleDomainName ?? false; if (this.assembleDomainName) { - let bucketName: string | undefined = undefined; if (!Token.isUnresolved(bucket.bucketName)) { // this is the case when bucket is from Bucket.fromBucketName - bucketName = bucket.bucketName; + this.bucketName = bucket.bucketName; } else if (!Token.isUnresolved((bucket.node.defaultChild as CfnBucket)?.bucketName)) { // this is the case when bucket is a L2 bucket with bucketName set - bucketName = (bucket.node.defaultChild as CfnBucket)?.bucketName; + this.bucketName = (bucket.node.defaultChild as CfnBucket)?.bucketName; } - if (!bucketName) { + if (!this.bucketName) { throw new Error(`Cannot assemble static DomainName as bucket ${bucket.node.id} has no bucketName set.`); } } @@ -129,7 +129,7 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { const bucketStack = Stack.of(bucket); const domainName = this.assembleDomainName ? - `${(bucket.node.defaultChild as CfnBucket).bucketName}.s3.${bucketStack.region}.${bucketStack.urlSuffix}` : + `${this.bucketName}.s3.${bucketStack.region}.${bucketStack.urlSuffix}` : bucket.bucketRegionalDomainName; // Update configuration to set OriginControlAccessId property diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts index a0c7b26de219c..69fb92aee248a 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts @@ -613,6 +613,77 @@ describe('S3BucketOrigin', () => { }); }); + it('should not throw if bucket is imported with Bucket.fromBucketName', () => { + const stack = new Stack(); + const bucketName = '7e036c5f-9bdc-43cc-a1e7-95756401dc20'; + new cloudfront.Distribution(stack, 'MyDistributionA', { + defaultBehavior: { + origin: origins.S3BucketOrigin.withOriginAccessControl( + s3.Bucket.fromBucketName(stack, 'my-bucket', bucketName), { assembleDomainName: true }, + ), + }, + }); + const template = Template.fromStack(stack); + expect(template.toJSON().Resources).toEqual({ + MyDistributionAOrigin1S3OriginAccessControlE2649D73: { + Type: 'AWS::CloudFront::OriginAccessControl', + Properties: { + OriginAccessControlConfig: { + Name: 'MyDistributionAOrigin1S3OriginAccessControl2859DD54', + OriginAccessControlOriginType: 's3', + SigningBehavior: 'always', + SigningProtocol: 'sigv4', + }, + }, + }, + MyDistributionA2150CE0F: { + Type: 'AWS::CloudFront::Distribution', + Properties: { + DistributionConfig: { + DefaultCacheBehavior: { + CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6', + Compress: true, + TargetOriginId: 'MyDistributionAOrigin11BE8FF8C', + ViewerProtocolPolicy: 'allow-all', + }, + Enabled: true, + HttpVersion: 'http2', + IPV6Enabled: true, + Origins: [ + { + DomainName: { + 'Fn::Join': [ + '', + [ + '7e036c5f-9bdc-43cc-a1e7-95756401dc20.s3.', + { + Ref: 'AWS::Region', + }, + '.', + { + Ref: 'AWS::URLSuffix', + }, + ], + ], + }, + Id: 'MyDistributionAOrigin11BE8FF8C', + OriginAccessControlId: { + 'Fn::GetAtt': [ + 'MyDistributionAOrigin1S3OriginAccessControlE2649D73', + 'Id', + ], + }, + S3OriginConfig: { + OriginAccessIdentity: '', + }, + }, + ], + }, + }, + }, + }); + }); + it('should throw error if the bucket does not have a bucketName set', () => { const bucketConstructId = 'SomeBucket'; expect(() => { From 626a8b3ca034b1c9e1ecd84864145c65ce8a4592 Mon Sep 17 00:00:00 2001 From: Samson Keung Date: Wed, 28 Aug 2024 12:13:07 -0700 Subject: [PATCH 3/6] remove assembleDomainName and use wildcard key policy --- .../lib/s3-bucket-origin.ts | 44 +-- .../test/s3-bucket-origin.test.ts | 331 +----------------- 2 files changed, 10 insertions(+), 365 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts index cbb4ae0eca259..43b005c3870f1 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts @@ -3,8 +3,8 @@ import * as cloudfront from '../../aws-cloudfront'; import { AccessLevel } from '../../aws-cloudfront'; import * as iam from '../../aws-iam'; import { IKey } from '../../aws-kms'; -import { CfnBucket, IBucket } from '../../aws-s3'; -import { Annotations, Aws, DefaultTokenResolver, Names, Stack, StringConcat, Token, Tokenization } from '../../core'; +import { IBucket } from '../../aws-s3'; +import { Annotations, Aws, Names, Stack } from '../../core'; const BUCKET_ACTIONS: Record = { READ: ['s3:GetObject'], @@ -38,15 +38,6 @@ export interface S3BucketOriginWithOACProps extends S3BucketOriginBaseProps { * @default AccessLevel.READ */ readonly originAccessLevels?: AccessLevel[]; - - /** - * This flag only works with bucket with the bucketName set. - * Setting this flag to `true` will cause the code to assemble the origin DomainName using static values (i.e. without - * referencing the bucket resource.). This provides a workaround to circular dependency error when bucket is encrypted - * with a KMS key. - * @default false - */ - readonly assembleDomainName?: boolean; } /** @@ -70,27 +61,10 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { public static withOriginAccessControl(bucket: IBucket, props?: S3BucketOriginWithOACProps): cloudfront.IOrigin { return new class extends S3BucketOrigin { private originAccessControl?: cloudfront.IOriginAccessControl; - private readonly assembleDomainName?: boolean - private readonly bucketName?: string constructor() { super(bucket, { ...props }); this.originAccessControl = props?.originAccessControl; - this.assembleDomainName = props?.assembleDomainName ?? false; - - if (this.assembleDomainName) { - if (!Token.isUnresolved(bucket.bucketName)) { - // this is the case when bucket is from Bucket.fromBucketName - this.bucketName = bucket.bucketName; - } else if (!Token.isUnresolved((bucket.node.defaultChild as CfnBucket)?.bucketName)) { - // this is the case when bucket is a L2 bucket with bucketName set - this.bucketName = (bucket.node.defaultChild as CfnBucket)?.bucketName; - } - - if (!this.bucketName) { - throw new Error(`Cannot assemble static DomainName as bucket ${bucket.node.id} has no bucketName set.`); - } - } } public bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig { @@ -117,7 +91,7 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { ); 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', @@ -127,18 +101,12 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { const originBindConfig = this._bind(scope, options); - const bucketStack = Stack.of(bucket); - const domainName = this.assembleDomainName ? - `${this.bucketName}.s3.${bucketStack.region}.${bucketStack.urlSuffix}` : - bucket.bucketRegionalDomainName; - // Update configuration to set OriginControlAccessId property return { ...originBindConfig, originProperty: { ...originBindConfig.originProperty!, originAccessControlId: this.originAccessControl.originAccessControlId, - domainName, }, }; } @@ -179,7 +147,7 @@ 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, @@ -187,8 +155,8 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { 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/*`, }, }, }, diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts index 69fb92aee248a..1313b15aba5ad 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts @@ -167,7 +167,7 @@ describe('S3BucketOrigin', () => { }); describe('when using bucket with KMS Customer Managed key', () => { - it('should match expected template resource (template contains circular dependency)', () => { + it('should match expected template resource', () => { const stack = new Stack(); const kmsKey = new kms.Key(stack, 'myKey'); const bucket = new s3.Bucket(stack, 'myEncryptedBucket', { @@ -178,7 +178,7 @@ describe('S3BucketOrigin', () => { new cloudfront.Distribution(stack, 'MyDistributionA', { defaultBehavior: { origin: origins.S3BucketOrigin.withOriginAccessControl(bucket) }, }); - const template = Template.fromStack(stack, { skipCyclicalDependenciesCheck: true }); + const template = Template.fromStack(stack); expect(template.toJSON().Resources).toEqual({ myKey441A1E73: { @@ -212,7 +212,7 @@ describe('S3BucketOrigin', () => { { Action: 'kms:Decrypt', Condition: { - StringEquals: { + ArnLike: { 'AWS:SourceArn': { 'Fn::Join': [ '', @@ -225,10 +225,7 @@ describe('S3BucketOrigin', () => { { Ref: 'AWS::AccountId', }, - ':distribution/', - { - Ref: 'MyDistributionA2150CE0F', - }, + ':distribution/*', ], ], }, @@ -382,326 +379,6 @@ describe('S3BucketOrigin', () => { }, }); }); - - describe('when setting assembleDomainName to true', () => { - it('should use bucketName to assemble a static DomainName for the origin', () => { - const stack = new Stack(); - const kmsKey = new kms.Key(stack, 'myKey'); - const bucketName = '7e036c5f-9bdc-43cc-a1e7-95756401dc20'; - const bucket = new s3.Bucket(stack, 'myEncryptedBucket', { - bucketName, - encryption: s3.BucketEncryption.KMS, - encryptionKey: kmsKey, - objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED, - }); - new cloudfront.Distribution(stack, 'MyDistributionA', { - defaultBehavior: { - origin: origins.S3BucketOrigin.withOriginAccessControl(bucket, { assembleDomainName: true }), - }, - }); - const template = Template.fromStack(stack); - expect(template.toJSON().Resources).toEqual({ - myKey441A1E73: { - Type: 'AWS::KMS::Key', - Properties: { - KeyPolicy: { - Statement: [ - { - Action: 'kms:*', - Effect: 'Allow', - Principal: { - AWS: { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':iam::', - { - Ref: 'AWS::AccountId', - }, - ':root', - ], - ], - }, - }, - Resource: '*', - }, - { - Action: 'kms:Decrypt', - Condition: { - StringEquals: { - 'AWS:SourceArn': { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':cloudfront::', - { - Ref: 'AWS::AccountId', - }, - ':distribution/', - { - Ref: 'MyDistributionA2150CE0F', - }, - ], - ], - }, - }, - }, - Effect: 'Allow', - Principal: { - Service: 'cloudfront.amazonaws.com', - }, - Resource: '*', - }, - ], - Version: '2012-10-17', - }, - }, - UpdateReplacePolicy: 'Retain', - DeletionPolicy: 'Retain', - }, - myEncryptedBucket939A51C0: { - Type: 'AWS::S3::Bucket', - Properties: { - BucketEncryption: { - ServerSideEncryptionConfiguration: [ - { - ServerSideEncryptionByDefault: { - KMSMasterKeyID: { - 'Fn::GetAtt': [ - 'myKey441A1E73', - 'Arn', - ], - }, - SSEAlgorithm: 'aws:kms', - }, - }, - ], - }, - BucketName: '7e036c5f-9bdc-43cc-a1e7-95756401dc20', - OwnershipControls: { - Rules: [ - { - ObjectOwnership: 'BucketOwnerEnforced', - }, - ], - }, - }, - UpdateReplacePolicy: 'Retain', - DeletionPolicy: 'Retain', - }, - myEncryptedBucketPolicyF516140B: { - Type: 'AWS::S3::BucketPolicy', - Properties: { - Bucket: { - Ref: 'myEncryptedBucket939A51C0', - }, - PolicyDocument: { - Statement: [ - { - Action: 's3:GetObject', - Condition: { - StringEquals: { - 'AWS:SourceArn': { - 'Fn::Join': [ - '', - [ - 'arn:', - { - Ref: 'AWS::Partition', - }, - ':cloudfront::', - { - Ref: 'AWS::AccountId', - }, - ':distribution/', - { - Ref: 'MyDistributionA2150CE0F', - }, - ], - ], - }, - }, - }, - Effect: 'Allow', - Principal: { - Service: 'cloudfront.amazonaws.com', - }, - Resource: { - 'Fn::Join': [ - '', - [ - { - 'Fn::GetAtt': [ - 'myEncryptedBucket939A51C0', - 'Arn', - ], - }, - '/*', - ], - ], - }, - }, - ], - Version: '2012-10-17', - }, - }, - }, - MyDistributionAOrigin1S3OriginAccessControlE2649D73: { - Type: 'AWS::CloudFront::OriginAccessControl', - Properties: { - OriginAccessControlConfig: { - Name: 'MyDistributionAOrigin1S3OriginAccessControl2859DD54', - OriginAccessControlOriginType: 's3', - SigningBehavior: 'always', - SigningProtocol: 'sigv4', - }, - }, - }, - MyDistributionA2150CE0F: { - Type: 'AWS::CloudFront::Distribution', - Properties: { - DistributionConfig: { - DefaultCacheBehavior: { - CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6', - Compress: true, - TargetOriginId: 'MyDistributionAOrigin11BE8FF8C', - ViewerProtocolPolicy: 'allow-all', - }, - Enabled: true, - HttpVersion: 'http2', - IPV6Enabled: true, - Origins: [ - { - DomainName: { - 'Fn::Join': [ - '', - [ - '7e036c5f-9bdc-43cc-a1e7-95756401dc20.s3.', - { - Ref: 'AWS::Region', - }, - '.', - { - Ref: 'AWS::URLSuffix', - }, - ], - ], - }, - Id: 'MyDistributionAOrigin11BE8FF8C', - OriginAccessControlId: { - 'Fn::GetAtt': [ - 'MyDistributionAOrigin1S3OriginAccessControlE2649D73', - 'Id', - ], - }, - S3OriginConfig: { - OriginAccessIdentity: '', - }, - }, - ], - }, - }, - }, - }); - }); - - it('should not throw if bucket is imported with Bucket.fromBucketName', () => { - const stack = new Stack(); - const bucketName = '7e036c5f-9bdc-43cc-a1e7-95756401dc20'; - new cloudfront.Distribution(stack, 'MyDistributionA', { - defaultBehavior: { - origin: origins.S3BucketOrigin.withOriginAccessControl( - s3.Bucket.fromBucketName(stack, 'my-bucket', bucketName), { assembleDomainName: true }, - ), - }, - }); - const template = Template.fromStack(stack); - expect(template.toJSON().Resources).toEqual({ - MyDistributionAOrigin1S3OriginAccessControlE2649D73: { - Type: 'AWS::CloudFront::OriginAccessControl', - Properties: { - OriginAccessControlConfig: { - Name: 'MyDistributionAOrigin1S3OriginAccessControl2859DD54', - OriginAccessControlOriginType: 's3', - SigningBehavior: 'always', - SigningProtocol: 'sigv4', - }, - }, - }, - MyDistributionA2150CE0F: { - Type: 'AWS::CloudFront::Distribution', - Properties: { - DistributionConfig: { - DefaultCacheBehavior: { - CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6', - Compress: true, - TargetOriginId: 'MyDistributionAOrigin11BE8FF8C', - ViewerProtocolPolicy: 'allow-all', - }, - Enabled: true, - HttpVersion: 'http2', - IPV6Enabled: true, - Origins: [ - { - DomainName: { - 'Fn::Join': [ - '', - [ - '7e036c5f-9bdc-43cc-a1e7-95756401dc20.s3.', - { - Ref: 'AWS::Region', - }, - '.', - { - Ref: 'AWS::URLSuffix', - }, - ], - ], - }, - Id: 'MyDistributionAOrigin11BE8FF8C', - OriginAccessControlId: { - 'Fn::GetAtt': [ - 'MyDistributionAOrigin1S3OriginAccessControlE2649D73', - 'Id', - ], - }, - S3OriginConfig: { - OriginAccessIdentity: '', - }, - }, - ], - }, - }, - }, - }); - }); - - it('should throw error if the bucket does not have a bucketName set', () => { - const bucketConstructId = 'SomeBucket'; - expect(() => { - const stack = new Stack(); - const kmsKey = new kms.Key(stack, 'myKey'); - const bucket = new s3.Bucket(stack, bucketConstructId, { - encryption: s3.BucketEncryption.KMS, - encryptionKey: kmsKey, - objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED, - }); - new cloudfront.Distribution(stack, 'MyDistributionA', { - defaultBehavior: { - origin: origins.S3BucketOrigin.withOriginAccessControl(bucket, { assembleDomainName: true }), - }, - }); - }).toThrow(`Cannot assemble static DomainName as bucket ${bucketConstructId} has no bucketName set.`); - }); - }); }); describe('when attaching to a multiple distribution', () => { From 4a8f9c80f419d90f5f44bfc0eee1788703167696 Mon Sep 17 00:00:00 2001 From: Samson Keung Date: Wed, 28 Aug 2024 12:58:02 -0700 Subject: [PATCH 4/6] warn user about wildcard in key policy --- .../aws-cloudfront-origins/lib/s3-bucket-origin.ts | 3 +++ .../aws-cloudfront-origins/test/s3-bucket-origin.test.ts | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts index 43b005c3870f1..02a015e6bfa12 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts @@ -161,6 +161,9 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { }, }, ); + Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', + 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + + 'To further scope down the policy, see the "Using pre-existing S3 buckets" section of module\'s README.'); const result = key.addToResourcePolicy(oacKeyPolicyStatement); return result; } diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts index 1313b15aba5ad..27e8ed18cdf16 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts @@ -167,7 +167,7 @@ describe('S3BucketOrigin', () => { }); describe('when using bucket with KMS Customer Managed key', () => { - it('should match expected template resource', () => { + it('should match expected template resource and warn user about wildcard key policy', () => { const stack = new Stack(); const kmsKey = new kms.Key(stack, 'myKey'); const bucket = new s3.Bucket(stack, 'myEncryptedBucket', { @@ -378,6 +378,9 @@ describe('S3BucketOrigin', () => { }, }, }); + Annotations.fromStack(stack).hasWarning('/Default', + 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + + 'To further scope down the policy, see the "Using pre-existing S3 buckets" section of module\'s README. [ack: @aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy]'); }); }); From 9449c3c238499c476b977f920421512cb610fa5e Mon Sep 17 00:00:00 2001 From: Samson Keung Date: Wed, 28 Aug 2024 12:59:40 -0700 Subject: [PATCH 5/6] warning wording update --- .../aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts | 2 +- .../aws-cloudfront-origins/test/s3-bucket-origin.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts index 02a015e6bfa12..95d47f4f2bd55 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts @@ -163,7 +163,7 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { ); Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + - 'To further scope down the policy, see the "Using pre-existing S3 buckets" section of module\'s README.'); + '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; } diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts index 27e8ed18cdf16..f06ece81fb772 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts @@ -380,7 +380,7 @@ describe('S3BucketOrigin', () => { }); Annotations.fromStack(stack).hasWarning('/Default', 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + - 'To further scope down the policy, see the "Using pre-existing S3 buckets" section of module\'s README. [ack: @aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy]'); + 'To further scope down the policy, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README. [ack: @aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy]'); }); }); From dd11f459b91375839210f876ea2706f16e9325aa Mon Sep 17 00:00:00 2001 From: Samson Keung Date: Wed, 28 Aug 2024 14:25:54 -0700 Subject: [PATCH 6/6] warning wording update and removed redundant warning --- .../aws-cloudfront-origins/lib/s3-bucket-origin.ts | 13 ++++--------- .../test/s3-bucket-origin.test.ts | 5 +++-- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts index 95d47f4f2bd55..dd8690c7d1e2e 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts @@ -84,12 +84,6 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { } if (bucket.encryptionKey) { - Annotations.of(scope).addInfo( - `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', - ); - const keyPolicyActions = this.getKeyPolicyActions(props?.originAccessLevels ?? [cloudfront.AccessLevel.READ]); const keyPolicyResult = this.grantDistributionAccessToKey(keyPolicyActions, bucket.encryptionKey); // Failed to update key policy, assume using imported key @@ -161,9 +155,10 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase { }, }, ); - Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy', - 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + - 'To further scope down the policy, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README.'); + Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicyForOac', + 'To avoid circular dependency between the KMS key, Bucket, and Distribution,' + + 'a wildcard is used to match all Distribution IDs in Key policy condition.\n' + + 'To further scope down the policy for best security practices, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README.'); const result = key.addToResourcePolicy(oacKeyPolicyStatement); return result; } diff --git a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts index f06ece81fb772..830835b9e07ee 100644 --- a/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts +++ b/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts @@ -379,8 +379,9 @@ describe('S3BucketOrigin', () => { }, }); Annotations.fromStack(stack).hasWarning('/Default', - 'Using wildcard to match all Distribution IDs in Key policy condition.\n' + - 'To further scope down the policy, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README. [ack: @aws-cdk/aws-cloudfront-origins:wildcardKeyPolicy]'); + 'To avoid circular dependency between the KMS key, Bucket, and Distribution,' + + 'a wildcard is used to match all Distribution IDs in Key policy condition.\n' + + 'To further scope down the policy for best security practices, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README. [ack: @aws-cdk/aws-cloudfront-origins:wildcardKeyPolicyForOac]'); }); });