From 62f764a9a39973033b8e29dfce69a52e7f812569 Mon Sep 17 00:00:00 2001 From: biffgaut Date: Fri, 30 Dec 2022 18:39:12 +0000 Subject: [PATCH 1/3] Issue 23513 - KMS_MANAGED log buckets not an error --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 6 +- packages/@aws-cdk/aws-s3/test/bucket.test.ts | 88 ++++++++++++++++++-- 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 36f99fcb24290..b652ea4947330 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -2046,12 +2046,12 @@ export class Bucket extends BucketBase { if (!props.serverAccessLogsBucket && !props.serverAccessLogsPrefix) { return undefined; } + if ( // The current bucket is being used and is configured for default SSE-KMS - !props.serverAccessLogsBucket && ( + (!props.serverAccessLogsBucket && ( props.encryptionKey || - props.encryption === BucketEncryption.KMS || - props.encryption === BucketEncryption.KMS_MANAGED) || + props.encryption === BucketEncryption.KMS )) || // Another bucket is being used that is configured for default SSE-KMS props.serverAccessLogsBucket?.encryptionKey ) { diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts index 1ff083281004e..ce9ee11ef90c6 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -337,26 +337,98 @@ describe('bucket', () => { }); - test('throws error if using KMS-Managed key and server access logging to self', () => { + test('logs to self, no encryption does not throw error', () => { + const stack = new cdk.Stack(); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.UNENCRYPTED, serverAccessLogsPrefix: 'test' }); + }).not.toThrowError(); + }); + + test('logs to self, S3 encryption does not throw error', () => { + const stack = new cdk.Stack(); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.S3_MANAGED, serverAccessLogsPrefix: 'test' }); + }).not.toThrowError(); + }); + + test('logs to self, KMS_MANAGED encryption does not throw error', () => { const stack = new cdk.Stack(); expect(() => { new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.KMS_MANAGED, serverAccessLogsPrefix: 'test' }); - }).toThrow('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets'); + }).not.toThrowError(); + }); + + test('logs to self, KMS encryption without key throws error', () => { + const stack = new cdk.Stack(); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.KMS, serverAccessLogsPrefix: 'test' }); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); + }); + + test('logs to self, KMS encryption with key throws error', () => { + const stack = new cdk.Stack(); + const key = new kms.Key(stack, 'TestKey'); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { encryptionKey: key, encryption: s3.BucketEncryption.KMS, serverAccessLogsPrefix: 'test' }); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); - test('throws error if using KMS CMK and server access logging to self', () => { + + test('logs to self, KMS key with no specific encryption specified throws error', () => { const stack = new cdk.Stack(); const key = new kms.Key(stack, 'TestKey'); expect(() => { new s3.Bucket(stack, 'MyBucket', { encryptionKey: key, serverAccessLogsPrefix: 'test' }); - }).toThrow('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets'); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); + }); + + test('logs to separate bucket, no encryption does not throw error', () => { + const stack = new cdk.Stack(); + const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.UNENCRYPTED }); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).not.toThrowError(); + }); + + test('logs to self, S3 encryption does not throw error', () => { + const stack = new cdk.Stack(); + const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.S3_MANAGED }); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).not.toThrowError(); + }); + + test('logs to self, KMS_MANAGED encryption does not throw error', () => { + const stack = new cdk.Stack(); + const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.KMS_MANAGED }); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).not.toThrowError(); }); - test('throws error if enabling server access logging to bucket with SSE-KMS', () => { + + test('logs to self, KMS encryption without key throws error', () => { + const stack = new cdk.Stack(); + const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.KMS }); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); + }); + + test('logs to self, KMS encryption with key throws error', () => { + const stack = new cdk.Stack(); + const key = new kms.Key(stack, 'TestKey'); + const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryptionKey: key, encryption: s3.BucketEncryption.KMS }); + expect(() => { + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); + }); + + test('logs to self, KMS key with no specific encryption specified throws error', () => { const stack = new cdk.Stack(); const key = new kms.Key(stack, 'TestKey'); - const targetBucket = new s3.Bucket(stack, 'TargetBucket', { encryptionKey: key } ); + const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryptionKey: key }); expect(() => { - new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: targetBucket }); - }).toThrow('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets'); + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); test('bucket with versioning turned on', () => { From ec7fe2833885f1528b312fc28555c6944f263eaa Mon Sep 17 00:00:00 2001 From: biffgaut Date: Fri, 30 Dec 2022 18:39:28 +0000 Subject: [PATCH 2/3] Comment fix --- packages/@aws-cdk/aws-s3/test/bucket.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts index ce9ee11ef90c6..31c771fe6a3de 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -389,7 +389,7 @@ describe('bucket', () => { }).not.toThrowError(); }); - test('logs to self, S3 encryption does not throw error', () => { + test('logs to separate bucket, S3 encryption does not throw error', () => { const stack = new cdk.Stack(); const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.S3_MANAGED }); expect(() => { @@ -397,7 +397,7 @@ describe('bucket', () => { }).not.toThrowError(); }); - test('logs to self, KMS_MANAGED encryption does not throw error', () => { + test('logs to separate bucket, KMS_MANAGED encryption does not throw error', () => { const stack = new cdk.Stack(); const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.KMS_MANAGED }); expect(() => { @@ -405,7 +405,7 @@ describe('bucket', () => { }).not.toThrowError(); }); - test('logs to self, KMS encryption without key throws error', () => { + test('logs to separate bucket, KMS encryption without key throws error', () => { const stack = new cdk.Stack(); const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.KMS }); expect(() => { @@ -413,7 +413,7 @@ describe('bucket', () => { }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); - test('logs to self, KMS encryption with key throws error', () => { + test('logs to separate bucket, KMS encryption with key throws error', () => { const stack = new cdk.Stack(); const key = new kms.Key(stack, 'TestKey'); const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryptionKey: key, encryption: s3.BucketEncryption.KMS }); @@ -422,7 +422,7 @@ describe('bucket', () => { }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); - test('logs to self, KMS key with no specific encryption specified throws error', () => { + test('logs to separate bucket, KMS key with no specific encryption specified throws error', () => { const stack = new cdk.Stack(); const key = new kms.Key(stack, 'TestKey'); const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryptionKey: key }); From cd245867f6a68533a862299c4f3a24681b080577 Mon Sep 17 00:00:00 2001 From: biffgaut Date: Thu, 5 Jan 2023 16:25:44 +0000 Subject: [PATCH 3/3] Address provided KMS_MANAGED log bucket --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 4 +++- packages/@aws-cdk/aws-s3/test/bucket.test.ts | 11 +++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index b652ea4947330..233ae85fdf892 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -2048,9 +2048,11 @@ export class Bucket extends BucketBase { } if ( - // The current bucket is being used and is configured for default SSE-KMS + // KMS can't be used for logging since the logging service can't use the key - logs don't write + // KMS_MANAGED can't be used for logging since the account can't access the logging service key - account can't read logs (!props.serverAccessLogsBucket && ( props.encryptionKey || + props.encryption === BucketEncryption.KMS_MANAGED || props.encryption === BucketEncryption.KMS )) || // Another bucket is being used that is configured for default SSE-KMS props.serverAccessLogsBucket?.encryptionKey diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts index 31c771fe6a3de..ac1f1af4aab3a 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -351,11 +351,11 @@ describe('bucket', () => { }).not.toThrowError(); }); - test('logs to self, KMS_MANAGED encryption does not throw error', () => { + test('logs to self, KMS_MANAGED encryption throws error', () => { const stack = new cdk.Stack(); expect(() => { new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.KMS_MANAGED, serverAccessLogsPrefix: 'test' }); - }).not.toThrowError(); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); test('logs to self, KMS encryption without key throws error', () => { @@ -397,12 +397,15 @@ describe('bucket', () => { }).not.toThrowError(); }); - test('logs to separate bucket, KMS_MANAGED encryption does not throw error', () => { + // When provided an external bucket (as an IBucket), we cannot detect KMS_MANAGED encryption. Since this + // check is impossible, we skip thist test. + // eslint-disable-next-line jest/no-disabled-tests + test.skip('logs to separate bucket, KMS_MANAGED encryption throws error', () => { const stack = new cdk.Stack(); const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryption: s3.BucketEncryption.KMS_MANAGED }); expect(() => { new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); - }).not.toThrowError(); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); test('logs to separate bucket, KMS encryption without key throws error', () => {