feat(appmesh): allow configuring mutual TLS #15101

Seiya6329 · 2021-06-11T22:18:14Z

This is the last part of the series to implement mTLS feature.
For the breakdown on series, please refer to this comment.

Collaborators

@alexbrjo and @dfezzie. Thank you!

REV

Adding SDS certificate source to TlsCertificate and TlsValidationTrust.
Adding subjectAlternativeNames property to TlsValidation.
Adding mutualTlsCertificate property to ClientPolicyTlsOptions
Adding mutualTlsValidation property to ListenerTlsOptions
Updating README to include mTLS implementation example.
Updating TlsValidationTrustConfig to have single property

Design Note:

Client certificates in a TLS Client Policy and server validation in a listener TLS configuration can only be sourced from SDS or File certificate. ACM certificate is currently not supported (reference doc)
- Compile-error is implemented to block assigning ACM certificate.
TlsValidationTrustConfig is updated to have single property to reduce the repetitions since all four properties share same shape.
SubjectAlternativeNames is modeled as an abstract class to include the factory method. In future if more match patterns are added, use or() method to extend.

Closes #12733.

BREAKING CHANGE: static methods from TlsValidationTrust have been changed to accept positional arguments

appmesh: static methods from TlsCertificate have been changed to accept positional arguments
appmesh: the type TlsListener has been renamed to ListenerTlsOptions

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

gitpod-io · 2021-06-11T22:18:16Z

mergify · 2021-06-11T22:18:52Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

skinny85

I have to say, I have some trouble following the justification for many of the changes in this PR.

@Seiya6329 I've left you a few comments, and a lot of questions. Would appreciate some clarification 🙂.

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway-listener.ts

packages/@aws-cdk/aws-appmesh/lib/virtual-node-listener.ts

packages/@aws-cdk/aws-appmesh/test/test.virtual-gateway.ts

Pull request has been modified.

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway-listener.ts

skinny85

Thanks a lot for the answers to my questions @Seiya6329! They were super helpful. I now have a good vision of the goals of this PR, and how it should look.

Let me know what you think of my proposed changes!

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/private/utils.ts

packages/@aws-cdk/aws-appmesh/lib/tls-certificate.ts

packages/@aws-cdk/aws-appmesh/lib/tls-client-policy.ts

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/tls-client-policy.ts

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts

packages/@aws-cdk/aws-appmesh/lib/tls-certificate.ts

Pull request has been modified.

Seiya6329 · 2021-06-18T23:09:41Z

REV#3:

Replacing to use compile-time error to restrict ACM certificate source to be selected for mTLS
Changing the name for:
- TlsListener => ListenerTls
- TlsClientPolicy => ClientPolicyTls
Updating TlsCertificate and TlsValidationTrust's factory methods to use positional argument instead of props
Updating README to reflect changes.

dfezzie

Overall LGTM. Just a nit about a comment and I agree with the SAN match change.

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts

Seiya6329 · 2021-06-21T21:27:30Z

REV4:

Changing the SubjectAlternativeNames to an abstract class with the factory method
Renaming few objects
- ClientPolicyTls => `ClientPolicyTlsOptions
- 'ListernerTls=>ListenerTlsOptions`
- MutualTLS... => MutualTls...
- SubjectiveAlternativeNames => SubjectAlternativeNames
Updating the @default for subjectAlternativeNames property

skinny85

This is really looking great! Some comments, mainly related to naming.

Also, make sure to edit the description of the PR to include all of the breaking changes that are made as part of this PR, like you did in #14856 (comment).

packages/@aws-cdk/aws-appmesh/lib/shared-interfaces.ts

packages/@aws-cdk/aws-appmesh/lib/listener-tls-options.ts

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts

packages/@aws-cdk/aws-appmesh/lib/private/utils.ts

packages/@aws-cdk/aws-appmesh/test/test.virtual-node.ts

Seiya6329 · 2021-06-21T23:36:37Z

Thanks @skinny85 for pointing out about the breaking change! I totally overlooked at it since my original plan was not to introduce any of them.

Pull request has been modified.

Seiya6329 · 2021-06-22T21:51:38Z

REV5:

Renaming
- classes from MutualTlsAuthEligible... => MutualTls...
- property from mutualTlsAuth... -> mutualTls...
- the interface
- unused error check
- the method
- static method in the SubjectAlternativeNames
Updating README to use up-to-date property names

Seiya6329 · 2021-06-23T00:06:34Z

REV6

Changing the matchingExactly() to accept rest parameter
Reverting back tlsClientPolicy name

Seiya6329 · 2021-06-23T00:08:44Z

@skinny85 - I think I was able to address all the pending feedback and updated the main comment. Thanks for working along with me on this!

skinny85

The code looks perfect @Seiya6329! Fix the few remaining ReadMe issues, and we'll merge this in.

packages/@aws-cdk/aws-appmesh/README.md

Seiya6329 · 2021-06-23T18:55:23Z

REV7:

Fixing the property names in README

skinny85

Looks great!

Pull request has been modified.

mergify · 2021-06-23T20:38:16Z

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

aws-cdk-automation · 2021-06-23T21:15:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: b14f72e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-06-23T21:15:41Z

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

@alexbrjo

This is the last part of the series to implement mTLS feature. For the breakdown on series, please refer to [this comment](aws#14782 (comment)). #### Collaborators @alexbrjo and @dfezzie. Thank you! #### REV - Adding SDS certificate source to `TlsCertificate` and `TlsValidationTrust`. - Adding `subjectAlternativeNames` property to `TlsValidation`. - Adding `mutualTlsCertificate` property to `ClientPolicyTlsOptions` - Adding `mutualTlsValidation` property to `ListenerTlsOptions` - Updating `README` to include mTLS implementation example. - Updating `TlsValidationTrustConfig` to have single property #### Design Note: - Client certificates in a TLS Client Policy and server validation in a listener TLS configuration can only be sourced from `SDS` or `File` certificate. ACM certificate is currently not supported ([reference doc](https://docs.aws.amazon.com/app-mesh/latest/userguide/mutual-tls.html)) - Compile-error is implemented to block assigning ACM certificate. - `TlsValidationTrustConfig` is updated to have single property to reduce the repetitions since all four properties share same shape. - `SubjectAlternativeNames` is modeled as an abstract class to include the factory method. In future if more match patterns are added, use `or()` method to extend. Closes aws#12733. BREAKING CHANGE: static methods from `TlsValidationTrust` have been changed to accept positional arguments - **appmesh**: static methods from `TlsCertificate` have been changed to accept positional arguments - **appmesh**: the type `TlsListener` has been renamed to `ListenerTlsOptions` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Seiya6329 force-pushed the mTLS branch from 30eecbd to a8953cf Compare June 13, 2021 07:21

Seiya6329 changed the title ~~feat(appmesh) : implementing mutual TLS feature~~ feat(appmesh): implementing mutual TLS feature Jun 13, 2021

Seiya6329 force-pushed the mTLS branch 2 times, most recently from a104a0d to 5f0795c Compare June 13, 2021 07:26

skinny85 previously requested changes Jun 15, 2021

View reviewed changes

Seiya6329 requested a review from skinny85 June 16, 2021 17:20

skinny85 reviewed Jun 16, 2021

View reviewed changes

packages/@aws-cdk/aws-appmesh/README.md Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway-listener.ts Outdated Show resolved Hide resolved

skinny85 previously requested changes Jun 16, 2021

View reviewed changes

peterwoodworth assigned skinny85 Jun 17, 2021

peterwoodworth added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Jun 17, 2021

Seiya6329 force-pushed the mTLS branch from cd08541 to 00e6f16 Compare June 18, 2021 22:45

dfezzie reviewed Jun 21, 2021

View reviewed changes

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts Show resolved Hide resolved

packages/@aws-cdk/aws-appmesh/lib/tls-validation.ts Outdated Show resolved Hide resolved

Seiya6329 requested a review from skinny85 June 21, 2021 21:27

skinny85 previously requested changes Jun 21, 2021

View reviewed changes

skinny85 added effort/small Small work item – less than a day of effort p1 labels Jun 22, 2021

Seiya6329 requested a review from skinny85 June 22, 2021 21:52

Seiya6329 added 4 commits June 22, 2021 16:54

feat(appmesh): implementing mutual TLS feature

ddaca59

Linting README file

4fbb055

Updating README and tests

7cac2ae

Updating logic for certificate source eligibility and other nits

f52a0d1

Seiya6329 added 5 commits June 22, 2021 17:01

Modeling SANs as an abstract class and renaming few objects

16ad83e

Changing the object name to MutualTls...

1e9766c

Fixing README and parameter names

e4136c8

Renaming clases to mutualTls...

43182d8

Reverting tlsClientPolicy name and using rest parameter

c653f83

Seiya6329 force-pushed the mTLS branch from ab77589 to c653f83 Compare June 23, 2021 00:02

Minor update on javastring

7b66269

skinny85 reviewed Jun 23, 2021

View reviewed changes

Fixing README discrepancies

e97cdbb

Seiya6329 requested a review from skinny85 June 23, 2021 19:12

skinny85 added 2 commits June 23, 2021 13:00

exactMatch -> matchingExactly in ReadMe for SubjectAlternativeNames

4e49c4e

Change SubjectAlternativeNames to use variadic arguments

dfa1313

skinny85 previously approved these changes Jun 23, 2021

View reviewed changes

skinny85 approved these changes Jun 23, 2021

View reviewed changes

Merge branch 'master' into mTLS

b14f72e

skinny85 changed the title ~~feat(appmesh): implementing mutual TLS feature~~ feat(appmesh): allow configuring mutual TLS Jun 23, 2021

mergify bot merged commit e47a01f into aws:master Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appmesh): allow configuring mutual TLS #15101

feat(appmesh): allow configuring mutual TLS #15101

Seiya6329 commented Jun 11, 2021 •

edited by skinny85

gitpod-io bot commented Jun 11, 2021

mergify bot commented Jun 11, 2021

skinny85 left a comment

skinny85 left a comment

Seiya6329 commented Jun 18, 2021

dfezzie left a comment

Seiya6329 commented Jun 21, 2021 •

edited

skinny85 left a comment

Seiya6329 commented Jun 21, 2021

Seiya6329 commented Jun 22, 2021 •

edited

Seiya6329 commented Jun 23, 2021

Seiya6329 commented Jun 23, 2021

skinny85 left a comment

Seiya6329 commented Jun 23, 2021

skinny85 left a comment

mergify bot commented Jun 23, 2021

aws-cdk-automation commented Jun 23, 2021

mergify bot commented Jun 23, 2021

feat(appmesh): allow configuring mutual TLS #15101

feat(appmesh): allow configuring mutual TLS #15101

Conversation

Seiya6329 commented Jun 11, 2021 • edited by skinny85

Collaborators

REV

Design Note:

gitpod-io bot commented Jun 11, 2021

mergify bot commented Jun 11, 2021

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Seiya6329 commented Jun 18, 2021

dfezzie left a comment

Choose a reason for hiding this comment

Seiya6329 commented Jun 21, 2021 • edited

REV4:

skinny85 left a comment

Choose a reason for hiding this comment

Seiya6329 commented Jun 21, 2021

Seiya6329 commented Jun 22, 2021 • edited

Seiya6329 commented Jun 23, 2021

REV6

Seiya6329 commented Jun 23, 2021

skinny85 left a comment

Choose a reason for hiding this comment

Seiya6329 commented Jun 23, 2021

REV7:

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 23, 2021

aws-cdk-automation commented Jun 23, 2021

AWS CodeBuild CI Report

mergify bot commented Jun 23, 2021

Seiya6329 commented Jun 11, 2021 •

edited by skinny85

Seiya6329 commented Jun 21, 2021 •

edited

Seiya6329 commented Jun 22, 2021 •

edited