-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.Extensions.Options; | ||
using Microsoft.AspNetCore.HttpsPolicy; | ||
|
||
namespace OrchardCore.Https.Services | ||
{ | ||
public class HttpsRedirectionConfiguration : IConfigureOptions<HttpsRedirectionOptions> | ||
{ | ||
private readonly IHttpsService _httpsService; | ||
|
||
public HttpsRedirectionConfiguration(IHttpsService httpsService) | ||
{ | ||
_httpsService = httpsService; | ||
} | ||
|
||
public void Configure(HttpsRedirectionOptions options) | ||
{ | ||
var httpsSettings = _httpsService.GetSettingsAsync().GetAwaiter().GetResult(); | ||
|
||
options.RedirectStatusCode = httpsSettings.RequireHttpsPermanent | ||
? StatusCodes.Status301MovedPermanently | ||
: StatusCodes.Status302Found; | ||
options.HttpsPort = httpsSettings.SslPort; | ||
} | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,8 @@ | ||
using System; | ||
using Microsoft.AspNetCore.Builder; | ||
using Microsoft.AspNetCore.Rewrite; | ||
using Microsoft.AspNetCore.Hosting; | ||
using Microsoft.AspNetCore.Routing; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.DependencyInjection.Extensions; | ||
using Microsoft.Extensions.Options; | ||
using OrchardCore.DisplayManagement.Handlers; | ||
using OrchardCore.Environment.Navigation; | ||
using OrchardCore.Https.Drivers; | ||
|
@@ -16,24 +14,38 @@ namespace OrchardCore.Https | |
{ | ||
public class Startup : StartupBase | ||
{ | ||
public override void Configure(IApplicationBuilder app, IRouteBuilder routes, IServiceProvider serviceProvider) | ||
public override void ConfigureServices(IServiceCollection services) | ||
{ | ||
var rewriteOptions = new RewriteOptions(); | ||
services.AddScoped<INavigationProvider, AdminMenu>(); | ||
services.AddScoped<IDisplayDriver<ISite>, HttpsSettingsDisplayDriver>(); | ||
services.AddSingleton<IHttpsService, HttpsService>(); | ||
|
||
// 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 commentThe 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.ConfigureOptions<HttpsRedirectionConfiguration>(); | ||
|
||
app.UseRewriter(rewriteOptions); | ||
services.AddHsts(options => | ||
{ | ||
options.MaxAge = TimeSpan.FromDays(365); | ||
options.Preload = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. You should add these as comments in the code |
||
options.IncludeSubDomains = true; | ||
}); | ||
} | ||
|
||
public override void ConfigureServices(IServiceCollection services) | ||
public override void Configure(IApplicationBuilder app, IRouteBuilder routes, IServiceProvider serviceProvider) | ||
{ | ||
services.AddScoped<INavigationProvider, AdminMenu>(); | ||
services.AddScoped<IDisplayDriver<ISite>, HttpsSettingsDisplayDriver>(); | ||
services.AddSingleton<IHttpsService, HttpsService>(); | ||
services.AddSingleton(new RewriteOptions()); | ||
|
||
services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<RewriteOptions>, RewriteOptionsHttpsConfiguration>()); | ||
// Determine if SSL redirects are enabled | ||
var settings = serviceProvider.GetService<IHttpsService>().GetSettingsAsync().GetAwaiter().GetResult(); | ||
|
||
if (settings.RequireHttps) | ||
{ | ||
var env = serviceProvider.GetService<IHostingEnvironment>(); | ||
if (!env.IsDevelopment()) | ||
{ | ||
app.UseHsts(); | ||
} | ||
|
||
app.UseHttpsRedirection(); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
using OrchardCore.Modules; | ||
using OrchardCore.Security; | ||
using OrchardCore.Security.Permissions; | ||
using OrchardCore.Security.SecurityHeaders; | ||
using OrchardCore.Settings; | ||
using OrchardCore.Setup.Events; | ||
using OrchardCore.Users.Commands; | ||
|
@@ -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 commentThe 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 commentThe 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 commentThe 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. |
||
} | ||
|
||
public override void ConfigureServices(IServiceCollection services) | ||
{ | ||
services.AddSecurity(); | ||
services.AddSecurityHeaders(); | ||
|
||
// Adds the default token providers used to generate tokens for reset passwords, change email | ||
// and change telephone number operations, and for two factor authentication token generation. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
using System; | ||
using Microsoft.AspNetCore.Builder; | ||
using OrchardCore.Security.SecurityHeaders; | ||
|
||
namespace OrchardCore.Security | ||
{ | ||
/// <summary> | ||
/// Extension methods for the security headers middleware. | ||
/// </summary> | ||
public static class ApplicationBuilderExtensions | ||
{ | ||
/// <summary> | ||
/// Adds middleware for using common security headers. | ||
/// </summary> | ||
/// <param name="app">The <see cref="IApplicationBuilder"/> instance this method extends.</param> | ||
/// <returns>The <see cref="IApplicationBuilder"/>.</returns> | ||
public static IApplicationBuilder UseSecurityHeaders(this IApplicationBuilder app) | ||
{ | ||
if (app == null) | ||
{ | ||
throw new ArgumentNullException(nameof(app)); | ||
} | ||
|
||
return app.UseMiddleware<SecurityHeadersMiddleware>(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
namespace OrchardCore.Security.SecurityHeaders | ||
{ | ||
/// <summary> | ||
/// The referrer policy for the security header. | ||
/// </summary> | ||
/// <remarks> | ||
/// See: https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-header | ||
/// </remarks> | ||
public enum ReferrerPolicy | ||
{ | ||
/// <summary> | ||
/// The Referer header will be omitted entirely from browser requests. No referrer information is sent along with requests. | ||
/// </summary> | ||
NoReferrer, | ||
|
||
/// <summary> | ||
/// This is the user agent's default behavior if no policy is specified. The origin is sent as a referrer | ||
/// when the protocol security level stays the same (HTTPS->HTTPS), but isn't sent to a less secure | ||
/// destination (HTTPS->HTTP). | ||
/// </summary> | ||
NoReferrerWhenDowngrade, | ||
|
||
/// <summary> | ||
/// Only send the origin of the document as the referrer in all cases. | ||
/// The document https://example.com/page.html will send the referrer https://example.com/. | ||
/// </summary> | ||
Origin, | ||
|
||
/// <summary> | ||
/// Send a full URL when performing a same-origin request, but only send the origin of the document for other cases. | ||
/// </summary> | ||
OriginWhenCrossOrigin, | ||
|
||
/// <summary> | ||
/// A referrer will be sent for same-site origins, but cross-origin requests will contain no referrer information. | ||
/// </summary> | ||
SameOrigin, | ||
|
||
/// <summary> | ||
/// Only send the origin of the document as the referrer to a-priori as-much-secure destination (HTTPS->HTTPS), | ||
/// but don't send it to a less secure destination (HTTPS->HTTP). | ||
/// </summary> | ||
StrictOrigin, | ||
|
||
/// <summary> | ||
/// Send a full URL when performing a same-origin request, only send the origin of the document to a-priori as-much-secure destination (HTTPS->HTTPS), | ||
/// and send no header to a less secure destination (HTTPS->HTTP). | ||
/// </summary> | ||
StrictOriginWhenCrossOrigin, | ||
|
||
/// <summary> | ||
/// Send a full URL when performing a same-origin or cross-origin request. | ||
/// This policy will leak origins and paths from TLS-protected resources to insecure origins. Carefully consider the impact of this setting. | ||
/// </summary> | ||
UnsafeUrl | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
|
||
namespace OrchardCore.Security.SecurityHeaders | ||
{ | ||
/// <summary> | ||
/// Options for the security headers to be added to all responses | ||
/// </summary> | ||
public class SecurityHeaderOptions | ||
{ | ||
/// <summary> | ||
/// A collection of headers to be added to all the server's responses. | ||
/// </summary> | ||
public IDictionary<string, string> AddHeaders { get; } = new Dictionary<string, string>(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
using System; | ||
using Microsoft.Extensions.Options; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Http.Extensions; | ||
using Microsoft.AspNetCore.Http.Internal; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Net.Http.Headers; | ||
using System.Threading.Tasks; | ||
|
||
namespace OrchardCore.Security.SecurityHeaders | ||
{ | ||
public class SecurityHeadersMiddleware | ||
{ | ||
private readonly RequestDelegate _next; | ||
private readonly SecurityHeaderOptions _headers; | ||
|
||
public SecurityHeadersMiddleware(RequestDelegate next, IOptions<SecurityHeaderOptions> headers) | ||
{ | ||
if (headers == null) | ||
{ | ||
throw new ArgumentNullException(nameof(headers)); | ||
} | ||
|
||
_next = next ?? throw new ArgumentNullException(nameof(next)); | ||
|
||
_headers = headers.Value; | ||
} | ||
|
||
public async Task Invoke(HttpContext context) | ||
{ | ||
if (context == null) | ||
{ | ||
throw new ArgumentNullException(nameof(context)); | ||
} | ||
|
||
var response = context.Response; | ||
|
||
if (response == null) | ||
{ | ||
throw new ArgumentNullException(nameof(response)); | ||
} | ||
|
||
var headers = response.Headers; | ||
|
||
foreach (var header in _headers.AddHeaders) | ||
{ | ||
headers[header.Key] = header.Value; | ||
} | ||
|
||
await _next(context); | ||
} | ||
} | ||
} |
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.