Kestrel should not hardcode acceptible TLS versions but rather honor OS settings

[jhudsoncedaron]

Discovered while switching on TLS 1.3 globally.

Says right in the xmldoc (and verified in the code):

    /// <summary>
    /// Specifies allowable SSL protocols. Defaults to <see cref="SslProtocols.Tls12" /> and <see cref="SslProtocols.Tls11"/>.
    /// </summary>
   public SslProtocols SslProtocols { get; set; }

But the best practices doc says don't do this.

I haven't yet located the API that says what the OS-enabled protocols are (we want to force TLS-1.3 on where possible but not all servers support it yet); however our code should be smarter and not force it on when it's a downgrade, so we would want to call it.

The idea of having switches like this in the OS settings is when protocols get broken, the admin can switch them off immediately without waiting for application upgrades.

[blowdart]

@halter73 I thought we left it as defaulting to the OS?

[halter73]

The allowed algorithms are controlled by the OS, but SslProtocols default to Tls11 | Tls12 in order to ensure SslProtocols.Tls (which represents TLS 1.0) doesn't get used on any OS.

You asked for and approved the change over 3 years ago @blowdart. Even back then, you didn't really want to support TLS 1.1, but we continued to support it because not doing so would have broken too many clients at the time IIRC.

Should we go back to using OS defaults?

[blowdart]

My memory is failing in my old age 😀 Ok yes it’s time I think. Too big for 3.1 maybe? Definitely in 5.0.

…

________________________________ From: Stephen Halter <[email protected]> Sent: Monday, October 14, 2019 6:30:49 PM To: aspnet/AspNetCore <[email protected]> Cc: Barry Dorrans <[email protected]>; Mention <[email protected]> Subject: Re: [aspnet/AspNetCore] Kestrel should not hardcode acceptible TLS versions but rather honor OS settings (#14997) The allowed algorithms are controlled by the OS, but SslProtocols default to Tls11 | Tls12 in order to ensure SslProtocols.Tls (which represents TLS 1.0) doesn't get used on any OS. You asked for and approved the change over 3 years ago<aspnet/KestrelHttpServer#676 (comment)> @blowdart<https://github.com/blowdart>. Even back then, you didn't really want to support TLS 1.1, but we continued to support it because not doing so would have broken too many clients at the time IIRC. Should we go back to using OS defaults? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#14997>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAGCNCWGFARNZZIMCWJ7VIDQOUMMTANCNFSM4JATMAYQ>.

[blowdart]

@anurse It's time to change the kestrel defaults. 5.0?

[Tratcher]

https://github.com/aspnet/AspNetCore/blob/d35b33f29470a51876da5060f6fa46f9794fa941/src/Servers/Kestrel/Core/src/HttpsConnectionAdapterOptions.cs#L27
The new default would be SslProtocols.None https://docs.microsoft.com/en-us/dotnet/api/system.security.authentication.sslprotocols?view=netcore-3.0

[analogrelay]

Let's get it in to 5.0 for sure. I think this is a viable candidate for asking for a 3.1 backport though, given the new TLS version (1.3) and the LTS status of 3.1.

[analogrelay]

The new default would be SslProtocols.None docs.microsoft.com/en-us/dotnet/api/system.security.authentication.sslprotocols?view=netcore-3.0

To confirm, we are OK with using None now even though that could potentially include 1.0? (i.e. we're ready to trust OSes to have already blocked TLS 1.0/1.1 usage)

[Tratcher]

I don't recommend this for 3.1, you'll end up downgrading people on some platforms. E.g. do we know what the platform defaults are on Win7, 8, MacOs, Linux, etc?

[jhudsoncedaron]

According to the documentation I can find, Windows 10 has not!. I'm building up several hundred lines of code to look up the supported versions and chop the list.

[analogrelay]

What about just adding Tls3 to the flag set?

@jhudsoncedaron Thanks for checking in to that!

[blowdart]

Adding Tls3 paints us into the same hole again when Tls4 comes along. And we keep telling people not to do this, to let the OS decide, but then we do it ourselves.

[analogrelay]

If @blowdart is happy with None, I'm not opposed. As @halter73 said, I believe the choice to avoid None was intentional but if we feel like we can trust the OS here, I'm OK with that. Certainly for 5.0 at least. We can discuss what (if any) we backport to 3.1.

[blowdart]

We could log warnings if the OS is configured to allow what we consider bad, but not stop.

[halter73]

Is there an easy way to check how the OS is configured?

[jhudsoncedaron]

@halter73 : I have it in 178 lines but I'm not absolutely sure it's right.

[blowdart]

@jhudsoncedaron Remember that TLS versions are not cipher suites.

There's only 5 options here. Ssl2, Ssl3, Tls1, Tls11, Tls12

[blowdart]

http://blog.whatsupduck.net/2014/10/checking-ssl-and-tls-versions-with-powershell.html would do it if you have an https port open

[jhudsoncedaron]

@blowdart : Reading the OS config takes a lot of lines. The number of versions is essentially irrelevant. The only weirdness I had to handle was the enum was named Tls instead of Tls10.

[blowdart]

I feel this is too risky for 3.1

[analogrelay]

Fair enough

[shirhatti]

@blowdart We're considering this for 5.0. Thoughts?

[shirhatti]

On older OSes this change would allow TLS 1.0

[blowdart]

Hurrah :)

And yes, I know, we don't have an exclusionary api in windows unfortunately, but hopefully the older OSes are configured correctly ... we'd provide instructions in release notes etc.

[jhudsoncedaron]

@shirhatti : Yeah it would. There should have been an OS patch out to change that already but that's water under the bridge at this point.

If you don't want to do it, you could just give an API to tell us what protocols are actually enabled, but I found this has its own ugliness because the code should be able to pick up TLS 1.4 (not a typo I really do mean to reference a version that doesn't exist yet) from the SSL provider with no changes to .NET Core at all. Right now, the only way to get that behavior is to just never assign to the property.

[BrennanConroy]

Lets get this into preview6