Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: ele-4721 add support for security headers for cdn site hosting #60
feat: ele-4721 add support for security headers for cdn site hosting #60
Changes from 14 commits
203b436
d5c31cc
7924b6d
d8ffac8
dc1c06c
00b2b56
47aace0
b6c25bf
82e5947
fb16b67
bc9a5bf
3e96297
898175a
15356f0
8fceec4
7258ca3
3e12d8d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
question: Has this been renamed to
domainNames
below?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.
Yup.
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.
question: Has this been renamed to
sslSupportMethod
below?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.
Yup.
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.
question: Has this been renamed to
minimumProtocolVersion
below?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.
Yup.
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.
question: how have we decided on the defaults for all of these headers?
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've just taken all the defaults from the recommended from commented link above it.
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.
This is only setting
default-src
. Is this default strong enough on all the other CSP directives?https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy
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.
... though it's good it's covering the major fetch directives...
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.
How will this affect authentication in our SPAs?
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.
comment: I guess we'll have to override this in Otis, for LTI, etc?
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.
Yup, but for most apps it should be deny which is why I have set it as DENY (recommended).
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.
question: Is this no longer an issue?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
customOriginSource
was a hold-your-nose solution because the default documented solution for attaching to an S3 bucket didn't seem to work via CDK.This may be another benefit of the change, rather than something we need to recreate.
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.
question: Has this been renamed
errorResponses
?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.
Yup.
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.
question: Have these been renamed
httpStatus
andresponseHttpStatus
?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.
Yup.
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.
IIRC, this will be used for non-production stacks, so we could use
cdk destroy
thencdk deploy
without concern.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 think this is intended to represent the environment in which we are provisioning the entire stack, not just a Cloudfront Distribution certificate.
Let's say we wanted to add more resources to the construct that are region-specific, so we have a mixture of things that are supposed to be statically
us-east-1
and things that are supposed to be provisioned in the specified region. If we useus-east-1
as our test case, we would not be able to successfully test that our our region specifications are correct, since it'd be entirely possible that either:us-east-1
.For this reason, I'd be reluctant to hard-code to
us-east-1
.(To mitigate point 1, we should ultimately be testing to ensure that it respects different specified regions.)
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.
Your broader point about
us-east-1
would then involve hard-coding that correct region into line 20, IIUC.