From 1e8926fa9bcf561135beaa31379ec1f1e6f79901 Mon Sep 17 00:00:00 2001 From: biffgaut <78155736+biffgaut@users.noreply.github.com> Date: Thu, 9 Feb 2023 23:15:07 -0500 Subject: [PATCH] fix(s3): logging bucket blocks KMS_MANAGED encryption (#23514) ---- closes #23513 ### All Submissions: * [ x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 10 ++- packages/@aws-cdk/aws-s3/test/bucket.test.ts | 91 ++++++++++++++++++-- 2 files changed, 89 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 2b2f930dd0f49..5bd9697cec170 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -2104,12 +2104,14 @@ 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 && ( + // 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 || - props.encryption === BucketEncryption.KMS_MANAGED) || + 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 e2a6d1238da97..cd0f962e8d703 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -338,26 +338,101 @@ 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 throws 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'); + }).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', () => { + 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 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(() => { + new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket }); + }).not.toThrowError(); + }); + + // 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 }); + }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/); }); - test('throws error if enabling server access logging to bucket with SSE-KMS', () => { + + 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(() => { + 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 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 }); + 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 separate bucket, 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', () => {