-
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
fix quic cert validation with OpenSSL #51015
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis depends on pending microsoft/msquic#1450. Note that support for certificate chains is still pending. e.g. CertificateContext. fixes #50156
|
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.
LGTM
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.
Just code-styling nits, otherwise LGTM. Disclaimer: IANA SSL expert.
Tests?
And I agree with James, this is worth some coverage,
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
Outdated
Show resolved
Hide resolved
existing tests will fail when msquic is built without STUB and without the hack to accept all certificates. |
I modified the test validation callback to verify that what is passed in is same as what server uses. This should now work on all platforms instead blindly assume it is ok via lambda function. |
@@ -214,25 +214,26 @@ private static uint HandleEventPeerCertificateReceived(State state, ref Connecti | |||
|
|||
try | |||
{ | |||
if (OperatingSystem.IsWindows()) | |||
if (connectionEvent.Data.PeerCertificateReceived.PlatformCertificateChainHandle != IntPtr.Zero) |
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.
Is this right? Is there any relation between PlatformCertificateChainHandle
and PlatformCertificateHandle
? Like that if the first is not null then the other won't be as well?
Because originally you had different null-check conditions there.
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.
good catch. In general they should be both set or empty but it is strange to test one filed and then use other. I did yet another attempt to refactor and make that more clear.
For Unix path, the PlatformCertificateHandle points to DER encoded certificate while PlatformCertificateChainHandle points to PKCS7 container containing possibly list of certificates. (the format allows additional parts but that is not generated by msquic)
On Windows the intermediate CA cache is maintained by OS so we don't need to bother.
On Linux is everything is trusted and intermediates present we could simple grab the leaf cert.
But we don't know that so we attempt to get as many certs provided by the peer to improve our chances that X509Chain con construct full chain.
This depends on pending microsoft/msquic#1450.
Adds "normal" certificate validation and validation callback for Linux & macOS e.g. OpenSSL TLS.
Note that support for certificate chains is still pending. e.g. CertificateContext.
fixes #50156
cc: @nibanks @JamesNK