Fix performance regression in SSL handshake - #66077

Merged
rzikm merged 2 commits into
dotnet:mainfrom
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged

Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom
rzikm:66012-query-secpkg-regression

Conversation

@rzikm

@rzikmrzikm commented Mar 2, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Fixes Issue #66012

The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).

local run of the affected benchmark:

BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |

The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.

@ghostghost assigned rzikmMar 2, 2022
@ghost

ghost commented Mar 2, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes Issue #66012

The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).

BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:

area-System.Net.Security

Milestone:-

stephentoub
stephentoub reviewed Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs Outdated
if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
{
SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
// The query can fail if TLS handshake has not completed yet. In that case we fallback to querying

@stephentoubstephentoubMar 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?

@rzikmrzikmMar 3, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:

  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.

I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.

@wfurt

wfurt commented Mar 2, 2022

Copy link
Copy Markdown
Member

Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.

@rzikm

rzikm commented Mar 3, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Can know upfront if we are in handshake or not and do only one of the calls as appropriate?

If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.

And possibly know if the call is supported at all via static bool ?

I see a few options:

  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate

I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.

@rzikm

rzikm commented Mar 3, 2022

Copy link
Copy Markdown
MemberAuthor

Updated benchmarks look promising

// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |

@rzikm

rzikm commented Mar 3, 2022

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Mar 3, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022
Closed
Closed
wfurt
wfurt approved these changes Mar 3, 2022
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.

@rzikm

rzikm commented Mar 7, 2022

Copy link
Copy Markdown
MemberAuthor

CI failures are #66143, no relevant test failures to this change

@rzikm
rzikm merged commit 7698a9a into dotnet:mainMar 7, 2022
@rzikmrzikm mentioned this pull request Mar 7, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Apr 6, 2022
@karelzkarelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Fix performance regression in SSL handshake - #66077
Merged
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Mar 7, 2022
Merged
Fix performance regression in SSL handshake#66077
rzikm merged 2 commits into
dotnet:mainfrom 
rzikm:66012-query-secpkg-regression
Conversation
@rzikm
@rzikmrzikm
commented
Mar 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
local run of the affected benchmark:
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
The perf diff seems to be small on my machine, but maybe it is more pronounced on Win10 where the product benchmarks run.
@ghostghost
assigned rzikmMar 2, 2022
@ghost
ghost
commented
Mar 2, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes Issue #66012
The perf regression seems to be due to SECPKG_ATTR_REMOTE_CERT_CHAIN being more expensive to query than SECPKG_ATTR_REMOTE_CERT_CONTEXT used until #65134. This PR swaps the order of their querying so that CERT_CHAIN is queried only when we really need the certificate before TLS handshake is completed (i.e. during LocalClientCertificateSelectionCallback).
BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22151.18 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-CGHVCJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-GSIUQU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-RHRBBQ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-CGHVCJ | \7.0.0\corerun.exe | 1.677 ms | 0.0928 ms | 0.1069 ms | 1.632 ms | 1.523 ms | 1.846 ms | 0.99 | 0.10 | 5.59 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-GSIUQU | \main\corerun.exe | 1.736 ms | 0.0469 ms | 0.0521 ms | 1.726 ms | 1.656 ms | 1.850 ms | 1.02 | 0.07 | 5.58 KB | 0.99 |
| DefaultHandshakeContextIPv6Async | Job-RHRBBQ | \reverted\corerun.exe | 1.709 ms | 0.0915 ms | 0.1054 ms | 1.679 ms | 1.545 ms | 1.856 ms | 1.00 | 0.00 | 5.62 KB | 1.00 |
Author:rzikm
Assignees:rzikm
Labels:
area-System.Net.Security
Milestone:-
stephentoub
stephentoub
reviewed
Mar 2, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Outdated

 if (!SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext))
 {
 SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
 // The query can fail if TLS handshake has not completed yet. In that case we fallback to querying
@stephentoubstephentoubMar 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we expect this to be rare?
@rzikmrzikmMar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the only case when we need the remote cert before handshake completes is during the second call of the LocalClientCertificateSelectionCallback, which provides acceptable issuers and the server certificate as arguments. So the conditions for that happening should be:
  • Server requires client certificate
  • Client uses LocalClientCertificateSelectionCallback which during the first call returns null or a cert which does not match trusted issuers required by server.
I don't know how common that usage is, but since not many people complained that until #65134 the server cert was always null on Windows then I suppose it is indeed rare.
@wfurt
wfurt
commented
Mar 2, 2022

Copy link


Copy Markdown

Member
Can know upfront if we are in handshake or not and do only one of the calls as appropriate? And possibly know if the call is supported at all via static bool ? This fall-back logic still little bit worries me.
@rzikm
rzikm
commented
Mar 3, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Can know upfront if we are in handshake or not and do only one of the calls as appropriate?
If I see correctly, the CertificateValidationPal.GetRemoteCertificate has 2 overloads, one called during TLS handshake and one after the handshake completes, so we can differentiate between those two situations. I will try it out.
And possibly know if the call is supported at all via static bool ?
I see a few options:
  • set a flag once we encounter first failure, i.e. check for SEC_E_UNSUPPORTED_FUNCTION inside SSPIWrapper and short-circuit all following calls to SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN
  • put OS version check in CertificateValidationPal.GetRemoteCertificate
I am not sure I like either of them. Since we can differentiate between in-handshake and after-handshake calls, I think we should just leave it be. Win8 and newer support the function (I am not sure about Win7, the test that requires it is disabled there) and besides, until now we already called QueryContextAttributes at least once, so there will be no additional perf hit on Win7 since the call will fail as before.
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
Updated benchmarks look promising
// * Summary * BenchmarkDotNet=v0.13.1.1702-nightly, OS=Windows 11 (10.0.22000.493/21H2) Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores .NET SDK=7.0.100-preview.3.22152.16 [Host] : .NET 7.0.0 (7.0.22.11609), X64 RyuJIT Job-QSWYNC : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-YFPROJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT Job-IHEQPI : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15 WarmupCount=1 | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------------------- |----------- |---------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|--------:|----------:|------------:|
| DefaultHandshakeContextIPv6Async | Job-QSWYNC | \7.0.0\corerun.exe | 1.439 ms | 0.0178 ms | 0.0167 ms | 1.438 ms | 1.411 ms | 1.475 ms | 1.00 | 0.02 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-YFPROJ | \main\corerun.exe | 1.489 ms | 0.0232 ms | 0.0217 ms | 1.488 ms | 1.455 ms | 1.519 ms | 1.03 | 0.03 | 5.58 KB | 1.00 |
| DefaultHandshakeContextIPv6Async | Job-IHEQPI | \reverted\corerun.exe | 1.446 ms | 0.0254 ms | 0.0237 ms | 1.444 ms | 1.416 ms | 1.502 ms | 1.00 | 0.00 | 5.6 KB | 1.00 |
@rzikm
rzikm
commented
Mar 3, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Mar 3, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 3, 2022

Closed


Closed

wfurt
wfurt
approved these changes
Mar 3, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me if it fixes the regression. I think it is OK if we don't get certificate in callback on Windows 7. It was like this for a while and it is unlikely that we get new major customers there.
@rzikm
rzikm
commented
Mar 7, 2022

Copy link


Copy Markdown

MemberAuthor
CI failures are #66143, no relevant test failures to this change
@rzikm
rzikm
merged commit 7698a9a
into
dotnet:mainMar 7, 2022
@rzikmrzikm
mentioned this pull request
Mar 7, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Apr 6, 2022
@karelzkarelz
added this to the 7.0.0 milestone
Apr 8, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub approved these changes
@wfurtwfurtwfurt approved these changes

Assignees

@rzikmrzikm

Labels

area-System.Net.Security

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@rzikm@wfurt@stephentoub@karelz