feat: ele-4721 add support for security headers for cdn site hosting

[Crevitus]

Problem

In Otis, we wish to add security headers to requests - talis/elevate-app#4721

After some investigation and chatting with platform and David, the consensus is that we would prefer to do this in the CDK constructs rather than adding a lambda to intercept responses.

We can do this using the new config that Amazon now has for the cloudfront.Distribution class - https://aws.amazon.com/blogs/networking-and-content-delivery/amazon-cloudfront-introduces-response-headers-policies/

Solution

Currently for static sites talis-cdk-constructs is using the cloudfront.CloudFrontWebDistribution class that does not support adding security headers. In order to add security headers we have converted to the new Distribution class.

Some sensible defaults have been chosen so that all projects using cdn-site-hosting-construct in the future will get security headers by default. All the headers can be overridden individually by the client projects.

It seems that CloudFrontWebDistribution is no longer getting new features and may soon be deprecated so switching over to the new Distribution class has multiple benefits: aws/aws-cdk#17755 (comment)

As this creates a different config underneath (aws/aws-cdk#12707) client projects would have to create a new stack (using some date or number prefix) and then when ready switch the DNS over to the new stack.

I will, therefore, mark this as a breaking change to the project.

[camillef]

Is there any way to test that these new default security headers are applied to a stack, and that they can be overridden, in our tests? https://github.com/talis/talis-cdk-constructs/blob/fb16b6735fbaf8280ab967d3abf55048ab6c27d5/test/infra/cdn-site-hosting/cdn-site-hosting-construct.test.ts

[camillef]

question: Has this been renamed to sslSupportMethod below?

[Crevitus]

Yup.

[camillef]

question: Has this been renamed to minimumProtocolVersion below?

[Crevitus]

Yup.

[camillef]

question: Has this been renamed to domainNames below?

[Crevitus]

Yup.

[camillef]

question: Is this no longer an issue?

[Crevitus]

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?

[junglebarry]

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.

[camillef]

question: Have these been renamed httpStatus and responseHttpStatus?

[Crevitus]

Yup.

[camillef]

comment: I guess we'll have to override this in Otis, for LTI, etc?

[Crevitus]

Yup, but for most apps it should be deny which is why I have set it as DENY (recommended).

[camillef]

question: how have we decided on the defaults for all of these headers?

[Crevitus]

I've just taken all the defaults from the recommended from commented link above it.

[junglebarry]

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

[junglebarry]

... though it's good it's covering the major fetch directives...

[junglebarry]

How will this affect authentication in our SPAs?

[Crevitus]

Is there any way to test that these new default security headers are applied to a stack, and that they can be overridden, in our tests? fb16b67/test/infra/cdn-site-hosting/cdn-site-hosting-construct.test.ts

Yeah I would like to add some tests - I tried it in here:

test("provisions a CloudFront distribution linked to S3", () => {
      expectCDK(stack).to(countResources("AWS::CloudFront::Distribution", 1));
      expectCDK(stack).to(
        haveResourceLike("AWS::CloudFront::Distribution", {
          DistributionConfig: {
            Aliases: [fakeFqdn],
            DefaultRootObject: "index.html",
            ViewerCertificate: {
              AcmCertificateArn: fakeCertificateArn,
            },
            Origins: [
              {
                CustomOriginConfig: {
                  OriginProtocolPolicy: "http-only",
                },
              },
            ],
          },
        })
      );
    });

But the stack just has a name for the security headers policy not the details of it itself so I wasn't sure how to test it? Open to adding tests if someone knows how.

[camillef]

But the stack just has a name for the security headers policy not the details of it itself so I wasn't sure how to test it? Open to adding tests if someone knows how.

I would have a poke around using import { Template } from "@aws-cdk/assertions"; which I had to use in my test here:

https://github.com/talis/talis-cdk-constructs/blob/fb16b6735fbaf8280ab967d3abf55048ab6c27d5/test/infra/cdn-site-hosting/cdn-site-hosting-construct.test.ts#L299-L310

It seems to have more detail about resources in the stack and their properties

[Crevitus]

But the stack just has a name for the security headers policy not the details of it itself so I wasn't sure how to test it? Open to adding tests if someone knows how.

I would have a poke around using import { Template } from "@aws-cdk/assertions"; which I had to use in my test here:

https://github.com/talis/talis-cdk-constructs/blob/fb16b6735fbaf8280ab967d3abf55048ab6c27d5/test/infra/cdn-site-hosting/cdn-site-hosting-construct.test.ts#L299-L310

It seems to have more detail about resources in the stack and their properties

Template is waaay better thanks! Added some tests.

[junglebarry]

After some investigation and chatting with platform and David, the consensus is that we would prefer to do this in the CDK constructs rather than adding a lambda to intercept responses.

Our preference, IIUC, is that this is much simpler, and avoids running hand-crafted Lambdas on every request.

Currently for static sites talis-cdk-constructs is using the cloudfront.CloudFrontWebDistribution class that does not support adding security headers. In order to add security headers we have converted to the new Distribution class.

Sounds like a strong benefit.

Some sensible defaults have been chosen so that all projects using cdn-site-hosting-construct in the future will get security headers by default. All the headers can be overridden individually by the client projects.

These seem good to me.

I wonder, though: when we cannot provide the headers in this way using the defaults, how are we going to add the security headers in? Do we already have a plan for that?

It seems that CloudFrontWebDistribution is no longer getting new features and may soon be deprecated so switching over to the new Distribution class has multiple benefits: aws/aws-cdk#17755 (comment)

Until that's made official by deprecation policies, we should not assume this to be the case - it could easily be heresay.

I'm more interested in what we will lose than what we will gain. Do we know what this might be?

As this creates a different config underneath (aws/aws-cdk#12707) client projects would have to create a new stack (using some date or number prefix) and then when ready switch the DNS over to the new stack.

I will, therefore, mark this as a breaking change to the project.

Irrespective of the need for a new new stack, this would be a breaking change predominantly because it changes the external interface of the constructs.

How is the DNS controlled? Asking because I thought it was part of the construct...

Does this mean that applications using this will not be able to "simply run cdk deploy", but instead will need to take another approach to deployment?

[junglebarry]

Have we checked the projects to see if we're relying on this in some way?

[Crevitus]

I haven't but as I was intending a breaking change to the project I thought it wouldn't matter?

[junglebarry]

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.

[junglebarry]

IIRC, this will be used for non-production stacks, so we could use cdk destroy then cdk deploy without concern.

[junglebarry]

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 use us-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:

1. We'd hard-coded everything to a single region, erroneously.
2. We'd made everything respect the specified region when some things should be hard-coded to 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.)

[junglebarry]

Your broader point about us-east-1 would then involve hard-coding that correct region into line 20, IIUC.

[Crevitus]

Thanks for the great feedback!

I wonder, though: when we cannot provide the headers in this way using the defaults, how are we going to add the security headers in? Do we already have a plan for that?

Not sure I follow here, if people do not want to use the defaults in this project they can pass in a securityHeaders property on the props object and override one or many of the defaults as they choose.

For example in Otis I plan to do:

const securityHeaders: cloudfront.ResponseSecurityHeadersBehavior = {
  contentSecurityPolicy: {
    override: true,
    contentSecurityPolicy:
      "default-src 'none'; script-src 'self' *.talis.com *.pendo.io *.instana.io" +
      " static.zdassets.com *.wistia.com zendesk-eu.my.sentry.io 'unsafe-inline';" +
      " connect-src 'self' *.talis.com ekr.zendesk.com talisaspire.zendesk.com " +
      "*.wistia.com *.instana.io; img-src 'self' data: *.pendo.io *.talis.com; " +
      "font-src 'self' data: fonts.gstatic.com; style-src 'self' 'unsafe-inline'" +
      " fonts.googleapis.com; base-uri 'self'; form-action 'self'; manifest-src 'self'",
  },
  // Otis is rendered in an iframe in the VLE
  frameOptions: undefined,
};

Until that's made official by deprecation policies, we should not assume this to be the case - it could easily be heresay.

I'm more interested in what we will lose than what we will gain. Do we know what this might be?

That's a good point, I'm not entirely sure as I have not been able to find a direct comparison and the properties on the respective classes are often named differently so it can be hard to determine. Looking at Github issues there are certainly several requests to port features over from cloudfront.Distribution to cloudfront. CloudFrontWebDistribution. As far as I can tell in my test Otis stack we haven't lost any features from the switch over.

How is the DNS controlled? Asking because I thought it was part of the construct...

Oh the DNS isn't controlled in any different way to how it was before I believe, what I meant was that people found the deployment failed when trying to switch over as CDK would try to create a new stack with the same domain and therefore DNS.

Does this mean that applications using this will not be able to "simply run cdk deploy", but instead will need to take another approach to deployment?

Applications can run cdk deploy but they need to ensure that the stack has a different name first and then afterwards delete their old stack. Alternatively, they could delete the existing stack and then deploy as usual but then there would be more downtime.

[junglebarry]

Applications can run cdk deploy but they need to ensure that the stack has a different name first and then afterwards delete their old stack. Alternatively, they could delete the existing stack and then deploy as usual but then there would be more downtime.

That's a really useful nugget we should be sharing in the changelog.

[junglebarry]

I wonder, though: when we cannot provide the headers in this way using the defaults, how are we going to add the security headers in? Do we already have a plan for that?

Not sure I follow here, if people do not want to use the defaults in this project they can pass in a securityHeaders property on the props object and override one or many of the defaults as they choose.

The question is poorly asked. I mean: if we cannot use security headers, and need to provide the security programmatically, do we already have a plan for that? The only reason I was asking is to see if the presence of these defaults is likely to pose problems, but honestly I cannot see how - just disable them, as you say.

Ignore this.

[malcyL]

As this creates a different config underneath (aws/aws-cdk#12707) client projects would have to create a new stack (using some date or number prefix) and then when ready switch the DNS over to the new stack.

I will, therefore, mark this as a breaking change to the project.

I think this is an important thing to understand.

Obviously we are marking this as a breaking change, but it means that, all apps will have to do this on the next release after upgrading to this version of the library - even if the reason for the upgrade is related to a different construct.

Doing this will not necessarily be trivial. For example, doing this in smalti will also require data to be migrated from one stack to another. (Perhaps we should look at ways we can mitigate this in future. We made samlti a single release. Should the ui and backend be two releases? Census has multiple stacks to help here - that's another approach. But neither of these are for this PR)

If these upgrades are left until the next time we want to make a change in those applications, it will be a nasty trip wire for someone to run up against when trying to make some other change.

@ShaunParsons Should we have a plan to make this upgrade, reprovision stacks, migrate data etc, for all the consuming applications, so that this doesn't become an unexpected chore when trying to make a small change/fix in the future.

[junglebarry]

Brilliantly observed, @malcyL.

This is why I think we really need to go easy on "make everything a construct" until we've really understood complexities. It is clearly not always straightforward to update the infrastructure just by having a construct. It's also clear that breaking changes in one construct will impact on the entire library.

@ShaunParsons Should we have a plan to make this upgrade, reprovision stacks, migrate data etc, for all the consuming applications, so that this doesn't become an unexpected chore when trying to make a small change/fix in the future.

We might, but we haven't merged it yet. Maybe this is a sign that we should instead take a step back and consider our options.

First, perhaps we need a replacement construct rather than an update, so we can deprecate this one, and apps are back in control of the migration. They could even bring up both constructs, swap the DNS, then remove the old construct.

Second, perhaps we go back to lambda@edge, since it's not a breaking change.

What other options might be available to us? Are any of those any better at backwards compatibility?

[malcyL]

First, perhaps we need a replacement construct rather than an update, so we can deprecate this one, and apps are back in control of the migration.

+1 to this - unless there is another option that doesn't require this at all

I also think this should make us look at smalti because of what this has highlighted:

• should we have two releases, one for the SPA, one for the microservice
• should we have multiple stacks in the microservice, a permanent stack simillar to Census with the data decoupled from the api
• should we do both of these

Update:

• should we have three stacks : ui / service / data

[Crevitus]

Brilliantly observed, @malcyL.

This is why I think we really need to go easy on "make everything a construct" until we've really understood complexities. It is clearly not always straightforward to update the infrastructure just by having a construct. It's also clear that breaking changes in one construct will impact on the entire library.

@ShaunParsons Should we have a plan to make this upgrade, reprovision stacks, migrate data etc, for all the consuming applications, so that this doesn't become an unexpected chore when trying to make a small change/fix in the future.

We might, but we haven't merged it yet. Maybe this is a sign that we should instead take a step back and consider our options.

First, perhaps we need a replacement construct rather than an update, so we can deprecate this one, and apps are back in control of the migration. They could even bring up both constructs, swap the DNS, then remove the old construct.

Second, perhaps we go back to lambda@edge, since it's not a breaking change.

What other options might be available to us? Are any of those any better at backwards compatibility?

So to clarify, shall I make a new construct to avoid the breaking changes?

[malcyL]

I don't want to confuse things, but I am wondering if this is something worth mentioning - something I am looking at at the moment.

aws/aws-cdk#4733

This is a discussion on refactoring with the CDK constructs, problems this creates with logical id's, and a possible way around this using RenameLogicalId.

I know that you have changed the class your using in the CDK from cloudfront.CloudFrontWebDistribution to Distribution. But I'm assuming the actual resource in AWS hasn't changed, has it? I wonder if the approach discussed in this issue to use RenameLogicalId to rename the id's created by your refactored construct to match the id's of the original construct would fix the deployment.

Don't know how it's going to turn out for my problem yet, but perhaps it could help.

[Crevitus]

I don't want to confuse things, but I am wondering if this is something worth mentioning - something I am looking at at the moment.

aws/aws-cdk#4733

This is a discussion on refactoring with the CDK constructs, problems this creates with logical id's, and a possible way around this using RenameLogicalId.

I know that you have changed the class your using in the CDK from cloudfront.CloudFrontWebDistribution to Distribution. But I'm assuming the actual resource in AWS hasn't changed, has it? I wonder if the approach discussed in this issue to use RenameLogicalId to rename the id's created by your refactored construct to match the id's of the original construct would fix the deployment.

Don't know how it's going to turn out for my problem yet, but perhaps it could help.

The problem with overriding the logical id is that you need to know the logical id of the old stack first - which means that you need to pass this into talis-cdk-constructs from client applications. It would still be a breaking change requiring manual deployment first time round and its a little hacky. The benefit of this is that you keep the same stack in the end so no need to migrate data - @malcyL @junglebarry I can go with this approach if you approve? Adding an extra prop to take the old logical id to override.

[junglebarry]

@Crevitus I don't think we should be rushing to implement that.

If you want to implement urgently, a separate construct gives you a pathway that doesn't break everything else. If it's not urgent that we implement, we should be spiking the above to understand it before we commit to it.

I'd at least sleep on it for the weekend and see how you feel on Monday - or widen the discussion if you want to consider more options. But that's obviously more time...

[ShaunParsons]

As this creates a different config underneath (aws/aws-cdk#12707) client projects would have to create a new stack (using some date or number prefix) and then when ready switch the DNS over to the new stack.
I will, therefore, mark this as a breaking change to the project.

I think this is an important thing to understand.

Obviously we are marking this as a breaking change, but it means that, all apps will have to do this on the next release after upgrading to this version of the library - even if the reason for the upgrade is related to a different construct.

If this is a breaking change, then it should be a major version bump. We then need a way to communicate with the teams that we need to create tickets to upgrade to the latest version and deal with those breaking changes. This is the first time we're experiencing this pain with the shared constructs.

Doing this will not necessarily be trivial. For example, doing this in smalti will also require data to be migrated from one stack to another. (Perhaps we should look at ways we can mitigate this in future. We made samlti a single release. Should the ui and backend be two releases? Census has multiple stacks to help here - that's another approach. But neither of these are for this PR)

If these upgrades are left until the next time we want to make a change in those applications, it will be a nasty trip wire for someone to run up against when trying to make some other change.

@ShaunParsons Should we have a plan to make this upgrade, reprovision stacks, migrate data etc, for all the consuming applications, so that this doesn't become an unexpected chore when trying to make a small change/fix in the future.

So, smalti should not block this PR at all. But it does raise other issues with how we have provisioned infrastructure. I think the key thing I missed from the Infrastructure Best Practices is that persistent storage should be provisioned independently of our business logic. Apart from our persistent storage, all our cloudformation stacks should be transient. We don't need to worry about logical ID's, it's easier to deploy a new stack, test it, then switch the DNS and terminate the old stack. Cloudformation will do some funky stuff with resources from time to time that get us into a state you can't recover from.

Essentially, if our Cloudformation stacks don't have any persistence, replacing them is trivial. In K8s, the databases are completely independent of the pods, this allows us to replace pods just because we can. We need to get to the same state with Cloudformation stacks.

I guess what I'm proposing is:

• Breaking change is made to the construct
• All developers are informed of the breaking change and tickets to upgrade are created
• Each application is updated and a new Cloudformation stack is provisioned
• DNS is switched over to the new stack
• The old stack is terminated

I don't think we're going to get much further discussing this on this PR, so I propose @Crevitus finds a time in the calendar for us all to have a chat about this and our next steps forwards. I also think we need to update Infrastructure Best Practices to say that persistent storage should be deployed independently of our business logic.

[malcyL]

I don't think we're going to get much further discussing this on this PR, so I propose @Crevitus finds a time in the calendar for us all to have a chat about this and our next steps forwards. I also think we need to update Infrastructure Best Practices to say that persistent storage should be deployed independently of our business logic.

+1

I would definitely like to be involved in this meeting please @Crevitus

[Crevitus]

@malcyL @junglebarry @ShaunParsons @pogotc - closing this after discussion documented here: https://docs.google.com/document/d/13taKSjJ222lrjSrby4iREL7RioCWOjCYk8ZO7bh-ClA/edit?usp=sharing

The current approach will be to create a new construct with the new functionality to avoid breaking changes. This change will be made in a new PR.