-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove dependency on System.Security.Cryptography.Native.OpenSsl in QUIC #117472
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
Remove dependency on System.Security.Cryptography.Native.OpenSsl in QUIC #117472
Conversation
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.
Pull Request Overview
This PR removes the dependency on System.Security.Cryptography.Native.OpenSsl for hostname matching in the QUIC library on macOS by replacing native OpenSSL certificate hostname validation with the managed X509Certificate2.MatchesHostname implementation.
- Restructures project file to exclude OpenSSL cryptography interop files from macOS builds
- Replaces native OpenSSL hostname matching with managed X509Certificate2.MatchesHostname API
- Simplifies certificate validation logic by removing unsafe native code handling
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
System.Net.Quic.csproj | Reorganizes ItemGroup conditions to exclude OpenSSL cryptography interop files from macOS builds while maintaining them for Linux/FreeBSD |
CertificateValidation.OSX.cs | Replaces native OpenSSL hostname matching with managed X509Certificate2.MatchesHostname implementation |
src/libraries/Common/src/System/Net/Security/CertificateValidation.OSX.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/Net/Security/CertificateValidation.OSX.cs
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/ncl |
One of the tests is failing with the hostname It's not immediately clear to me if this is "right". My interpretation of the relevant RFCs is that a space is not permitted in host or DNS labels, so perhaps the test should simply omit the spaces. |
It should probably also be said: using libmsquic on macOS with .NET on Apple Silicon machines is very difficult. I see the tests failing on x64 macOS because libmsquic is available, but the hardened runtime on Apple Silicon makes it very difficult to enable. |
Does this change mean that we can get rid of runtime/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs Lines 10 to 48 in 8300e38
And do the same thing as we do on Unix? runtime/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs Lines 11 to 22 in e4e866a
I looked at the code recently in the TLS 1.3 support PR and it seems to me that |
I am also confused by the presence of spaces in the hostname, @krwq, your name shows up on git blame, do you remember why you put spaces in the test case? I am personally ok with removing them |
I don't remember - that was 7 years ago... but I think per this comment: dotnet/corefx#28278 (comment) all ASCII are legal and UTF-8 used to be legal at one point. Looking at RFC 4366 3.1:
"non US-ASCII" characters are a bit of a moot term for things like space so let's look at the IDNA ToASCII definition... IDNA is defined in RFC3490 4.1: (with my comments)
There is also RFC6066 Appendix A:
so IMO all ASCII (0x00-0x7F) are legal for decoding given it's not guaranteed that UseSTD3ASCIIRules is used for encoding. But in theory we could block them with some sort of "strict validation" setting given it's possible to encode them. |
I think we are concerned more with how hostnames match against certificates, not what is permitted in the SNI extension. RFC 1035 does not permit spaces in DNS labels. https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1 It also doesn't permit underscores, and we know OpenSSL enforces that.
Maybe, assuming we can get this change though :-).
Yeah Ideally we could get rid of the AppleCrypto hostname check... they don't expose a straight forward API for "Is this certificate okay for this host". The only way we could get something to work was to just build a whole chain and see if the chain had a host mismatch name error. We should keep using the native hostname checks for OpenSSL and Windows though. |
would it make sense to sue the managed implementation also on Linux for consistency @vcsjones ??? Or are the OpenSSL internal check unavoidable? |
OpenSSL is going to do it itself in other places, so it should be consistent with itself. Apple is already using two implementations, Security.framework and OpenSSL, so we are just changing it to a... different... two implementations. |
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, thanks!
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.
As I already mentioned offline, it should be ok to break this until we see someone actually demonstrating space is needed for their scenario (which sounds very unlikely even if technically allowed). My recommendation would be to only allow same characters UseSTD3ASCIIRules talks about but I'm ok with this as is.
Quic failures are unrelated and should be addressed by #117495. |
Over at #117465, I am trying to get rid of S.S.C.Native.OpenSsl on macOS, but System.Net.Quic current uses parts of it for matching a certificate to a hostname. This makes sense since Apple's native APIs don't offer a similar API. But good news, we have an entirely managed implementation we can use on macOS.
This pull request replaces the use of the native PAL for hostname matching with our managed implementation, X509Certificate2.MatchesHostname, on macOS.