fix(cloudfront): bucket policy for Origin Access Identities is overly 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*

Author: njlynch

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

@@ -1,4 +1,5 @@
 import * as cloudfront from '@aws-cdk/aws-cloudfront';
+import * as iam from '@aws-cdk/aws-iam';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as cdk from '@aws-cdk/core';
 import { HttpOrigin } from './http-origin';
@@ -68,7 +69,7 @@ class S3BucketOrigin extends cloudfront.OriginBase {
   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 when `grantRead` is called,
+      // 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);
@@ -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);
+
+      // 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 super.bind(scope, options);
   }

packages/@aws-cdk/aws-cloudfront-origins/package.json

@@ -79,6 +79,7 @@
   "dependencies": {
     "@aws-cdk/aws-cloudfront": "0.0.0",
     "@aws-cdk/aws-elasticloadbalancingv2": "0.0.0",
+    "@aws-cdk/aws-iam": "0.0.0",
     "@aws-cdk/aws-s3": "0.0.0",
     "@aws-cdk/core": "0.0.0",
     "constructs": "^3.2.0"
@@ -88,8 +89,9 @@
     "@aws-cdk/core": "0.0.0",
     "constructs": "^3.2.0",
     "@aws-cdk/aws-cloudfront": "0.0.0",
-    "@aws-cdk/aws-s3": "0.0.0",
-    "@aws-cdk/aws-elasticloadbalancingv2": "0.0.0"
+    "@aws-cdk/aws-elasticloadbalancingv2": "0.0.0",
+    "@aws-cdk/aws-iam": "0.0.0",
+    "@aws-cdk/aws-s3": "0.0.0"
   },
   "engines": {
     "node": ">= 10.13.0 <13 || >=13.7.0"

packages/@aws-cdk/aws-cloudfront-origins/test/integ.origin-group.expected.json

@@ -14,11 +14,7 @@
         "PolicyDocument": {
           "Statement": [
             {
-              "Action": [
-                "s3:GetObject*",
-                "s3:GetBucket*",
-                "s3:List*"
-              ],
+              "Action": "s3:GetObject",
               "Effect": "Allow",
               "Principal": {
                 "CanonicalUser": {
@@ -28,28 +24,20 @@
                   ]
                 }
               },
-              "Resource": [
-                {
-                  "Fn::GetAtt": [
-                    "Bucket83908E77",
-                    "Arn"
-                  ]
-                },
-                {
-                  "Fn::Join": [
-                    "",
-                    [
-                      {
-                        "Fn::GetAtt": [
-                          "Bucket83908E77",
-                          "Arn"
-                        ]
-                      },
-                      "/*"
-                    ]
+              "Resource": {
+                "Fn::Join": [
+                  "",
+                  [
+                    {
+                      "Fn::GetAtt": [
+                        "Bucket83908E77",
+                        "Arn"
+                      ]
+                    },
+                    "/*"
                   ]
-                }
-              ]
+                ]
+              }
             }
           ],
           "Version": "2012-10-17"

packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json

@@ -14,11 +14,7 @@
         "PolicyDocument": {
           "Statement": [
             {
-              "Action": [
-                "s3:GetObject*",
-                "s3:GetBucket*",
-                "s3:List*"
-              ],
+              "Action": "s3:GetObject",
               "Effect": "Allow",
               "Principal": {
                 "CanonicalUser": {
@@ -28,28 +24,20 @@
                   ]
                 }
               },
-              "Resource": [
-                {
-                  "Fn::GetAtt": [
-                    "Bucket83908E77",
-                    "Arn"
-                  ]
-                },
-                {
-                  "Fn::Join": [
-                    "",
-                    [
-                      {
-                        "Fn::GetAtt": [
-                          "Bucket83908E77",
-                          "Arn"
-                        ]
-                      },
-                      "/*"
-                    ]
+              "Resource": {
+                "Fn::Join": [
+                  "",
+                  [
+                    {
+                      "Fn::GetAtt": [
+                        "Bucket83908E77",
+                        "Arn"
+                      ]
+                    },
+                    "/*"
                   ]
-                }
-              ]
+                ]
+              }
             }
           ],
           "Version": "2012-10-17"

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

@@ -87,9 +87,13 @@ describe('With bucket', () => {
     expect(stack).toHaveResourceLike('AWS::S3::BucketPolicy', {
       PolicyDocument: {
         Statement: [{
+          Action: 's3:GetObject',
           Principal: {
             CanonicalUser: { 'Fn::GetAtt': ['DistOrigin1S3Origin87D64058', 'S3CanonicalUserId'] },
           },
+          Resource: {
+            'Fn::Join': ['', [{ 'Fn::GetAtt': ['Bucket83908E77', 'Arn'] }, '/*']],
+          },
         }],
       },
     });

packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts

@@ -1027,9 +1027,15 @@ export class CloudFrontWebDistribution extends cdk.Resource implements IDistribu
       // first case for backwards compatibility
       if (originConfig.s3OriginSource.originAccessIdentity) {
         // grant CloudFront OriginAccessIdentity read access to S3 bucket
-        originConfig.s3OriginSource.s3BucketSource.grantRead(
-          originConfig.s3OriginSource.originAccessIdentity,
-        );
+        // 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/
+        originConfig.s3OriginSource.s3BucketSource.addToResourcePolicy(new iam.PolicyStatement({
+          resources: [originConfig.s3OriginSource.s3BucketSource.arnForObjects('*')],
+          actions: ['s3:GetObject'],
+          principals: [originConfig.s3OriginSource.originAccessIdentity.grantPrincipal],
+        }));
 
         s3OriginConfig = {
           originAccessIdentity: `origin-access-identity/cloudfront/${originConfig.s3OriginSource.originAccessIdentity.originAccessIdentityName}`,

packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json

@@ -14,11 +14,7 @@
         "PolicyDocument": {
           "Statement": [
             {
-              "Action": [
-                "s3:GetObject*",
-                "s3:GetBucket*",
-                "s3:List*"
-              ],
+              "Action": "s3:GetObject",
               "Effect": "Allow",
               "Principal": {
                 "AWS": {
@@ -37,28 +33,20 @@
                   ]
                 }
               },
-              "Resource": [
-                {
-                  "Fn::GetAtt": [
-                    "Bucket83908E77",
-                    "Arn"
+              "Resource": {
+                "Fn::Join": [
+                  "",
+                  [
+                    {
+                      "Fn::GetAtt": [
+                        "Bucket83908E77",
+                        "Arn"
+                      ]
+                    },
+                    "/*"
                   ]
-                },
-                {
-                  "Fn::Join": [
-                    "",
-                    [
-                      {
-                        "Fn::GetAtt": [
-                          "Bucket83908E77",
-                          "Arn"
-                        ]
-                      },
-                      "/*"
-                    ]
-                  ]
-                }
-              ]
+                ]
+              }
             }
           ],
           "Version": "2012-10-17"

packages/@aws-cdk/aws-cloudfront/test/web-distribution.test.ts

@@ -10,6 +10,7 @@ import {
   GeoRestriction,
   KeyGroup,
   LambdaEdgeEventType,
+  OriginAccessIdentity,
   PublicKey,
   SecurityPolicyProtocol,
   SSLMethod,
@@ -197,6 +198,59 @@ nodeunitShim({
     test.done();
   },
 
+  'distribution with bucket and OAI'(test: Test) {
+    const stack = new cdk.Stack();
+    const s3BucketSource = new s3.Bucket(stack, 'Bucket');
+    const originAccessIdentity = new OriginAccessIdentity(stack, 'OAI');
+
+    new CloudFrontWebDistribution(stack, 'AnAmazingWebsiteProbably', {
+      originConfigs: [{
+        s3OriginSource: { s3BucketSource, originAccessIdentity },
+        behaviors: [{ isDefaultBehavior: true }],
+      }],
+    });
+
+    expect(stack).to(haveResourceLike('AWS::CloudFront::Distribution', {
+      DistributionConfig: {
+        Origins: [
+          {
+            ConnectionAttempts: 3,
+            ConnectionTimeout: 10,
+            DomainName: {
+              'Fn::GetAtt': [
+                'Bucket83908E77',
+                'RegionalDomainName',
+              ],
+            },
+            Id: 'origin1',
+            S3OriginConfig: {
+              OriginAccessIdentity: {
+                'Fn::Join': ['', ['origin-access-identity/cloudfront/', { Ref: 'OAIE1EFC67F' }]],
+              },
+            },
+          },
+        ],
+      },
+    }));
+
+    expect(stack).to(haveResourceLike('AWS::S3::BucketPolicy', {
+      PolicyDocument: {
+        Statement: [{
+          Action: 's3:GetObject',
+          Principal: {
+            CanonicalUser: { 'Fn::GetAtt': ['OAIE1EFC67F', 'S3CanonicalUserId'] },
+          },
+          Resource: {
+            'Fn::Join': ['', [{ 'Fn::GetAtt': ['Bucket83908E77', 'Arn'] }, '/*']],
+          },
+        }],
+      },
+    }));
+
+    test.done();
+  },
+
+
   'distribution with trusted signers on default distribution'(test: Test) {
     const stack = new cdk.Stack();
     const sourceBucket = new s3.Bucket(stack, 'Bucket');