fix(elasticloadbalancingv2): ApplicationLoadBalancer.logAccessLogs does not grant all necessary permissions (#18558)

njlynch · web-flow · commit bde179502933 · 2022-01-24T22:42:44.000Z
`ALB.logAccessLogs` today grants the ELB account Put/Abort on the bucket. The NLB code extends this to also grant permissions to the `delivery.logs.amazonaws.com` service principal. The ALB documentation now states that the permissions required for ALB are the same as NLB, so consolidating the code back into the base. fixes #18367 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-load-balancer.ts
@@ -1,7 +1,5 @@
 import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
 import * as ec2 from '@aws-cdk/aws-ec2';
-import { PolicyStatement, ServicePrincipal } from '@aws-cdk/aws-iam';
-import { IBucket } from '@aws-cdk/aws-s3';
 import * as cxschema from '@aws-cdk/cloud-assembly-schema';
 import { Resource } from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
@@ -125,41 +123,6 @@ export class NetworkLoadBalancer extends BaseLoadBalancer implements INetworkLoa
     });
   }
 
-  /**
-   * Enable access logging for this load balancer.
-   *
-   * A region must be specified on the stack containing the load balancer; you cannot enable logging on
-   * environment-agnostic stacks. See https://docs.aws.amazon.com/cdk/latest/guide/environments.html
-   *
-   * This is extending the BaseLoadBalancer.logAccessLogs method to match the bucket permissions described
-   * at https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-access-logs.html#access-logging-bucket-requirements
-   */
-  public logAccessLogs(bucket: IBucket, prefix?: string) {
-    super.logAccessLogs(bucket, prefix);
-
-    const logsDeliveryServicePrincipal = new ServicePrincipal('delivery.logs.amazonaws.com');
-
-    bucket.addToResourcePolicy(
-      new PolicyStatement({
-        actions: ['s3:PutObject'],
-        principals: [logsDeliveryServicePrincipal],
-        resources: [
-          bucket.arnForObjects(`${prefix ? prefix + '/' : ''}AWSLogs/${this.stack.account}/*`),
-        ],
-        conditions: {
-          StringEquals: { 's3:x-amz-acl': 'bucket-owner-full-control' },
-        },
-      }),
-    );
-    bucket.addToResourcePolicy(
-      new PolicyStatement({
-        actions: ['s3:GetBucketAcl'],
-        principals: [logsDeliveryServicePrincipal],
-        resources: [bucket.bucketArn],
-      }),
-    );
-  }
-
   /**
    * Return the given named metric for this Network Load Balancer
    *
@@ -326,4 +289,4 @@ class LookedUpNetworkLoadBalancer extends Resource implements INetworkLoadBalanc
       ...props,
     });
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts
@@ -1,5 +1,6 @@
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
+import { PolicyStatement, ServicePrincipal } from '@aws-cdk/aws-iam';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as cxschema from '@aws-cdk/cloud-assembly-schema';
 import { ContextProvider, IResource, Lazy, Resource, Stack, Token } from '@aws-cdk/core';
@@ -258,7 +259,27 @@ export abstract class BaseLoadBalancer extends Resource {
       throw new Error(`Cannot enable access logging; don't know ELBv2 account for region ${region}`);
     }
 
+    const logsDeliveryServicePrincipal = new ServicePrincipal('delivery.logs.amazonaws.com');
     bucket.grantPut(new iam.AccountPrincipal(account), `${(prefix ? prefix + '/' : '')}AWSLogs/${Stack.of(this).account}/*`);
+    bucket.addToResourcePolicy(
+      new PolicyStatement({
+        actions: ['s3:PutObject'],
+        principals: [logsDeliveryServicePrincipal],
+        resources: [
+          bucket.arnForObjects(`${prefix ? prefix + '/' : ''}AWSLogs/${this.stack.account}/*`),
+        ],
+        conditions: {
+          StringEquals: { 's3:x-amz-acl': 'bucket-owner-full-control' },
+        },
+      }),
+    );
+    bucket.addToResourcePolicy(
+      new PolicyStatement({
+        actions: ['s3:GetBucketAcl'],
+        principals: [logsDeliveryServicePrincipal],
+        resources: [bucket.bucketArn],
+      }),
+    );
 
     // make sure the bucket's policy is created before the ALB (see https://github.com/aws/aws-cdk/issues/1633)
     this.node.addDependency(bucket);
diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts
@@ -2,7 +2,7 @@ import { Match, Template } from '@aws-cdk/assertions';
 import { Metric } from '@aws-cdk/aws-cloudwatch';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as s3 from '@aws-cdk/aws-s3';
-import { testFutureBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag';
+import { testFutureBehavior, testLegacyBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag';
 import * as cdk from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
 import * as elbv2 from '../../lib';
@@ -146,105 +146,208 @@ describe('tests', () => {
     expect(loadBalancer.listeners).toContain(listener);
   });
 
-  testFutureBehavior('Access logging', s3GrantWriteCtx, cdk.App, (app) => {
-    // GIVEN
-    const stack = new cdk.Stack(app, undefined, { env: { region: 'us-east-1' } });
-    const vpc = new ec2.Vpc(stack, 'Stack');
-    const bucket = new s3.Bucket(stack, 'AccessLoggingBucket');
-    const lb = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc });
+  describe('logAccessLogs', () => {
 
-    // WHEN
-    lb.logAccessLogs(bucket);
+    function loggingSetup(app: cdk.App): { stack: cdk.Stack, bucket: s3.Bucket, lb: elbv2.ApplicationLoadBalancer } {
+      const stack = new cdk.Stack(app, undefined, { env: { region: 'us-east-1' } });
+      const vpc = new ec2.Vpc(stack, 'Stack');
+      const bucket = new s3.Bucket(stack, 'AccessLoggingBucket');
+      const lb = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc });
+      return { stack, bucket, lb };
+    }
 
-    // THEN
+    test('sets load balancer attributes', () => {
+      // GIVEN
+      const app = new cdk.App();
+      const { stack, bucket, lb } = loggingSetup(app);
 
-    // verify that the LB attributes reference the bucket
-    Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
-      LoadBalancerAttributes: Match.arrayWith([
-        {
-          Key: 'access_logs.s3.enabled',
-          Value: 'true',
-        },
-        {
-          Key: 'access_logs.s3.bucket',
-          Value: { Ref: 'AccessLoggingBucketA6D88F29' },
-        },
-        {
-          Key: 'access_logs.s3.prefix',
-          Value: '',
-        },
-      ]),
-    });
+      // WHEN
+      lb.logAccessLogs(bucket);
 
-    // verify the bucket policy allows the ALB to put objects in the bucket
-    Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
-      PolicyDocument: {
-        Version: '2012-10-17',
-        Statement: [
+      //THEN
+      Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
+        LoadBalancerAttributes: Match.arrayWith([
           {
-            Action: ['s3:PutObject', 's3:Abort*'],
-            Effect: 'Allow',
-            Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::127311923021:root']] } },
-            Resource: {
-              'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/AWSLogs/',
-                { Ref: 'AWS::AccountId' }, '/*']],
-            },
+            Key: 'access_logs.s3.enabled',
+            Value: 'true',
           },
-        ],
-      },
+          {
+            Key: 'access_logs.s3.bucket',
+            Value: { Ref: 'AccessLoggingBucketA6D88F29' },
+          },
+          {
+            Key: 'access_logs.s3.prefix',
+            Value: '',
+          },
+        ]),
+      });
     });
 
-    // verify the ALB depends on the bucket *and* the bucket policy
-    Template.fromStack(stack).hasResource('AWS::ElasticLoadBalancingV2::LoadBalancer', {
-      DependsOn: ['AccessLoggingBucketPolicy700D7CC6', 'AccessLoggingBucketA6D88F29'],
+    test('adds a dependency on the bucket', () => {
+      // GIVEN
+      const app = new cdk.App();
+      const { stack, bucket, lb } = loggingSetup(app);
+
+      // WHEN
+      lb.logAccessLogs(bucket);
+
+      // THEN
+      // verify the ALB depends on the bucket *and* the bucket policy
+      Template.fromStack(stack).hasResource('AWS::ElasticLoadBalancingV2::LoadBalancer', {
+        DependsOn: ['AccessLoggingBucketPolicy700D7CC6', 'AccessLoggingBucketA6D88F29'],
+      });
     });
-  });
 
-  testFutureBehavior('access logging with prefix', s3GrantWriteCtx, cdk.App, (app) => {
-    // GIVEN
-    const stack = new cdk.Stack(app, undefined, { env: { region: 'us-east-1' } });
-    const vpc = new ec2.Vpc(stack, 'Stack');
-    const bucket = new s3.Bucket(stack, 'AccessLoggingBucket');
-    const lb = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc });
+    testLegacyBehavior('legacy bucket permissions', cdk.App, (app) => {
+      const { stack, bucket, lb } = loggingSetup(app);
 
-    // WHEN
-    lb.logAccessLogs(bucket, 'prefix-of-access-logs');
+      // WHEN
+      lb.logAccessLogs(bucket);
 
-    // THEN
-    // verify that the LB attributes reference the bucket
-    Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
-      LoadBalancerAttributes: Match.arrayWith([
-        {
-          Key: 'access_logs.s3.enabled',
-          Value: 'true',
-        },
-        {
-          Key: 'access_logs.s3.bucket',
-          Value: { Ref: 'AccessLoggingBucketA6D88F29' },
+      // THEN
+      // verify the bucket policy allows the ALB to put objects in the bucket
+      Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
+        PolicyDocument: {
+          Version: '2012-10-17',
+          Statement: [
+            {
+              Action: ['s3:PutObject*', 's3:Abort*'],
+              Effect: 'Allow',
+              Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::127311923021:root']] } },
+              Resource: {
+                'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/AWSLogs/',
+                  { Ref: 'AWS::AccountId' }, '/*']],
+              },
+            },
+            {
+              Action: 's3:PutObject',
+              Effect: 'Allow',
+              Principal: { Service: 'delivery.logs.amazonaws.com' },
+              Resource: {
+                'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/AWSLogs/',
+                  { Ref: 'AWS::AccountId' }, '/*']],
+              },
+              Condition: { StringEquals: { 's3:x-amz-acl': 'bucket-owner-full-control' } },
+            },
+            {
+              Action: 's3:GetBucketAcl',
+              Effect: 'Allow',
+              Principal: { Service: 'delivery.logs.amazonaws.com' },
+              Resource: {
+                'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'],
+              },
+            },
+          ],
         },
-        {
-          Key: 'access_logs.s3.prefix',
-          Value: 'prefix-of-access-logs',
+      });
+    });
+
+    testFutureBehavior('logging bucket permissions', s3GrantWriteCtx, cdk.App, (app) => {
+      // GIVEN
+      const { stack, bucket, lb } = loggingSetup(app);
+
+      // WHEN
+      lb.logAccessLogs(bucket);
+
+      // THEN
+      // verify the bucket policy allows the ALB to put objects in the bucket
+      Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
+        PolicyDocument: {
+          Version: '2012-10-17',
+          Statement: [
+            {
+              Action: ['s3:PutObject', 's3:Abort*'],
+              Effect: 'Allow',
+              Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::127311923021:root']] } },
+              Resource: {
+                'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/AWSLogs/',
+                  { Ref: 'AWS::AccountId' }, '/*']],
+              },
+            },
+            {
+              Action: 's3:PutObject',
+              Effect: 'Allow',
+              Principal: { Service: 'delivery.logs.amazonaws.com' },
+              Resource: {
+                'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/AWSLogs/',
+                  { Ref: 'AWS::AccountId' }, '/*']],
+              },
+              Condition: { StringEquals: { 's3:x-amz-acl': 'bucket-owner-full-control' } },
+            },
+            {
+              Action: 's3:GetBucketAcl',
+              Effect: 'Allow',
+              Principal: { Service: 'delivery.logs.amazonaws.com' },
+              Resource: {
+                'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'],
+              },
+            },
+          ],
         },
-      ]),
+      });
     });
 
-    // verify the bucket policy allows the ALB to put objects in the bucket
-    Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
-      PolicyDocument: {
-        Version: '2012-10-17',
-        Statement: [
+    testFutureBehavior('access logging with prefix', s3GrantWriteCtx, cdk.App, (app) => {
+      // GIVEN
+      const { stack, bucket, lb } = loggingSetup(app);
+
+      // WHEN
+      lb.logAccessLogs(bucket, 'prefix-of-access-logs');
+
+      // THEN
+      // verify that the LB attributes reference the bucket
+      Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
+        LoadBalancerAttributes: Match.arrayWith([
           {
-            Action: ['s3:PutObject', 's3:Abort*'],
-            Effect: 'Allow',
-            Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::127311923021:root']] } },
-            Resource: {
-              'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/prefix-of-access-logs/AWSLogs/',
-                { Ref: 'AWS::AccountId' }, '/*']],
-            },
+            Key: 'access_logs.s3.enabled',
+            Value: 'true',
+          },
+          {
+            Key: 'access_logs.s3.bucket',
+            Value: { Ref: 'AccessLoggingBucketA6D88F29' },
+          },
+          {
+            Key: 'access_logs.s3.prefix',
+            Value: 'prefix-of-access-logs',
           },
-        ],
-      },
+        ]),
+      });
+
+      // verify the bucket policy allows the ALB to put objects in the bucket
+      Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
+        PolicyDocument: {
+          Version: '2012-10-17',
+          Statement: [
+            {
+              Action: ['s3:PutObject', 's3:Abort*'],
+              Effect: 'Allow',
+              Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::127311923021:root']] } },
+              Resource: {
+                'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/prefix-of-access-logs/AWSLogs/',
+                  { Ref: 'AWS::AccountId' }, '/*']],
+              },
+            },
+            {
+              Action: 's3:PutObject',
+              Effect: 'Allow',
+              Principal: { Service: 'delivery.logs.amazonaws.com' },
+              Resource: {
+                'Fn::Join': ['', [{ 'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'] }, '/prefix-of-access-logs/AWSLogs/',
+                  { Ref: 'AWS::AccountId' }, '/*']],
+              },
+              Condition: { StringEquals: { 's3:x-amz-acl': 'bucket-owner-full-control' } },
+            },
+            {
+              Action: 's3:GetBucketAcl',
+              Effect: 'Allow',
+              Principal: { Service: 'delivery.logs.amazonaws.com' },
+              Resource: {
+                'Fn::GetAtt': ['AccessLoggingBucketA6D88F29', 'Arn'],
+              },
+            },
+          ],
+        },
+      });
     });
   });
 
