-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
#854 Security Headers #1680
#854 Security Headers #1680
Conversation
@@ -48,6 +48,7 @@ | |||
<MicrosoftAspNetCoreHttpFeaturesPackageVersion>2.0.2</MicrosoftAspNetCoreHttpFeaturesPackageVersion> | |||
<MicrosoftAspNetCoreHttpOverridesPackageVersion>2.0.2</MicrosoftAspNetCoreHttpOverridesPackageVersion> | |||
<MicrosoftAspNetCoreHttpPackageVersion>2.0.2</MicrosoftAspNetCoreHttpPackageVersion> | |||
<MicrosoftAspNetCoreHttpsPolicyPackageVersion>2.1.0-preview2-final</MicrosoftAspNetCoreHttpsPolicyPackageVersion> |
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.
Will update this when we move to 2.1.
|
||
// Configure the rewrite options. | ||
serviceProvider.GetService<IConfigureOptions<RewriteOptions>>().Configure(rewriteOptions); | ||
services.AddHttpsRedirection(options => {}); |
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.
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.
services.AddHsts(options => | ||
{ | ||
options.MaxAge = TimeSpan.FromDays(365); | ||
options.Preload = false; |
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.
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
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.
You should add these as comments in the code
@@ -48,11 +49,14 @@ public override void Configure(IApplicationBuilder builder, IRouteBuilder routes | |||
template: LoginPath, | |||
defaults: new { controller = "Account", action = "Login" } | |||
); | |||
|
|||
builder.UseSecurityHeaders(); |
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.
Where should we call this from instead of the Users module?
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 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.
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 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.
Out of curiosity, why not leveraging an existing library like NWebsec? |
Hi Kévin, Thanks for checking out my pull request, here's my reasoning for not using NWebsec or other similar libraries:
|
Rediscovering this PR since you mentioned it in gitter, how can we make progress on it? |
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 ?? |
cc'ing @MatthijsKrempel as he's working on a separate CORS feature |
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 |
Closing, and created #2680 to track CORS |
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:
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: