fix(cloudfront): bucket policy for Origin Access Identities is overly permissive #13087

njlynch · 2021-02-16T18:53:57Z

For both CloudFrontWebDistribution and Distribution, if an origin access
identity (OAI) is used to access a S3 bucket origin, the default
bucket.grantRead() method is used to grant read access to the bucket for the
OAI. grantRead will create a statement in the bucket policy that grants:
s3:GetObject*, s3:GetBucket*, and s3:List*
to the OAI.

The official documentation in the developer guide
states that to allow the OAI to read objects in the bucket, only s3:GetObject
is necessary.

The impact of the additional permissions is that if a user navigates to the root
of the distribution (e.g., https://d12345abcdef.cloudfront.net/) AND no default
root object (e.g., index.html) is set for the distribution, the user will receive
an XML list of the objects in the bucket. This is likely undesired behavior.

This same policy has been the behavior for OAI + S3 buckets since the first
introduction of the CloudFrontWebDistribution construct years ago. However,
the CloudFrontWebDistribution also always defaults the root object to
'index.html' (see #3486 for discussion on that). The side-effect of this is
that unless the user selects to use an OAI and overrides the default root object
to an empty string, visiting the root will result either in valid content or an
AccessDenied message.

However, since the Distribution constructs do not default the root object,
exposing the bucket index becomes much more likely.

This change restricts the permissions granted to the OAI user to just the
necessary s3:GetObject permissions.

fixes #13086

NOTE: This PR corrects the behavior without introducing a new feature flag.
Any customers who rely on the bucket listing behavior would be broken by this
change. However, I anticipate that represents a very small minority, versus the
number of customers who may accidentally expose their bucket contents. I would
very much like to have a discussion about whether this is the correct choice.

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

… permissive For both `CloudFrontWebDistribution` and `Distribution`, if an origin access identity (OAI) is used to access a S3 bucket origin, the default `bucket.grantRead()` method is used to grant read access to the bucket for the OAI. `grantRead` will create a statement in the bucket policy that grants: `s3:GetObject*`, `s3:GetBucket*`, and `s3:List*` to the OAI. The (official documentation)[https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-restricting-access-to-s3.html] in the developer guide states that to allow the OAI to read objects in the bucket, only `s3:GetObject` is necessary. The impact of the additional permissions is that if a user navigates to the root of the distribution (e.g., https://d12345abcdef.cloudfront.net/) *AND* no default root object (e.g., index.html) is set for the distribution, the user will receive an XML list of the objects in the bucket. This is likely undesired behavior, and potential information leakage. This same policy has been the behavior for OAI + S3 buckets since the first introduction of the `CloudFrontWebDistribution` construct years ago. However, the `CloudFrontWebDistribution` also always defaults the root object to 'index.html' (see #3486 for discussion on that). The side-effect of this is that unless the user selects to use an OAI and overrides the default root object to an empty string, visiting the root will result either in valid content or an AccessDenied message. However, since the `Distribution` constructs do *not* default the root object, exposing the bucket index becomes much more likely. This change restricts the permissions granted to the OAI user to just the necessary `s3:GetObject` permissions. fixes #13086 --- **NOTE:** This PR corrects the behavior without introducing a new feature flag. Any customers who rely on the bucket listing behavior would be broken by this change. However, I anticipate that represents a very small minority, versus the number of customers who may accidentally expose their bucket contents. I would very much like to have a discussion about whether this is the correct choice.

gitpod-io · 2021-02-16T18:54:00Z

skinny85

LGTM! I agree with you that I don't think we need a feature flag for this.

nija-at

Feature flag is definitely the safest option here. If we feel that customers are unlikely to depend on this behaviour, it might be prudent to skip this.

Could we at least provide an option on the Distribution construct for users to restore the old behaviour?

nija-at · 2021-02-17T11:46:01Z

packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts

@@ -79,7 +80,16 @@ class S3BucketOrigin extends cloudfront.OriginBase {
      this.originAccessIdentity = new cloudfront.OriginAccessIdentity(oaiScope, oaiId, {
        comment: `Identity for ${options.originId}`,
      });
-      this.bucket.grantRead(this.originAccessIdentity);


Isn't there a grant() API on bucket that takes a list of actions?

There's a private one:
https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-s3/lib/bucket.ts#L716

I briefly toyed with the idea of adding a grantReadObject method, but felt it was a bit niche.

It's a common pattern for constructs to expose a grant(grantee, ...actions) API for such niche use cases.

njlynch · 2021-02-17T12:03:06Z

Could we at least provide an option on the Distribution construct for users to restore the old behaviour?

I'd rather not, unless we get significant pushback. Exposing the bucket contents at the distribution root doesn't seem like a typically-desirable behavior, and there's a reasonable workaround.

// Before - Does the bucket listing by default
const bucket = new s3.Bucket(this, 'Bucket');
const s3Origin = new origins.S3Origin(this, 'S3Origin', { bucket });

// After - Customers who want to intentionally have the bucket listing
const bucket = new s3.Bucket(this, 'Bucket');
const originAccessIdentity = new cloudfront.OriginAccessIdentity(this, 'OAI');
bucket.grantRead(originAccessIdentity);
const s3Origin = new origins.S3Origin(this, 'S3Origin', { bucket, originAccessIdentity });

mergify · 2021-02-17T12:04:13Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2021-02-17T12:36:18Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-02-17T12:36:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: c18cc64
Result: SUCCEEDED
Build Logs (available for 30 days)

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

… permissive (aws#13087) For both `CloudFrontWebDistribution` and `Distribution`, if an origin access identity (OAI) is used to access a S3 bucket origin, the default `bucket.grantRead()` method is used to grant read access to the bucket for the OAI. `grantRead` will create a statement in the bucket policy that grants: `s3:GetObject*`, `s3:GetBucket*`, and `s3:List*` to the OAI. The [official documentation](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-restricting-access-to-s3.html) in the developer guide states that to allow the OAI to read objects in the bucket, only `s3:GetObject` is necessary. The impact of the additional permissions is that if a user navigates to the root of the distribution (e.g., https://d12345abcdef.cloudfront.net/) *AND* no default root object (e.g., index.html) is set for the distribution, the user will receive an XML list of the objects in the bucket. This is likely undesired behavior. This same policy has been the behavior for OAI + S3 buckets since the first introduction of the `CloudFrontWebDistribution` construct years ago. However, the `CloudFrontWebDistribution` also always defaults the root object to 'index.html' (see aws#3486 for discussion on that). The side-effect of this is that unless the user selects to use an OAI and overrides the default root object to an empty string, visiting the root will result either in valid content or an AccessDenied message. However, since the `Distribution` constructs do *not* default the root object, exposing the bucket index becomes much more likely. This change restricts the permissions granted to the OAI user to just the necessary `s3:GetObject` permissions. fixes aws#13086 --- **NOTE:** This PR corrects the behavior without introducing a new feature flag. Any customers who rely on the bucket listing behavior would be broken by this change. However, I anticipate that represents a very small minority, versus the number of customers who may accidentally expose their bucket contents. I would very much like to have a discussion about whether this is the correct choice. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

… permissive (#13087) For both `CloudFrontWebDistribution` and `Distribution`, if an origin access identity (OAI) is used to access a S3 bucket origin, the default `bucket.grantRead()` method is used to grant read access to the bucket for the OAI. `grantRead` will create a statement in the bucket policy that grants: `s3:GetObject*`, `s3:GetBucket*`, and `s3:List*` to the OAI. The [official documentation](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-restricting-access-to-s3.html) in the developer guide states that to allow the OAI to read objects in the bucket, only `s3:GetObject` is necessary. The impact of the additional permissions is that if a user navigates to the root of the distribution (e.g., https://d12345abcdef.cloudfront.net/) *AND* no default root object (e.g., index.html) is set for the distribution, the user will receive an XML list of the objects in the bucket. This is likely undesired behavior. This same policy has been the behavior for OAI + S3 buckets since the first introduction of the `CloudFrontWebDistribution` construct years ago. However, the `CloudFrontWebDistribution` also always defaults the root object to 'index.html' (see #3486 for discussion on that). The side-effect of this is that unless the user selects to use an OAI and overrides the default root object to an empty string, visiting the root will result either in valid content or an AccessDenied message. However, since the `Distribution` constructs do *not* default the root object, exposing the bucket index becomes much more likely. This change restricts the permissions granted to the OAI user to just the necessary `s3:GetObject` permissions. fixes #13086 --- **NOTE:** This PR corrects the behavior without introducing a new feature flag. Any customers who rely on the bucket listing behavior would be broken by this change. However, I anticipate that represents a very small minority, versus the number of customers who may accidentally expose their bucket contents. I would very much like to have a discussion about whether this is the correct choice. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

njlynch requested a review from a team February 16, 2021 18:53

njlynch self-assigned this Feb 16, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Feb 16, 2021

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Feb 16, 2021

skinny85 approved these changes Feb 16, 2021

View reviewed changes

Merge branch 'master' into njlynch/cf-s3-perms

79cc1e4

nija-at reviewed Feb 17, 2021

View reviewed changes

njlynch removed the pr/do-not-merge This PR should not be merged at this time. label Feb 17, 2021

Merge branch 'master' into njlynch/cf-s3-perms

c18cc64

mergify bot merged commit cc28312 into master Feb 17, 2021

mergify bot deleted the njlynch/cf-s3-perms branch February 17, 2021 12:36

ferdingler mentioned this pull request Feb 25, 2021

(aws-cloudfront): Bucket policy permissions broke CloudFront integration #13272

Closed

njlynch mentioned this pull request Apr 7, 2021

(aws-cloudfront-origins): S3Origin needs s3:ListBucket permission for CloudFront to return 404 #13983

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudfront): bucket policy for Origin Access Identities is overly permissive #13087

fix(cloudfront): bucket policy for Origin Access Identities is overly permissive #13087

njlynch commented Feb 16, 2021 •

edited by ericzbeard

Loading

gitpod-io bot commented Feb 16, 2021 •

edited

Loading

skinny85 left a comment

nija-at left a comment •

edited

Loading

nija-at Feb 17, 2021

njlynch Feb 17, 2021

nija-at Feb 17, 2021

njlynch commented Feb 17, 2021

mergify bot commented Feb 17, 2021

mergify bot commented Feb 17, 2021

aws-cdk-automation commented Feb 17, 2021

fix(cloudfront): bucket policy for Origin Access Identities is overly permissive #13087

fix(cloudfront): bucket policy for Origin Access Identities is overly permissive #13087

Conversation

njlynch commented Feb 16, 2021 • edited by ericzbeard Loading

gitpod-io bot commented Feb 16, 2021 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

nija-at left a comment • edited Loading

Choose a reason for hiding this comment

nija-at Feb 17, 2021

Choose a reason for hiding this comment

njlynch Feb 17, 2021

Choose a reason for hiding this comment

nija-at Feb 17, 2021

Choose a reason for hiding this comment

njlynch commented Feb 17, 2021

mergify bot commented Feb 17, 2021

mergify bot commented Feb 17, 2021

aws-cdk-automation commented Feb 17, 2021

AWS CodeBuild CI Report

njlynch commented Feb 16, 2021 •

edited by ericzbeard

Loading

gitpod-io bot commented Feb 16, 2021 •

edited

Loading

nija-at left a comment •

edited

Loading