fix(cloudfront): bucket policy for Origin Access Identities is overly…

… 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*
NovakGu · Feb 18, 2021 · 6ddd9d7 · 6ddd9d7
1 parent fa105a8
commit 6ddd9d7
Show file tree

Hide file tree

Showing 8 changed files with 125 additions and 85 deletions.
diff --git a/packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts b/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);
   }

diff --git a/packages/@aws-cdk/aws-cloudfront-origins/package.json b/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"

diff --git a/packages/@aws-cdk/aws-cloudfront-origins/test/integ.origin-group.expected.json b/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"

diff --git a/packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json b/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"

diff --git a/packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts b/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'] }, '/*']],
+          },
         }],
       },
     });

diff --git a/packages/@aws-cdk/aws-cloudfront/lib/web-distribution.ts b/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}`,

diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json b/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"

diff --git a/packages/@aws-cdk/aws-cloudfront/test/web-distribution.test.ts b/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');