feat(elasticloadbalancingv2): support Mutual Authentication with TLS for Application Load Balancer by mazyu36 · Pull Request #30784 · aws/aws-cdk

mazyu36 · 2024-07-08T09:33:00Z

Issue # (if applicable)

Closes #28206.

Reason for this change

To support mTLS for ALB

Description of changes

Add TrustStore and TrustStoreRevocation class
Add MutualAuthentication property for ApplicationListener

Description of how you validated changes

add unit tests and integ tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mazyu36 · 2024-07-08T09:45:20Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

+
+  constructor(scope: Construct, id: string, props: TrustStoreProps) {
+    super(scope, id, {
+      physicalName: props.trustStoreName ?? Lazy.string({


In CFN document, name property is optional.

However, deploying without setting it resulted in an error, so I made it generate a name instead.

mazyu36 · 2024-07-08T09:46:50Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

+  /**
+   * The bucket that the trust store is hosted in
+   */
+  readonly bucket: IBucket;


In CFn document, this property is optional.
However, deploying without setting it resulted in an error, so I made it required.

Also, the documentation states the following:

You must specify CaCertificatesBundleS3Bucket and CaCertificatesBundleS3Key.

mazyu36 · 2024-07-08T09:47:02Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

+  /**
+   * The key in S3 to look at for the trust store
+   */
+  readonly key: string;


In CFn document, this property is optional.
However, deploying without setting it resulted in an error, so I made it required.

Also, the documentation states the following:

You must specify CaCertificatesBundleS3Bucket and CaCertificatesBundleS3Key.

mazyu36 · 2024-07-08T09:48:32Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store-revocation.ts

+  /**
+   * The Amazon S3 bucket for the revocation file
+   */
+  readonly bucket: IBucket;


In CFn document, this property is optional.
However, deploying without setting it resulted in an error, so I made it required.

Also, the documentation states the following:

You must specify S3Bucket and S3Key.

mazyu36 · 2024-07-08T09:48:38Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store-revocation.ts

+  /**
+   * The Amazon S3 path for the revocation file
+   */
+  readonly key: string;


In CFn document, this property is optional.
However, deploying without setting it resulted in an error, so I made it required.

Also, the documentation states the following:

You must specify S3Bucket and S3Key.

mazyu36 · 2024-07-08T09:50:45Z

packages/aws-cdk-lib/awslint.json

    "props-physical-name:aws-cdk-lib.aws_elasticloadbalancing.LoadBalancerProps",
    "props-physical-name:aws-cdk-lib.aws_elasticloadbalancingv2.ApplicationListenerProps",
    "props-physical-name:aws-cdk-lib.aws_elasticloadbalancingv2.NetworkListenerProps",
+    "props-physical-name:aws-cdk-lib.aws_elasticloadbalancingv2.TrustStoreRevocationProps",


AWS::ElasticLoadBalancingV2::TrustStoreRevocation doesn't have name property, so I've added.

mazyu36 · 2024-07-08T15:47:02Z

packages/@aws-cdk-testing/framework-integ/test/aws-elasticloadbalancingv2/test/mtls/crl.pem

@@ -0,0 +1 @@
+dummy


I thought it wasn't good to store the pem file, but I decided to place a dummy file because the build fails if the file doesn't exist. When actually running the integration test, you need to create the pem file using a command.
If there's a better way, please let me know.

mazyu36 · 2024-07-08T15:47:28Z

...s/@aws-cdk-testing/framework-integ/test/aws-elasticloadbalancingv2/test/mtls/rootCA_cert.pem

@@ -0,0 +1 @@
+dummy


I thought it wasn't good to store the pem file, but I decided to place a dummy file because the build fails if the file doesn't exist. When actually running the integration test, you need to create the pem file using a command.
If there's a better way, please let me know.

lpizzinidev

Nice 👍 Left comments for minor adjustments

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

lpizzinidev · 2024-07-30T13:18:52Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

+      }),
+    });
+
+    const resource = new CfnTrustStore(this, 'Resource', {


Can you please add validation for props.trustStoreName?

Thanks. I've added.

lpizzinidev · 2024-07-30T13:24:16Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

+/**
+ * The client certificate handling method
+ */
+export enum Mode {


Suggested change

export enum Mode {

export enum MutualAuthenticationMode {

More meaningful?

Thanks. I've updated.

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

…store.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

mazyu36 · 2024-08-02T02:44:02Z

@Leo10Gama
Thank you for the review. I've addressed all your comments.

packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-store.ts

xazhao · 2024-08-02T20:44:25Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

+    if (props.mutualAuthentication) {
+      const currentMode = props.mutualAuthentication.mutualAuthenticationMode;
+
+      if (currentMode === MutualAuthenticationMode.VERIFY && !props.mutualAuthentication.trustStore) {


I think these two if blocks can be merged with something like this.

if props.mutualAuthentication.trustStore if currentMode !== MutualAuthenticationMode.VERIFY throw new Error ('trustStore can only be set when the mode is verify'

Thank you. I've extracted the validation into a separate function to make it cleaner.
What do you think of this approach?

https://github.com/aws/aws-cdk/pull/30784/files#diff-7bb91966b981c92a62f92075cd5bc4de58634ddcbe15dd492b0466f24b49383cR1041

Leo10Gama

Thanks for the quick response and changes! Just a few nits and typo fixes, but otherwise LGTM

…ner.test.ts Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Pull request has been modified.

Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

…store.ts Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

mazyu36 · 2024-08-03T09:17:02Z

@Leo10Gama
Thanks.
I’ve addressed all comments.

Leo10Gama

LGTM, thanks for your contribution!

mergify · 2024-08-06T17:11:35Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-08-06T17:43:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 366f8a8
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-08-06T17:43:32Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-08-06T17:43:48Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

aws-cdk-automation requested a review from a team July 8, 2024 09:33

github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jul 8, 2024

mazyu36 commented Jul 8, 2024

View reviewed changes

mazyu36 force-pushed the alb-mtls-28206 branch 5 times, most recently from 6dde3d4 to 0bbe528 Compare July 8, 2024 12:58

feat: alb mtls

98bba74

mazyu36 force-pushed the alb-mtls-28206 branch from 0bbe528 to 98bba74 Compare July 8, 2024 13:31

mazyu36 marked this pull request as ready for review July 8, 2024 13:50

mazyu36 added 3 commits July 8, 2024 22:50

Merge branch 'main' into alb-mtls-28206

6b46d25

fix

71f2bbf

fix README.md

5da43b4

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 8, 2024

mazyu36 commented Jul 8, 2024

View reviewed changes

github-actions bot mentioned this pull request Jul 15, 2024

Weekly PR metrics report #30858

Closed

lpizzinidev suggested changes Jul 30, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 30, 2024

mazyu36 and others added 2 commits July 30, 2024 23:56

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md

b0d5e4d

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-…

d9c3852

…store.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

Leo10Gama reviewed Aug 2, 2024

View reviewed changes

xazhao reviewed Aug 2, 2024

View reviewed changes

Leo10Gama previously requested changes Aug 2, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 2, 2024

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/liste…

8fe139c

…ner.test.ts Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 3, 2024

mazyu36 and others added 10 commits August 3, 2024 10:52

Update listener.test.ts

c395f73

Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Update listener.test.ts

ae81045

Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Update application-listener.ts

9453cfc

Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Update trust-store-revocation.ts

e9e91a3

Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Update trust-store.ts

8907791

Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-…

b4cbedf

…store.ts Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

Update packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/trust-…

4e4eaea

…store.ts Co-authored-by: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com>

fix unit tests

4c3d37d

add validation

0b52315

Merge branch 'main' into alb-mtls-28206

a2e994c

mazyu36 requested a review from Leo10Gama August 3, 2024 09:17

Leo10Gama approved these changes Aug 6, 2024

View reviewed changes

Merge branch 'main' into alb-mtls-28206

366f8a8

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 6, 2024

mergify bot merged commit 7eae4d1 into aws:main Aug 6, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2024

mazyu36 deleted the alb-mtls-28206 branch August 6, 2024 22:38

Conversation

mazyu36 commented Jul 8, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mazyu36 commented Aug 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leo10Gama left a comment

Choose a reason for hiding this comment

Uh oh!

mazyu36 commented Aug 3, 2024

Uh oh!

Leo10Gama left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 6, 2024

Uh oh!

aws-cdk-automation commented Aug 6, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 6, 2024

Uh oh!

github-actions bot commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

mazyu36 Jul 8, 2024 •

edited

Loading

mazyu36 Jul 8, 2024 •

edited

Loading

mazyu36 Jul 8, 2024 •

edited

Loading

mazyu36 Jul 8, 2024 •

edited

Loading