') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); fix SslStream.IsMutuallyAuthenticated with cached credentials by wfurt · Pull Request #79128 · dotnet/runtime · GitHub
Skip to content

fix SslStream.IsMutuallyAuthenticated with cached credentials - #79128

Merged
wfurt merged 6 commits into
dotnet:mainfrom
wfurt:iIsMutuallyAuthenticated
Dec 13, 2022
Merged

fix SslStream.IsMutuallyAuthenticated with cached credentials#79128
wfurt merged 6 commits into
dotnet:mainfrom
wfurt:iIsMutuallyAuthenticated

Conversation

@wfurt

@wfurtwfurt commented Dec 2, 2022

Copy link
Copy Markdown
Member

fixes#65563

We can query Schannel after handshake is done to see if certificate was used. This is no-op for test of the platforms at the moment.
The test is essentially repro @rzikm put together during investigation.

not 100% about android. cc: @simonrozsival

@wfurt
wfurt requested review from liveans and rzikmDecember 2, 2022 01:29
@wfurtwfurt self-assigned this Dec 2, 2022
@ghost

ghost commented Dec 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 #65563

We can query Schannel after handshake is done to see if certificate was used. This is no-op for test of the platforms at the moment.
The test is essentially repro @rzikm put together during investigation.

not 100% about android. cc: @simonrozsival

Author:wfurt
Assignees:wfurt
Labels:

area-System.Net.Security

Milestone:-

simonrozsival
simonrozsival reviewed Dec 2, 2022
View reviewed changes

@simonrozsivalsimonrozsival left a comment •
edited
Loading

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'm not very familiar with the client selection code on Android and it's not very easy to follow, but this is my current interpretation of the what we do:

  • whenever there's a CertificateContext in the auth options, we initialize the Java KeyStore with the client certificate. The Java SSLEngine may or may not send the client certificate to the server. We never call the SelectClientCertificate. We don't check if the client sent the certificate to the server or not.
  • the CertificateContext is set by the AcquireClientCredentials method which calls SelectClientCertificate method that sets the _selectedClientCertificate field.
  • So from what I can tell, on Android the IsMutuallyAuthenticated property could be true even though we never sent the client certificate to the server.

But... don't we call AcquireClientCredentials on all platforms? Isn't that the same case also on mac/iOS?

I think we should at least run tests on Android for this PR and see if the new test fails or not. I can do some more digging if necessary. It should be fairly easy to implement IsLocalCertificateUsed to actually check which certificate we sent to the server (using SSLSession#getLocalCertificates()).

Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs Outdated
@simonrozsival

simonrozsival commented Dec 2, 2022

Copy link
Copy Markdown
Member

/azp run runtime-android

@azure-pipelines

azure-pipelinesBot commented Dec 2, 2022

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

@rzikm

rzikm commented Dec 5, 2022

Copy link
Copy Markdown
Member

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-79128-merge-1d0bff68b72b460f94/System.Net.Security.Tests/3/console.e5e0dbcd.log?helixlogtype=result

 System.Net.Security.Tests.SslStreamMutualAuthenticationTest.SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect(protocol: Tls12) [FAIL]
Assert.Equal() Failure
Expected: True
Actual: False
Stack Trace:
/_/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs(121,0): at System.Net.Security.Tests.SslStreamMutualAuthenticationTest.SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect(SslProtocols protocol)
--- End of stack trace from previous location ---

Looks like we fail on Win7

rzikm
rzikm reviewed Dec 5, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs Outdated
Comment threadsrc/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
@simonrozsival

simonrozsival commented Dec 5, 2022

Copy link
Copy Markdown
Member

@wfurt The test is failing on Android. I have a fix ready so you can skip the test on Android and I'll open a follow-up PR once this one is merged.

@wfurt

wfurt commented Dec 5, 2022

Copy link
Copy Markdown
MemberAuthor

One day I'll learn how to run tests on Android locally ... but it may not be this PR.

wfurtand others added 2 commits December 5, 2022 21:24
@wfurt
fix test
3d03cb0
@wfurt@rzikm@simonrozsival
Apply suggestions from code review
fceba83
Co-authored-by: Radek Zikmund <32671551+rzikm@users.noreply.github.com>
Co-authored-by: Simon Rozsival <simon@rozsival.com>
@wfurt

wfurt commented Dec 6, 2022

Copy link
Copy Markdown
MemberAuthor

I disabled the test on Android with active issue pointing at original one. I'll keep it open until you are ready with the fix.
(and thanks for looking into it)

@build-analysisbuild-analysisBot mentioned this pull request Dec 6, 2022
Closed
@wfurt

wfurt commented Dec 9, 2022

Copy link
Copy Markdown
MemberAuthor

Last test (CertificateValidationClientServer_EndToEnd_Ok) was failing on server 2012. I investigated and the call would finish with SEC_E_NO_CREDENTIALS if the connection was resumed. That seems like issue with older schannel versions - never windows return the certificate status according to the previous.

For now, I updated the test avoid this case. I'm not sure what we can do about it. I feel it would be better if we report the status correctly on current Windows, hopefully 2012 will go out of support soon.
Let me know if you have any suggestions @rzikm.

@wfurt
wfurt merged commit cdb9f5f into dotnet:mainDec 13, 2022
@wfurt
wfurt deleted the iIsMutuallyAuthenticated branch December 13, 2022 00:09
@simonrozsivalsimonrozsival mentioned this pull request Dec 13, 2022
Merged
wfurt added a commit to wfurt/runtime that referenced this pull request Dec 29, 2022
@wfurt
undodotnet#79128test change
0423de5
@wfurtwfurt mentioned this pull request Dec 29, 2022
Merged
@ghostghost locked as resolved and limited conversation to collaborators Jan 12, 2023
@karelzkarelz added this to the 8.0.0 milestone Mar 22, 2023
@wfurt

wfurt commented Aug 31, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actionsgithub-actionsBot unlocked this conversation Aug 31, 2023
@github-actions

github-actionsBot commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/6040352662

@wfurt

wfurt commented Aug 31, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0

@github-actions

github-actionsBot commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/6040355498

@wfurt

wfurt commented Aug 31, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-staging

@github-actions

github-actionsBot commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

@wfurt backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: fix SslStream.IsMutuallyAuthenticated with cached credentials
Using index info to reconstruct a base tree...
M	src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
M	src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs
M	src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs
M	src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs
M	src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
A	src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
A	src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs deleted in HEAD and modified in fix SslStream.IsMutuallyAuthenticated with cached credentials. Version fix SslStream.IsMutuallyAuthenticated with cached credentials of src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs left in tree.
Auto-merging src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
CONFLICT (content): Merge conflict in src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
Auto-merging src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
Auto-merging src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs
Auto-merging src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs
Auto-merging src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs
Auto-merging src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
CONFLICT (content): Merge conflict in src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix SslStream.IsMutuallyAuthenticated with cached credentials
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions

github-actionsBot commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

@wfurt an error occurred while backporting to release/6.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@github-actions

github-actionsBot commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6040373273

@github-actionsgithub-actionsBot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@simonrozsivalsimonrozsivalsimonrozsival left review comments
@rzikmrzikmrzikm approved these changes
@liveansliveansAwaiting requested review from liveans

Assignees

@wfurtwfurt

Labels

area-System.Net.Security

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

SslStream.IsMutuallyAuthenticated returns wrong value when cached creds are used

4 participants

@wfurt@simonrozsival@rzikm@karelz