Skip to content

Commit

Permalink
fix(cli): assets are KMS-encrypted using wrong key (#18340)
Browse files Browse the repository at this point in the history
Even though a custom KMS key is specified as the default encryption key
on the file assets bucket, all uploaded assets are encrypted using the
default key.

The reason is that in #17668 we added an explicit `encryption: kms`
parameter to the `putObject` operation, so that an SCP that is
commonly in use across large organizations to prevent files from
ending up unencrypted, can be used (the SCP can only validate
call parameters, such as whether the `putObject` call includes
the parameter that reuests encryption, not the effective end result,
such as whether a file would end up encrypted).

However, we did not include the KMS Key Id into the `putObject`
request, which caused S3 to fall back to the default key.

Solution: also look up the key id and pass that along as well.

Fixes #18262.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Jan 10, 2022
1 parent 2256680 commit 64ae9f3
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 23 deletions.
49 changes: 27 additions & 22 deletions packages/cdk-assets/lib/private/handlers/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,23 @@ export class FileAssetHandler implements IAssetHandler {
// required for SCP rules denying uploads without encryption header
let paramsEncryption: {[index: string]:any}= {};
const encryption2 = await bucketInfo.bucketEncryption(s3, destination.bucketName);
switch (encryption2) {
case BucketEncryption.NO_ENCRYPTION:
switch (encryption2.type) {
case 'no_encryption':
break;
case BucketEncryption.SSEAlgorithm_AES256:
case 'aes256':
paramsEncryption = { ServerSideEncryption: 'AES256' };
break;
case BucketEncryption.SSEAlgorithm_aws_kms:
paramsEncryption = { ServerSideEncryption: 'aws:kms' };
case 'kms':
// We must include the key ID otherwise S3 will encrypt with the default key
paramsEncryption = {
ServerSideEncryption: 'aws:kms',
SSEKMSKeyId: encryption2.kmsKeyId,
};
break;
case BucketEncryption.DOES_NOT_EXIST:
case 'does_not_exist':
this.host.emitMessage(EventType.DEBUG, `No bucket named '${destination.bucketName}'. Is account ${await account()} bootstrapped?`);
break;
case BucketEncryption.ACCES_DENIED:
case 'access_denied':
this.host.emitMessage(EventType.DEBUG, `Could not read encryption settings of bucket '${destination.bucketName}': uploading with default settings ("cdk bootstrap" to version 9 if your organization's policies prevent a successful upload or to get rid of this message).`);
break;
}
Expand Down Expand Up @@ -126,13 +130,13 @@ enum BucketOwnership {
SOMEONE_ELSES_OR_NO_ACCESS
}

enum BucketEncryption {
NO_ENCRYPTION,
SSEAlgorithm_AES256,
SSEAlgorithm_aws_kms,
ACCES_DENIED,
DOES_NOT_EXIST
}
type BucketEncryption =
| { readonly type: 'no_encryption' }
| { readonly type: 'aes256' }
| { readonly type: 'kms'; readonly kmsKeyId?: string }
| { readonly type: 'access_denied' }
| { readonly type: 'does_not_exist' }
;

async function objectExists(s3: AWS.S3, bucket: string, key: string) {
/*
Expand Down Expand Up @@ -218,23 +222,24 @@ class BucketInformation {
const encryption = await s3.getBucketEncryption({ Bucket: bucket }).promise();
const l = encryption?.ServerSideEncryptionConfiguration?.Rules?.length ?? 0;
if (l > 0) {
let ssealgo = encryption?.ServerSideEncryptionConfiguration?.Rules[0]?.ApplyServerSideEncryptionByDefault?.SSEAlgorithm;
if (ssealgo === 'AES256') return BucketEncryption.SSEAlgorithm_AES256;
if (ssealgo === 'aws:kms') return BucketEncryption.SSEAlgorithm_aws_kms;
const apply = encryption?.ServerSideEncryptionConfiguration?.Rules[0]?.ApplyServerSideEncryptionByDefault;
let ssealgo = apply?.SSEAlgorithm;
if (ssealgo === 'AES256') return { type: 'aes256' };
if (ssealgo === 'aws:kms') return { type: 'kms', kmsKeyId: apply?.KMSMasterKeyID };
}
return BucketEncryption.NO_ENCRYPTION;
return { type: 'no_encryption' };
} catch (e) {
if (e.code === 'NoSuchBucket') {
return BucketEncryption.DOES_NOT_EXIST;
return { type: 'does_not_exist' };
}
if (e.code === 'ServerSideEncryptionConfigurationNotFoundError') {
return BucketEncryption.NO_ENCRYPTION;
return { type: 'no_encryption' };
}

if (['AccessDenied', 'AllAccessDisabled'].includes(e.code)) {
return BucketEncryption.ACCES_DENIED;
return { type: 'access_denied' };
}
return BucketEncryption.NO_ENCRYPTION;
return { type: 'no_encryption' };
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/cdk-assets/test/files.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ test('upload with server side encryption AES256 header', async () => {
// We'll just have to assume the contents are correct
});

test('upload with server side encryption aws:kms header', async () => {
test('upload with server side encryption aws:kms header and key id', async () => {
const pub = new AssetPublishing(AssetManifest.fromPath('/simple/cdk.out'), { aws });

aws.mockS3.getBucketEncryption = mockedApiResult({
Expand All @@ -192,6 +192,7 @@ test('upload with server side encryption aws:kms header', async () => {
{
ApplyServerSideEncryptionByDefault: {
SSEAlgorithm: 'aws:kms',
KMSMasterKeyID: 'the-key-id',
},
BucketKeyEnabled: false,
},
Expand All @@ -209,6 +210,7 @@ test('upload with server side encryption aws:kms header', async () => {
Key: 'some_key',
ContentType: 'application/octet-stream',
ServerSideEncryption: 'aws:kms',
SSEKMSKeyId: 'the-key-id',
}));

// We'll just have to assume the contents are correct
Expand Down

0 comments on commit 64ae9f3

Please sign in to comment.