Adds Https Redirection and Hsts Middlewares #264

jkotalik · 2017-10-03T02:07:39Z

Implementation of #258.
Final items:

Verify which browsers respect the Strict-Transport-Security and check behavior on browser.
Naming
Any design/ usability changes

jkotalik · 2017-10-03T21:20:55Z

Per offline feedback,
~~HttpsEnforcement~~ -> HttpsPolicy for everything?

Tratcher · 2017-10-04T17:49:49Z

samples/HttpsSample/Startup.cs

+                .UseKestrel(
+                options =>
+                {
+                    options.Listen(new IPEndPoint(IPAddress.Loopback, 443), listenOptions =>


Demo using a custom port.

Tratcher · 2017-10-04T17:51:10Z

samples/HttpsSample/Startup.cs

+
+        public void Configure(IApplicationBuilder app)
+        {
+            var httpsEnforcementOptions = new HttpsEnforcementOptions();


Options should all be moved to ConfiguredServices as services.Configure<MyOptions>(options => ...);

We should have a helper method for this.

services.AddHttpsPolicy(o => o. ...);

Tratcher · 2017-10-04T17:52:45Z

src/Microsoft.AspNetCore.HttpsEnforcement/HstsMiddleware.cs

+        /// <returns></returns>
+        public async Task Invoke(HttpContext context)
+        {
+            context.Response.OnStarting(() =>


Why not just do this before calling next? OnStarting is not required.

Tratcher · 2017-10-04T18:05:14Z

src/Microsoft.AspNetCore.HttpsEnforcement/HstsOptions.cs

+        /// Max-age is required; defaults to 0.
+        /// See: https://tools.ietf.org/html/rfc6797#section-6.1.1
+        /// </remarks>
+        public int MaxAge { get; set; }


TimeSpan.
Also, 0 means to disable HSTS, so it can't be the default.

NOTE: A max-age value of zero (i.e., "max-age=0") signals the UA to cease regarding the host as a Known HSTS Host, including the includeSubDomains directive (if asserted for that HSTS Host).

Though https://tools.ietf.org/html/rfc6797#section-11.2 suggests 0 as the default and making users set it so that it's not using an arbitrary value.
If we require users to set it then we should make it nullable and throw if it's not set. If not then we need to come up with a non-zero default. Talk to @blowdart.

https://tools.ietf.org/html/rfc6797#section-11.2 defines an alternative to a fixed second count where you would define a fixed date and then Max-Age would reflect a count-down to that date. Not sure if that's in wide use or worth implementing. Do other providers offer this variation?

I'm in favor of having it nullable and throw if it isn't set. The fixed second count is odd.

Ask the PMs.

Tratcher · 2017-10-04T18:15:37Z

src/Microsoft.AspNetCore.HttpsEnforcement/HstsMiddleware.cs

+        {
+            context.Response.OnStarting(() =>
+            {
+                if (context.Request.Scheme == "https")


Request.IsHttps

Tratcher · 2017-10-04T18:51:44Z

src/Microsoft.AspNetCore.HttpsEnforcement/HstsOptions.cs

+        /// Sets the preload parameter of the Strict-Transport-Security header.
+        /// </summary>
+        /// <remarks>
+        /// Preload is not part of the RFC specification, but is supported by web browsers


remove the Chrome reference, just say some web browsers.

The RFC has it "IncludeSubDomains" but one could argue the property should be named IncludeSubdomains, I don't know.

I'll stick with the RFC here. 😃

Tratcher · 2017-10-04T18:53:08Z

src/Microsoft.AspNetCore.HttpsEnforcement/HttpsEnforcementExtensions.cs

+    public static class HttpsEnforcementExtensions
+    {
+        /// <summary>
+        /// Adds middleware for enforcing HTTPS for all HttpRequests.


Including redirecting from HTTP to HTTPS and adding the HSTS header.

Tratcher · 2017-10-04T18:54:08Z

src/Microsoft.AspNetCore.HttpsEnforcement/HttpsEnforcementOptions.cs

+        /// <summary>
+        /// The status code to be used for Url Redirection
+        /// </summary>
+        public int StatusCode { get; set; } = 301; // TODO throw ArgumentOutOfRangeException from UrlRewrite redirect rule?


Use the constant

Tratcher · 2017-10-04T18:54:52Z

src/Microsoft.AspNetCore.HttpsEnforcement/HttpsEnforcementOptions.cs

+        /// <summary>
+        /// Whether to use HTTP Strict-Transport-Security (HSTS) on all HTTPS requests.
+        /// </summary>
+        public bool EnforceHsts { get; set; }


SendHsts? SetHsts? The HSTS enforcement happens on the client.

javiercn

Off to a good start!

javiercn · 2017-10-04T19:33:12Z

samples/HttpsSample/Startup.cs

+
+        public void Configure(IApplicationBuilder app)
+        {
+            var httpsEnforcementOptions = new HttpsEnforcementOptions();


We should have a helper method for this.

services.AddHttpsPolicy(o => o. ...);

javiercn · 2017-10-04T19:34:15Z

src/Microsoft.AspNetCore.HttpsEnforcement/HstsExtensions.cs

+                throw new ArgumentNullException(nameof(app));
+            }
+
+            return app.UseHsts(new HstsOptions());


HstsOptions here should come from DI.

javiercn · 2017-10-04T19:35:01Z

src/Microsoft.AspNetCore.HttpsEnforcement/HstsMiddleware.cs

+namespace Microsoft.AspNetCore.HttpsEnforcement
+{
+    /// <summary>
+    /// Enables Http Strict Transport Security (HSTS)


nit: HTTP (casing)

javiercn · 2017-10-04T19:39:01Z

src/Microsoft.AspNetCore.HttpsEnforcement/HttpsEnforcementExtensions.cs

+            {
+                throw new ArgumentNullException(nameof(app));
+            }
+            var httpsEnforcementOptions = new HttpsEnforcementOptions();


This should come from DI

javiercn

We need to figure out how we will flow the custom port into this middle-ware for development scenarios.

Tratcher · 2017-10-04T20:57:02Z

My standing suggestion is a top level config key / environment variable HttpsPort that would be set by VS when launching the process. VS is also the one setting URLs at startup.

…lection

javiercn · 2017-10-05T20:46:54Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyBuilderExtensions.cs

+            }
+            var httpsEnforcementOptions = new HttpsPolicyOptions();
+
+            return app.UseHttpsPolicy(httpsEnforcementOptions);


nit: Policy

javiercn · 2017-10-05T20:48:58Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyOptions.cs

+        /// <summary>
+        /// The status code to be used for Url Redirection
+        /// </summary>
+        public int StatusCode { get; set; } = StatusCodes.Status301MovedPermanently; // TODO throw ArgumentOutOfRangeException from UrlRewrite redirect rule?


It's ok as it is. I don't think we need to over protect against a non redirect status code. I would change the property name and the doc comment to reflect that is a redirect status code. Property would be RedirectStatusCode. What do you think?

Yes RedirectStatusCode.

javiercn · 2017-10-05T20:51:54Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyOptions.cs

+        /// <summary>
+        ///  Options for using HSTS
+        /// </summary>
+        public HstsOptions HstsOptions { get; set; } // TODO should this be initialized at all? We could remove the bool if so.


I'm happy with this being initialized by default (to whatever defaults we choose) and for it being null to have the meaning of SetHsts = false. That way there can't be invalid combinations (like the user initializes the setting and forgets to set the flag or viceversa). I also think we follow this pattern in other places. @Tratcher do you have anything against my proposal?

javiercn

Looks good! It's almost there!

muratg · 2017-10-05T23:56:52Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyBuilderExtensions.cs

@@ -65,5 +46,9 @@ public static IApplicationBuilder UseHttpsPolicy(this IApplicationBuilder app, H

            return app;
        }
+
+        public static IApplicationBuilder UseHttpsPolicy(this IApplicationBuilder app, bool enableHsts)
+        {


Why empty? Do we need a comment explaining it?

Sorry redesign for this. I need to do a bit of work still!

Oh, OK, sorry didn't notice the [WIP] in the title.

Tratcher · 2017-10-09T19:44:36Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsExtensions.cs

+using Microsoft.AspNetCore.Builder;
+using Microsoft.Extensions.Options;
+
+namespace Microsoft.AspNetCore.HttpsPolicy


Builder namespace

Tratcher · 2017-10-09T19:45:49Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsMiddleware.cs

+            var includeSubdomains = hstsOptions.IncludeSubDomains ? IncludeSubDomains : StringSegment.Empty;
+            var preload = hstsOptions.Preload ? Preload : StringSegment.Empty;
+
+            _nameValueHeaderValue = new StringValues($"max-age={hstsOptions.MaxAge}{includeSubdomains}{preload}");


Tratcher · 2017-10-09T19:46:30Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsMiddleware.cs

+            {
+                context.Response.Headers[HeaderNames.StrictTransportSecurity] = _nameValueHeaderValue;
+            }
+            await _next(context);


no async/await required

Tratcher · 2017-10-09T19:49:01Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsOptions.cs

+        /// Max-age is required; defaults to 0.
+        /// See: https://tools.ietf.org/html/rfc6797#section-6.1.1
+        /// </remarks>
+        public int MaxAge { get; set; } // TODO set this to a different default in dev/not dev?


0 cannot be the default, that has a specific meaning in the spec. Ask the PMs for a specific value. 1 day? 1 week?
We may not need a separate value for dev as the templates won't have this enabled by default.

Discussed offline with @javiercn. 0 isn't a valid default, and using a nullable would unnecessarily complicate the user-experience. We just need to come up with a good default. Take it up with @blowdart over email if needs be.

@shirhatti as I know you were opinionated on having 0 as the default.

Tratcher · 2017-10-09T19:49:41Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyOptions.cs

+    public class HttpsPolicyOptions
+    {
+        /// <summary>
+        /// Whether to use HTTP Strict-Transport-Security (HSTS) on all HTTPS requests.


Tratcher · 2017-10-09T19:49:52Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyOptions.cs

+        /// <summary>
+        /// The status code to be used for Url Redirection
+        /// </summary>
+        public int StatusCode { get; set; } = StatusCodes.Status301MovedPermanently; // TODO throw ArgumentOutOfRangeException from UrlRewrite redirect rule?


Yes RedirectStatusCode.

Tratcher · 2017-10-09T19:50:25Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsPolicyOptions.cs

+        public int StatusCode { get; set; } = StatusCodes.Status301MovedPermanently; // TODO throw ArgumentOutOfRangeException from UrlRewrite redirect rule?
+
+        /// <summary>
+        /// The TLS port to be added to the redirected URL.


The default port (443) will be used if not provided.

javiercn · 2017-10-09T20:41:44Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HttpsPolicyMiddlewareTests.cs

+        [InlineData(302, 5050, 0, true, true, "max-age=0; includeSubDomains; preload", "https://localhost:5050/")]
+        public async Task SetOptions_AddHstsMiddlewareAndEnableInPolicy(int statusCode, int? tlsPort, int maxAge, bool includeSubDomains, bool preload, string expectedHstsHeader, string expectedUrl)
+        {
+


nit: remove empty line

javiercn · 2017-10-09T20:43:38Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HttpsPolicyMiddlewareTests.cs

+                .Configure(app =>
+                {
+                    app.UseHttpsPolicy();
+                    app.UseHsts();


I thought UseHttpsPolicy would add Hsts if needed, why is this line here. Is that not the case?

That is the case. This is just showing that you can still add Hsts again after calling UseHttps.

javiercn · 2017-10-09T20:47:13Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HttpsPolicyMiddlewareTests.cs

+
+            var response = await client.SendAsync(request);
+
+            // By default, we will not redirect if we only use the Hsts Middleware.


This comment is confusing, I'm not sure what you are trying to convey here, but it seems out of context

javiercn · 2017-10-09T20:49:39Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HstsMiddlewareTests.cs

+
+            var response = await client.SendAsync(request);
+
+            // By default, we will not redirect if we only use the Hsts Middleware.


You copy pasted this comment below and doesn't make sense there. I don't think this adds a lot of value here, so I would just remove it.

jkotalik · 2017-10-10T06:37:50Z

We should consider making HttpsPolicy a shared options collection with RedirectToHttpsOptions and HstsOptions. To configure the HttpPolicy, if we require the max-age to be set, users will have to call services.Configure.

Tratcher · 2017-10-12T23:15:11Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsOptions.cs

        /// See: https://tools.ietf.org/html/rfc6797#section-6.1.1
        /// </remarks>
-        public int MaxAge { get; set; }
+        public int MaxAge { get; set; } = 2592000;


This should be a TimeSpan so people don't have to do the math themselves.

vanillajonathan · 2017-10-12T23:21:08Z

samples/HttpsPolicySample/Startup.cs

+        public void ConfigureServices(IServiceCollection services)
+        {
+            services.Configure<HttpsRedirectionOptions>(options => {
+                options.RedirectStatusCode = 301;


Use StatusCodes.Status301MovedPermanently

vanillajonathan · 2017-10-12T23:25:31Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsMiddleware.cs

+        /// <returns></returns>
+        public Task Invoke(HttpContext context)
+        {
+


We've got a blank line here.

vanillajonathan · 2017-10-12T23:25:53Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsMiddleware.cs

+            {
+                context.Response.Headers[HeaderNames.StrictTransportSecurity] = _strictTransportSecurityValue;
+            }
+            return  _next(context);


Should have a blank line before the return statement.

javiercn · 2017-10-13T00:41:53Z

samples/HttpsPolicySample/Startup.cs

+        // For more information on how to configure your application, visit https://go.microsoft.com/fwlink/?LinkID=398940
+        public void ConfigureServices(IServiceCollection services)
+        {
+            services.Configure<HttpsRedirectionOptions>(options => {


Update the samples to use the helper methods

jkotalik · 2017-10-13T06:24:25Z

@javiercn @Tratcher We need to talk about how configuration options will be propagated to the middleware. Aka does this middleware need to do anything different or will configuration call app.UseHsts() or app.HttpsRedirection().

Tratcher · 2017-10-13T16:04:48Z

src/Microsoft.AspNetCore.HttpsPolicy/HstsMiddleware.cs

+            var includeSubdomains = hstsOptions.IncludeSubDomains ? IncludeSubDomains : StringSegment.Empty;
+            var preload = hstsOptions.Preload ? Preload : StringSegment.Empty;
+
+            _strictTransportSecurityValue = new StringValues($"max-age={hstsOptions.MaxAge.TotalSeconds}{includeSubdomains}{preload}");


TotalSeconds needs formatting. Compare to https://github.com/aspnet/Security/blob/bd07f8b683ce793490d108b2310fa6112953d172/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs#L361-L362

nvrm realized I realized that security has max age as nullable.

Tratcher · 2017-10-13T16:06:06Z

src/Microsoft.AspNetCore.HttpsPolicy/HttpsRedirectionOptions.cs

+    /// </summary>
+    public class HttpsRedirectionOptions
+    {
+


Tratcher · 2017-10-13T16:07:41Z

src/Microsoft.AspNetCore.HttpsPolicy/Microsoft.AspNetCore.HttpsPolicy.csproj

+    <PackageTags>aspnetcore;https;hsts</PackageTags>
+  </PropertyGroup>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Http.Extensions" />


What is Http.Extensions needed for?

Tratcher · 2017-10-13T16:11:10Z

The current Options usage looks fine.

javiercn · 2017-10-13T18:02:40Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HstsMiddlewareTests.cs

+                .UseUrls("https://*:5050")
+                .ConfigureServices(services =>
+                {
+                    services.Configure<HstsOptions>(options => {


Fix the usages on the tests to use the service collection extensions

There are two sets of tests, one that uses Configure and AddHsts(). I can remove these tests if you would like 😄

javiercn · 2017-10-13T18:03:19Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HttpsPolicyTests.cs

+                .UseUrls("https://*:5050", "http://*:5050")
+                .ConfigureServices(services =>
+                {
+                    services.Configure<HttpsRedirectionOptions>(options =>


javiercn · 2017-10-13T18:03:49Z

test/Microsoft.AspNetCore.HttpsEnforcement.Tests/HttpsRedirectionMiddlewareTests.cs

+            var builder = new WebHostBuilder()
+                .ConfigureServices(services =>
+                {
+                    services.Configure<HttpsRedirectionOptions>(options =>


javiercn

Final cleanups, but it's good to go

jkotalik requested review from Tratcher and javiercn October 3, 2017 02:07

dnfclas added the cla-already-signed label Oct 3, 2017

Adds HttpEnforcement and Hsts Middleware

d9f364d

jkotalik force-pushed the jkotalik/httpsMiddleware branch from 6dbf757 to d9f364d Compare October 3, 2017 02:11

Tratcher suggested changes Oct 4, 2017

View reviewed changes

javiercn reviewed Oct 4, 2017

View reviewed changes

javiercn suggested changes Oct 4, 2017

View reviewed changes

jkotalik added 2 commits October 4, 2017 19:00

most of the feedback. TODO clean up httpsPolicyBuilder and ServiceCol…

18d6c96

…lection

trying to fix DI issues

9d3c8b2

javiercn reviewed Oct 5, 2017

View reviewed changes

javiercn suggested changes Oct 5, 2017

View reviewed changes

Redesign of middleware incoming

4d27202

muratg reviewed Oct 5, 2017

View reviewed changes

jkotalik mentioned this pull request Oct 6, 2017

Adds Microsoft.AspNetCore.HttpsPolicy to packages. aspnet/Universe#583

Merged

feedback

4a9495e

Tratcher reviewed Oct 9, 2017

View reviewed changes

javiercn reviewed Oct 9, 2017

View reviewed changes

Feedback, TODO default for Hsts

e9a1f5a

jkotalik added 3 commits October 9, 2017 23:38

Visual studio version

82401ac

Revert sln changes

e38ac6e

Offline feedback and discussion

8c11086

Tratcher reviewed Oct 12, 2017

View reviewed changes

Comments!

a588a98

vanillajonathan reviewed Oct 12, 2017

View reviewed changes

Feedback!

3aa120c

javiercn reviewed Oct 13, 2017

View reviewed changes

jkotalik added 2 commits October 12, 2017 23:18

modify samples

f72cb24

super small nit

a704d29

Tratcher approved these changes Oct 13, 2017

View reviewed changes

Feedback

23412eb

jkotalik changed the title ~~[WIP] Adds HttpsEnforcement and Hsts Middleware~~ Adds HttpsEnforcement and Hsts Middleware Oct 13, 2017

jkotalik changed the title ~~Adds HttpsEnforcement and Hsts Middleware~~ Adds Https Redirection and Hsts Middlewares Oct 13, 2017

javiercn reviewed Oct 13, 2017

View reviewed changes

javiercn approved these changes Oct 13, 2017

View reviewed changes

jkotalik merged commit 758f9fc into dev Oct 13, 2017

jkotalik deleted the jkotalik/httpsMiddleware branch October 13, 2017 19:15

This was referenced Oct 13, 2017

Add first class support for HTTPS though a middleware #258

Closed

Add HttpsRedirection and HSTS middleware template aspnet/Templates#871

Closed

Add HttpsRedirection and Hsts middleware documentation dotnet/AspNetCore.Docs#4568

Closed

Eilon mentioned this pull request Oct 13, 2017

Add HttpsRedirection and HSTS middleware template aspnet/Templating#66

Closed


		var response = await client.SendAsync(request);

		// By default, we will not redirect if we only use the Hsts Middleware.

Adds Https Redirection and Hsts Middlewares #264

Adds Https Redirection and Hsts Middlewares #264

Conversation

jkotalik commented Oct 3, 2017

jkotalik commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Tratcher commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment