[release/8.0] Correctly set sendTrustList flag when saving credentials to cache #94402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones Issue DetailsBackport of #92731 to release/8.0 /cc @rzikm Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
This was referenced Nov 6, 2023
wfurt
reviewed
Nov 6, 2023
rzikm
force-pushed
the
backport/pr-92731-to-release/8.0
branch
from
c90cd06
to
ca42a8e
Compare
|
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete). |
|
Tactics approved by @SteveMCarroll on 11/2 via email -- adding Servicing-approved label. |
karelz
added
Servicing-approved
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #92731 to release/8.0, same as #94080 was for 6.0
/cc @rzikm
Customer Impact
The change fixes high CPU usage on the server in scenarios which utilize mutual authentication (TLS feature where the server sends the list of trusted certificate issuers to the client when requesting client certificates) by correctly caching credentials. Lower CPU usage means higher server throughput (up to +50% for targeted repro).
Mutual authentication is fairly advanced scenario for high throughput services. On Windows it requires a registry key to be set for OS to send the certificates.
The problem in the code is that we cache the credentials always with
sendInHandshake=false, regardless if the credentials were sent or not on the wire. That means we will never find it in the cache when we look it up next time withsendInHandshake=true, which leads to creation of new Schannel credentials for each incoming connection - wasting CPU cycles.Testing
Verified on customer-provided repro.
Risk
Low. The change is very small and affects only a very specific scenario using mutual authentication on Windows (which is not common).