Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(assets): S3 asset publishing does not use the specified KMS Key #18262

Closed
robertjan-b opened this issue Jan 4, 2022 · 5 comments · Fixed by #18340
Closed

(assets): S3 asset publishing does not use the specified KMS Key #18262

robertjan-b opened this issue Jan 4, 2022 · 5 comments · Fixed by #18340
Assignees
Labels
@aws-cdk/assets Related to the @aws-cdk/assets package bug This issue is a bug. effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. p1

Comments

@robertjan-b
Copy link

What is the problem?

When publishing assets to the assets bucket, the files are not encrypted using the same KMS key as the default encryption method of the bucket. If the buckets' default encryption is set to KMS with a Customer Managed key, the objects will be encrypted with the AWS Managed key. This will make it impossible to shared the objects with the rest of the Organizational accounts. In this case it is not possible to deploy a stackset with assets.

Reproduction Steps

Create the bootstrapping stack with a KMS key:
cdk bootstrap --bootstrap-kms-key-id '' aws:///

This will create a bucket that uses a new Customer Managed key as default encryption (not AWS_MANAGED_KEY)

Create a cdk app with assets and synthesize the app.

Publish assets:
cdk-assets publish --path cdk.out/-.assets.json

What did you expect to happen?

The objects should be encrypted with the Customer managed key.

What actually happened?

The object are encrypted with the AWS Managed KMS Key

CDK CLI Version

2.3.0

Framework Version

No response

Node.js Version

16

OS

Mac

Language

Typescript

Language Version

No response

Other information

I think the behaviour changed with the following change:
8191f1f#diff-1f59ad129e9c6ce901748465a75693927875be1babb51d0f0871530c31678842

If I remove the s3:GetEncryptionConfiguration permissions, objects will be uploaded using the proper default encryption method (without the encryption header).

@robertjan-b robertjan-b added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 4, 2022
@github-actions github-actions bot added the @aws-cdk/assets Related to the @aws-cdk/assets package label Jan 4, 2022
@skinny85 skinny85 changed the title (module name): short issue description (assets): S3 asset publishing does not use the specified KMS Key Jan 4, 2022
@skinny85 skinny85 assigned rix0rrr and unassigned eladb Jan 4, 2022
@skinny85
Copy link
Contributor

skinny85 commented Jan 4, 2022

@rix0rrr I assume you'll want to take a look at this one.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 10, 2022

I had trouble reproducing this, but you are indeed correct. This must be because of #17668,

where the putObject call has been turned from:

[AWS s3 200 0.761s 0 retries] putObject({
  Body: <Buffer 7b 0a 20 20 22 52 65 73 6f 75 72 63 65 73 22 3a 20 7b 0a 20 20 20 20 22 43 44 4b 4d 65 74 61 64 61 74 61 22 3a 20 7b 0a 20 20 20 20 20 20 22 54 79 70 ... 5543 more bytes>,
  Bucket: 'cdk-hnb659fds-assets-993655754359-us-east-1',
  Key: '2104ec0fbda68e2cf7f709ccc126dcff7c930b3c3eeb2cea4285eca991f8ceb7.json',
  ContentType: 'application/json',
})

into

[AWS s3 200 0.761s 0 retries] putObject({
  Body: <Buffer 7b 0a 20 20 22 52 65 73 6f 75 72 63 65 73 22 3a 20 7b 0a 20 20 20 20 22 43 44 4b 4d 65 74 61 64 61 74 61 22 3a 20 7b 0a 20 20 20 20 20 20 22 54 79 70 ... 5543 more bytes>,
  Bucket: 'cdk-hnb659fds-assets-993655754359-us-east-1',
  Key: '2104ec0fbda68e2cf7f709ccc126dcff7c930b3c3eeb2cea4285eca991f8ceb7.json',
  ContentType: 'application/json',
  ServerSideEncryption: 'aws:kms'
})

I'm guessing the inclusion of ServerSideEncryption but lack of key ID is making it fall back to the default key.

Though Arlind assured me he had tested it. More investigation is necessary.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 10, 2022

Trace:

$ npx cdk deploy -vvv
...
[AWS s3 200 0.761s 0 retries] putObject({
  Body: <Buffer 7b 0a 20 20 22 52 65 73 6f 75 72 63 65 73 22 3a 20 7b 0a 20 20 20 20 22 43 44 4b 4d 65 74 61 64 61 74 61 22 3a 20 7b 0a 20 20 20 20 20 20 22 54 79 70 ... 5543 more bytes>,
  Bucket: 'cdk-hnb659fds-assets-993655754359-us-east-1',
  Key: '2104ec0fbda68e2cf7f709ccc126dcff7c930b3c3eeb2cea4285eca991f8ceb7.json',
  ContentType: 'application/json',
  ServerSideEncryption: 'aws:kms'
})
...

aws s3api head-object --bucket cdk-hnb659fds-assets-993655754359-us-east-1 --key 2104ec0fbda68e2cf7f709ccc126dcff7c930b3c3eeb2cea4285eca991f8ceb7.json
{
    "AcceptRanges": "bytes",
    "LastModified": "Mon, 10 Jan 2022 09:13:08 GMT",
    "ContentLength": 5593,
    "ETag": "\"424a9c40c6ee67b6c5ce8aaa9126fcfd\"",
    "VersionId": "g8WQ2qvS_PFPZ.nfDADRP_lC3TLJs2d0",
    "ContentType": "application/json",
    "ServerSideEncryption": "aws:kms",
    "Metadata": {},
    "SSEKMSKeyId": "arn:aws:kms:us-east-1:993655754359:key/cd259848-1b06-4137-8eea-1014a15f3720"
}

aws cloudformation describe-stack-resources --stack-name CDKToolkit --output text --query 'StackResources[].[LogicalResourceId,PhysicalResourceId]' | grep EncryptionKey
FileAssetsBucketEncryptionKey	d3307e9f-858f-4a8b-ad49-719b9a09bea5
FileAssetsBucketEncryptionKeyAlias	alias/cdk-hnb659fds-assets-key

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 10, 2022

Suspicion confirmed. With a locally fixed version of the CLI that includes the key ID it works:

$ /path/to/my/cdk deploy -vvv
[AWS s3 200 0.706s 0 retries] putObject({
  Body: <Buffer 7b 0a 20 20 22 52 65 73 6f 75 72 63 65 73 22 3a 20 7b 0a 20 20 20 20 22 43 44 4b 4d 65 74 61 64 61 74 61 22 3a 20 7b 0a 20 20 20 20 20 20 22 54 79 70 ... 5543 more bytes>,
  Bucket: 'cdk-hnb659fds-assets-993655754359-us-east-1',
  Key: '2104ec0fbda68e2cf7f709ccc126dcff7c930b3c3eeb2cea4285eca991f8ceb7.json',
  ContentType: 'application/json',
  ServerSideEncryption: 'aws:kms',
  SSEKMSKeyId: '***SensitiveInformation***'
})

aws s3api head-object --bucket cdk-hnb659fds-assets-993655754359-us-east-1 --key 2104ec0fbda68e2cf7f709ccc126dcff7c930b3c3eeb2cea4285eca991f8ceb7.json
{
    "AcceptRanges": "bytes",
    "LastModified": "Mon, 10 Jan 2022 09:30:38 GMT",
    "ContentLength": 5593,
    "ETag": "\"063b5ddce37bd511c3b3e4f80bee34cc\"",
    "VersionId": "KcgVGm_NwCANDHlDRK5QLKFNHZYGwfXr",
    "ContentType": "application/json",
    "ServerSideEncryption": "aws:kms",
    "Metadata": {},
    "SSEKMSKeyId": "arn:aws:kms:us-east-1:993655754359:key/d3307e9f-858f-4a8b-ad49-719b9a09bea5"
}

rix0rrr added a commit that referenced this issue Jan 10, 2022
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.
@rix0rrr rix0rrr added the effort/small Small work item – less than a day of effort label Jan 10, 2022
@mergify mergify bot closed this as completed in #18340 Jan 10, 2022
mergify bot pushed a commit that referenced this issue Jan 10, 2022
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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
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 aws#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 aws#18262.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/assets Related to the @aws-cdk/assets package bug This issue is a bug. effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants