Skip to content

Commit

Permalink
fix(elasticloadbalancingv2): ApplicationLoadBalancer.logAccessLogs do…
Browse files Browse the repository at this point in the history 
…es not grant all necessary permissions (aws#18558)

`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 aws#18367


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@njlynch @TikiTDO
njlynch authored and TikiTDO committed Feb 21, 2022
1 parent 888f6b9 commit 6de5d06
Show file tree
Hide file tree
Showing 3 changed files with 207 additions and 120 deletions.
39 changes: 1 addition & 38 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-load-balancer.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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
*
Expand Down Expand Up @@ -326,4 +289,4 @@ class LookedUpNetworkLoadBalancer extends Resource implements INetworkLoadBalanc
...props,
});
}
}
}
21 changes: 21 additions & 0 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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);
Expand Down
267 changes: 185 additions & 82 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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'],
},
},
],
},
});
});
});

Expand Down

0 comments on commit 6de5d06

Please sign in to comment.