[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684

Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom
wfurt:isMA
Jan 10, 2024
Merged

[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom
wfurt:isMA

Conversation

@wfurt

@wfurtwfurt commented Sep 27, 2023•
edited by karelz
Loading

Copy link
Copy Markdown
Member

This is backport of PR #88488 and PR #79128 and parts of PR #63945.
It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.

This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.

Customer Impact

The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.

Testing

This brings all the current tests from 8.0 branch.
Customer validated on private bits in production - neither functional, nor perf regression.

Risk

Medium.
While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.

@ghostghost assigned wfurtSep 27, 2023
@ghost

ghost commented Sep 27, 2023

Copy link
Copy Markdown

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

Issue Details

PoC

Author:wfurt
Assignees:wfurt
Labels:

area-System.Net.Security

Milestone:-

@build-analysisbuild-analysisBot mentioned this pull request Sep 29, 2023
Open
@wfurtwfurt changed the title fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt requested review from karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt marked this pull request as ready for review October 18, 2023 00:55
rzikm
rzikm approved these changes Oct 18, 2023
View reviewed changes

@rzikmrzikm 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.

LGTM if CI is green

@karelzkarelz added this to the 6.0.x milestone Oct 18, 2023
@build-analysisbuild-analysisBot mentioned this pull request Oct 18, 2023
Closed
@carlossanlop

carlossanlop commented Oct 19, 2023

Copy link
Copy Markdown
Contributor

LGTM if CI is green

@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.

@karelz

karelz commented Oct 19, 2023

Copy link
Copy Markdown
Member

@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.

karelz
karelz reviewed Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm

rzikm commented Nov 2, 2023

Copy link
Copy Markdown
Member

/azp run runtime

@azure-pipelines

azure-pipelinesBot commented Nov 2, 2023

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

@carlossanlop

carlossanlop commented Nov 9, 2023

Copy link
Copy Markdown
Contributor

Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).

@karelz

karelz commented Nov 14, 2023

Copy link
Copy Markdown
Member

Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.

@carlossanlopcarlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 15, 2023
@wfurtwfurt removed their assignment Nov 15, 2023
@rzikmrzikm mentioned this pull request Dec 7, 2023
Merged
@karelzkarelz added Servicing-consider Issue for next servicing release review and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Jan 9, 2024
stephentoub
stephentoub reviewed Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs
This was referenced Jan 10, 2024
Closed
Open
@karelz

karelz commented Jan 10, 2024

Copy link
Copy Markdown
Member

Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.

@karelzkarelz added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 10, 2024
@rzikm
rzikm merged commit f27366f into dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 10, 2024
@karelzkarelz modified the milestones: 6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees

No one assigned

Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects

None yet

Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub
, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub

, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub

, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub

, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub

, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub

, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub

, '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
[release/6.0] fix IsMutuallyAuthenticated on SslStream - #92684
Merged
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Jan 10, 2024
Merged
[release/6.0] fix IsMutuallyAuthenticated on SslStream#92684
rzikm merged 9 commits into
dotnet:release/6.0-stagingfrom 
wfurt:isMA
Conversation
@wfurt
@wfurtwfurt
commented
Sep 27, 2023•

edited by karelz

Loading

Copy link


Copy Markdown

Member
This is backport of PR #88488 and PR #79128 and parts of PR #63945.

It also brings spirit of test-only PR #68009 to get test coverage for TLS 1.3.
This only covers Windows to minimize the code delta i.e. it does not bring all the changes from PR #63945 to cover Linux & macOS.
Customer Impact
The property IsMutuallyAuthenticated on SslStream indicates if mutual TLS authentication is performed with client certificate. Current 6.0 implementation can get confused in several cases, so the value is unreliable for security audits.
Testing
This brings all the current tests from 8.0 branch.

Customer validated on private bits in production - neither functional, nor perf regression.
Risk
Medium.

While the change is quite large, it should be specific just to that property i.e. it should not impact TLS handshake or any other I/O on SslStream. Since the IsMutuallyAuthenticated is already unreliable this should bring it up to 8.0 code base to fix all known cases when it is incorrect. To reduce complexity, this fixes only Windows as macOS & Linux changes from PR #68009 had more significant impact on functionality and flow.
@ghostghost
assigned wfurtSep 27, 2023
@ghost
ghost
commented
Sep 27, 2023

Copy link


Copy Markdown

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

See info in area-owners.md if you want to be subscribed.
Issue Details
PoC
Author:wfurt
Assignees:wfurt
Labels:
area-System.Net.Security
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 29, 2023

Open

@wfurtwfurt
changed the title
fix IsMutuallyAuthenticated[release/6.0] fix IsMutuallyAuthenticated on SslStreamOct 18, 2023
@wfurt
wfurt
requested review from
karelz and rzikmOctober 18, 2023 00:55
@wfurt
wfurt
marked this pull request as ready for review
October 18, 2023 00:55
rzikm
rzikm
approved these changes
Oct 18, 2023
View reviewed changes
@rzikmrzikm
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.

LGTM if CI is green
@karelzkarelz
added this to the 6.0.x milestone
Oct 18, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 18, 2023

Closed

@carlossanlop
carlossanlop
commented
Oct 19, 2023

Copy link


Copy Markdown

Contributor
LGTM if CI is green
@wfurt please send an email to Tactics requesting approval and add the servicing-consider label. I couldn't find an email yet. We still have time to include this in the November release.
@karelz
karelz
commented
Oct 19, 2023

Copy link


Copy Markdown

Member
@carlossanlop we will bring it in for December. We need to prepare also 7.0 backport - fixing only 6.0 would be weird. And as you see from the delta, it is rather involved change, so I don't want to rush it.
karelz
karelz
reviewed
Oct 19, 2023
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs
@rzikm
rzikm
commented
Nov 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Nov 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@carlossanlop
carlossanlop
commented
Nov 9, 2023

Copy link


Copy Markdown

Contributor
Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).
@karelz
karelz
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Thanks @carlossanlop we want to get validation on privates before we send it to Tactics. We will miss also December release.
@carlossanlopcarlossanlop
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Nov 15, 2023
@wfurtwfurt
removed their assignment
Nov 15, 2023
@rzikmrzikm
mentioned this pull request
Dec 7, 2023
 Merged

@karelzkarelz
added

Servicing-consider
Issue for next servicing release review
and removed

NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Jan 9, 2024
stephentoub
stephentoub
reviewed
Jan 9, 2024
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs

This was referenced Jan 10, 2024

Closed


Open

@karelz
karelz
commented
Jan 10, 2024

Copy link


Copy Markdown

Member
Approved by Tactics (@SteveMCarroll) on 1/9 via email. Adding Servicing-approved label accordingly.
@karelzkarelz
added

Servicing-approved
Approved for servicing release
and removed

Servicing-consider
Issue for next servicing release review
labels
Jan 10, 2024
@rzikm
rzikm
merged commit f27366f
into
dotnet:release/6.0-stagingJan 10, 2024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 10, 2024
@karelzkarelz
modified the milestones:
6.0.x, 6.0.27Jun 25, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments
@rzikmrzikmrzikm approved these changes
@stephentoubstephentoubstephentoub approved these changes

Assignees


No one assigned


Labels

area-System.Net.SecurityServicing-approvedApproved for servicing release

Projects


None yet


Milestone

6.0.27

Development

Successfully merging this pull request may close these issues.

5 participants

@wfurt@carlossanlop@karelz@rzikm@stephentoub