Add first class support for HTTPS though a middleware

[javiercn]

We want to have a middleware that does HTTP to HTTPS redirection and possibly other things like HSTS.

[javiercn]

This is part of our plan for HTTPS in 2.1

[javiercn]

@muratg Can you label this appropiately? There is no needs-design lable here and I can't create one.

[muratg]

cc @Tratcher

[Tratcher]

There's an implementation available here: https://github.com/aspnet/BasicMiddleware/blob/793a49fe111d86895f22300297fa70f710459406/src/Microsoft.AspNetCore.Rewrite/Internal/RedirectToHttpsRule.cs

[jkotalik]

We could add another extension method that would force HSTS redirection.

[javiercn]

I'm more inclined to follow the same approach we do in StaticFiles as @Tratcher suggested to me. We probably should have a meeting to discuss the details.

[muratg]

cc @shirhatti @glennc @davidfowl

[jkotalik]

@javiercn can you point me to this approach in StaticFiles?

[Tratcher]

He's referring to https://github.com/aspnet/StaticFiles/blob/5d2b1000f19dc3e892611f387b46cd519976a9f0/src/Microsoft.AspNetCore.StaticFiles/FileServerExtensions.cs#L91-L101 The features like redirect-to-https and hsts are functionally independent and can be built as separate middleware. Then you add one extension that pulls them all in together. That allows customers to choose which pieces they use, and keeps the implementations simpler.

[jkotalik]

@javiercn @Tratcher @shirhatti I have been chunking away at this in my spare time. Here is my stab on the design for the middleware.

Note, it is required that UseHsts comes before UseRedirectToHttps as UseRedirectToHttps will redirect if the url is not https. See https://tools.ietf.org/html/rfc6797 and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security for more details on Hsts.

Extension methods

public static IApplicationBuilder UseHttps(this IApplicationBuilder app)
{
}
public static IApplicationBuilder UseHttps(this IApplicationBuilder app, bool enforceHsts)
{
}
public static IApplicationBuilder UseHttps(this IApplicationBuilder app, bool enforceHsts, bool EnforceHttps)
{
}
public static IApplicationBuilder UseHttps(this IApplicationBuilder app, HttpsOptions options)
{
}

Options:

public class HttpsOptions
{
    public bool EnforceHsts { get; set; }
    public bool EnforceRedirectToHttps { get; set; }= true;
    public RedirectToHttpOptions {get; set;} = new RedirectToHttpOptions();
    public HstsOptions {get; set;} = new HstsOptions();
}
public class RedirectToHttpOptions 
{
    // TODO consider exposing the RewriteOptions here.
    public int StatusCode { get; set; } = 301
    public int? SslPort { get; set; }
}
public class HstsOptions
{
    public int MaxAge { get; set; } = 31536000 
    public bool IncludeSubDomains { get; set; }
    public bool Preload { get; set; }
}

Middleware

The HttpsMiddleware will just be calling app.UseRewriter(new RewriteOptions.RedirectToHttps(params));

public class HstsMiddleware
{
    private readonly RequestDelegate _next;
    private readonly StringValues _nameValueHeaderValue;

    public HstsMiddleware(RequestDelegate next, IOptions<HstsOptions> options)
    {
        _nameValueHeaderValue = MethodToCreateStrictTransportSecurityHeader();
    }

    public async Task Invoke(HttpContext context)
    {
        // We probably need to verify that this header still exists after starting to pipe to the response.
        context.Response.Headers[HeaderNames.StrictTransportSecurity] = _nameValueHeaderValue;
        await _next(context);
    }
}

[Tratcher]

Off to a good start.

The names are a bit misleading, none of this is implementing https / adding https to your server. These are all augmentations. UseHttpsHelpers? UseHttpsEnforcement?

[vanillajonathan]

@jkotalik Perhaps it should not expose the StatusCode property since that could be set to anything while the only relevant ones are the 3xx ones.

Perhaps the property SslPort should be called TlsPort. Not sure why it would be desirable to have it declared as nullable.

[jkotalik]

@vanillajonathan Thanks for the feedback! Maybe we can restrict it to only 3xx status codes. The main reason to expose it is for people to add any 3xx status code they want (ex 301, 302, etc). TlsPort probably makes more sense over SslPort. The reason it was nullable is if no port is provided (null) we default to not adding any to the url. Ex http://example.com would be written to https://example.com without adding the tls port.

[shirhatti]

Note, it is required that UseHsts comes before UseRedirectToHttps as UseRedirectToHttps will redirect if the url is not https

Not sure I follow. The RFC states that an HSTS Host MUST NOT include the STS header field in HTTP responses conveyed over non-secure transport. So, shouldn't the order be other way around?

[shirhatti]

public int MaxAge { get; set; } = 31536000

Reading from the spec

   Additionally, server implementers should consider employing a default
   max-age value of zero in their deployment configuration systems.
   This will require deployers to willfully set max-age in order to have
   UAs enforce the HSTS Policy for their host and will protect them from
   inadvertently enabling HSTS with some arbitrary non-zero duration.

[vanillajonathan]

@jkotalik I am not sure if being able to set it to any 3xx status code is desirable or just have a boolean that configures whether to use temporary to permanent redirection.

[Tratcher]

A boolean is inadequate @vanillajonathan, there are at least four valid redirect status codes.

[javiercn]

I would get rid of

public static IApplicationBuilder UseHttps(this IApplicationBuilder app, bool enforceHsts)
{
}

public static IApplicationBuilder UseHttps(this IApplicationBuilder app, bool enforceHsts, bool EnforceHttps)
{
}

Either your options come from DI or you pass them manually, but no need for those additional overloads

[davidfowl]

Agreed. We moved away from specifying args on the method itself and prefer options for the most part.

[jkotalik]

Not sure I follow. The RFC states that an HSTS Host MUST NOT include the STS header field in HTTP responses conveyed over non-secure transport. So, shouldn't the order be other way around?

The problem is by adding the HTTPS Redirection, the request ends there and return back to the sender. Unless the HSTS middleware goes first, we will not be able to add the header to the response. @Tratcher Is there anyway to delay writing response header values until the httpresponse has started?

I will change the default to max-age=0, though I think enabling Hsts should imply that we don't use max-age=0?

[javiercn]

public class HttpsOptions
{
    public int StatusCode { get; set; } = 301
    public int? SslPort { get; set; }
    public HstsOptions {get; set;} = new HstsOptions();
}

public class HstsOptions
{
    public int MaxAge { get; set; } = 31536000 
    public bool IncludeSubDomains { get; set; }
    public bool Preload { get; set; }
}

I would flatten the options too, like above. If you only want HSTS, then have something like

app.UseHsts();

app.Use<HstsMiddleware>(hstsOptions);

If you only want HTTPS redirection you would do:

app.UseHttpsRedirect()

or something like that.

I wouldn't expose the UrlRewriteOptions either. If you have something more complex, then use the UrlRewriteMiddleware directly. This middleware is for the 99% of people that just want plain HTTPS redirection.

[javiercn]

The problem is by adding the HTTPS Redirection, the request ends there and return back to the sender. Unless the HSTS middleware goes first, we will not be able to add the header to the response. @Tratcher Is there anyway to delay writing response header values until the httpresponse has started?

I believe you can have it be before the redirect and plug in an event into the OnSendingResponseHeaders or something like that. At that point the response is being written to the wire and you can look at the "final" response and decide what to do.

[javiercn]

Small things, but off to a good start!!

[jkotalik]

@javiercn I originally had the options like that, but decided to match the approach in Static files. I do like your approach more. I'd assume that we want to allow people to use the HSTS middleware without enabling HTTPS redirection? I can see a scenario where people want to add HSTS for HTTPS sites but for any http sites, they do not want them redirected. @shirhatti With @javiercn approach, we will check that the scheme is https before adding the response header.

Based on feedback, this is now what I have:

Options

public class HstsOptions
{
    public int MaxAge { get; set; } // should we let this be 0 per the spec or set it to a default value?
    public bool IncludeSubDomains { get; set; }
    public bool Preload { get; set; }
}

public class HttpsEnforcementOptions // HttpsEnforcement is weird, but HttpsOptions isn't correct as we are not setting up Https, only enforcing it.
{
    public HstsOptions HstsOptions { get; set; }
    public bool EnforceHsts { get; set; } 
    public int StatusCode { get; set; } = 301;
    public int? TlsPort { get; set; } 
}

Extensions

public static IApplicationBuilder UseHttpsEnforcement(this IApplicationBuilder app)
{
}
public static IApplicationBuilder UseHttpsEnforcement(this IApplicationBuilder app, HttpsEnforcementOptions options)
{
}
public static IApplicationBuilder UseHsts(this IApplicationBuilder app)
{
}
public static IApplicationBuilder UseHsts(this IApplicationBuilder app, HstsOptions options)
{
}

Middleware

The HttpsMiddleware will just be calling app.UseRewriter(new RewriteOptions.RedirectToHttps(params));

    public class HstsMiddleware
    {
        private readonly RequestDelegate _next;
        private readonly StringValues _nameValueHeaderValue;
        public HstsMiddleware(RequestDelegate next, IOptions<HstsOptions> options)
        {
            _nameValueHeaderValue = CreateHeaderValueFromOptions();
        }

        public async Task Invoke(HttpContext context)
        {
            context.Response.OnStarting(() =>
            {
                if (context.Request.Scheme == "https")
                {
                    context.Response.Headers[HeaderNames.StrictTransportSecurity] = _nameValueHeaderValue;
                }
                return Task.CompletedTask;
            });
            await _next(context);
        }
    }

[javiercn]

public static IApplicationBuilder UseHttpsEnforcement(this IApplicationBuilder app, Action<HttpsEnforcementOptions> options) // I added the action equivalent for each of these. 
{
}

We don't do these anymore

[javiercn]

@jkotalik We need to find a better name than UseHttpsEnforcement I think. @Tratcher @davidfowl @glennc any suggestion?

[javiercn]

I'm more inclined to name it UseHttpsRedirection or just call it UseHttps. I don't think it would create that much confusion, and we can have a doc comment clarifying what the method does.

[Tratcher]

HSTS isn't redirection. UseHttpsEnforcement was my suggestion.

[jkotalik]

I think we can still have UseHsts and then UseHttpsRedirection. UseHttpsRedirection will allow you to still specify Hsts options.

[shirhatti]

Do we have any extension methods that hang off of IApplicationBuilder that don't use the pattern UseXxxx()? It'll probably be less ambiguous if we use a different verb in place of use.

[Tratcher]

Use is the only verb we've defined for this scenario. Did you have other suggestions?

[davidfowl]

Lets not diverge from the Use paradigm.

[javiercn]

UseHttpsPolicy (as in the PR) looks to me like the best option

[jkotalik]

Done with #264

[vanillajonathan]

This needs to be added to the documentation. https://github.com/aspnet/Docs/tree/master/aspnetcore/security

[Tratcher]

Yes, please file a 2.1 doc bug.

[jkotalik]

https://github.com/aspnet/Docs/issues/4568