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

RFC 617: CloudFront Origin Access Control L2 #619

Merged
merged 7 commits into from
Jun 25, 2024

Conversation

gracelu0
Copy link
Contributor

This is a request for comments about CloudFront Origin Access Control. See #617 for
additional details.

APIs are signed off by @colifran .


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

distribution has been created via the `GetKeyPolicy()` and `PutKeyPolicy()` API calls.

```ts
class S3BucketOacOrigin extends cloudfront.OriginBase {
Copy link

@colifran colifran Jun 13, 2024

Choose a reason for hiding this comment

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

I think at this point we shouldn't add a new origin class. Instead, what if we use S3BucketOrigin as an abstract class that defines a contract for an origin identity class (for existing behavior) and a origin access class using your implementation. This could look like this:

abstract class S3BucketOrigin extends cloudfront.OriginBase {
  public static withAccessIdentity(bucket: s3.IBucket, props: S3OriginProps = {}): S3BucketOrigin {
    return new (class OriginAccessIdentity extends S3BucketOrigin {
      private originAccessIdentity?: cloudfront.IOriginAccessIdentity;

      public constructor() {
        super(bucket, props);
		this.originAccessIdentity = props.originAccessIdentity;
      }

      public bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
        if (!this.originAccessIdentity) {
          // Using a bucket from another stack creates a cyclic reference with
          // the bucket taking a dependency on the generated S3CanonicalUserId for the grant principal,
          // and the distribution having a dependency on the bucket's domain name.
          // Fix this by parenting the OAI in the bucket's stack when cross-stack usage is detected.
          const bucketStack = cdk.Stack.of(this.bucket);
          const bucketInDifferentStack = bucketStack !== cdk.Stack.of(scope);
          const oaiScope = bucketInDifferentStack ? bucketStack : scope;
          const oaiId = bucketInDifferentStack ? `${cdk.Names.uniqueId(scope)}S3Origin` : 'S3Origin';

          this.originAccessIdentity = new cloudfront.OriginAccessIdentity(oaiScope, oaiId, {
            comment: `Identity for ${options.originId}`,
          });
        }
        // Used rather than `grantRead` because `grantRead` will grant overly-permissive policies.
        // Only GetObject is needed to retrieve objects for the distribution.
        // This also excludes KMS permissions; currently, OAI only supports SSE-S3 for buckets.
        // Source: https://aws.amazon.com/blogs/networking-and-content-delivery/serving-sse-kms-encrypted-content-from-s3-using-cloudfront/
        this.bucket.addToResourcePolicy(new iam.PolicyStatement({
          resources: [this.bucket.arnForObjects('*')],
          actions: ['s3:GetObject'],
          principals: [this.originAccessIdentity.grantPrincipal],
        }));
        return this._bind(scope, options);
      }

      protected renderS3OriginConfig(): cloudfront.CfnDistribution.S3OriginConfigProperty | undefined {
        if (!this.originAccessIdentity) {
          throw new Error('Origin access identity cannot be undefined');
        }
        return { originAccessIdentity: `origin-access-identity/cloudfront/${this.originAccessIdentity.originAccessIdentityId}` };
      }
    })();
  }

  public static withAccessControl(bucket: s3.IBucket, props: S3OriginProps = {}): S3BucketOrigin {
    return new (class OriginAccessControl extends S3BucketOrigin {
	  private originAccessControl?: cloudfront.IOriginAccessControl;

      public constructor() {
        super(bucket, props);
		this.originAccessControl = props.originAccessControl;
      }

      // Add your origin access control implementation here ...
    })();
  }

  protected constructor(protected readonly bucket: s3.IBucket, props: S3OriginProps = {}) {
    super(bucket.bucketRegionalDomainName, props);
  }

  public abstract bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig;

  protected abstract renderS3OriginConfig(): cloudfront.CfnDistribution.S3OriginConfigProperty | undefined;

  protected _bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
    return super.bind(scope, options);
  }
}

Copy link

@colifran colifran left a comment

Choose a reason for hiding this comment

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

Great job so far, @gracelu0! I love where this is going. Left some feedback on how I think we could improve the API a bit.

@mergify mergify bot dismissed colifran’s stale review June 18, 2024 20:12

Pull request has been modified.

Copy link

@colifran colifran left a comment

Choose a reason for hiding this comment

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

Great job, @gracelu0!

@gracelu0 gracelu0 merged commit 663ad91 into aws:main Jun 25, 2024
2 checks passed
gracelu0 added a commit that referenced this pull request Jun 26, 2024
gracelu0 added a commit that referenced this pull request Jun 26, 2024
Reverts #619

Reverting to re-open for final comment period
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants