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.

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

feat(apigateway): DomainName supports SecurityPolicy #6374

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-apigateway/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,8 @@ You can also define a `DomainName` resource directly in order to customize the d
new apigw.DomainName(this, 'custom-domain', {
domainName: 'example.com',
certificate: acmCertificateForExampleCom,
endpointType: apigw.EndpointType.EDGE // default is REGIONAL
endpointType: apigw.EndpointType.EDGE, // default is REGIONAL
securityPolicy: apigw.SecurityPolicy.TLS_1_2
});
```

Expand Down
18 changes: 17 additions & 1 deletion packages/@aws-cdk/aws-apigateway/lib/domain-name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,15 @@ import * as acm from '@aws-cdk/aws-certificatemanager';
import { Construct, IResource, Resource } from '@aws-cdk/core';
import { CfnDomainName } from './apigateway.generated';
import { BasePathMapping, BasePathMappingOptions } from './base-path-mapping';
import { EndpointType, IRestApi} from './restapi';
import { EndpointType, IRestApi } from './restapi';

/**
* The minimum version of the SSL protocol that you want Api Gateway to use for HTTPS connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The minimum version of the SSL protocol that you want Api Gateway to use for HTTPS connections.
* The minimum version of the SSL protocol that you want API Gateway to use for HTTPS connections.

*/
export enum SecurityPolicy {
TLS_1_0 = 'TLS_1_0',
TLS_1_2 = 'TLS_1_2'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather avoid the awslint exclusions in the package.json.

Suggested change
TLS_1_0 = 'TLS_1_0',
TLS_1_2 = 'TLS_1_2'
/** Cipher suite TLS 1.0 */
TLS_1_0 = 'TLS_1_0',
/** Cipher suite TLS 1.2 */
TLS_1_2 = 'TLS_1_2'

Choose a reason for hiding this comment

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

TLS 1.0 is EOL on the march 31, 2020 ( ie, in 11 days) - so we should use 1.2 ?

}

export interface DomainNameOptions {
/**
Expand All @@ -22,6 +30,13 @@ export interface DomainNameOptions {
* @default REGIONAL
*/
readonly endpointType?: EndpointType;

/**
* The Transport Layer Security (TLS) version + cipher suite for this domain name.
* @default undefined. This field is optional in AWS::ApiGateway::DomainName SecurityPolicy
Copy link
Contributor

Choose a reason for hiding this comment

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

What does CloudFormation default to? We should document that as the default here.

You can identify this by not setting this in CloudFormation, deploying the stack and calling the get-domain-name from the CLI.
I'm hoping it'll be TLS 1.2; if not, I think the CDK should pick that as the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually the reason I looked into this; ApiGateway/CloudFormation defaults this to TLS 1.0. I'm in favor of defaulting it to TLS 1.2, but would that be a potentially breaking change since it restricts the allowed ciphers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, that's a very good point. We should keep it as TLS 1.0 then.

* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-domainname.html
*/
readonly securityPolicy?: SecurityPolicy
}

export interface DomainNameProps extends DomainNameOptions {
Expand Down Expand Up @@ -90,6 +105,7 @@ export class DomainName extends Resource implements IDomainName {
certificateArn: edge ? props.certificate.certificateArn : undefined,
regionalCertificateArn: edge ? undefined : props.certificate.certificateArn,
endpointConfiguration: { types: [endpointType] },
securityPolicy: props.securityPolicy
});

this.domainName = resource.ref;
Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-apigateway/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@
"docs-public-apis:@aws-cdk/aws-apigateway.Stage",
"docs-public-apis:@aws-cdk/aws-apigateway.Stage.restApi",
"docs-public-apis:@aws-cdk/aws-apigateway.Stage.stageName",
"docs-public-apis:@aws-cdk/aws-apigateway.SecurityPolicy.TLS_1_0",
"docs-public-apis:@aws-cdk/aws-apigateway.SecurityPolicy.TLS_1_2",
"docs-public-apis:@aws-cdk/aws-apigateway.UsagePlan",
"docs-public-apis:@aws-cdk/aws-apigateway.UsagePlan.usagePlanId",
"docs-public-apis:@aws-cdk/aws-apigateway.VpcLink.addTargets",
Expand Down Expand Up @@ -296,4 +298,4 @@
]
},
"stability": "stable"
}
}
35 changes: 35 additions & 0 deletions packages/@aws-cdk/aws-apigateway/test/test.domains.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,41 @@ export = {
test.done();
},

'accepts different security policies'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a 3rd verification, that when left unspecified, the value is absent in the CF template. Use ABSENT to do this.

// GIVEN
const stack = new Stack();
const cert = new acm.Certificate(stack, 'Cert', { domainName: 'example.com' });

// WHEN
new apigw.DomainName(stack, 'my-domain', {
domainName: 'old.example.com',
certificate: cert,
securityPolicy: apigw.SecurityPolicy.TLS_1_0
});

new apigw.DomainName(stack, 'your-domain', {
domainName: 'new.example.com',
certificate: cert,
securityPolicy: apigw.SecurityPolicy.TLS_1_2
});

// THEN
expect(stack).to(haveResource('AWS::ApiGateway::DomainName', {
"DomainName": "old.example.com",
"EndpointConfiguration": { "Types": [ "REGIONAL" ] },
"RegionalCertificateArn": { "Ref": "Cert5C9FAEC1" },
"SecurityPolicy": "TLS_1_0"
}));

expect(stack).to(haveResource('AWS::ApiGateway::DomainName', {
"DomainName": "new.example.com",
"EndpointConfiguration": { "Types": [ "REGIONAL" ] },
"RegionalCertificateArn": { "Ref": "Cert5C9FAEC1" },
"SecurityPolicy": "TLS_1_2"
}));
test.done();
},

'"mapping" can be used to automatically map this domain to the deployment stage of an API'(test: Test) {
// GIVEN
const stack = new Stack();
Expand Down