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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[HTTP/3] Suppress LocalCertificateSelectionCallback when QUIC is used #56094

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ies/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.SocketsHttpHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ protected static HttpClientHandler CreateHttpClientHandler(Version useVersion =
{
SocketsHttpHandler socketsHttpHandler = (SocketsHttpHandler)GetUnderlyingSocketsHttpHandler(handler);
SetQuicImplementationProvider(socketsHttpHandler, quicImplementationProvider);
socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = null;
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU: HttpClientHandler.ClientCertificateOptions

Copy link
Member Author

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:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs

Line 48 in b823f14

ClientCertificateOptions = ClientCertificateOption.Manual;

Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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... 🤔

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HttpClientHandler callback:

runtime/src/libraries/Common/src/System/Net/Security/CertificateHelper.cs

Lines 29 to 62 in 913facd

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:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs

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...

Copy link
Member Author

@ManickaP ManickaP Jul 21, 2021 •
edited
Loading

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.

Copy link
Member

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.

socketsHttpHandler.SslOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => true;
}

Expand Down