From 050ee57c31c69a999c002a202fe1b8267d36317f Mon Sep 17 00:00:00 2001 From: Jonathan Ifegunni Date: Wed, 7 Feb 2024 11:55:02 -0800 Subject: [PATCH 1/4] Cross account bucket access --- .../aws-lambda-event-sources/test/s3.test.ts | 25 +++++++++++++++++++ .../aws-s3-notifications/lib/lambda.ts | 2 +- packages/aws-cdk-lib/aws-s3/lib/bucket.ts | 20 +++++++++------ 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts index f5ab8a7fc4bb3..de5692a3b8e72 100644 --- a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts +++ b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts @@ -226,6 +226,31 @@ describe('S3EventSource', () => { }, ], }, + test('Cross account buckect access', () => { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'stack'); + const fn = new TestFunction(stack, 'Fn'); + + let accountB = '1234567'; + //WHEN + const foreignBucket = + s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', { + bucketArn: 'arn:aws:s3:::some-bucket-not-in-this-account', + // The account the bucket really lives in + account: accountB, + }); + + // This will generate the IAM bindings + fn.addEventSource(new sources.S3EventSource(foreignBucket as s3.Bucket, + { events: [s3.EventType.OBJECT_CREATED] })); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', { + 'Principal': 's3.amazonaws.com', + 'SourceAccount': '1234567', + 'SourceArn': 'arn:aws:s3:::some-bucket-not-in-this-account', }); }); }); + diff --git a/packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.ts b/packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.ts index e6159ff8c22aa..d7e9758eeecb8 100644 --- a/packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.ts +++ b/packages/aws-cdk-lib/aws-s3-notifications/lib/lambda.ts @@ -21,7 +21,7 @@ export class LambdaDestination implements s3.IBucketNotificationDestination { if (bucket.node.tryFindChild(permissionId) === undefined) { this.fn.addPermission(permissionId, { - sourceAccount: Stack.of(bucket).account, + sourceAccount: bucket.account || Stack.of(bucket).account, principal: new iam.ServicePrincipal('s3.amazonaws.com'), sourceArn: bucket.bucketArn, // Placing the permissions node in the same scope as the s3 bucket. diff --git a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts index a799062afc692..d1d1c255f3513 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts +++ b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts @@ -95,6 +95,11 @@ export interface IBucket extends IResource { */ policy?: BucketPolicy; + /** + * The account bucket belongs to. + */ + readonly account?: string; + /** * Adds a statement to the resource policy for a principal (i.e. * account/role/service) to perform actions on this bucket and/or its @@ -1754,6 +1759,7 @@ export class Bucket extends BucketBase { public readonly bucketWebsiteNewUrlFormat = attrs.bucketWebsiteNewUrlFormat ?? false; public readonly encryptionKey = attrs.encryptionKey; public readonly isWebsite = attrs.isWebsite ?? false; + public readonly account = attrs.account; public policy?: BucketPolicy = undefined; protected autoCreatePolicy = false; protected disallowPublicAccess = false; @@ -1967,11 +1973,11 @@ export class Bucket extends BucketBase { if (props.serverAccessLogsBucket instanceof Bucket) { props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix); - // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket` - // that cannot have the ACLs or bucket policy applied. In that scenario, we should only - // setup log delivery permissions to `this` if a bucket was not specified at all, as documented. - // For example, we should not allow log delivery to `this` if given an imported bucket or - // another situation that causes `instanceof` to fail + // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket` + // that cannot have the ACLs or bucket policy applied. In that scenario, we should only + // setup log delivery permissions to `this` if a bucket was not specified at all, as documented. + // For example, we should not allow log delivery to `this` if given an imported bucket or + // another situation that causes `instanceof` to fail } else if (!props.serverAccessLogsBucket && props.serverAccessLogsPrefix) { this.allowLogDelivery(this, props.serverAccessLogsPrefix); } else if (props.serverAccessLogsBucket) { @@ -2225,7 +2231,7 @@ export class Bucket extends BucketBase { function parseLifecycleRule(rule: LifecycleRule): CfnBucket.RuleProperty { const enabled = rule.enabled ?? true; if ((rule.expiredObjectDeleteMarker) - && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) { + && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) { // ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters. throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.'); } @@ -2350,7 +2356,7 @@ export class Bucket extends BucketBase { } if (accessControlRequiresObjectOwnership && this.objectOwnership === ObjectOwnership.BUCKET_OWNER_ENFORCED) { - throw new Error (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`); + throw new Error(`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`); } return { From 240e8f8ac7c9829faaf269e9fcc98ef89622b1b6 Mon Sep 17 00:00:00 2001 From: Jonathan Ifegunni Date: Wed, 7 Feb 2024 11:55:02 -0800 Subject: [PATCH 2/4] Added more test --- .../aws-lambda-event-sources/test/s3.test.ts | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts index de5692a3b8e72..9299ba3fbd01c 100644 --- a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts +++ b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts @@ -252,5 +252,60 @@ describe('S3EventSource', () => { 'SourceArn': 'arn:aws:s3:::some-bucket-not-in-this-account', }); }); -}); + test('Test bucket account is referenced intrinsicly', () => { + // GIVEN + const stack = new cdk.Stack(); + const fn = new TestFunction(stack, 'Fn'); + const bucket = new s3.Bucket(stack, 'B'); + + // WHEN + fn.addEventSource(new sources.S3EventSource(bucket, { + events: [s3.EventType.OBJECT_CREATED, s3.EventType.OBJECT_REMOVED], + filters: [ + { prefix: 'prefix/' }, + { suffix: '.png' }, + ], + })); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', { + 'Principal': 's3.amazonaws.com', + 'SourceAccount': { + 'Ref': 'AWS::AccountId', + }, + 'SourceArn': { + 'Fn::GetAtt': ['B08E7C7AF', 'Arn'], + }, + }); + }); + + test('Default to stack account if bucket account doesnt exist', () => { + // GIVEN + const app = new cdk.App(); + const stack = new cdk.Stack(app, 'stack'); + const fn = new TestFunction(stack, 'Fn'); + + let accountB = ''; + //WHEN + const foreignBucket = + s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', { + bucketArn: 'arn:aws:s3:::some-bucket-not-in-this-account', + // The account the bucket really lives in + account: accountB, + }); + + // This will generate the IAM bindings + fn.addEventSource(new sources.S3EventSource(foreignBucket as s3.Bucket, + { events: [s3.EventType.OBJECT_CREATED] })); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', { + 'Principal': 's3.amazonaws.com', + 'SourceAccount': { + 'Ref': 'AWS::AccountId', + }, + 'SourceArn': 'arn:aws:s3:::some-bucket-not-in-this-account', + }); + }); +}); From 7308698f2c8bbbe0e24bdf2d2d64d319032814b5 Mon Sep 17 00:00:00 2001 From: Jonathan Ifegunni Date: Fri, 9 Feb 2024 11:36:32 -0800 Subject: [PATCH 3/4] Fix lint issues --- packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts index 9299ba3fbd01c..67b63190e315f 100644 --- a/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts +++ b/packages/aws-cdk-lib/aws-lambda-event-sources/test/s3.test.ts @@ -226,6 +226,8 @@ describe('S3EventSource', () => { }, ], }, + }); + }); test('Cross account buckect access', () => { // GIVEN const app = new cdk.App(); From 609f8ce451e355c30d64400d2e0db2df8e160774 Mon Sep 17 00:00:00 2001 From: Jonathan Ifegunni Date: Mon, 26 Feb 2024 13:49:58 -0800 Subject: [PATCH 4/4] remove all unnecessary spaces --- packages/aws-cdk-lib/aws-s3/lib/bucket.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts index d1d1c255f3513..0c9f330163511 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts +++ b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts @@ -1973,11 +1973,11 @@ export class Bucket extends BucketBase { if (props.serverAccessLogsBucket instanceof Bucket) { props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix); - // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket` - // that cannot have the ACLs or bucket policy applied. In that scenario, we should only - // setup log delivery permissions to `this` if a bucket was not specified at all, as documented. - // For example, we should not allow log delivery to `this` if given an imported bucket or - // another situation that causes `instanceof` to fail + // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket` + // that cannot have the ACLs or bucket policy applied. In that scenario, we should only + // setup log delivery permissions to `this` if a bucket was not specified at all, as documented. + // For example, we should not allow log delivery to `this` if given an imported bucket or + // another situation that causes `instanceof` to fail } else if (!props.serverAccessLogsBucket && props.serverAccessLogsPrefix) { this.allowLogDelivery(this, props.serverAccessLogsPrefix); } else if (props.serverAccessLogsBucket) { @@ -2231,7 +2231,7 @@ export class Bucket extends BucketBase { function parseLifecycleRule(rule: LifecycleRule): CfnBucket.RuleProperty { const enabled = rule.enabled ?? true; if ((rule.expiredObjectDeleteMarker) - && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) { + && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) { // ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters. throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.'); } @@ -2356,7 +2356,7 @@ export class Bucket extends BucketBase { } if (accessControlRequiresObjectOwnership && this.objectOwnership === ObjectOwnership.BUCKET_OWNER_ENFORCED) { - throw new Error(`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`); + throw new Error (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`); } return {