-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 assert in ssl options clone #72326
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue Detailsfixes #71233 The assert we are hitting was added to detect cases when we add new property but we fail to update the clone method. return _underlyingHandler.SslOptions.ClientCertificates ??
(_underlyingHandler.SslOptions.ClientCertificates = new X509CertificateCollection()); The tests (like SendMoreThanStreamLimitRequestsConcurrently_LastWaits) triggering the assert use This fix is to use underlying object without attempt to initialize it and guarding against
|
@@ -19,6 +19,7 @@ public static SslClientAuthenticationOptions ShallowClone(this SslClientAuthenti | |||
AllowRenegotiation = options.AllowRenegotiation, | |||
ApplicationProtocols = options.ApplicationProtocols != null ? new List<SslApplicationProtocol>(options.ApplicationProtocols) : null, | |||
CertificateRevocationCheckMode = options.CertificateRevocationCheckMode, | |||
CertificateChainPolicy = options.CertificateChainPolicy, |
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.
Are we missing a test that should have failed without this?
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.
yes, we should add test to HTTP once QUIC supports CertificateChainPolicy.
fixes #71233
The assert we are hitting was added to detect cases when we add new property but we fail to update the clone method.
(it failed that purpose and I added the new missing CertificateChainPolicy as well)
However, in this case it fails because the given
SslClientAuthenticationOptions
is being modified while cloned.While the underlying issue existed for a while I think the recent failures are triggered by #70716.
Registered LocalCertificateSelectionCallback will access Handler's
ClientCertificates
and that will triggerThe tests (like SendMoreThanStreamLimitRequestsConcurrently_LastWaits) triggering the assert use
Parallel.For
to blast bunch of requests, each racing the initialization in chain setup and modifying ClientCertificates fromnull
to empty collection.This fix is to use underlying object without attempt to initialize it and guarding against
null
inGetEligibleClientCertificate
.