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
fix(cloudfront): Set MinimumProtocolVersion and SslSupportMethod when specifying distribution certificate #9200
fix(cloudfront): Set MinimumProtocolVersion and SslSupportMethod when specifying distribution certificate #9200
Conversation
…when specifying distribution certificate Per the CloudFormation documentation, if you specify an ACM certificate ARN, you must also specify values for MinimumProtocolVersion and SslSupportMethod in AWS::CloudFront::Distribution ViewerCertificate. We are using the recommended SslSupportMethod of "sni-only" and default MinimumProtocolVersion of "TLSv1." Fixes: #9193
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
@@ -221,6 +222,14 @@ export class Distribution extends Resource implements IDistribution { | |||
}); | |||
} | |||
|
|||
private addViewerCertificate(certificate: acm.ICertificate): CfnDistribution.ViewerCertificateProperty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 'render' to match the other similar methods.
private addViewerCertificate(certificate: acm.ICertificate): CfnDistribution.ViewerCertificateProperty { | |
private renderViewerCertificate(certificate: acm.ICertificate): CfnDistribution.ViewerCertificateProperty { |
return { | ||
acmCertificateArn: certificate.certificateArn, | ||
sslSupportMethod: SSLMethod.SNI, | ||
minimumProtocolVersion: SecurityPolicyProtocol.TLS_V1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit) - I'd prefer we opt for a higher standard by default, per the docs:
We recommend that you specify TLSv1.2_2018 unless your viewers are using browsers or devices that don’t support TLSv1.2.
Given TLSv1.2 is supported now by all major browsers, I think it's a sane default here.
@@ -3,6 +3,7 @@ import { Construct, IResource, Lazy, Resource, Stack, Token, Duration } from '@a | |||
import { CfnDistribution } from './cloudfront.generated'; | |||
import { Origin } from './origin'; | |||
import { CacheBehavior } from './private/cache-behavior'; | |||
import { SSLMethod, SecurityPolicyProtocol } from './web_distribution'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing -- I've been incrementally moving these sorts of common enums from './web_distribution' to this file, as the (very long-term) plan is to deprecate web_distribution in favor of this construct. Bonus points if you move these two over as part of this change. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
… specifying distribution certificate (aws#9200) Per the CloudFormation documentation, if you specify an ACM certificate ARN, you must also specify values for MinimumProtocolVersion and SslSupportMethod in AWS::CloudFront::Distribution ViewerCertificate. We are using the recommended SslSupportMethod of "sni-only" and MinimumProtocolVersion of "TLSv1.2_2018." Fixes: aws#9193 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… specifying distribution certificate (aws#9200) Per the CloudFormation documentation, if you specify an ACM certificate ARN, you must also specify values for MinimumProtocolVersion and SslSupportMethod in AWS::CloudFront::Distribution ViewerCertificate. We are using the recommended SslSupportMethod of "sni-only" and MinimumProtocolVersion of "TLSv1.2_2018." Fixes: aws#9193 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Per the CloudFormation documentation, if you specify an ACM certificate ARN, you must also specify values for
MinimumProtocolVersion and SslSupportMethod in AWS::CloudFront::Distribution ViewerCertificate. We are using
the recommended SslSupportMethod of "sni-only" and MinimumProtocolVersion of "TLSv1.2_2018."
Fixes: #9193
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license