Allow opt out of HttpSys client cert negotiation

[Tratcher]

#14806 @avparuch

Background:
Http.Sys has a unique capability not surfaced in other servers, the ability to renegotiate the TLS connection during a request to get a client certificate. This has been the standard behavior all the way back to HttpSys's origins in HttpListener. This can be useful because it means you can delay prompting for a cert until the user performs an action that requires it. However it also has some side effects like high latency and a potential protocol deadlock on POST requests if the TCP windows are full. Renegotiation is also not supported on HTTP/2.

In HttpSys it is not great that we trigger IO during a sync API call where people aren't expecting it (HttpContext.Connection.ClientCertificate). An async version is available (GetClientCertificateAsync).

The odd thing is that Http.Sys provides a simpler way to get the client certificate that was never implemented by HttpListener or HttpSys. If the client certificate has already been negotiated for a connection then it will be directly included in the request structure, no additional API calls or renegotiation is required. Clients rarely volunteer a certificate unless prompted, so it does require enabling the clientcertnegotiation option on the netsh binding to get the client cert at the start of the connection. This is the same approach IIS and Kestrel use.

Change:
I've added ClientCertificateMode to let you control the behavior of HttpContext.Connection.ClientCertificate for HttpSys. The the old behavior is still the default for compat reasons, but I'll file an issue to change that in 5.0. If you change the setting to AllowCertificate then it will only populate certs if they're present in the request structure. For NoCertificate it will always return null.
HttpContext.Connection.GetClientCertificateAsync will maintain the old behavior regardless of the new option. We should also change this in 5.0 to prefer the request cert before attempting a renegotiation (needs testing).

Testing: There are only some skipped manual tests for client certs because they are not constantly available on machines. Also the netsh clientcertnegotiation setting is machine state can't be changed during a test so the new Allow mode would never return a cert. Verified manually and we have a customer waiting to test previews.

[analogrelay]

FYI, this is not yet approved for 3.1, but the request is out.

[avparuch]

@Tratcher : After this change is implemented, if I choose the "allow certificate" option, the configuration in http.sys (Configured via netsh) will guide what happens during the original TLS handshake (either requesting a client, not requesting a client cert, not requesting client cert, but client still sending one anyway) and nothing in the application code later can trigger a TLS renegotiation. Am I understanding the change correctly?

[Tratcher]

Yes, except for GetClientCertificateAsync which can still request a renegotiation (if no cert is already present?).

[avparuch]

Thanks, also, in the current implementation of the ClientCertificate, the source code indicates that it is a blocking call. (calling .Result), so while the TLS renegotiation happens (IO based), the thread is actually blocked. I'm hoping after the change, the call is not blocking anymore?

[Tratcher]

Thanks, also, in the current implementation of the ClientCertificate, the source code indicates that it is a blocking call. (calling .Result), so while the TLS renegotiation happens (IO based), the thread is actually blocked. I'm hoping after the change, the call is not blocking anymore?

See https://github.com/aspnet/AspNetCore/pull/14839/files#diff-8913a426c378a817f8b15deba026e9b1R320-R327
The old behavior is still the default for compat reasons, but the new behavior doesn't do any IO, it only reads from existing structures. I've filed #14840 to track changing the default in the next major release.

GetClientCertificateAsync is the nonblocking API to call if you do want the renegotiation.

[jkotalik]

@davidfowl new public api here, could use approval from you.

[davidfowl]

New public API that matches the exact same types as kestrel? We can’t keep doing this. It causes type conflicts.

[Tratcher]

New public API that matches the exact same types as kestrel? We can’t keep doing this. It causes type conflicts.

Names are hard 😁. It fills the same role but has different values. Open to alternate suggestions.

[davidfowl]

Can you confirm what a type conflict looks like?