fix(s3): KMS encryption works fine for server access logging target buckets #24789

zdf1230 · 2023-03-26T06:12:43Z

KMS encryption works fine for server access logging target buckets with proper permission being setup

There were 2 changes wants to solve the issue that buckets with SSE-KMS silently fail to receive logs.
#23514 & #23385

The changes would let it fail early in build time when SSE-KMS being set for server access logging bucket. But it actually works when user use SSE-KMS with customized encryption key. Just need to add KMS Read Write policy for logging.s3.amazonaws.com to the key and add S3 Put Object permission for logging.s3.amazonaws.com.

So this change is removing the condition failing for the SSE-KMS with customized encryption key case.

However, it is not possible to know which encryption type for the server access logging bucket, so the only checking can be applied after this change merged is failing when logging to self case using BucketEncryption.KMS_MANAGED.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

zdf1230 · 2023-03-26T06:28:26Z

Exemption Request

Integration tests succeeded and this change won't change snapshot.

TheRealAmazonKendra

Thank you for your contribution. Please see my comments inline.

TheRealAmazonKendra · 2023-03-27T19:02:31Z

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

+    }).not.toThrowError();
  });

-  test('logs to self, KMS encryption with key throws error', () => {
+  test('logs to self, KMS encryption with key does nor throw 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/);
+    }).not.toThrowError();
  });

-  test('logs to self, KMS key with no specific encryption specified throws error', () => {
+  test('logs to self, KMS key with no specific encryption specified does not throw 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/);
+    }).not.toThrowError();


These should test against the expected template output, not just that is doesn't throw an error.

TheRealAmazonKendra · 2023-03-27T19:04:25Z

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

+    if (!props.serverAccessLogsBucket && props.encryption === BucketEncryption.KMS_MANAGED) {
+      throw new Error('Default bucket encryption with KMS managed key is not supported for Server Access Logging target buckets');


If this is the case, we should have integ tests where encryption is set to BucketEncryption.KMS with an encryptionKey and without an encryptionKey set.

The case you talked about is checked under function parseEncryption.

// if encryption key is set, encryption must be set to KMS. if (encryptionType !== BucketEncryption.KMS && props.encryptionKey) { throw new Error(`encryptionKey is specified, so 'encryption' must be set to KMS (value: ${encryptionType})`); }

The change I modified here is mean to fail the build when the server access logs bucket being set as itself and the bucket encryption is set to KMS_MANAGED. In this case, though the resource can be successfully deployed, we cannot modified the encryption key owned by KMS. The key and the bucket need proper permission to make the whole logging workflow work. So, people won't able to get access log from the bucket or the log would be unreadable.

This change won't affect snapshot at all.

TheRealAmazonKendra · 2023-03-27T19:05:29Z

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

    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/);
+    }).not.toThrowError();


These should test against the expected template output, not just that is doesn't throw an error.

The original logic would throw the error for the case which should not be. And none of these changes are affecting template output. They just mean to fail the build earlier to prevent some non-working situation.

My change is just remove some failed checking condition which is actually working case.

As my change won't affect generating template output, I don't think there is anything I can do there.

TheRealAmazonKendra · 2023-03-27T19:05:33Z

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

    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/);
+    }).not.toThrowError();


These should test against the expected template output, not just that is doesn't throw an error.

TheRealAmazonKendra · 2023-03-27T19:05:39Z

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

    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/);
+    }).not.toThrowError();


These should test against the expected template output, not just that is doesn't throw an error.

TheRealAmazonKendra · 2023-03-27T19:05:42Z

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

    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/);
+    }).not.toThrowError();


These should test against the expected template output, not just that is doesn't throw an error.

TheRealAmazonKendra · 2023-03-27T19:05:46Z

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

    const stack = new cdk.Stack();
    const key = new kms.Key(stack, 'TestKey');
    const logBucket = new s3.Bucket(stack, 'testLogBucket', { encryptionKey: key });
    expect(() => {
      new s3.Bucket(stack, 'MyBucket', { serverAccessLogsBucket: logBucket });
-    }).toThrow(/SSE-S3 is the only supported default bucket encryption for Server Access Logging target buckets/);
+    }).not.toThrowError();


These should test against the expected template output, not just that is doesn't throw an error.

TheRealAmazonKendra · 2023-03-27T19:11:55Z

Removing request for exemption per my comments in my review.

zdf1230 · 2023-03-27T19:39:28Z

Thanks for the comments. I replied them. Please let me know if anything is uncleared. As I commented, I still think I need the Exemption Request.

zdf1230 · 2023-03-29T01:14:52Z

Clarification Request

aws-cdk-automation · 2023-04-02T00:10:44Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-04-03T00:09:15Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-04-04T00:09:14Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-04-04T22:17:53Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to a test file.
❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation · 2023-04-04T23:04:46Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a9d6966
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Mar 26, 2023

aws-cdk-automation requested a review from a team March 26, 2023 06:12

aws-cdk-automation requested changes Mar 26, 2023

View reviewed changes

zdf1230 changed the title ~~KMS encryption works fine for server access logging target buckets~~ fix(s3): KMS encryption works fine for server access logging target buckets Mar 26, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 26, 2023

TheRealAmazonKendra requested changes Mar 27, 2023

View reviewed changes

TheRealAmazonKendra removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 27, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 27, 2023

zdf1230 requested review from TheRealAmazonKendra and aws-cdk-automation and removed request for TheRealAmazonKendra and aws-cdk-automation March 27, 2023 23:10

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Mar 29, 2023

zdf1230 requested review from aws-cdk-automation and TheRealAmazonKendra and removed request for TheRealAmazonKendra and aws-cdk-automation March 29, 2023 21:38

zdf1230 closed this Apr 4, 2023

zdf1230 force-pushed the main branch from c3495a8 to a9d6966 Compare April 4, 2023 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(s3): KMS encryption works fine for server access logging target buckets #24789

fix(s3): KMS encryption works fine for server access logging target buckets #24789

zdf1230 commented Mar 26, 2023

aws-cdk-automation left a comment

zdf1230 commented Mar 26, 2023

TheRealAmazonKendra left a comment

TheRealAmazonKendra Mar 27, 2023

TheRealAmazonKendra Mar 27, 2023

zdf1230 Mar 27, 2023

TheRealAmazonKendra Mar 27, 2023

zdf1230 Mar 27, 2023

TheRealAmazonKendra Mar 27, 2023

TheRealAmazonKendra Mar 27, 2023

TheRealAmazonKendra Mar 27, 2023

TheRealAmazonKendra Mar 27, 2023

TheRealAmazonKendra commented Mar 27, 2023

zdf1230 commented Mar 27, 2023

zdf1230 commented Mar 29, 2023

aws-cdk-automation commented Apr 2, 2023

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation commented Apr 4, 2023

aws-cdk-automation commented Apr 4, 2023

aws-cdk-automation commented Apr 4, 2023

		if (!props.serverAccessLogsBucket && props.encryption === BucketEncryption.KMS_MANAGED) {
		throw new Error('Default bucket encryption with KMS managed key is not supported for Server Access Logging target buckets');

fix(s3): KMS encryption works fine for server access logging target buckets #24789

fix(s3): KMS encryption works fine for server access logging target buckets #24789

Conversation

zdf1230 commented Mar 26, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

zdf1230 commented Mar 26, 2023

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Mar 27, 2023

zdf1230 commented Mar 27, 2023

zdf1230 commented Mar 29, 2023

aws-cdk-automation commented Apr 2, 2023

aws-cdk-automation commented Apr 3, 2023

aws-cdk-automation commented Apr 4, 2023

aws-cdk-automation commented Apr 4, 2023

aws-cdk-automation commented Apr 4, 2023

AWS CodeBuild CI Report