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.

Sign up for GitHub

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

Jump to bottom

Use wildcard in KMS key policy instead of referencing Distribution to avoid circular dependency #31227

Merged
merged 6 commits into from
Aug 28, 2024
Merged

Use wildcard in KMS key policy instead of referencing Distribution to avoid circular dependency #31227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b08605e
introduce assembleDomainName option in S3BucketOrigin.withOriginAcces…
samson-keung Aug 27, 2024
f5c308c
fix using imported bucket with assembleDomainName to true
samson-keung Aug 27, 2024
626a8b3
remove assembleDomainName and use wildcard key policy
samson-keung Aug 28, 2024
4a8f9c8
warn user about wildcard in key policy
samson-keung Aug 28, 2024
9449c3c
warning wording update
samson-keung Aug 28, 2024
dd11f45
warning wording update and removed redundant warning
samson-keung Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 9 additions & 18 deletions packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { AccessLevel } from '../../aws-cloudfront';
import * as iam from '../../aws-iam';
import { IKey } from '../../aws-kms';
import { IBucket } from '../../aws-s3';
import { Annotations, Aws, DefaultTokenResolver, Names, Stack, StringConcat, Token, Tokenization } from '../../core';
import { Annotations, Aws, Names, Stack } from '../../core';

const BUCKET_ACTIONS: Record<string, string[]> = {
READ: ['s3:GetObject'],
Expand Down Expand Up @@ -84,21 +84,8 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase {
}

if (bucket.encryptionKey) {
let bucketName = bucket.bucketName;
if (Token.isUnresolved(bucket.bucketName)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this because turns out bucket.bucketName is always unresolved as it returns CFN reference to the bucket (i.e. !Ref bucket). It is set here.

bucketName = JSON.stringify(Tokenization.resolve(bucket.bucketName, {
scope,
resolver: new DefaultTokenResolver(new StringConcat()),
}));
}
Annotations.of(scope).addInfo(
`Granting OAC permissions to access KMS key for S3 bucket origin ${bucketName} may cause a circular dependency error when this stack deploys.\n` +
'The key policy references the distribution\'s id, the distribution references the bucket, and the bucket references the key.\n'+
'See the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README for more details.\n',
);

const keyPolicyActions = this.getKeyPolicyActions(props?.originAccessLevels ?? [cloudfront.AccessLevel.READ]);
const keyPolicyResult = this.grantDistributionAccessToKey(distributionId!, keyPolicyActions, bucket.encryptionKey);
const keyPolicyResult = this.grantDistributionAccessToKey(keyPolicyActions, bucket.encryptionKey);
// Failed to update key policy, assume using imported key
if (!keyPolicyResult.statementAdded) {
Annotations.of(scope).addWarningV2('@aws-cdk/aws-cloudfront-origins:updateImportedKeyPolicy',
Expand Down Expand Up @@ -154,20 +141,24 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase {
return result;
}

grantDistributionAccessToKey(distributionId: string, actions: string[], key: IKey): iam.AddToResourcePolicyResult {
grantDistributionAccessToKey(actions: string[], key: IKey): iam.AddToResourcePolicyResult {
const oacKeyPolicyStatement = new iam.PolicyStatement(
{
effect: iam.Effect.ALLOW,
principals: [new iam.ServicePrincipal('cloudfront.amazonaws.com')],
actions,
resources: ['*'],
conditions: {
StringEquals: {
'AWS:SourceArn': `arn:${Aws.PARTITION}:cloudfront::${Aws.ACCOUNT_ID}:distribution/${distributionId}`,
ArnLike: {
'AWS:SourceArn': `arn:${Aws.PARTITION}:cloudfront::${Aws.ACCOUNT_ID}:distribution/*`,
},
},
},
);
Annotations.of(key.node.scope!).addWarningV2('@aws-cdk/aws-cloudfront-origins:wildcardKeyPolicyForOac',
'To avoid circular dependency between the KMS key, Bucket, and Distribution,' +
'a wildcard is used to match all Distribution IDs in Key policy condition.\n' +
'To further scope down the policy for best security practices, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README.');
const result = key.addToResourcePolicy(oacKeyPolicyStatement);
return result;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-static-website-origin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ export interface S3StaticWebsiteOriginProps extends HttpOriginProps {
export class S3StaticWebsiteOrigin extends HttpOrigin {
constructor(props: S3StaticWebsiteOriginProps) {
super(props.bucket.bucketWebsiteDomainName, {
protocolPolicy: cloudfront.OriginProtocolPolicy.HTTP_ONLY, // S3 only supports HTTP for website buckets
// S3 only supports HTTP for website buckets. See https://docs.aws.amazon.com/AmazonS3/latest/userguide/WebsiteEndpoints.html
protocolPolicy: cloudfront.OriginProtocolPolicy.HTTP_ONLY,
...props,
});
}
Expand Down
222 changes: 221 additions & 1 deletion packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Annotations, Template } from '../../assertions';
import * as cloudfront from '../../aws-cloudfront/index';
import * as origins from '../../aws-cloudfront-origins';
import * as kms from '../../aws-kms';
import * as s3 from '../../aws-s3/index';
import { App, Duration, Stack } from '../../core';
import { App, Duration, Fn, Stack } from '../../core';

describe('S3BucketOrigin', () => {
describe('withOriginAccessControl', () => {
Expand Down Expand Up @@ -165,6 +166,225 @@ describe('S3BucketOrigin', () => {
});
});

describe('when using bucket with KMS Customer Managed key', () => {
it('should match expected template resource and warn user about wildcard key policy', () => {
const stack = new Stack();
const kmsKey = new kms.Key(stack, 'myKey');
const bucket = new s3.Bucket(stack, 'myEncryptedBucket', {
encryption: s3.BucketEncryption.KMS,
encryptionKey: kmsKey,
objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
});
new cloudfront.Distribution(stack, 'MyDistributionA', {
defaultBehavior: { origin: origins.S3BucketOrigin.withOriginAccessControl(bucket) },
});
const template = Template.fromStack(stack);

expect(template.toJSON().Resources).toEqual({
myKey441A1E73: {
Type: 'AWS::KMS::Key',
Properties: {
KeyPolicy: {
Statement: [
{
Action: 'kms:*',
Effect: 'Allow',
Principal: {
AWS: {
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':iam::',
{
Ref: 'AWS::AccountId',
},
':root',
],
],
},
},
Resource: '*',
},
{
Action: 'kms:Decrypt',
Condition: {
ArnLike: {
'AWS:SourceArn': {
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':cloudfront::',
{
Ref: 'AWS::AccountId',
},
':distribution/*',
],
],
},
},
},
Effect: 'Allow',
Principal: {
Service: 'cloudfront.amazonaws.com',
},
Resource: '*',
},
],
Version: '2012-10-17',
},
},
UpdateReplacePolicy: 'Retain',
DeletionPolicy: 'Retain',
},
myEncryptedBucket939A51C0: {
Type: 'AWS::S3::Bucket',
Properties: {
BucketEncryption: {
ServerSideEncryptionConfiguration: [
{
ServerSideEncryptionByDefault: {
KMSMasterKeyID: {
'Fn::GetAtt': [
'myKey441A1E73',
'Arn',
],
},
SSEAlgorithm: 'aws:kms',
},
},
],
},
OwnershipControls: {
Rules: [
{
ObjectOwnership: 'BucketOwnerEnforced',
},
],
},
},
UpdateReplacePolicy: 'Retain',
DeletionPolicy: 'Retain',
},
myEncryptedBucketPolicyF516140B: {
Type: 'AWS::S3::BucketPolicy',
Properties: {
Bucket: {
Ref: 'myEncryptedBucket939A51C0',
},
PolicyDocument: {
Statement: [
{
Action: 's3:GetObject',
Condition: {
StringEquals: {
'AWS:SourceArn': {
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':cloudfront::',
{
Ref: 'AWS::AccountId',
},
':distribution/',
{
Ref: 'MyDistributionA2150CE0F',
},
],
],
},
},
},
Effect: 'Allow',
Principal: {
Service: 'cloudfront.amazonaws.com',
},
Resource: {
'Fn::Join': [
'',
[
{
'Fn::GetAtt': [
'myEncryptedBucket939A51C0',
'Arn',
],
},
'/*',
],
],
},
},
],
Version: '2012-10-17',
},
},
},
MyDistributionAOrigin1S3OriginAccessControlE2649D73: {
Type: 'AWS::CloudFront::OriginAccessControl',
Properties: {
OriginAccessControlConfig: {
Name: 'MyDistributionAOrigin1S3OriginAccessControl2859DD54',
OriginAccessControlOriginType: 's3',
SigningBehavior: 'always',
SigningProtocol: 'sigv4',
},
},
},
MyDistributionA2150CE0F: {
Type: 'AWS::CloudFront::Distribution',
Properties: {
DistributionConfig: {
DefaultCacheBehavior: {
CachePolicyId: '658327ea-f89d-4fab-a63d-7e88639e58f6',
Compress: true,
TargetOriginId: 'MyDistributionAOrigin11BE8FF8C',
ViewerProtocolPolicy: 'allow-all',
},
Enabled: true,
HttpVersion: 'http2',
IPV6Enabled: true,
Origins: [
{
DomainName: {
'Fn::GetAtt': [
'myEncryptedBucket939A51C0',
'RegionalDomainName',
],
},
Id: 'MyDistributionAOrigin11BE8FF8C',
OriginAccessControlId: {
'Fn::GetAtt': [
'MyDistributionAOrigin1S3OriginAccessControlE2649D73',
'Id',
],
},
S3OriginConfig: {
OriginAccessIdentity: '',
},
},
],
},
},
},
});
Annotations.fromStack(stack).hasWarning('/Default',
'To avoid circular dependency between the KMS key, Bucket, and Distribution,' +
'a wildcard is used to match all Distribution IDs in Key policy condition.\n' +
'To further scope down the policy for best security practices, see the "Using OAC for a SSE-KMS encrypted S3 origin" section in the module README. [ack: @aws-cdk/aws-cloudfront-origins:wildcardKeyPolicyForOac]');
});
});

describe('when attaching to a multiple distribution', () => {
let stack: Stack;
let bucket: s3.Bucket;
Expand Down
Loading