Skip to content

Commit 1b7a384

Browse files
authored
fix(s3): buckets with SSE-KMS silently fail to receive logs (#23385)
AWS S3 Server Access Logging does not support logging to buckets that use SSE-KMS, only to buckets without default encryption or to buckets that use SSE-S3. At least in some cases, this misconfiguration can be caught within the CDK (when logging to the same bucket or when the target bucket is using a KMS CMK). This will still fail to catch scenarios where the target bucket is using SSE-KMS using a KMS-managed key because the `encryptionKey` property is not set on the Bucket in that scenario. This may be a breaking change for some users; what is currently a mostly silent misconfiguration will become an error when synthesizing. ---- ### 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*
1 parent 9cc318d commit 1b7a384

File tree

2 files changed

+33
-0
lines changed

2 files changed

+33
-0
lines changed

packages/@aws-cdk/aws-s3/lib/bucket.ts

+11
Original file line numberDiff line numberDiff line change
@@ -2044,6 +2044,17 @@ export class Bucket extends BucketBase {
20442044
if (!props.serverAccessLogsBucket && !props.serverAccessLogsPrefix) {
20452045
return undefined;
20462046
}
2047+
if (
2048+
// The current bucket is being used and is configured for default SSE-KMS
2049+
!props.serverAccessLogsBucket && (
2050+
props.encryptionKey ||
2051+
props.encryption === BucketEncryption.KMS ||
2052+
props.encryption === BucketEncryption.KMS_MANAGED) ||
2053+
// Another bucket is being used that is configured for default SSE-KMS
2054+
props.serverAccessLogsBucket?.encryptionKey
2055+
) {
2056+
throw new Error('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets');
2057+
}
20472058

20482059
return {
20492060
destinationBucketName: props.serverAccessLogsBucket?.bucketName,

packages/@aws-cdk/aws-s3/test/bucket.test.ts

+22
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,28 @@ describe('bucket', () => {
337337

338338
});
339339

340+
test('throws error if using KMS-Managed key and server access logging to self', () => {
341+
const stack = new cdk.Stack();
342+
expect(() => {
343+
new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.KMS_MANAGED, serverAccessLogsPrefix: 'test' });
344+
}).toThrow('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets');
345+
});
346+
test('throws error if using KMS CMK and server access logging to self', () => {
347+
const stack = new cdk.Stack();
348+
const key = new kms.Key(stack, 'TestKey');
349+
expect(() => {
350+
new s3.Bucket(stack, 'MyBucket', { encryptionKey: key, serverAccessLogsPrefix: 'test' });
351+
}).toThrow('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets');
352+
});
353+
test('throws error if enabling server access logging to bucket with SSE-KMS', () => {
354+
const stack = new cdk.Stack();
355+
const key = new kms.Key(stack, 'TestKey');
356+
const targetBucket = new s3.Bucket(stack, 'TargetBucket', { encryptionKey: key } );
357+
expect(() => {
358+
new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: targetBucket });
359+
}).toThrow('SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets');
360+
});
361+
340362
test('bucket with versioning turned on', () => {
341363
const stack = new cdk.Stack();
342364
new s3.Bucket(stack, 'MyBucket', {

0 commit comments

Comments
 (0)