-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Unify certificate validation code on OSX between SslStream and QuicConnection #117611
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
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 legacy macOS-specific hostname‐matching code paths and redirects certificate validation on OSX to the shared BuildChainAndVerifyProperties
logic.
- Removes
AppleCryptoNative_SslIsHostnameMatch
implementation and related P/Invoke/entrypoint. - Refactors
CertificateValidationPal.OSX.VerifyCertificateProperties
to call the unified validator. - Adds common validation helper file and updates the project to include it.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.h | Dropped the old hostname-matching export. |
src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.c | Removed the entire SslIsHostnameMatch implementation. |
src/native/libs/System.Security.Cryptography.Native.Apple/entrypoints.c | Deleted the DllImport entry for hostname matching. |
src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs | Replaced platform code with call to BuildChainAndVerifyProperties . |
src/libraries/System.Net.Security/src/System.Net.Security.csproj | Added new common validation source for OSX. |
src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs | Removed IDN mapping and hostname-check P/Invoke shim. |
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs:8
- [nitpick] Consider using a file-scoped namespace declaration (e.g.,
namespace System.Net;
) to align with project conventions and reduce indentation.
namespace System.Net
src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs:20
- Add unit tests covering the unified certificate validation logic on macOS—particularly hostname matching and chain validation—to verify parity with the previous implementation.
return CertificateValidation.BuildChainAndVerifyProperties(chain, remoteCertificate, checkCertName, isServer, hostName, Span<byte>.Empty);
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
Seems like this is affecting some test cases: OSX 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.
LGTM. I think this will simplify changes we made in NW PR?
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.
nice. lot of deletes. I hope the more code works properly :)
/ba-g infrastructure faliures are unrelated |
Follow-up on #117472, now that we have managed implementation of X509 name check, we can get rid of the code which was originally bound to SslContext. This PR unifies certificate validation code between SslStream and QuicConnection.