fix(s3): logging bucket blocks KMS_MANAGED encryption

[biffgaut]

---

closes #23513

All Submissions:

• [ x] Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

• This PR adds new construct runtime dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • 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

[gitpod-io]

[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.

[biffgaut]

Exception to PR Linter requested:

This PR changes what inputs are flagged as an error, it has not effect on synthesized output - so there is no updated integration test to push.

[biffgaut]

Correction on versions - the framework version is 2.58.0, the CLI version is 2.55.1

[kylelaker]

Hey! I originally wrote #23385 based on an issue with log delivery and the code is based on https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html

You can use default bucket encryption on the target bucket only if you use AES256 (SSE-S3). Default encryption with AWS KMS keys (SSE-KMS) is not supported.

I am curious if a PR needs to be made to https://github.com/awsdocs/amazon-s3-userguide/blob/main/doc_source/enable-server-access-logging.md as well

[biffgaut]

I am curious if a PR needs to be made to https://github.com/awsdocs/amazon-s3-userguide/blob/main/doc_source/enable-server-access-logging.md as well

The outlier is KMS_MANAGED - which is not a setting recognized by the S3 console/api. It is apparently a CDK concept, which the CDK translates into a configuration that works for self-logging buckets. I'm running yet another test just to prove to myself again that I saw this work.

[kylelaker]

The outlier is KMS_MANAGED - which is not a setting recognized by the S3 console/api. It is apparently a CDK concept, which the CDK translates into a configuration that works for self-logging buckets

This is actually really interesting to me! I am curious where this breaks. The only difference that I really see is in how the CloudFormation is generated

For KMS_MANAGED:

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

Lines 1982 to 1992 in f03cc85

	if (encryptionType === BucketEncryption.KMS_MANAGED) {
	const bucketEncryption = {
	serverSideEncryptionConfiguration: [
	{
	bucketKeyEnabled: props.bucketKeyEnabled,
	serverSideEncryptionByDefault: { sseAlgorithm: 'aws:kms' },
	},
	],
	};
	return { bucketEncryption };
	}

And for KMS:

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

Lines 1953 to 1970 in f03cc85

	if (encryptionType === BucketEncryption.KMS) {
	const encryptionKey = props.encryptionKey || new kms.Key(this, 'Key', {
	description: `Created by ${this.node.path}`,
	});
	const bucketEncryption = {
	serverSideEncryptionConfiguration: [
	{
	bucketKeyEnabled: props.bucketKeyEnabled,
	serverSideEncryptionByDefault: {
	sseAlgorithm: 'aws:kms',
	kmsMasterKeyId: encryptionKey.keyArn,
	},
	},
	],
	};
	return { encryptionKey, bucketEncryption };
	}

So that setup is basically just whether the KMSMasterKeyID is specified (which would use the aws/s3 if it's not; or at least that's the value I see in the Console when inspecting after building). And the encryption is still set to aws:kms (or SSE-KMS, the same as if it was a KMS CMK). And the grant for logging (in 2.58.1 and earlier) just sets the canned LogDeliveryWrite ACL.

I've tried reproducing with:

import * as cdk from 'aws-cdk-lib';
import * as s3 from 'aws-cdk-lib/aws-s3';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const target = new s3.Bucket(stack, 'Target', { encryption: s3.BucketEncryption.KMS_MANAGED });
const bucket = new s3.Bucket(stack, 'Bucket', {
  serverAccessLogsBucket: target,
  serverAccessLogsPrefix: 'test/',
});

I don't see any logs being delivered. But I am wondering if this can be made to work some other way? Because I would personally love to be able to use SSE-KMS instead of SSE-S3 for default encryption on my log target buckets!

But even when I do:

import * as cdk from 'aws-cdk-lib';
import * as s3 from 'aws-cdk-lib/aws-s3';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');

const selfLog = new s3.Bucket(stack, 'SelfLogging', {
  encryption: s3.BucketEncryption.KMS_MANAGED,
  serverAccesLogsPrefix: 'test/',
});
// Ensure LogDeliverWrite is applied, aws/aws-cdk#23513
(selfLog.node.defaultChild as s3.CfnBucket).addPropertyOverride('AccessControl', 'LogDeliveryWrite');

I am not getting any logs delivered into the bucket. I also tried to go into the console, and have it apply the bucket policy to see if that would allow log delivery and while logs did seem to be delivered, they were encrypted with a key I couldn't use (I didn't recognize the account number and it didn't seem to match the key for the aws/s3 key)

[biffgaut]

Make sure you are waiting long enough for your tests - logs can take up to an hour to show up.

Allow some time for recent logging configuration changes to take effect

Turning on server access logging for the first time, or changing the target bucket for logs, can take time to fully implement. During the hour after you turn on logging, some requests might not be logged. During the hour after you change the target bucket, some logs might still be delivered to the previous target bucket. After you make a configuration change to logging, be sure to wait around one hour after the change to verify logs. For more information, see Best effort server log delivery.

[biffgaut]

const target = new s3.Bucket(stack, 'Target', { encryption: s3.BucketEncryption.KMS_MANAGED });
const bucket = new s3.Bucket(stack, 'Bucket', {
  serverAccessLogsBucket: target,
  serverAccessLogsPrefix: 'test/',
});

I just ran a test that is essentially that same as the above. Here's my test:

    const loggingBucketThree = new s3.Bucket(this, 'LogBucketKmsManagedEnc',{
      bucketName: 'bgc-logbucket-kmsmanaged-encryption',
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      autoDeleteObjects: true,
      encryption: BucketEncryption.KMS_MANAGED
    });
    new s3.Bucket(this, 'BucketKmsManagedEnc', {
      bucketName: 'bgc-source-bucket-kmsmanaged-encryption',
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      autoDeleteObjects: true,
      serverAccessLogsPrefix: 'logs',
      serverAccessLogsBucket: loggingBucketThree
    });

I saw logs, but they took nearly an hour to show up.

(ignore the autoDelete and RemovalPolicy, I get tired of having to clean up my retained S3 buckets every couple weeks). I launched self-logging buckets in the same stack that still show no logs. In about 10 minutes I will manually add the logDeliveryWrite ACL - I expect a flood of logs to follow). I'm running my tests in 2.56.0.

[kylelaker]

but they took nearly an hour to show up

Out of curiousity, can you actually read the objects? In both my examples, the objects are encrypted with key a owned by 858827067514 (which seems to be the S3 Log Delivery service principal account). Trying to read the logs gives me AccessDenied with:

The ciphertext refers to a customer master key that does not exist, does not exist in this region, or you are not allowed to access.

[biffgaut]

Ha! Never crossed my mind to actually try to open them - I cannot read them either. My unverified assumption had been that KMS logging was unavailable because of issues sharing a CMK with the logging process.

This would suggest that the configuration generated by KMS_MANAGED (without a key), uses a CMK owned by the logging process so sharing the key is still a problem.

[kylelaker]

This would suggest that the configuration generated by KMS_MANAGED (without a key), uses a CMK owned by the logging process so sharing the key is still a problem

Yeah, so the annoying thing about Default Bucket Encryption is that it's just the default. If something does PutObject and the put operation request specifies an encryption setting, that will be accepted (unless the Bucket Policy rejects it). This is why it's actually possible to write SSE-S3 objects to a bucket with SSE-KMS default. So it would seem that when putting the object into the target bucket, the log delivery service specifies the KMS key to use.

A lot of recommendations include using a bucket policy to enforce the correct encryption settings to prevent using the wrong type of encryption or the wrong key. Some information is available at https://repost.aws/knowledge-center/s3-aws-kms-default-encryption

I am curious why this doesn't impact buckets with SSE-S3 and without default encryption. But that must be an implementation detail I am unaware of.

The configuration generated by KMS_MANAGED sets the default encryption to use the AWS-managed KMS Key with the alias aws/s3 (and this is because KMSMasterKeyId is left unspecified). But it doesn't prevent the usage of other incorrect encryption settings. So when log delivery ends up writing the object, it's one that we can't read because it uses its own key, not the bucket's default (and we don't have Decrypt on that key). And because the default is an AWS-managed key, you can't modify the key policy to grant the service access to Encrypt with the key either (but even if you could, I don't know if the service would use it).

[kylelaker]

logs can take up to an hour to show up

Definitely forgot this detail by the way! I thought it was 10ish minutes! Went back and checked all the buckets; and they're all receiving the logs but I can't Decrypt them with SSE-KMS.

[biffgaut]

I think I need to update this PR to replace KMS_MANAGED in the list of excepted encryptions. Since I'm in here anyway, I'll add a comment explaining how it is different, but still doesn't work. I'll also leave the tests (more complete) and the parenthesis that make the precedence in the condition more explicit.

[biffgaut]

The extra tests point out that we're not flagging KMS_MANAGED for separate buckets. Since the logging bucket is an IBucket at that point in the code, the .encryption prop is not available, so KMS_MANAGED is "not a thing" for IBucket. Diving a little deeper to see if there's another way to flag KMS_MANAGED buckets.

[rix0rrr]

From the sound of it, the additional validation preventing some scenarios, while technically a deploy-time regression, isn't actually breaking any valid use cases. Did I get that right?

[biffgaut]

From the sound of it, the additional validation preventing some scenarios, while technically a deploy-time regression, isn't actually breaking any valid use cases. Did I get that right?

Correct. The only remaining question is that there is no way to alert the client if they provide a logging bucket using KMS_MANAGED encryption, as the logging bucket prop is IBucket, which does not expose the .encryption value from the BucketProps used to create it. This situation will create a stack and store logs on the provided bucket, but those logs will be unreadable.

[biffgaut]

The changes requested are an automated requirement to add an integration test - but after all discussion, this PR only increases unit test coverage to make it more complete and adds a set of parenthesis to make the precedence explicit instead of implicit. So there's nothing to check with an integration test.

[kylelaker]

Since it's just tests that being added, you may want to consider removing the non-comment changes from bucket.ts and categorizing this as chore instead of fix. For example:

chore(s3): add more test cases for KMS encryption settings

I don't believe that a "chore" requires integration test changes; a "fix" is meant to be a bug fix, so usually there are integration test changes to avoid a regression.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

[comcalvi]

Thanks to both @biffgaut and @kylelaker for investigating this...these S3 encryption settings are non trivial.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[TheRealAmazonKendra]

Mergify out here failing on the job. Manually rebasing. Also, I think this is more of a fix but the unit tests here are sufficient so I'm going to change this to a fix but exempt the test.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[zdf1230]

I agree that KMS_MANAGED would not work here since the key is not owned by the stack. But I believe KMS case should work here because we can add permission for logging.s3.amazonaws.com to the customized key. I had a working case and checked the log content is readable.

[biffgaut]

As this PR is merged, the CDK team may not see the comments (I'm not a member of the CDK team). I suggest you open a new Issue describing your situation and referencing this PR.