-
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
[release/6.0] fix IsMutuallyAuthenticated on SslStream #92684
Conversation
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones Issue DetailsPoC
|
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 if CI is green
@wfurt please send an email to Tactics requesting approval and add the |
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete). |
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release. |
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly. |
This is backport of PR #88488 and PR #79128 and parts of PR #63945.
It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property
IsMutuallyAuthenticated
onSslStream
indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.Testing
This brings all the current tests from 8.0 branch.
Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.
While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on
SslStream
. Since theIsMutuallyAuthenticated
is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.