-
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
ignore name mismatch when IgnoreInvalidName is set #73745
Conversation
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue Detailscontributes to #71191, adds test for issue fixed by #72326 I bump to this when I was trying to improve test coverage and I wanted to override authentication errors using newly added policy. Aside from this, this plugs some test gaps.
|
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.
There are some build errors but otherwise LGTM if fixed and clean CI
src/libraries/Common/tests/System/Net/Configuration.Certificates.cs
Outdated
Show resolved
Hide resolved
using (X509Store store = new X509Store(storeName, StoreLocation.LocalMachine)) | ||
{ | ||
store.Open(OpenFlags.ReadWrite); | ||
foreach (X509Certificate2 cert in store.Certificates) |
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.
All of these need to be disposed of, right? Same for the iteration below.
src/libraries/Common/tests/System/Net/Configuration.Certificates.cs
Outdated
Show resolved
Hide resolved
@@ -3728,6 +3730,132 @@ public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http3 : | |||
protected override Version UseVersion => HttpVersion.Version30; | |||
} | |||
|
|||
public abstract class SocketsHttpHandler_SslTest : HttpClientHandlerTestBase |
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.
Nit: SslTest => SslOptionsTest? (If you change it, also change the derived type names)
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 was not sure. The intention is to test security related functions e.g. SslStream, Quic & certificate handling...
contributes to #71191, adds test for issue fixed by #72326
I bump to this when I was trying to improve test coverage and I wanted to override authentication errors using newly added policy.
The spirit of #71191 was ability to override inside SslStream validation magic to customize validation.
While the name validation is out of the Chain itself, I hook it up so X509VerificationFlags.IgnoreInvalidName flag is respected.
Aside from this, this plugs some test gaps.
GenerateCertificates did come from SslStream and Quic start using it as convenience.
I did not want to add another project reference to HTTP so I moved it to Common and updated Quic and Http to use it.
I will follow-up for SslStream but I did not want to make this change too big.