feat(aws-cloudfront-s3): update construct to use origin access controls; add support for CMK-encrypted buckets #1038

hayesry · 2023-12-08T05:38:57Z

Issue #, if available:
#831 #979

Description of changes:
This pull request updates the aws-cloudfront-s3 construct to use an origin access control (OAC) instead of an origin access identity (OAI) for accessing bucket that is either provisioned by or provided to the construct. This pull request also resolves an issue with the construct in working with CMK-encrypted buckets by adding a custom resource that updates the key policy whenever 1.) a CMK encryptionKey is provided as a bucketProp to be provisioned by the construct; or 2.) an existing bucket that is encrypted with a CMK is provided to the construct using the existingBucketObj property.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

biffgaut · 2023-12-08T13:32:06Z

...-solutions-constructs/aws-cloudfront-s3/lib/custom-resources/kms-key-policy-updater/index.ts

@@ -0,0 +1,109 @@
+/**


This module needs to move to resources/lib/kms-key-policy-updater-custom-resource.ts - we've already got a structure for where to put custom resources.

Moved this into the /resources folder in commit f2a7053

biffgaut · 2023-12-08T14:48:48Z

source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts

+    l1CloudFrontDistribution.addPropertyOverride('DistributionConfig.Origins.0.S3OriginConfig.OriginAccessIdentity', '');
+
+    // Grant CloudFront permission to get the objects from the s3 bucket origin
+    bucket.addToResourcePolicy(


let's change the name of this variable to originBucket

Fixed in commit 7e82a23

biffgaut · 2023-12-08T14:50:02Z

source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts

+    // Define the OriginAccessControl
+    const originAccessControl = new cloudfront.CfnOriginAccessControl(this, 'CloudFrontOac', {
+      originAccessControlConfig: {
+        name: `cloudfront-default-oac-${new Date().getTime().toString(16)}`,


This won't work - name will be different each time it runs and cause issues with Stack Updates. Use GeneratePhysicalName() to get a reproducible unique name based on the Stack ID

Fixed this by using generatePhysicalName('aws-cloudfront-s3', ['oac'], 16) to create a unique name. Tested and answered the following questions before settling on this approach:

Does not specifying anything for name tell CloudFormation to give it a unique name? Didn't get that far, turns out its a required property by CDK, so that didn't work.

Does giving the OAC a general name like cloudfront-s3-oac tell CloudFormation to still make it unique? For example, cloudfront-s3-oac-stackNameHere-someOtherRandomValues? No, it's a literal property specification, so if you provide name: cloudfront-s3-oac, that will be the name. No adjustments or prefixes/suffixes added by CFN. This caused issues when running integration tests because one or more OACs were named the same, and the same issue would pop up if a customer had greater than one deployment of this construct.

biffgaut · 2023-12-08T14:53:07Z

source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts

+    );
+
+    // We need to create a custom resource to introduce the indirection necessary to avoid
+    // a circular dependency when granting the cloud front distribution access to use the


I'm going to be sharing this A LOT in situations where people are discussing circular dependencies in CloudFormation, so let's change the comment to say CloudFront properly. :-)

Good call, updated in e0a323a

biffgaut · 2023-12-08T14:55:50Z

source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts

+    //   * The S3 Bucket references the KMS Key
+    //   * The CloudFront Distribution references the Bucket
+    //   * The KMS Key references the CloudFront Distribution
+    let encryptionKey: IKey | undefined;


A third option here needs to be enableEncryptionWithCustomerManagedKey?: boolean to be consistent with other Constructs that offer optional custom managed key functionality. See README entry in aws-lambda-sqs for one example.

Pinging this comment again

Discussed together; going to hold off on this for now as we don't have a defined design pattern for taking CMK's from the top-level for use with encrypting buckets. Users can continue supplying their CMK via bucketProps if using the construct-provided bucket, or supplying their own encrypted bucket (either w/ CMK or AWS managed key) via existingBucketObj.

…taging'

aws-solutions-constructs-team · 2024-01-05T03:11:34Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: e9b0063
Result: SUCCEEDED
Build Logs (available for 30 days)

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

…ging

aws-solutions-constructs-team · 2024-01-05T03:26:37Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 379fdd1
Result: SUCCEEDED
Build Logs (available for 30 days)

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

biffgaut · 2024-01-05T14:59:45Z

source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/README.md

@@ -21,10 +21,6 @@
 ## Overview
 This AWS Solutions Construct provisions an Amazon CloudFront Distribution that serves objects from an AWS S3 Bucket via an Origin Access Control (OAC).

-> **Note:** This AWS Solutions Construct was updated in December 2023 to replace the use of Origin Access Identities (OAIs) with Origin Access Controls (OACs) for accessing objects from the Bucket. Due to the
-> current configuration of one or more underlying AWS CDK constructs, this Construct will still provison an OAI, but it will be "orphaned" and the OAC will be used for accessing the Bucket. The 


This is no longer accurate with the use of S3OacOrigin, correct?

Correct, content removed as a result

biffgaut · 2024-01-05T15:17:25Z

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

@@ -113,28 +113,34 @@ export interface CloudFrontDistributionForS3Response {
 /**
 * @internal This is an internal core function and should not be called directly by Solutions Constructs clients.
 */
-export function CloudFrontDistributionForS3(
+export function createCloudFrontDistributionForS3(
  scope: Construct,
  sourceBucket: s3.IBucket,
  cloudFrontDistributionProps?: cloudfront.DistributionProps | any,


Can we put the last 4 args into a CreateCloudFrontDistributionForS3Props interface? I'm not sure at what point SonarQube is going to yell at us for too many arguments, plus we use that pattern in many other locations - especially with a response interface.

Addressed this in 65e8c32

biffgaut · 2024-01-05T15:20:13Z

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

+  if (!sourceBucket.isWebsite) {
+    originAccessControl = new cloudfront.CfnOriginAccessControl(scope, 'CloudFrontOac', {
+      originAccessControlConfig: {
+        name: generatePhysicalName('aws-cloudfront-s3', ['oac'], 16),


We need to add an ID to incoming arguments (not as part of the props interface) and include that in the physical name. Currently if a stack includes 2 instances of this construct we will have a name collision. Sent the ID sent to the construct constructor for the value.

Addressed this in 65e8c32

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

biffgaut · 2024-01-05T15:37:36Z

source/patterns/@aws-solutions-constructs/core/lib/s3-oac-origin.ts

+      // If the bucket is configured for website hosting, set up an HttpOrigin to support legacy clients
+      printWarning(`Bucket ${bucket.bucketName} is being provided as an origin bucket to aws-cloudfront-s3, but currently
+        has static website hosting settings enabled. Please review whether this is the intended configuration. If so,
+        construct will configure an HttpOrigin to allow the distribution to access the bucket. If not, we strongly recommend


Isn't the underlying issue here that the bucket and objects need to be public? We should call that out, then specifically say "AWS strongly recommends against public settings on S3 buckets and objects"

Updated language in 0b48016

biffgaut · 2024-01-05T16:00:03Z

source/patterns/@aws-solutions-constructs/resources/lib/key-policy-updater.ts

@@ -43,13 +44,13 @@ export function createKeyPolicyUpdaterCustomResource(
    lambdaFunctionProps: {
      runtime: lambda.Runtime.NODEJS_18_X,
      handler: 'index.handler',
-      description: 'kms-key-policy-updater',
+      description: 'Custom resource function that updates the key policy of an existing encryption key to allow CloudFront access.',


biffgaut · 2024-01-05T16:05:38Z

source/patterns/@aws-solutions-constructs/resources/lib/shared/index.ts

+ *  and limitations under the License.
+ */
+
+import * as lambda from 'aws-cdk-lib/aws-lambda';


index doesn't seem like the correct name for this file - index.ts usually the top of the hierarchy in some way. This just looks like a utils file?

We can switch it to utils.ts, made the update in 545e005

…tps://github.com/awslabs/aws-solutions-constructs into feature/hayesry/cloudfront-s3-oac-update-staging

georgebearden · 2024-01-08T15:14:38Z

...patterns/@aws-solutions-constructs/resources/lib/key-policy-updater-custom-resource/index.ts

+      const keyPolicy = JSON.parse(getKeyPolicyCommandResponse?.Policy);
+      const keyPolicyStatementSid: string = 'Grant-CloudFront-Distribution-Key-Usage';
+
+      if (checkForExistingKeyPolicyStatement(keyPolicy, keyPolicyStatementSid)) {


What if the sid already exists, but a new CMK is used, for example? We probably wouldn't want to return here in that case. Maybe the better logic would be to always overwrite the existing statement we are looking for, even if it ends up being a no-op .

Addressed this in 7f24545

Let me know if I'm missing anything with the implementation

georgebearden · 2024-01-08T15:15:23Z

...patterns/@aws-solutions-constructs/resources/lib/key-policy-updater-custom-resource/index.ts

+ */
+export const checkForExistingKeyPolicyStatement = (parsedKeyPolicy: any, sid: string) => {
+  const matches = parsedKeyPolicy.Statement.find((statement: any) => statement.Sid === sid);
+  return matches ? true : false;


nit - you could just return matches here, as its already the boolean result

Addressed in 7f24545

georgebearden · 2024-01-08T15:16:16Z

source/patterns/@aws-solutions-constructs/resources/lib/key-policy-updater.ts

+    lambdaFunctionProps: {
+      runtime: lambda.Runtime.NODEJS_18_X,
+      handler: 'index.handler',
+      description: 'Custom resource function that updates the key policy of an existing encryption key to allow CloudFront access.',


Is the word existing here accurate? What if its a new CMK being provisioned as part of the construct?

Touched this up in 7f24545

georgebearden

lgtm

biffgaut · 2024-01-08T21:10:34Z

source/patterns/@aws-solutions-constructs/aws-cloudfront-s3/lib/index.ts

-      props.insertHttpSecurityHeaders,
-      props.cloudFrontLoggingBucketProps,
-      props.responseHeadersPolicyProps
+      'cloudfront-dist-for-s3',


Passing the ID gives us no benefit if we pass a constant value, we'll still get a name collision if we launch multiple constructs in 1 stack. The idea was to pass the id of this instance of the construct (the id argument to the constructor).

That... makes more sense. Switching it now.

…onForS3

aws-solutions-constructs-team · 2024-01-09T06:11:02Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 702ad2e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

hayesry added 5 commits December 6, 2023 08:47

feat: upgrade aws-cloudfront-s3 to use oac instead of oai

1f4eda2

feat: added testing and documentation notes

d39a81d

merge: conflict resolution between main and feature branch

01a834e

merge: alignment with #1037

372a244

fix: version number for aws-cloudfront-s3 custom resource

a02f795

hayesry requested review from biffgaut, mobri2a, emcfins, mickychetta and georgebearden as code owners December 8, 2023 05:38

This comment was marked as outdated.

Sign in to view

fix: edits to custom resource package.json file

a1e9872

This comment was marked as outdated.

Sign in to view

fix: issue with installing deps for custom resource

2b7fc52

This comment was marked as outdated.

Sign in to view

biffgaut reviewed Dec 8, 2023

View reviewed changes

feat: create new S3OacOrigin, consolidate custom resources

f2a7053

This comment was marked as outdated.

Sign in to view

merge: branch 'main' into 'feature/hayesry/cloudfront-s3-oac-update-s…

2fbce66

…taging'

This comment was marked as outdated.

Sign in to view

chore: change bucket variable naming in 'aws-cloudfront-s3/lib/index.ts'

7e82a23

This comment was marked as outdated.

Sign in to view

chore: consolidate custom resources, additional cleanup

721d2ad

This comment was marked as outdated.

Sign in to view

chore: update resources/package.json file

c231611

This comment was marked as outdated.

Sign in to view

chore: add resources dependency to aws-cloudfront-s3 package.json file

b7b583d

This comment was marked as outdated.

Sign in to view

chore: edit source/package.json to revert workspaces specification

dac63df

This comment was marked as outdated.

Sign in to view

chore: additional testing, readme updates

e9b0063

Merge branch 'main' into feature/hayesry/cloudfront-s3-oac-update-sta…

379fdd1

…ging

hayesry marked this pull request as ready for review January 5, 2024 03:36

biffgaut requested changes Jan 5, 2024

View reviewed changes

hayesry added 2 commits January 7, 2024 23:43

feat: refactored props for createCloudFrontDistributionForS3()

65e8c32

Merge branch 'feature/hayesry/cloudfront-s3-oac-update-staging' of ht…

e512955

…tps://github.com/awslabs/aws-solutions-constructs into feature/hayesry/cloudfront-s3-oac-update-staging

This comment was marked as outdated.

Sign in to view

chore: print warnings language updates

0b48016

This comment was marked as outdated.

Sign in to view

chore: switch file naming for shared functions in resources/

545e005

This comment was marked as outdated.

Sign in to view

chore: update imports in resources/

aa61318

This comment was marked as outdated.

Sign in to view

georgebearden reviewed Jan 8, 2024

View reviewed changes

feat: overwrite existing key policy, update unit tests

7f24545

This comment was marked as outdated.

Sign in to view

georgebearden previously approved these changes Jan 8, 2024

View reviewed changes

biffgaut requested changes Jan 8, 2024

View reviewed changes

fix: pass id from the top level through to createCloudFrontDistributi…

7acf926

…onForS3

hayesry dismissed georgebearden’s stale review via 7acf926 January 8, 2024 22:00

This comment was marked as outdated.

Sign in to view

chore: update integration tests

702ad2e

biffgaut approved these changes Jan 9, 2024

View reviewed changes

biffgaut merged commit 012f9e7 into main Jan 9, 2024
1 check passed

biffgaut deleted the feature/hayesry/cloudfront-s3-oac-update-staging branch January 9, 2024 13:43

YikaiHu mentioned this pull request Jan 15, 2024

(aws-cloudfront-s3) AWS::CloudFront::OriginAccessControl not support in China Region #1050

Open

feat(aws-cloudfront-s3): update construct to use origin access controls; add support for CMK-encrypted buckets #1038

feat(aws-cloudfront-s3): update construct to use origin access controls; add support for CMK-encrypted buckets #1038

Conversation

hayesry commented Dec 8, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

aws-solutions-constructs-team commented Jan 5, 2024

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Jan 5, 2024

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

georgebearden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

aws-solutions-constructs-team commented Jan 9, 2024

AWS CodeBuild CI Report