TlsContext/TlsSession: document lifetime, tighten AttachSocket - #131457
Open
wfurt wants to merge 1 commit into
Open
TlsContext/TlsSession: document lifetime, tighten AttachSocket#131457wfurt wants to merge 1 commit into
wfurt wants to merge 1 commit into
Conversation
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the internal SslStream ↔ TlsSession “wedge” integration path (and associated internal seams), reverting SslStream handshaking to its existing PAL-driven GenerateToken flow on all platforms. It also tightens/clarifies parts of the standalone TlsContext/TlsSession implementation and documentation based on post-merge review feedback.
Changes:
- Delete the
SslStreamhandshake wedge (files, csproj wiring, andNextMessageshort-circuit) soSslStreamno longer routes handshakes throughTlsSession. - Simplify
TlsContext/TlsSessionby removing wedge-only state and APIs, and by always cloning per-session options. - Update
TlsContextXML remarks to document correct ownership/lifetime expectations; tightenTlsSession.AttachSocketaccessibility and add debug invariants.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSession.cs | Removes wedge-only members/surface area; tightens AttachSocket; adjusts disposal behavior. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsContext.OpenSsl.cs | Removes wedge-mode early-return so the context-owned SSL_CTX is always established when applicable. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsContext.cs | Removes wedge mode plumbing; always clones per-session options; updates XML remarks about ownership/lifetime. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.TlsSessionWedge.cs | Removed (eliminates the handshake routing wedge). |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs | Removes NextMessage wedge short-circuit and the partial method declaration. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.NoTlsSession.cs | Removed (no longer needed after wedge deletion). |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs | Stops setting the wedge-only SslStream reference on the options bag; keeps validator wiring. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs | Removes wedge-only SslStream field; updates clone/copy comments accordingly. |
| src/libraries/System.Net.Security/src/System.Net.Security.csproj | Removes conditional compilation entries for the wedge files. |
| { | ||
| _options.Dispose(); | ||
| } | ||
| _options?.Dispose(); |
wfurt
force-pushed
the
TlsSession-followup
branch
from
July 28, 2026 11:06
f57b2bb to
322f47f
Compare
wfurt
changed the title
Address post-merge review feedback on dotnet#130366: - TlsContext XML remarks: drop the promise that it is safe to Dispose the context while sessions are still in use. The native handles are ref-counted, but SslStreamCertificateContext is not IDisposable and has no refcount, so an internally-built cert chain gets released on the context's Dispose even when live sessions still hold a reference. Document the ownership rules (context owns cert contexts it built, caller-supplied ServerCertificateContext stays caller-owned) and the lifetime requirement (context must outlive all sessions), mirroring the model used by SslStream (r3624718621). - TlsSession.AttachSocket: tighten from internal to private protected, add Debug.Asserts that the session is fresh at the call site (no prior socket handle / managed Socket wrapper / PAL security context, not disposed), and update the comment to reflect that it is called from OnContextInitialized after the base InitializeFromContext runs (bartonjs r3562414767).
wfurt
force-pushed
the
TlsSession-followup
branch
from
July 28, 2026 11:14
322f47f to
72dce3a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up on #130366 — small, safe cleanup driven by post-merge review feedback. The larger
SslStream/TlsSessionunification (turningSslStreaminto a thin adapter overTlsBufferSessionand retiring the parallel PAL glue) will land as its own dedicated PR — that discussion had already outgrown this one.Changes
TlsContextXML remarks — drop the promise that it is safe toDisposethe context while sessions are still in use. Native handles (SSL_CTX, SChannel credentials) are ref-counted, butSslStreamCertificateContextis notIDisposableand has no refcount, so an internally-built cert chain gets released on the context'sDisposeeven when live sessions still hold a reference. The revised docs make the ownership contract explicit — the context retains cert contexts it built itself (rawServerCertificate), caller-suppliedServerCertificateContextstays caller-owned, and the caller keeps the context alive for as long as any session derived from it is in use. This mirrors the ownership model used bySslStreamtoday (r3624718621).TlsSession.AttachSocket— tighten frominternaltoprivate protected, addDebug.Asserts that the session is fresh at the call site (no prior socket handle, no managedSocketwrapper, no PAL security context, not disposed), and update the leading comment to reflect that it's called fromOnContextInitializedafter the baseInitializeFromContextruns, not fromTlsSocketSession's constructor as the old comment claimed (bartonjs r3562414767).No public API change. No behavior change outside
Debug.Asserts.Note
This PR description was drafted with GitHub Copilot assistance.