#854 Security Headers

[jrestall]

This is a pull request to get feedback on my code changes related to implementing a number of security headers. I haven't tested the code yet, it may not even build and there's definitely things to tidy up but I wanted to see if I'm on the right track in terms of the technical approach and default header values.

Related Issue: #854

This pull request completes the following items:

• X-XSS-Protection
• X-Frame-Options
• X-Content-Type-Options
• Referrer-Policy
• Strict-Transport-Security (Https module is now using Microsoft.AspNetCore.HttpsPolicy for HSTS and redirection)
• Remove Server header
• Remove X-Powered-By header
• Content-Security-Policy (CSP) (Would prefer waiting for Microsoft: https://github.com/aspnet/BasicMiddleware/issues/259)
• Public-Key-Pins (PKP) (Potential enhancement to the Https module with UI to enter key details)
• Access-Control-Allow-* (CORS) (Orchard.Cors already in progress)

I'm using 2.1 preview NuGet packages, so this pull request will have to wait until we move to 2.1.

As for the technical approach, I've tried to keep it as simple as possible, I looked at other libraries but didn't think it was worth taking a dependency on them for the small amount of code. I didn't add a UI for modifying these settings at runtime as to me they seem like a development time concern. I think it would be very edge-case for a site to want to turn off these headers completely, as opposed to changing the value, but I have added a services.RemoveSecurityHeader(name) method if they do want to.

Some items on the tidy up list:

• NuGet package references in csproj need to have msbuild version properties.
• Referencing AddSecurityHeaders from Users module. Commons seems better but then since Security references ISiteService it would pull in a lot more dependencies for those using Commons.
• Are we okay with the ServiceCollection security headers API? Should I copy the CorsPolicyBuilder approach instead for slightly more complexity but less extension method pollution? SecurityHeadersPolicyBuilder? https://github.com/aspnet/CORS/blob/dev/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsPolicyBuilder.cs
• Maybe I should make use of the new IStartupFilter support Adding IStartupFilter support #1675 to ensure the middleware is added early in the pipeline. Currently the static file middleware is likely added first so won't get these headers.

[jrestall]

Will update this when we move to 2.1.

[jrestall]

options => {} may look odd since we don't configure any options but I want to still call this in case in the future the library registers required services in this method.

[jrestall]

Making preload false by default. Preloading can have a permanent impact on users and should be an explicit choice. See here for additional explanation: roots/trellis#727

[sebastienros]

You should add these as comments in the code

[jrestall]

Where should we call this from instead of the Users module?

[sebastienros]

It should be in its own feature, and we should rename the Https module to Security. WE might even want to split all these options in separate features if they make sense independently.

[jrestall]

Thanks for the review @sebastienros , I'll go ahead and split up the security headers into different features and initialize them from the OrchardCore.Security module after renaming it from OrchardCore.Https.

[kevinchalet]

Out of curiosity, why not leveraging an existing library like NWebsec?

[jrestall]

Hi Kévin,

Thanks for checking out my pull request, here's my reasoning for not using NWebsec or other similar libraries:

• NWebSec doesn't use the IOptions pattern and adds middleware immediately for every call to its ApplicationBuilder extension methods. This doesn't provide the necessary extensibility we require since another module cannot override the configuration for headers added by other modules. My approach both allows for IConfigureOptions<SecurityHeaderOptions> and allows multiple calls to any of the builder extension methods to override any settings e.g. app.AddReferrerPolicy(ReferrerPolicy.NoReferrerWhenDowngrade).

• The code to implement support for the above security headers is very small and straightforward. I didn't want to force a dependency on NWebSec for those using a low level framework module such as OrchardCore.Security given the cost of maintaining this code should be small. This is especially true if we were to use the Content-Security-Policy code which uses attributes on controllers, forcing an even stronger hard dependency on NWebSec through all modules.

• In addition we don't need most of the functionality from NWebSec, Microsoft has released Microsoft.AspNetCore.HttpsPolicy which provides the redirection and Hsts support. With Microsoft planning a Microsoft.AspNetCore.Csp library, the reasons for taking a dependency on the large NWebSec library become even less as Csp is the majority of NWebSec's code.

[sebastienros]

Rediscovering this PR since you mentioned it in gitter, how can we make progress on it?
Can we postpone the other features and try to finalize it?

[petedavis]

This pull request mentions that CORS module is already in progress. However from what I can tell the pull request #849 has been closed. Does that work need to be integrated into this pull request @sebastienros ??

[sebastienros]

cc'ing @MatthijsKrempel as he's working on a separate CORS feature

[jrestall]

I'll probably close this pull request since it's so old and open other ones for the smaller changes as I get time.

I was waiting for https://github.com/aspnet/BasicMiddleware/issues/323 to be done so that we could just use that instead of my custom code but looks like they might just be providing docs.

Looks like CSP support will come in 3.0 though https://github.com/aspnet/BasicMiddleware/issues/259

[sebastienros]

Closing, and created #2680 to track CORS