Use default SslProtocols in Kestrel

[halter73]

Addresses #14997

I hope to get this into preview6. I will make a breaking change announcement.

[Tratcher]

Did you want to put it in the preview6 branch first? Or do you plan to cherry pick?

[Tratcher]

I'm a little concerned that you didn't have to update any tests...

[halter73]

I'm a little concerned that you didn't have to update any tests...

I did. I just pushed before tests finished 😆

I'll retarget to release/5.0-preview6.

[halter73]

@blowdart Do you want us to log anything if a client connects using TLS 1.0 or SSL? If so, should we log a warning the first time this happens? On every connection where this happens?

[blowdart]

With logging, sure. I think log protocol for every connection as informational. This is useful because it'll allow a developer to gather stats so they can, for example, know when it's safe to turn an older protocol off.

If the OS is configured correctly, you wouldn't even get to the log though would you? Http.sys for example would refuse the connection.

[halter73]

If the OS is configured correctly, you wouldn't even get to the log though would you? Http.sys for example would refuse the connection.

Kestrel refuses the connection but still logs the authentication failure at the debug level. Kestrel doesn't know why the handshake failed. It simply catches AuthenticationExceptions thrown by AuthenticateAsServerAsync and logs it.

With logging, sure. I think log protocol for every connection as informational.

I can log the protocol for every TLS/SSL connection. I think it would be at the debug level like we do for AuthenticationExceptions though.

@blowdart If we see that TLS 1.0 or SSL is being used, should we log something different or increase the log severity?

[blowdart]

Can we make the log severity configurable? It could get noisy, and the ability to let folks choose would be useful.

[halter73]

Can we make the log severity configurable? It could get noisy, and the ability to let folks choose would be useful.

I don't think we ever make the severity we log at configurable. We generally try to pick a reasonable level for each log and let customers configure a min log level with namespace granularity.

Debug is pretty noisy, but at least it's not trace. I'm OK with trying info if you think that's better.

[Tratcher]

What happens without this?

[halter73]

I'm deleting my last comment and resubmitting so those reading this via notifications or email will hopefully be less confused. I wrote "is not" in the last comment when I meant "is".

TLS 1.0 is not allowed by default on my Windows 10 Insider Preview machine. This was brought up as a problem in #14997, is why we didn't use the defaults in the first place, and is why I was asking about whether we should log at a higher level if TLS 1.0 or SSL is used.

[halter73]

@blowdart I've updated the PR to log the "SslProtocol" at the debug level after a successful handshake. This is what the logs look like at the start of a TLS 1.2 connection with debug logs enabled:

dbug: Microsoft.AspNetCore.Server.Kestrel[39]
      Connection id "0HM085QVC1C8G" accepted.
dbug: Microsoft.AspNetCore.Server.Kestrel[1]
      Connection id "0HM085QVC1C8G" started.
dbug: Microsoft.AspNetCore.Server.Kestrel.Https.Internal.HttpsConnectionMiddleware[3]
      Connection "0HM085QVC1C8G" established using the following protocol: Tls12
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/2 GET https://localhost:5001/ - -

Does this seem reasonable to you?

There's a lot of additional details we could log about the connection like the CipherAlgorithm, but we can address that separately. It's already possible for developers to log additional details themselves using the ITlsHandshakeFeature.

[blowdart]

Works for me :) A sample for logging the rest would be good.

[Pilchie]

I approve, and agree with trying to get it in early.

[halter73]

@Pilchie Can you merge this?