Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: throw an error when using an encrypted s3 bucket for capturing alb logs #22031

Closed
wants to merge 5 commits into from
Closed

fix: throw an error when using an encrypted s3 bucket for capturing alb logs #22031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc3c3e5
fix:add validation when using an encrypted S3 bucket
Sep 14, 2022
c7d25f7
Merge branch 'main' into bugfix/errorhandling_when_encrypted_bucket
mergify[bot] Oct 2, 2022
773f42a
fix:move check to the child class
Oct 6, 2022
c1730e1
Merge branch 'aws:main' into bugfix/errorhandling_when_encrypted_bucket
devnotfound Oct 6, 2022
9f3cd92
Merge branch 'bugfix/errorhandling_when_encrypted_bucket' of github.c…
Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ const securityGroup2 = new ec2.SecurityGroup(this, 'SecurityGroup2', { vpc });
lb.addSecurityGroup(securityGroup2);
```

NOTE: Enabling ALB logs using an encryption enabled Amazon S3 Bucket is currently unsupported
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is the case? The docs state that

With Network Load Balancer access logs, you can't use AWS managed keys, you must use customer managed keys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the linked issue there's a reproduction repo provided that uses a bucket with encryption set to KMS - toggling this setting on and off will allow deployment to either fail or succeed. Unless I'm mistaken this setting is creating a CMK

Copy link
Author

@devnotfound devnotfound Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is what the ALB doc says

When you enable access logs, you must specify an S3 bucket for the access logs. The bucket must meet the following requirements.

Requirements

The bucket must be located in the same Region as the load balancer. The bucket and the load balancer can be owned by different accounts.

The prefix that you specify must not include AWSLogs. We add the portion of the file name starting with AWSLogs after the bucket name and prefix that you specify.

The only server-side encryption option that's supported is Amazon S3-managed keys (SSE-S3). For more information, see [Amazon S3-managed encryption keys (SSE-S3)](https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingServerSideEncryption.html).

I can change the README to say -
NOTE: Enabling ALB logs using an encryption enabled Amazon S3 Bucket is only supported when using Amazon S3-managed keys (SSE-S3)
Would that help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the BaseLoadBalancer class is used by both the ApplicationLoadBalancer and the NetworkLoadBalancer. If there is a difference between behavior then we need to handle it in the specific subclass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, my bad, should have checked the inheritance tree. I can certainly move this change to the ApplicationLoadBalancer class and rejig the tests


### Conditions

It's possible to route traffic to targets based on conditions in the incoming
Expand Down
14 changes: 14 additions & 0 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as s3 from '@aws-cdk/aws-s3';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { Duration, Lazy, Names, Resource } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
Expand Down Expand Up @@ -415,6 +416,19 @@ export class ApplicationLoadBalancer extends BaseLoadBalancer implements IApplic
...props,
}).attachTo(this);
}

/**
* 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
*/
public logAccessLogs(bucket: s3.IBucket, prefix?: string) {
if (bucket.encryptionKey) {
throw new Error('Encryption key detected. Bucket encryption using KMS keys is unsupported');
}
return super.logAccessLogs(bucket, prefix);
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
"@aws-cdk/core": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"@aws-cdk/region-info": "0.0.0",
"@aws-cdk/aws-kms": "0.0.0",
"constructs": "^10.0.0"
},
"homepage": "https://github.com/aws/aws-cdk",
Expand All @@ -115,6 +116,7 @@
"@aws-cdk/core": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"@aws-cdk/region-info": "0.0.0",
"@aws-cdk/aws-kms": "0.0.0",
"constructs": "^10.0.0"
},
"engines": {
Expand Down
21 changes: 19 additions & 2 deletions packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,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 kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as cdk from '@aws-cdk/core';
import * as elbv2 from '../../lib';
Expand Down Expand Up @@ -147,15 +148,31 @@ describe('tests', () => {

describe('logAccessLogs', () => {

function loggingSetup(): { stack: cdk.Stack, bucket: s3.Bucket, lb: elbv2.ApplicationLoadBalancer } {
function loggingSetup(withEncryption: boolean = false): { stack: cdk.Stack, bucket: s3.Bucket, lb: elbv2.ApplicationLoadBalancer } {
const app = new cdk.App();
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');
let bucketProps = {};
if (withEncryption) {
const myKey = new kms.Key(stack, 'MyKey');
bucketProps = { ...bucketProps, encryption: s3.BucketEncryption.KMS, encryptionKey: myKey };
}
const bucket = new s3.Bucket(stack, 'AccessLoggingBucket', { ...bucketProps });
const lb = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc });
return { stack, bucket, lb };
}

test('should throw an error when a kms bucketkey is detected', () => {
// GIVEN
const { bucket, lb } = loggingSetup(true);

// WHEN
const logAccessLogFunctionTest = () => lb.logAccessLogs(bucket);

// THEN
expect(logAccessLogFunctionTest).toThrow('Encryption key detected. Bucket encryption using KMS keys is unsupported');
});

test('sets load balancer attributes', () => {
// GIVEN
const { stack, bucket, lb } = loggingSetup();
Expand Down