Skip to content
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

Kestrel should disable HTTP/2 by default on downlevel windows versions #16811

Closed
Tratcher opened this issue Nov 4, 2019 · 11 comments
Closed
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel

Comments

@Tratcher
Copy link
Member

Tratcher commented Nov 4, 2019

RE: #14350

HTTP/2 has two pre-requeists:
A) APLN (Win8.1+)
2) Ciphers (Win10 / 2016+)

When ALPN isn't available it gracefully falls back to HTTP/1.1. However, if ALPN is available but the correct ciphers aren't then all clients will fail to connect with the default configuration. This specifically applies to Win8.1 and 2012 R2. Win8.1 wasn't considered an issue since it has extremely low usage. We have had several reports of people using 2012 R2.

Note 2012 R2 has already exited mainstream support but is still in extended support through 2023. We'll want clarification from PMs before making changes to improve support for it.
https://support.microsoft.com/en-us/lifecycle/search?alpha=windows%20server%202012

@Tratcher
Copy link
Member Author

Tratcher commented Nov 4, 2019

Note we have to be careful checking versions, Environment.OSVersion does not report Win10 versions unless you enable Win10 support in the manifest. https://stackoverflow.com/a/33328814/2588374

We have a couple of options:
A) Check the registry version directly (not recommended)
B) Check Environment.OSVersion and only enable HTTP/2 if we see the correct minimum version. Apps would then have to opt in by enabling the manifest or directly setting the protocol version.
C) Do B and change the manifest by default in the templates.

Note the gRPC templates override the defaults and enable only HTTP/2. We added a mitigation on Mac to fault early because APLN support isn't available. Should we do that for 2012 R2? The difference being it might be possible to reconfigure 2012 R2 to work, it just doesn't work by default.

@jhudsoncedaron
Copy link

A) Check the registry version directly (not recommended)

Actually that's not that bad because you're explicitly checking for versions below current (Win 10).

@solrevdev
Copy link

If it helps anyone in the meantime here was my workaround: dotnet/AspNetCore.Docs#16434 (comment)

@jkotalik jkotalik added the good first issue Good for newcomers. label Jan 24, 2020
@jkotalik
Copy link
Contributor

I'd imagine changing the SelectProtocol method to check Environment.OSVersion would be the right direction. https://github.com/dotnet/aspnetcore/blob/master/src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs#L205

@Tratcher
Copy link
Member Author

There's no need to re-compute this per connection. We have an earlier check for H2 on Mac that fails on startup. In fact, we already have a Windows OS version check there too.

if (options.HttpProtocols == HttpProtocols.Http2)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
{
throw new NotSupportedException(CoreStrings.HTTP2NoTlsOsx);
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && Environment.OSVersion.Version < new Version(6, 2))
{
throw new NotSupportedException(CoreStrings.HTTP2NoTlsWin7);
}
}

We could update this to check for Http1AndHttp2, log, and downgrade to Http1 on older versions of Windows.

@jkotalik
Copy link
Contributor

Is it just me, or is it odd to have this check in HttpsConnectionMiddleware? It's odd to me that we are checking protocols there.

@jkotalik
Copy link
Contributor

We have an earlier check for H2 on Mac that fails on startup. In fact, we already have a Windows OS version check there too.

Is the windows check incorrect then?

@jhudsoncedaron
Copy link

jhudsoncedaron commented Jan 24, 2020

@jkotalik : It is incorrect. Environment.OSVersion was deliberately broken by the Windows OS developers. For some reason the version check can only be configured at the process level, leaving no working API that a dll can use to perform a version check.

@Tratcher
Copy link
Member Author

Is it just me, or is it odd to have this check in HttpsConnectionMiddleware? It's odd to me that we are checking protocols there.

Is the windows check incorrect then?

No, it makes sense. In that case the server is configured for only Http2 and this check was making sure your platform supported ALPN which is required for H2 over TLS.

On Win8/2012R2 we have ALPN but not the ciphers we need. The proposal now is to add an additional check for Win8/2012R2 and Http1AndHttp2 that gracefully disables Http2. This allows the default configuration to work.

If you only selected Http2 and are running on Win8 should we throw? Maybe. Or maybe not because this isn't the default configuration and you had to opt into it explicitly? The ciphers can in theory be reconfigured to work, but we haven't seen people do that successfully.

@jkotalik : It is incorrect. Environment.OSVersion was deliberately broken by the Windows OS developers.

It was accurate enough for detecting Win7 and ALPN, but we'll have trouble detecting Win8. There are suggestions outlined above but they aren't pretty.

@jhudsoncedaron
Copy link

@Tratcher : You just haven't had this get hit from a custom hosting process yet, where net core itself is being hosted by a plugin to a third-party exe. No, I don't do that either, but some people write shell plugins in net core. You get arbitrarily bad compatibility manifests that way.

@analogrelay analogrelay removed this from the 5.0.0-preview1 milestone Mar 11, 2020
@Tratcher
Copy link
Member Author

Environment.OSVersion was fixed for 5.0 dotnet/runtime#33651

@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 11, 2020
@JunTaoLuo JunTaoLuo removed good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue labels Jun 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jul 18, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel
Projects
None yet
Development

No branches or pull requests

9 participants