[release/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt
, '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/8.0] Removed unused sessions from SSL_CTX internal cache - #102095

Merged
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging
Jun 4, 2024
Merged

[release/8.0] Removed unused sessions from SSL_CTX internal cache#102095
rzikm merged 3 commits into
release/8.0-stagingfrom
backport/pr-101684-to-release/8.0-staging

Conversation

@github-actions

@github-actionsgithub-actionsBot commented May 10, 2024

Copy link
Copy Markdown
Contributor

Backport of #101684 to release/8.0-staging

/cc @wfurt@rzikm

Customer Impact

Reported by customer via official support. Small repro available.
Customers on Linux sees increased memory usage when establishing parallel connections to the same host (note that parallel requests on HTTP/1.1 will always use parallel connections). Measured overhead can be easily 100M+, which is problem for containers in k8s clusters limited to 300M memory.

It also helped one customer in general "memory problems with .NET 6 -> 8 upgrade" issue - see comment.

Workaround is lowering TLS cache size via:

  • System.Net.Security.TlsCacheSize AppCtx switch, or
  • DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE environment variable

Technical details:

The mechanism of the (bounded) memory leak is as follows:

  • On new TLS connection, we receive TLS session ticket so that future connections can use TLS Resume (for faster handshake) (this is new feature in .NET 7).
  • There are 2 TLS session caches: internal one in OpenSSL, and managed one in .NET. These caches are supposed to be in sync via callbacks (e.g. OpenSSL may remove an entry internally and notifies managed code via callback that a ticket has been invalidated).
  • When many connections are opened in a burst, we receive many TLS session tickets in quick succession, but the implementation keeps only the last one.
  • Sessions discarded from managed (.NET) TLS session cache were still being kept in the internal OpenSSL cache, until the ticket expired or the internal cache reached a predefined maximum size.

The fix is to keep the two caches in sync and remove the dropped TLS session tickets from the internal cache as well.

Regression

Yes, the bug is part of TLS Session resumption feature on Linux, introduced in .NET 7. For customers migrating from .NET 6 it manifests as E2E scenario regression.

Testing

Tested on customer provided minimal repro.
Customer was not willing to verify privates in production.
Note: Customer confirmed that the workaround helps them in production, which means we have high confidence, this fix is the real root cause of their production problems and will help them.

Risk

Low, the issue is well understood and the change is localized to the feature. Functional tests verified TLS resumption works.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm
rzikm requested review from karelz and wfurtMay 13, 2024 08:09
@rzikmrzikm changed the title [release/8.0-staging] Removed unused sessions from SSL_CTX internal cache[release/8.0] Removed unused sessions from SSL_CTX internal cacheMay 13, 2024

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

LGTM

@karelzkarelz added os-linux Linux OS (any supported distro) Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@karelz

Copy link
Copy Markdown
Member

Memory usage impact on production systems. E2E scenario regression from .NET 6. We should service it.

@rzikm

rzikm commented Jun 3, 2024

Copy link
Copy Markdown
Member

Approved via email on 06/03.

@rzikmrzikm added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 3, 2024
@rzikm
rzikm merged commit bfaf24e into release/8.0-stagingJun 4, 2024
@jkotas
jkotas deleted the backport/pr-101684-to-release/8.0-staging branch June 7, 2024 03:26
@karelz

Copy link
Copy Markdown
Member

Will be part of July release 8.0.7.

@karelzkarelz modified the milestones: 8.0.x, 8.0.7Jun 25, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Securityos-linuxLinux OS (any supported distro)Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@karelz@rzikm@wfurt