fix: throw an error when using an encrypted s3 bucket for capturing alb logs #22031
Conversation
|
|
github-actions
bot
added
bug
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yea, my bad, should have checked the inheritance tree. I can certainly move this change to the ApplicationLoadBalancer class and rejig the tests
|
@Mergifyio update |
✅ Branch has been successfully updated |
There was a problem hiding this comment.
The Pull Request Linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.
PRs must pass status checks before we can provide a meaningful review.
devnotfound
changed the title
devnotfound
changed the title
…om:devnotfound/aws-cdk into bugfix/errorhandling_when_encrypted_bucket sync-fork
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
In addition to the previous feedback, please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests).
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
aws-cdk-automation
added
the
closed-for-staleness
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. |
#29382) ### Issue # (if applicable) Closes #22031. ### Reason for this change Adds a validation with correct error indicating ALB Access log bucket does not support KMS encryption ### Description of changes Currently access logs bucket encryption with KMS is not supported in case of ALB but while deploying it throws an error indicating the failure with bucket permissions. This validation introduces an upfront check to throw an error if `bucket.encryptionKey `is defined. Documentation: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/enable-access-logging.html ### Description of how you validated changes Added unit tests for validation. ### Checklist - [ ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Currently if one tries to capture alb logs into an encrypted S3 bucket that uses a KMS key, they get an
Access Denied for bucketerror. This association is currently unsupported, however it delays the error at the time of the bucket association.This PR now introduces an upfront check that would error saying
Encryption key detected. Bucket encryption using KMS keys is unsupportedbefore the alb to s3 bucket log association is established.This closes #21947
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license