feat: ele-4721 add support for security headers for cdn site hosting #60
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import * as certificatemanager from "@aws-cdk/aws-certificatemanager"; | |
| import * as cloudfront from "@aws-cdk/aws-cloudfront"; | ||
| import * as s3 from "@aws-cdk/aws-s3"; | ||
| import * as s3deploy from "@aws-cdk/aws-s3-deployment"; | ||
| import * as origins from "@aws-cdk/aws-cloudfront-origins"; | ||
| import { getSiteDomain } from "./utils"; | ||
| import { CommonCdnSiteHostingProps } from "./cdn-site-hosting-props"; | ||
|
|
||
|
|
@@ -22,7 +23,7 @@ export interface CdnSiteHostingConstructProps | |
| */ | ||
| export class CdnSiteHostingConstruct extends cdk.Construct { | ||
| public readonly s3Bucket: s3.Bucket; | ||
| public readonly cloudfrontDistribution: cloudfront.Distribution; | ||
|
|
||
| constructor( | ||
| scope: cdk.Construct, | ||
|
|
@@ -35,18 +36,10 @@ export class CdnSiteHostingConstruct extends cdk.Construct { | |
|
|
||
| const siteDomain = getSiteDomain(props); | ||
|
|
||
|
There was a problem hiding this comment. question: Has this been renamed to |
||
|
There was a problem hiding this comment. question: Has this been renamed to |
||
|
There was a problem hiding this comment. question: Has this been renamed to |
||
| const certificate = certificatemanager.Certificate.fromCertificateArn( | ||
| this, | ||
| `${siteDomain}-cert`, | ||
| props.certificateArn | ||
| ); | ||
|
|
||
| let websiteErrorDocument: string | undefined = props.websiteErrorDocument; | ||
|
|
@@ -67,36 +60,75 @@ export class CdnSiteHostingConstruct extends cdk.Construct { | |
| }); | ||
| new cdk.CfnOutput(this, "Bucket", { value: this.s3Bucket.bucketName }); | ||
|
|
||
| const defaultSecurityHeaders: cloudfront.ResponseSecurityHeadersBehavior = { | ||
Crevitus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src | ||
| contentSecurityPolicy: { | ||
| contentSecurityPolicy: "default-src 'self';", | ||
|
There was a problem hiding this comment. question: how have we decided on the defaults for all of these headers? There was a problem hiding this comment. I've just taken all the defaults from the recommended from commented link above it. There was a problem hiding this comment. This is only setting https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy There was a problem hiding this comment. ... though it's good it's covering the major fetch directives... There was a problem hiding this comment. How will this affect authentication in our SPAs? |
||
| override: true, | ||
| }, | ||
| // https://web.dev/security-headers/#xcto | ||
| contentTypeOptions: { override: true }, | ||
| // https://web.dev/security-headers/#recommended-usages-4 | ||
| frameOptions: { | ||
| frameOption: cloudfront.HeadersFrameOption.DENY, | ||
|
There was a problem hiding this comment. comment: I guess we'll have to override this in Otis, for LTI, etc? There was a problem hiding this comment. Yup, but for most apps it should be deny which is why I have set it as DENY (recommended). |
||
| override: true, | ||
| }, | ||
| // https://web.dev/referrer-best-practices/#setting-your-referrer-policy:-best-practices | ||
| referrerPolicy: { | ||
| referrerPolicy: | ||
| cloudfront.HeadersReferrerPolicy.STRICT_ORIGIN_WHEN_CROSS_ORIGIN, | ||
| override: true, | ||
| }, | ||
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security#examples | ||
| strictTransportSecurity: { | ||
| accessControlMaxAge: cdk.Duration.days(365 * 2), | ||
| includeSubdomains: true, | ||
| preload: true, | ||
| override: true, | ||
| }, | ||
| // xxs-protection is overridden by the contentSecurityPolicy in modern browsers | ||
Crevitus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection | ||
| xssProtection: { protection: false, override: true }, | ||
| }; | ||
|
|
||
| const responseHeadersPolicy = new cloudfront.ResponseHeadersPolicy( | ||
| this, | ||
| "ResponseHeadersPolicy", | ||
| { | ||
| securityHeadersBehavior: { | ||
| ...defaultSecurityHeaders, | ||
| ...props.securityHeaders, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| // Cloudfront distribution | ||
| this.cloudfrontDistribution = new cloudfront.Distribution( | ||
| this, | ||
| "SiteDistribution", | ||
| { | ||
|
Comment on lines
-78
to
-79
There was a problem hiding this comment. question: Is this no longer an issue? There was a problem hiding this comment. There is no custom origin support in the new class as far as I am aware, I tried going to a bogus page in Otis in a stack I bought up and it returned the styled Otis error page rather than something from s3 so assume it works in the new class? There was a problem hiding this comment. I think the This may be another benefit of the change, rather than something we need to recreate. |
||
|
There was a problem hiding this comment. question: Has this been renamed |
||
| sslSupportMethod: cloudfront.SSLMethod.SNI, | ||
| minimumProtocolVersion: cloudfront.SecurityPolicyProtocol.TLS_V1_1_2016, | ||
| certificate, | ||
| domainNames: [siteDomain], | ||
| defaultRootObject: props.websiteIndexDocument, | ||
| defaultBehavior: { | ||
| origin: new origins.S3Origin(this.s3Bucket), | ||
| responseHeadersPolicy: responseHeadersPolicy, | ||
| }, | ||
| errorResponses: props.isRoutedSpa | ||
| ? [ | ||
| { | ||
|
Comment on lines
-90
to
-91
There was a problem hiding this comment. question: Have these been renamed |
||
| httpStatus: 404, | ||
| responseHttpStatus: 200, | ||
| responsePagePath: `/${props.websiteIndexDocument}`, | ||
| }, | ||
| ] | ||
| : undefined, | ||
| } | ||
| ); | ||
| new cdk.CfnOutput(this, "DistributionId", { | ||
| value: this.cloudfrontDistribution.distributionId, | ||
| }); | ||
|
|
||
| // Deploy site contents to S3 bucket | ||
|
|
@@ -125,7 +157,7 @@ export class CdnSiteHostingConstruct extends cdk.Construct { | |
| prune: isSingleDeploymentStep, | ||
| destinationBucket: this.s3Bucket, | ||
| distribution: isInvalidationRequired | ||
| ? this.cloudfrontDistribution | ||
| : undefined, | ||
| distributionPaths: isInvalidationRequired | ||
| ? distributionPathsToInvalidate | ||
|
|
@@ -145,7 +177,7 @@ export class CdnSiteHostingConstruct extends cdk.Construct { | |
| new s3deploy.BucketDeployment(this, "DeployAndInvalidate", { | ||
| sources: props.sources, | ||
| destinationBucket: this.s3Bucket, | ||
| distribution: this.cloudfrontDistribution, | ||
| distributionPaths: ["/*"], | ||
| }); | ||
| } | ||
|
|
||
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.
Have we checked the projects to see if we're relying on this in some way?
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.
I haven't but as I was intending a breaking change to the project I thought it wouldn't matter?
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.
It might just help highlight anything where we're relying on this for something that will no longer be available. In all likelihood, we won't be relying upon it in this way, but if we were doing that and several of the apps were then blocked from upgrading, the construct's usefulness would substantially lower.
Might be worth emailing round the devs to ask anyone who is using this to take a look at their own project and feed back (by a short deadline). I'll look at user-utils, for instance.