From c7c75f8069ea10006853c32bd95ea9d3f70f8a05 Mon Sep 17 00:00:00 2001 From: Pahud Hsieh Date: Thu, 16 May 2024 13:04:47 -0400 Subject: [PATCH] fix(s3): bucketKey does not support SSE-S3 (#30184) ### Issue # (if applicable) Closes #30183 ### Reason for this change ### Description of changes ### Description of how you validated changes ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- ...efaultTestDeployAssert56801A2F.assets.json | 2 +- .../aws-cdk-s3-bucket-encryption.assets.json | 6 +-- ...aws-cdk-s3-bucket-encryption.template.json | 17 +++++++++ .../cdk.out | 2 +- .../integ.json | 2 +- .../manifest.json | 12 +++++- .../tree.json | 37 ++++++++++++++++++- .../aws-s3/test/integ.bucket-encryption.ts | 5 +++ packages/aws-cdk-lib/aws-s3/lib/bucket.ts | 17 +++++---- .../aws-cdk-lib/aws-s3/test/bucket.test.ts | 34 +++++++++++++++-- 10 files changed, 112 insertions(+), 22 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/IntegTestDSSEBucketDefaultTestDeployAssert56801A2F.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/IntegTestDSSEBucketDefaultTestDeployAssert56801A2F.assets.json index 07c3aeffd32d2..a18b483236bea 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/IntegTestDSSEBucketDefaultTestDeployAssert56801A2F.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/IntegTestDSSEBucketDefaultTestDeployAssert56801A2F.assets.json @@ -1,5 +1,5 @@ { - "version": "32.0.0", + "version": "36.0.0", "files": { "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": { "source": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.assets.json index 7b60d22060be9..ae8d82eb471b7 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.assets.json @@ -1,7 +1,7 @@ { - "version": "32.0.0", + "version": "36.0.0", "files": { - "591ab3589c0620b6953fda9e284aa7d8fe4e472d41d449192bee0bcbec043c31": { + "11da7c5b54314bf0158da668aa6014c1e153d0d880b9ace65689ce2cfeceaa20": { "source": { "path": "aws-cdk-s3-bucket-encryption.template.json", "packaging": "file" @@ -9,7 +9,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "591ab3589c0620b6953fda9e284aa7d8fe4e472d41d449192bee0bcbec043c31.json", + "objectKey": "11da7c5b54314bf0158da668aa6014c1e153d0d880b9ace65689ce2cfeceaa20.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.template.json index 0b34c54b1d218..9bc25f1e72af0 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/aws-cdk-s3-bucket-encryption.template.json @@ -15,6 +15,23 @@ }, "UpdateReplacePolicy": "Retain", "DeletionPolicy": "Retain" + }, + "MySSES3Bucket6973690D": { + "Type": "AWS::S3::Bucket", + "Properties": { + "BucketEncryption": { + "ServerSideEncryptionConfiguration": [ + { + "BucketKeyEnabled": true, + "ServerSideEncryptionByDefault": { + "SSEAlgorithm": "AES256" + } + } + ] + } + }, + "UpdateReplacePolicy": "Retain", + "DeletionPolicy": "Retain" } }, "Parameters": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/cdk.out b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/cdk.out index f0b901e7c06e5..1f0068d32659a 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/cdk.out +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/cdk.out @@ -1 +1 @@ -{"version":"32.0.0"} \ No newline at end of file +{"version":"36.0.0"} \ No newline at end of file diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/integ.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/integ.json index d63da76715ed8..d056cd084f925 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/integ.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/integ.json @@ -1,5 +1,5 @@ { - "version": "32.0.0", + "version": "36.0.0", "testCases": { "IntegTestDSSEBucket/DefaultTest": { "stacks": [ diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/manifest.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/manifest.json index 21c43690e2cad..41427a553c276 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/manifest.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/manifest.json @@ -1,5 +1,5 @@ { - "version": "32.0.0", + "version": "36.0.0", "artifacts": { "aws-cdk-s3-bucket-encryption.assets": { "type": "cdk:asset-manifest", @@ -14,10 +14,11 @@ "environment": "aws://unknown-account/unknown-region", "properties": { "templateFile": "aws-cdk-s3-bucket-encryption.template.json", + "terminationProtection": false, "validateOnSynth": false, "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/591ab3589c0620b6953fda9e284aa7d8fe4e472d41d449192bee0bcbec043c31.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/11da7c5b54314bf0158da668aa6014c1e153d0d880b9ace65689ce2cfeceaa20.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ @@ -39,6 +40,12 @@ "data": "MyDSSEBucket8A2A1D0C" } ], + "/aws-cdk-s3-bucket-encryption/MySSES3Bucket/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "MySSES3Bucket6973690D" + } + ], "/aws-cdk-s3-bucket-encryption/BootstrapVersion": [ { "type": "aws:cdk:logicalId", @@ -67,6 +74,7 @@ "environment": "aws://unknown-account/unknown-region", "properties": { "templateFile": "IntegTestDSSEBucketDefaultTestDeployAssert56801A2F.template.json", + "terminationProtection": false, "validateOnSynth": false, "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/tree.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/tree.json index e837891174856..2f48c7893efd3 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/tree.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.js.snapshot/tree.json @@ -40,6 +40,39 @@ "version": "0.0.0" } }, + "MySSES3Bucket": { + "id": "MySSES3Bucket", + "path": "aws-cdk-s3-bucket-encryption/MySSES3Bucket", + "children": { + "Resource": { + "id": "Resource", + "path": "aws-cdk-s3-bucket-encryption/MySSES3Bucket/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::S3::Bucket", + "aws:cdk:cloudformation:props": { + "bucketEncryption": { + "serverSideEncryptionConfiguration": [ + { + "bucketKeyEnabled": true, + "serverSideEncryptionByDefault": { + "sseAlgorithm": "AES256" + } + } + ] + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_s3.CfnBucket", + "version": "0.0.0" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_s3.Bucket", + "version": "0.0.0" + } + }, "BootstrapVersion": { "id": "BootstrapVersion", "path": "aws-cdk-s3-bucket-encryption/BootstrapVersion", @@ -75,7 +108,7 @@ "path": "IntegTestDSSEBucket/DefaultTest/Default", "constructInfo": { "fqn": "constructs.Construct", - "version": "10.2.26" + "version": "10.3.0" } }, "DeployAssert": { @@ -121,7 +154,7 @@ "path": "Tree", "constructInfo": { "fqn": "constructs.Construct", - "version": "10.2.26" + "version": "10.3.0" } } }, diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.ts index 85d9fbe3bef68..143912a530dbd 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-encryption.ts @@ -10,6 +10,11 @@ new s3.Bucket(stack, 'MyDSSEBucket', { encryption: s3.BucketEncryption.DSSE_MANAGED, }); +new s3.Bucket(stack, 'MySSES3Bucket', { + encryption: s3.BucketEncryption.S3_MANAGED, + bucketKeyEnabled: true, +}); + new integ.IntegTest(app, 'IntegTestDSSEBucket', { testCases: [stack], }); diff --git a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts index d990029f6eb6c..a822d571ec59a 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts +++ b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts @@ -1440,7 +1440,7 @@ export interface BucketProps { * attendant cost implications of that). * - If enabled, S3 will use its own time-limited key instead. * - * Only relevant, when Encryption is set to `BucketEncryption.KMS` or `BucketEncryption.KMS_MANAGED`. + * Only relevant, when Encryption is not set to `BucketEncryption.UNENCRYPTED`. * * @default - false */ @@ -2105,6 +2105,7 @@ export class Bucket extends BucketBase { * | KMS | undefined | e | SSE-KMS, bucketKeyEnabled = e | new key | * | KMS_MANAGED | undefined | e | SSE-KMS, bucketKeyEnabled = e | undefined | * | S3_MANAGED | undefined | false | SSE-S3 | undefined | + * | S3_MANAGED | undefined | e | SSE-S3, bucketKeyEnabled = e | undefined | * | UNENCRYPTED | undefined | true | ERROR! | ERROR! | * | UNENCRYPTED | k | e | ERROR! | ERROR! | * | KMS_MANAGED | k | e | ERROR! | ERROR! | @@ -2127,12 +2128,9 @@ export class Bucket extends BucketBase { throw new Error(`encryptionKey is specified, so 'encryption' must be set to KMS or DSSE (value: ${encryptionType})`); } - // if bucketKeyEnabled is set, encryption must be set to KMS or DSSE. - if ( - props.bucketKeyEnabled && - ![BucketEncryption.KMS, BucketEncryption.KMS_MANAGED, BucketEncryption.DSSE, BucketEncryption.DSSE_MANAGED].includes(encryptionType) - ) { - throw new Error(`bucketKeyEnabled is specified, so 'encryption' must be set to KMS or DSSE (value: ${encryptionType})`); + // if bucketKeyEnabled is set, encryption can not be BucketEncryption.UNENCRYPTED + if (props.bucketKeyEnabled && encryptionType === BucketEncryption.UNENCRYPTED) { + throw new Error(`bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3 (value: ${encryptionType})`); } if (encryptionType === BucketEncryption.UNENCRYPTED) { @@ -2161,7 +2159,10 @@ export class Bucket extends BucketBase { if (encryptionType === BucketEncryption.S3_MANAGED) { const bucketEncryption = { serverSideEncryptionConfiguration: [ - { serverSideEncryptionByDefault: { sseAlgorithm: 'AES256' } }, + { + bucketKeyEnabled: props.bucketKeyEnabled, + serverSideEncryptionByDefault: { sseAlgorithm: 'AES256' }, + }, ], }; diff --git a/packages/aws-cdk-lib/aws-s3/test/bucket.test.ts b/packages/aws-cdk-lib/aws-s3/test/bucket.test.ts index 956f66e7d12e6..35ca6cddcc956 100644 --- a/packages/aws-cdk-lib/aws-s3/test/bucket.test.ts +++ b/packages/aws-cdk-lib/aws-s3/test/bucket.test.ts @@ -574,15 +574,41 @@ describe('bucket', () => { }); }); - test('throws error if bucketKeyEnabled is set, but encryption is not KMS', () => { + test('bucketKeyEnabled can be enabled with SSE-S3', () => { const stack = new cdk.Stack(); + // WHEN + new s3.Bucket(stack, 'MyBucket', { bucketKeyEnabled: true, encryption: s3.BucketEncryption.S3_MANAGED }); + Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', { + BucketEncryption: { + ServerSideEncryptionConfiguration: [ + { + ServerSideEncryptionByDefault: { SSEAlgorithm: 'AES256' }, + BucketKeyEnabled: true, + }, + ], + }, + }); + + }); + test('bucketKeyEnabled can not be enabled with UNENCRYPTED', () => { + const stack = new cdk.Stack(); + + // WHEN expect(() => { - new s3.Bucket(stack, 'MyBucket', { bucketKeyEnabled: true, encryption: s3.BucketEncryption.S3_MANAGED }); - }).toThrow("bucketKeyEnabled is specified, so 'encryption' must be set to KMS or DSSE (value: S3_MANAGED)"); + new s3.Bucket(stack, 'MyBucket', { + bucketKeyEnabled: true, + encryption: s3.BucketEncryption.UNENCRYPTED, + }); + }).toThrow(/bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3/); + }); + + test('bucketKeyEnabled can NOT be enabled with encryption undefined', () => { + const stack = new cdk.Stack(); + expect(() => { new s3.Bucket(stack, 'MyBucket3', { bucketKeyEnabled: true }); - }).toThrow("bucketKeyEnabled is specified, so 'encryption' must be set to KMS or DSSE (value: UNENCRYPTED)"); + }).toThrow("bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3 (value: UNENCRYPTED)"); });