-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[HTTP/3] Suppress LocalCertificateSelectionCallback when QUIC is used #56094
Conversation
Tagging subscribers to this area: @dotnet/ncl |
@@ -50,6 +50,7 @@ protected static HttpClientHandler CreateHttpClientHandler(Version useVersion = | |||
{ | |||
SocketsHttpHandler socketsHttpHandler = (SocketsHttpHandler)GetUnderlyingSocketsHttpHandler(handler); | |||
SetQuicImplementationProvider(socketsHttpHandler, quicImplementationProvider); | |||
socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defaults to null. What's setting it to non-null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU: HttpClientHandler.ClientCertificateOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it gets set in default ctor of client handler:
ClientCertificateOptions = ClientCertificateOption.Manual; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. What happens when this isn't nulled out? If our QUIC implementation doesn't support a non-null LocalCertificateSelectionCallback, doesn't this mean we're making our tests pass even though all normal usage will fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before #55877 LocalCertificateSelectionCallback was just ignored silently, so I guess if something weird was passed in the callback we didn't catch it, if that's what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that reverting might be safer because it's possible there were other things like this implicit LocalCertificateSelectionCallback... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into the code, all this hidden HttpClientHandler
LocalCertificateSelectionCallback
does is selects a cert from ClientCertificates
collection based on having private key and checking its extensions. And that first part is what we do in msquic (selecting the one with private key). So despite ignoring the callback, it might still work as excepted in most cases. But @wfurt would have to chime in here for more details, this the extent of my knowledge with certs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HttpClientHandler
callback:
internal static X509Certificate2? GetEligibleClientCertificate(X509Certificate2Collection candidateCerts) | |
{ | |
if (candidateCerts.Count == 0) | |
{ | |
return null; | |
} | |
foreach (X509Certificate2 cert in candidateCerts) | |
{ | |
if (!cert.HasPrivateKey) | |
{ | |
if (NetEventSource.Log.IsEnabled()) | |
{ | |
NetEventSource.Info(candidateCerts, $"Skipping current X509Certificate2 {cert.GetHashCode()} since it doesn't have private key. Certificate Subject: {cert.Subject}, Thumbprint: {cert.Thumbprint}."); | |
} | |
continue; | |
} | |
if (IsValidClientCertificate(cert)) | |
{ | |
if (NetEventSource.Log.IsEnabled()) | |
{ | |
NetEventSource.Info(candidateCerts, $"Choosing X509Certificate2 {cert.GetHashCode()} as the Client Certificate. Certificate Subject: {cert.Subject}, Thumbprint: {cert.Thumbprint}."); | |
} | |
return cert; | |
} | |
} | |
if (NetEventSource.Log.IsEnabled()) | |
{ | |
NetEventSource.Info(candidateCerts, "No eligible client certificate found."); | |
} | |
return null; | |
} |
S.N.Quic code:
Lines 57 to 72 in 913facd
if (options.ClientAuthenticationOptions.ClientCertificates != null) | |
{ | |
foreach (var cert in options.ClientAuthenticationOptions.ClientCertificates) | |
{ | |
try | |
{ | |
if (((X509Certificate2)cert).HasPrivateKey) | |
{ | |
// Pick first certificate with private key. | |
certificate = cert; | |
break; | |
} | |
} | |
catch { } | |
} | |
} |
It's not the same thing, but there are some some similarities...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But @stephentoub is right, new HttpClient
+ H/3 request will crash anyway. My hack mitigates this only in our tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverting #56097 was the right choice. Sorry for the mess, I'm not sure how I missed it. The problem with LocalCertificateSelectionCallback
is that it is not supported by MsQuic. so We either need to go back to ignoring it or fix the tests.
This still will need follow up in our code, since runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs Lines 244 to 275 in b823f14
Thus it gets sets indirectly and user might not have a clue what set off the check. @wfurt Do you have any ideas how to solve this somewhat nicely? |
Hello @ManickaP! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 minutes, a condition that will be fulfilled in about 94 seconds. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Closing in favor of revert PR #56097, which has been merged -- see above discussion. |
Fixes #56090