feat(apigateway): mTLS support #10521

doug-ferris-mondo · 2020-09-25T00:52:13Z

Resolves #10487

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

nija-at · 2020-09-30T08:21:08Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

+   * The mutual TLS authentication configuration for a custom domain name.
+   * @default - mTLS is not configured.
+   */
+  readonly mutualTlsAuthentication?: MutualTlsAuthenticationAttributes


mTLS is the industry standard abbreviation. Let's use that.

Suggested change

readonly mutualTlsAuthentication?: MutualTlsAuthenticationAttributes

readonly mtls?: MTLSAttributes

nija-at · 2020-09-30T08:25:14Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

+export interface MutualTlsAuthenticationAttributes {
+  /**
+   * An Amazon S3 URL that specifies the truststore for mutual TLS authentication, for example, s3://bucket-name/key-name.
+   * The truststore can contain certificates from public or private certificate authorities.
+   * To update the truststore, upload a new version to S3, and then update your custom domain name to use the new version.
+   * To update the truststore, you must have permissions to access the S3 object.
+   */
+  readonly truststoreUri: string;
+
+  /**
+   *  The version of the S3 object that contains your truststore.
+   *  To specify a version, you must have versioning enabled for the S3 bucket.
+   *  @default - Uses current version
+   */
+  readonly truststoreVersion?: string;
+}


Since this is a reference to an S3 object, this can be -

export interface MTLSAttributes { readonly bucket: s3.IBucket; readonly key: string; readonly version?: string; }

I was just starting to refactor towards this as well. Good to know I was going down the right path.

nija-at

Please also look into the build and linter failures.

doug-ferris-mondo · 2020-09-30T09:26:56Z

Please also look into the build and linter failures.

Had a look and was fixing this afternoon, the linting failure was lack of updated documentation, while doing the documentation I started to go down the process of refactoring the MTLS attributes similar to how you did. I'll get this updated tomorrow.

doug-ferris-mondo · 2020-09-30T23:54:33Z

I did a fresh clone based on the latest master, ran cfn2ts again and noticed that it did not have the relevant fields, so as mentioned in the #10521, this may have worked on paper due to the fact I had done a bump (and need to rebase to remove it).
I've moved this to draft pending 1.65.0 to see if that hopefully includes the bump I need to make this usable.

doug-ferris-mondo · 2020-10-01T07:04:47Z

Even after pulling 1.65.0, I'm seeing the field missing on a fresh clone of this repo and doing a cfn2ts.
I know in my issue I mentioned that I had accidentally done a bump which lead to this building locally, so I'm guessing there may be a difference of spec that was used when I first built this
@nija-at, is there anything I should be looking at to verify if a bump has happened upstream yet?

nija-at · 2020-10-01T15:06:15Z

Not sure about 1.65.0 but master should have this property included. It's now included in the changelog and I can also see it in the CF spec we clone.

If you rebase with the latest master branch and re-build the project, this propery should exist in the CfnDomainName construct.

Pull request has been modified.

nija-at

Thanks for addressing the previous comments. I have a few more around the implementation.

nija-at · 2020-10-05T15:05:02Z

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

@@ -562,6 +563,24 @@ Authorizers can also be passed via the `defaultMethodOptions` property within th
 explicitly overridden, the specified defaults will be applied across all `Method`s across the `RestApi` or across all `Resource`s,
 depending on where the defaults were specified.

+## Mutal TLS (mTLS)


Suggested change

## Mutal TLS (mTLS)

## Mutual TLS (mTLS)

nija-at · 2020-10-05T15:05:12Z

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

@@ -29,6 +29,7 @@ running on AWS Lambda, or any web application.
  - [IAM-based authorizer](#iam-based-authorizer)
  - [Lambda-based token authorizer](#lambda-based-token-authorizer)
  - [Lambda-based request authorizer](#lambda-based-request-authorizer)
+- [MTLS](#mutal-tls-mtls)


Suggested change

- [MTLS](#mutal-tls-mtls)

- [Mutual TLS](#mutual-tls-mtls)

nija-at · 2020-10-05T15:08:34Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

@@ -102,17 +110,25 @@ export class DomainName extends Resource implements IDomainName {

    const endpointType = props.endpointType || EndpointType.REGIONAL;
    const edge = endpointType === EndpointType.EDGE;
-
+    const getMTLSConfiguration = (mtlsConfig?: MTLSAttributes): CfnDomainName.MutualTlsAuthenticationProperty | undefined => {


Use function declaration instead of function expression. It can either be a private function of this class or a standalone function outside of the scope of this class.

Pushed a commit where I tried this just to verify I've done this in the way you expected.
I moved getMTLSConfiguration into a function instead of being in the constructor (note, I only put the function here to make this diff clean, I was going to put it elsewhere).

diff --git a/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts b/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts index a59b71892..b490c1c80 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts @@ -86,6 +86,14 @@ export interface IDomainName extends IResource { } +function getMTLSConfiguration(mtlsConfig?: MTLSConfig): CfnDomainName.MutualTlsAuthenticationProperty | undefined { + if (!mtlsConfig) return undefined; + return { + truststoreUri: mtlsConfig.bucket.s3UrlForObject(mtlsConfig.key), + truststoreVersion: mtlsConfig.version, + }; +}; + export class DomainName extends Resource implements IDomainName { /** @@ -115,7 +123,7 @@ export class DomainName extends Resource implements IDomainName { throw new Error('domainName does not support uppercase letters. ' + `got: '${props.domainName}'`); } - const mtlsConfig = this.getMTLSConfiguration(props.mtls); + const mtlsConfig = getMTLSConfiguration(props.mtls); const resource = new CfnDomainName(this, 'Resource', { domainName: props.domainName, certificateArn: edge ? props.certificate.certificateArn : undefined, @@ -153,14 +161,6 @@ export class DomainName extends Resource implements IDomainName { restApi: targetApi, ...options, }); - }

But now I get a jsii build error

[2020-10-06T08:57:54.987] [ERROR] jsii/compiler - Error during type model analysis: TypeError: Cannot read property 'getJsDocTags' of undefined TypeError: Cannot read property 'getJsDocTags' of undefined at _hasInternalJsDocTag (/home/dferris/aws-cdk/node_modules/jsii/lib/assembler.js:1617:19) at Assembler._isPrivateOrInternal (/home/dferris/aws-cdk/node_modules/jsii/lib/assembler.js:988:37) at Assembler._visitClass (/home/dferris/aws-cdk/node_modules/jsii/lib/assembler.js:834:26) at async Assembler._visitNode (/home/dferris/aws-cdk/node_modules/jsii/lib/assembler.js:579:24) at async Promise.all (index 86) at async Assembler.emit (/home/dferris/aws-cdk/node_modules/jsii/lib/assembler.js:102:9) at async Compiler._consumeProgram (/home/dferris/aws-cdk/node_modules/jsii/lib/compiler.js:148:30) at async /home/dferris/aws-cdk/node_modules/jsii/bin/jsii.js:70:39

I've tried it as both a private function on the class as well as standalone, still get the same behaviour.

As is probably clear, this is my first time working with jsii, so I'm not 100% clear about the why this is occurring.

If you have any suggestions on how to format my code to have the same intent but get around this, it would be greatly appreciated.

The problem is likely because the method has a get prefix. Changing this to mtlsConfiguration() should fix this.

Let me know if not and I'll dig into this.

I've tried making this diff locally

diff --git a/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts b/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts index a59b71892..71371765b 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/domain-name.ts @@ -86,6 +86,7 @@ export interface IDomainName extends IResource { } + export class DomainName extends Resource implements IDomainName { /** @@ -107,7 +108,6 @@ export class DomainName extends Resource implements IDomainName { constructor(scope: Construct, id: string, props: DomainNameProps) { super(scope, id); - const endpointType = props.endpointType || EndpointType.REGIONAL; const edge = endpointType === EndpointType.EDGE; @@ -115,13 +115,13 @@ export class DomainName extends Resource implements IDomainName { throw new Error('domainName does not support uppercase letters. ' + `got: '${props.domainName}'`); } - const mtlsConfig = this.getMTLSConfiguration(props.mtls); + // const mtlsConfig = this.mtlsConfiguration(props.mtls); const resource = new CfnDomainName(this, 'Resource', { domainName: props.domainName, certificateArn: edge ? props.certificate.certificateArn : undefined, regionalCertificateArn: edge ? undefined : props.certificate.certificateArn, endpointConfiguration: { types: [endpointType] }, - mutualTlsAuthentication: mtlsConfig, + // mutualTlsAuthentication: , securityPolicy: props.securityPolicy, }); @@ -153,15 +153,15 @@ export class DomainName extends Resource implements IDomainName { restApi: targetApi, ...options, }); - } - - private getMTLSConfiguration(mtlsConfig?: MTLSConfig): CfnDomainName.MutualTlsAuthenticationProperty | undefined { - if (!mtlsConfig) return undefined; - return { - truststoreUri: mtlsConfig.bucket.s3UrlForObject(mtlsConfig.key), - truststoreVersion: mtlsConfig.version, - }; }; + + // private mtlsConfiguration(mtlsConfig?: MTLSConfig): CfnDomainName.MutualTlsAuthenticationProperty | undefined { + // if (!mtlsConfig) return { }; + // return { + // truststoreUri: mtlsConfig.bucket.s3UrlForObject(mtlsConfig.key), + // truststoreVersion: mtlsConfig.version, + // }; + // }; }

Still getting the same error.
If you could have a look, that would be fantastic.
I would have thought me commenting out the code would have ruled out if it was my code directly.

To clarify the use of return {}
I had tried return undefined and return mtlsConfig and was just trying to try things to try and narrow down the scope of what was causing the break.
Running yarn build from inside of the aws-apigateway package threw the error every time, couldn't figure out why it was breaking

Sure. I'll take a look.

Lol, it seems the problem is the trailing semicolon at the end of the private method body. JSII seems to not like that. Remove that and you're problem will be fixed 😊

We're tracking this here - aws/jsii#2098

nija-at · 2020-10-05T15:08:54Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

@@ -102,17 +110,25 @@ export class DomainName extends Resource implements IDomainName {

    const endpointType = props.endpointType || EndpointType.REGIONAL;
    const edge = endpointType === EndpointType.EDGE;
-
+    const getMTLSConfiguration = (mtlsConfig?: MTLSAttributes): CfnDomainName.MutualTlsAuthenticationProperty | undefined => {
+      if (!mtlsConfig) return mtlsConfig;


Suggested change

if (!mtlsConfig) return mtlsConfig;

if (!mtlsConfig) return undefined;

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

nija-at · 2020-10-05T16:16:37Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

+/**
+ * The mTLS authentication configuration for a custom domain name.
+ */
+export interface MTLSAttributes {


Sorry, I missed this in the first round of reviews. We usually use the Attributes suffix in conjunction with import APIs - https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md#from-attributes.

What do you think about switching this to MTLSConfig instead?

nija-at · 2020-10-05T16:17:40Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

+  /**
+   *  The version of the S3 object that contains your truststore.
+   *  To specify a version, you must have versioning enabled for the S3 bucket.
+   *  @default - Uses current version


Suggested change

* @default - Uses current version

* @default - latest version

Pull request has been modified.

nija-at

Everything looks great. The only remaining piece is to fix the JSII build failure. See #10521 (comment)

Let me know if that doesn't help.

Pull request has been modified.

gitpod-io · 2020-10-12T22:21:01Z

doug-ferris-mondo · 2020-10-14T21:49:40Z

Everything looks great. The only remaining piece is to fix the JSII build failure. See #10521 (comment)

Let me know if that doesn't help.

All sorted, you were right about the trailing semicolon, thanks for the pointer there, would not have known where to start!

nija-at

Nice!

mergify · 2020-10-15T08:42:08Z

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 · 2020-10-15T09:38:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b64504f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-10-15T09:39:00Z

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).

SomayaB assigned nija-at Sep 25, 2020

nija-at changed the title ~~feat(api-gateway): Configured mutal TLS options~~ feat(apigateway): mTLS support Sep 30, 2020

nija-at previously requested changes Sep 30, 2020

View reviewed changes

nija-at reviewed Sep 30, 2020

View reviewed changes

doug-ferris-mondo marked this pull request as draft September 30, 2020 23:51

feat(api-gateway): Configured mTLS options

4dd6eb5

doug-ferris-mondo force-pushed the master branch from b4ee46f to 39fbd53 Compare October 2, 2020 06:12

Added documentation to mTLS configuration

b3135f4

doug-ferris-mondo force-pushed the master branch from 39fbd53 to b3135f4 Compare October 2, 2020 07:13

doug-ferris-mondo and others added 2 commits October 2, 2020 17:17

Merge branch 'master' into master

16bcadc

Removed previous interface from a bad merge

d9559bd

doug-ferris-mondo marked this pull request as ready for review October 4, 2020 22:15

nija-at previously requested changes Oct 5, 2020

View reviewed changes

Updates to documentation and code style based on PR feedback

de58a8b

nija-at previously requested changes Oct 7, 2020

View reviewed changes

RomainMuller mentioned this pull request Oct 9, 2020

Semicolon after method body causes "TypeError: Cannot read property 'getJsDocTags' of undefined" aws/jsii#2098

Closed

4 tasks

Updated to handle jsii failure

5d82321

Merge branch 'master' into master

2ef515a

Merge branch 'master' into master

d5e9fed

nija-at approved these changes Oct 15, 2020

View reviewed changes

Merge branch 'master' into master

b64504f

mergify bot merged commit eb2c568 into aws:master Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): mTLS support #10521

feat(apigateway): mTLS support #10521

doug-ferris-mondo commented Sep 25, 2020

nija-at Sep 30, 2020

nija-at Sep 30, 2020

doug-ferris-mondo Sep 30, 2020

nija-at left a comment

doug-ferris-mondo commented Sep 30, 2020

doug-ferris-mondo commented Sep 30, 2020

doug-ferris-mondo commented Oct 1, 2020

nija-at commented Oct 1, 2020

nija-at left a comment

nija-at Oct 5, 2020

nija-at Oct 5, 2020

nija-at Oct 5, 2020

doug-ferris-mondo Oct 5, 2020

nija-at Oct 6, 2020 •

edited

Loading

doug-ferris-mondo Oct 8, 2020

doug-ferris-mondo Oct 8, 2020

nija-at Oct 9, 2020

nija-at Oct 9, 2020 •

edited

Loading

nija-at Oct 5, 2020

nija-at Oct 5, 2020

nija-at Oct 5, 2020

nija-at left a comment

gitpod-io bot commented Oct 12, 2020 •

edited

Loading

doug-ferris-mondo commented Oct 14, 2020

nija-at left a comment

mergify bot commented Oct 15, 2020

aws-cdk-automation commented Oct 15, 2020

mergify bot commented Oct 15, 2020

	readonly mutualTlsAuthentication?: MutualTlsAuthenticationAttributes
	readonly mtls?: MTLSAttributes

	if (!mtlsConfig) return mtlsConfig;
	if (!mtlsConfig) return undefined;

	* @default - Uses current version
	* @default - latest version

feat(apigateway): mTLS support #10521

feat(apigateway): mTLS support #10521

Conversation

doug-ferris-mondo commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

doug-ferris-mondo commented Sep 30, 2020

doug-ferris-mondo commented Sep 30, 2020

doug-ferris-mondo commented Oct 1, 2020

nija-at commented Oct 1, 2020

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

gitpod-io bot commented Oct 12, 2020 • edited Loading

doug-ferris-mondo commented Oct 14, 2020

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 15, 2020

aws-cdk-automation commented Oct 15, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 15, 2020

nija-at Oct 6, 2020 •

edited

Loading

nija-at Oct 9, 2020 •

edited

Loading

gitpod-io bot commented Oct 12, 2020 •

edited

Loading