Add low-level TLS state machine API (TlsContext / TlsSession)#130366
Open
wfurt wants to merge 28 commits into
Open
Add low-level TLS state machine API (TlsContext / TlsSession)#130366wfurt wants to merge 28 commits into
wfurt wants to merge 28 commits into
Conversation
Introduce a caller-driven, non-blocking TLS state machine API for
System.Net.Security, enabling high-performance scenarios where the
caller controls buffer management and I/O scheduling.
New public API surface (marked [Experimental("SYSLIB5007")]):
- TlsContext: immutable, shareable TLS configuration (certificates,
protocols, ALPN). Created via CreateServer/CreateClient factories.
- TlsSession: per-connection TLS state machine with Encrypt/Decrypt
and handshake operations. Supports buffer-based and socket-based modes.
- TlsBufferSession / TlsSocketSession: concrete session types for
caller-managed buffers and OS socket fd binding respectively.
- TlsOperationStatus: enum indicating handshake/data operation results.
Platform support:
- Full implementation on OpenSSL (Linux, FreeBSD)
- Network.framework integration on macOS
- Stub (NotSupportedException) on Windows/other platforms
SslStream integration:
- Wire existing SslStream handshake through TlsSession on Unix platforms
(wedge mode), reducing code duplication while preserving all existing
behavior and passing all existing tests.
Native PAL additions:
- OpenSSL: BIO read/write helpers, SSL_set_retry_verify for deferred
certificate validation, ClientHello peek/parse
- Apple: Network.framework server connection bootstrap for testing
Contributor
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new experimental, low-level TLS state machine API in System.Net.Security (TlsContext, TlsSession, TlsBufferSession, TlsSocketSession, TlsOperationStatus) and adds the native/PAL plumbing needed to support non-blocking, caller-driven TLS flows across OpenSSL, SChannel, and Apple Network.framework.
Changes:
- Adds new experimental public API surface for a non-blocking TLS engine (
TlsContext/TlsSession+ buffer-driven and socket-bound session types). - Extends OpenSSL native shims to support fd-bound TLS (
SSL_set_fd), handshake driving, retry-verify plumbing, and a socket BIO that can replay a prefetched ClientHello. - Updates Apple Network.framework PAL to support server-side TLS (identity wiring + server bootstrap), and adds/adjusts tests to cover the new surface.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native/pal_ssl.h | Adds OpenSSL shims for fd binding, raw handshake, and retry-verify control. |
| src/native/libs/System.Security.Cryptography.Native/pal_ssl.c | Implements new OpenSSL shims; adjusts protocol support cleanup call. |
| src/native/libs/System.Security.Cryptography.Native/pal_bio.h | Declares socket-replay BIO + TLS frame peek/prefix APIs. |
| src/native/libs/System.Security.Cryptography.Native/pal_bio.c | Implements socket-replay BIO and TLS record peek support. |
| src/native/libs/System.Security.Cryptography.Native/opensslshim.h | Updates OpenSSL shim function requirements and includes. |
| src/native/libs/System.Security.Cryptography.Native/entrypoints.c | Exposes new BIO/SSL entrypoints to managed interop. |
| src/native/libs/System.Security.Cryptography.Native.Apple/pal_networkframework.m | Adds server-side NW bootstrap flow + per-session queueing changes. |
| src/native/libs/System.Security.Cryptography.Native.Apple/pal_networkframework.h | Updates NW create signature to accept server identity. |
| src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj | Links production OpenSSL options partial + fixes ReadWriteAdapter link path. |
| src/libraries/System.Net.Security/tests/UnitTests/Fakes/FakeSslStream.Implementation.cs | Extends fake options to carry a remote cert validator hook. |
| src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj | Suppresses SYSLIB5007 in tests; adds new TlsSessionTests compile item. |
| src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs | Adjusts skips/conditions for Network.framework behavior differences. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSocketSession.cs | Adds socket-bound, non-blocking TLS session wrapper type. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSession.Stub.cs | Adds PNSE stub implementations for unsupported platforms. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSession.OpenSsl.cs | Adds OpenSSL fd-mode fast paths and ClientHello peeking integration. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSession.cs | Implements core TLS state machine (buffered + socket-driven) and validation suspension flow. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsOperationStatus.cs | Introduces the experimental operation status enum. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsContext.OpenSsl.cs | Adds reusable OpenSSL SSL_CTX ownership/sharing for TlsContext. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsContext.cs | Adds the experimental configuration type and context creation APIs. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsBufferSession.cs | Adds buffer-driven session type exposing the core state machine surface. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs | Adjusts macOS PAL routing and sync/async handling for Network.framework. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.TlsSessionWedge.cs | Adds an initial “wedge” routing SslStream handshake steps through TlsSession. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs | Integrates the wedge and refactors cert validation for reuse. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.NotUnix.cs | Provides a stub wedge implementation for platforms not using it. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs | Tweaks Apple async handshake setup and fallback behavior. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs | Wires OpenSSL remote certificate validation delegate into options. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs | Minor refactor of cached credential lookup key creation. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.OpenSsl.cs | Adds OpenSSL-specific per-session plumbing fields (SSL_CTX, fd binding, replay BIO/prefix). |
| src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs | Makes options partial; adds clone/copy helpers and OpenSSL validation hook state. |
| src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteNwContext.cs | Improves NW transport EOF handling and disposal safety. |
| src/libraries/System.Net.Security/src/System/Net/Security/NetEventSource.Security.cs | Generalizes cert validation event logging to accept non-SslStream senders. |
| src/libraries/System.Net.Security/src/System/Net/Security/LocalAppContextSwitches.cs | Adds CaptureClientHello switch; adjusts NTLM default logic. |
| src/libraries/System.Net.Security/src/System.Net.Security.csproj | Adds experimental warning suppression + compiles new TLS engine sources. |
| src/libraries/System.Net.Security/src/Resources/Strings.resx | Adds new resource strings for TlsSession errors/platform support. |
| src/libraries/System.Net.Security/ref/System.Net.Security.csproj | Adds ref dependency on System.Net.Sockets for SafeSocketHandle. |
| src/libraries/System.Net.Security/ref/System.Net.Security.cs | Adds the new experimental public API surface to ref assembly. |
| src/libraries/Common/src/System/Experimentals.cs | Reserves SYSLIB5007 for the new experimental TLS API. |
| src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs | Adds P/Invokes for new OpenSSL SSL/BIO shims and new error code. |
| src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs | Updates OpenSSL context caching/ownership and cert-verify callback behavior. |
| src/libraries/Common/src/Interop/OSX/Interop.NetworkFramework.Tls.cs | Updates NW create signature to pass server identity. |
| docs/project/list-of-diagnostics.md | Documents new SYSLIB5007 experimental diagnostic ID. |
vcsjones
reviewed
Jul 8, 2026
Code fixes (all in TlsSession.cs): - Remove debug 'CAPTURE-DEBUG' InvalidOperationException that was leaking a debug marker to consumers; silently skip ClientHello capture when the parse succeeds but the frame length doesn't fit (already benign). - Fix ArrayPool leak in HandshakeSocketCore / ReadSocketCore: replace Array.Resize on the pool-rented _socketInBuf with a proper Rent/Copy/ Return via new GrowSocketInBuf() helper. Prevents leaking the original pooled buffer and prevents returning a non-pooled resized array back to the pool on Dispose. - Fix SetClientCertificateContext race with concurrent sessions: instead of disposing/nulling the shared TlsContext.CredentialsHandle, acquire a session-local credentials handle (mirroring the pattern in SetContext). ActiveCredentialsRef() already routes subsequent PAL calls through the session-local handle. Acquire eagerly so failures surface here. Squash-regression restores: - System.Net.Security.csproj: restore $(NetCoreAppCurrent)-openbsd TFM that was dropped from TargetFrameworks. - System.Net.Security.csproj: fix stale ReadWriteAdapter.cs path — the file lives at $(CommonPath)System/ (not System/Net/). - LocalAppContextSwitches.cs: restore IsOpenBsd flag used by NegotiateAuthenticationPal.ManagedSpnego on OpenBSD. - docs/project/list-of-diagnostics.md: restore SYSLIB0065 row and update the SYSLIB5007 row to .NET 11 with the wording proposed by @vcsjones.
MihaZupan
reviewed
Jul 8, 2026
- Interop.OpenSsl.cs: fix indentation of the ~120 lines that got left at the previous indent level when the AllocateSslHandle body was wrapped in a try/finally block (comment from @MihaZupan). - pal_ssl.c: drop the second argument from CryptoNative_EvpPkeyDestroy call (function signature is 1-arg on main). Fixes the linux-x64 native build (see CI 'dotnet-linker-tests (Build linux-x64 release Runtime_ Release)' failure).
…SL API refactor Reapply upstream 397e72c ("Prefer non-deprecated EC OSSL APIs where possible") on top of the TlsSession-specific additions (BioGetPrefix, BioNewSocketReplay, BioReadTlsFrame, SslDoHandshake, SslSetFd, SslSetRetryVerify) after the recent upstream/main merge.
- LocalAppContextSwitches.cs: restore IsOpenBsd branch in UseManagedNtlm defaultValue (was dropped in the squash; the IsOpenBsd field and its comment were restored in 51c03bc but the switch default was left inconsistent). Matches upstream/main. - TlsSession.EnsureCredentialsAcquired: short-circuit when _sessionCredentialsHandle is already set (via SetContext or SetClientCertificateContext). Prevents allocating and publishing an unused shared TlsContext.CredentialsHandle that could race with other sessions on the same context. ActiveCredentialsRef() already routes the PAL to the session-local handle. - TlsSession.Dispose: release _sessionCredentialsHandle. It was being acquired eagerly by SetContext/SetClientCertificateContext but never freed, so long-lived TlsContexts creating many sessions were leaking SafeFreeCredentials. - System.Net.Security.Unit.Tests.csproj: fix stale ReadWriteAdapter.cs path — the file lives at $(CommonPath)System/, not System/Net/. Same fix was applied to src/System.Net.Security.csproj in 51c03bc but the unit test csproj was missed. - JsonSourceGenerator.Parser.cs: mark AddTypeArgumentDiagnosticIds local function static (IDE0062). Unrelated regression pulled in from the recent upstream/main merge; blocks local libs build under TreatWarningsAsErrors.
Addresses @MihaZupan review feedback on PR dotnet#130366: 'replace these (and similar) with ArrayBuffer to simplify all buffer management'. Two sliding-window byte-buffer patterns migrated to the existing System.Net.ArrayBuffer helper (already used by SslStream, HttpConnection, Http2Connection, etc.): - _pending / _pendingOffset / _pendingLength (staged TLS output drained by DrainPendingOutput and the socket send loop) -> _pendingBuffer. The AppendPending compact+grow+rent bookkeeping (~30 lines) collapses to EnsureAvailableSpace + CopyTo + Commit; DrainTo collapses to ActiveSpan slice + Discard. - _socketInBuf / _socketInUsed (pre-fetched socket input consumed by HandshakeSocketCore / ReadSocketCore) -> _socketInBuffer. The custom GrowSocketInBuf helper and its manual BlockCopy compaction (~20 lines) go away; growth is EnsureAvailableSpace and consume is Discard. Net delta: -88 lines. All 10 target frameworks build clean; the existing TlsSession functional test results are unchanged from HEAD (same 42/44 pass, same 2 pre-existing GetClientHelloBytes tests failing before and after the refactor).
…o no-ops
Both ClientSession_GetClientHelloBytes_Throws and
ServerSession_GetClientHelloBytes_BeforeClientHello_Throws wrapped the
helper call inside a discard-typed lambda body:
Assert.Throws<InvalidOperationException>(() =>
{
ReadOnlySpan<byte> _ = GetClientHelloBytesHelper(session);
});
The C# compiler eliminates the entire assignment because the LHS is a
ref-struct local whose value is never read and cannot escape the scope,
compiling the lambda body to a single ret. The helper never runs, no
exception is thrown, and Assert.Throws fails with 'No exception was
thrown'.
Passing the helper call directly as the lambda body preserves the call
site (result is byte[], not a ref-struct discard) so the helper is
invoked, throws InvalidOperationException for len == 0, and the test
passes as intended.
The server-side ClientHello parse mutated the shared SslAuthenticationOptions bag (_options.TargetHost = parsed.Value.ServerName) so parallel sessions built from a deferred TlsContext could race on the SNI value. Since PAL reads of TargetHost are all guarded by !isServer (OpenSSL, SChannel, Apple, Android), that write was never needed by the handshake itself — it existed only to (a) surface the resolved host via TargetHostName and (b) hand the resolved host to ServerCertSelectionDelegate. Introduce a session-local _sessionTargetHost field on TlsSession: - Server sessions read/write _sessionTargetHost (default string.Empty until the first ClientHello is parsed). - Client sessions continue to read _options.TargetHost (never mutated by us after SetContext). - TargetHostName property routes on _context.IsServer. - ResolveServerCertificateFromClientHello, HandshakeBufferedCore managed parse, and the OpenSSL native-peek path all target _sessionTargetHost. This eliminates the largest silent-corruption path when a bootstrap TlsContext is shared across concurrent server sessions (SNI-dispatching front-end pattern). Removes one of the reasons _options.Clone() is called per session; other reasons (CertificateContext mutation, PAL scratch pockets on the bag) still require the clone.
Follow-up to the SNI-target-host change: the same "mutate the shared options bag" pattern applied to CertificateContext. When a caller called SetClientCertificateContext or when the server-side selector picked a cert from ServerCertificateSelectionCallback, we wrote directly onto _options.CertificateContext. That was safe today because _options is a per-session clone, but it kept a session-scoped mutation on the bag we would like to eventually share by reference. Introduce session-local state on TlsSession: - _sessionCertificateContext (SslStreamCertificateContext?): the effective cert context for this session. Initialized from _options at SetContext time; every mutation from SetClientCertificateContext and the server cert selector routes through SetSessionCertificateContext. - _ownsSessionCertificateContext (bool): true only when the session itself constructed the context via SslStreamCertificateContext.Create in the server-selector path. Caller-provided contexts and the template context inherited from TlsContext are not owned by the session. Same convention as SslAuthenticationOptions.OwnsCertificateContext, scoped to the session. - Private SessionCertificateContext get-only property used by all reads (LocalCertificate, HandshakeBufferedCore cert-resolution guard, ResolveServerCertificateFromClientHello guard). - SetSessionCertificateContext(context, takeOwnership) helper releases any prior session-owned context (unless the new one is the same reference), installs the new value, and mirrors onto _options.CertificateContext (with OwnsCertificateContext=false) so the PAL sees the effective value without changing its signature. That mirror line is the single point that goes away when the PAL is later reshaped to consume the session directly — one of the pre-requisites to eliminating the per-session Clone(). - Dispose releases the session-owned context via ReleaseResources(). _options.OwnsCertificateContext was flipped to false at SetContext, so _options.Dispose() no longer double-frees. InitializeFromContext transfers CertificateContext ownership from the options-clone to the session. Deferred SetContext (server SNI flow) re-seats via SetSessionCertificateContext after CopyFrom so the new per-tenant template value flows through the same code path. 44/44 TlsSessionTests still green across all 10 target frameworks.
Batch of drive-by fixes flagged by the latest Copilot review pass: - TlsOperationStatus.DestinationTooSmall: expand XML doc. On buffered APIs the code means "destination span too small", on socket-bound APIs it means "socket returned WouldBlock mid-write". Doc now covers both. - TlsSession.TargetHostName: honor the string? nullable contract. Getter now returns null when the underlying value is empty instead of surfacing "" and making callers unable to distinguish "not set" from "explicit empty string". Also adds an XML summary describing client vs server semantics. - TlsSession.EnsureCredentialsAcquired: atomic install of the shared TlsContext.CredentialsHandle via Interlocked.CompareExchange. Multiple sessions on the same TlsContext could both see the field null, each AcquireCredentialsHandle, and overwrite/leak one another. Now the loser of the race disposes its own handle. Non-Windows PALs return null from AcquireCredentialsHandle so the CompareExchange is a no-op there. - Interop.Ssl.cs: drop SetLastError=true from CryptoNative_SslSetFd and CryptoNative_SslDoHandshake. These shims report errors via out params and the OpenSSL error queue (SSL_get_error/ERR_get_error), never errno. Removes marshaller overhead for a value no caller reads. - TlsSessionTests.MeasureHandshakeBytesAsync: drop the ArrayPool rent that was never used. - TlsSessionTests.ServerSession_TlsResume_HonorsAllowTlsResumeOption: loosen the resumption savings threshold from < 0.6 to < 0.75. Observed on Alpine 3.24 x64 (Helix) with TLS 1.3: first=5504 second=3544 -> 64.4% (test failed at 60% threshold). The absolute savings of ~1960 bytes are the omitted Certificate + CertVerify records, which is normal for a small test cert. TLS 1.2 still comfortably clears 75%; TLS 1.3 with small certs is closer to 60-70%, so 75% gives headroom without hiding real regressions.
…mode-doublefree TlsSession: fix BIO context type-confusion heap corruption in fd-mode socket path
Comment on lines
788
to
+797
| internal ProtocolToken NextMessage(ReadOnlySpan<byte> incomingBuffer, out int consumed) | ||
| { | ||
| if (TryNextMessageViaTlsSession(incomingBuffer, out ProtocolToken wedged, out consumed)) | ||
| { | ||
| if (NetEventSource.Log.IsEnabled() && wedged.Failed) | ||
| { | ||
| NetEventSource.Error(this, $"Authentication failed. Status: {wedged.Status}, Exception message: {wedged.GetException()!.Message}"); | ||
| } | ||
| return wedged; | ||
| } |
…ression) MapSslError previously threw Interop.OpenSsl.SslException directly (per PR dotnet#130366 review comment 31 — 'use CreateSslException'). SslException derives from Exception, not AuthenticationException, which broke SocketBoundSession_DeferredOptions_ProtocolMismatch_Fails (that test asserts AuthenticationException, matching what SslStream throws for the same failure category). Wrap the SslException as InnerException of a new AuthenticationException with the generic SR.net_auth_SSPI message. The OpenSSL error-queue text remains available via the inner exception's message.
Comment on lines
+39
to
+42
| private partial bool TryNextMessageViaTlsSession(ReadOnlySpan<byte> incomingBuffer, out ProtocolToken token, out int consumed) | ||
| { | ||
| EnsureTlsSession(); | ||
|
|
Fixes CS0426 build break on osx/windows/ios unit tests: move the VerifyRemoteCertificateCallback delegate and RemoteCertificateValidator property from SslAuthenticationOptions.OpenSsl.cs (unix-only partial) to the cross-platform SslAuthenticationOptions.cs partial so the type is resolvable in FakeOptions on all TFMs of the unit-test project. Addresses PR dotnet#130366 review feedback: * Localize five InvalidOperationException messages in TlsSession that previously used interpolated strings; add three SR entries under the net_tlssession_* prefix to match the SslStream convention (all 58 other InvalidOperationException sites in System.Net.Security use SR-backed resources). * Document TlsContext lifetime: existing TlsSession instances keep the native TLS context alive via native refcounting (SSL_CTX_up_ref inside SSL_new, SChannel credentials handle), so it is safe to dispose the TlsContext while sessions are still in use. Only new session creation from a disposed context throws ObjectDisposedException. Verified: product build clean across all TFMs; unit tests build for osx/ios/windows/unix; all 45 TlsSessionTests pass on Debug testhost.
Comment on lines
+147
to
+152
| } | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
rzikm
reviewed
Jul 16, 2026
rzikm
reviewed
Jul 16, 2026
Replace the DOTNET_BIO_TAG_* magic-number sentinels stashed in the first field of each custom BIO context with OpenSSL's own type-identity: cache the int returned by BIO_get_new_index() at method-init time and compare against BIO_method_type(bio). Drop the 'tag' field from both ManagedSpanBioCtx and SocketReplayBioCtx. Wire BIO_method_type through opensslshim so the verify-so gate accepts the new dependency. Rationale: the tag-based validation added in PR #48 solved a real heap corruption (SocketReplayBio being reinterpreted as ManagedSpanBioCtx via Interop.OpenSsl.Encrypt/DoSslHandshake), but reviewers noted magic numbers were a bandaid. BIO_method_type uses the platform's own type system, removes 4 bytes of overhead per BIO instance, and cannot false- positive on a legitimate memory-BIO whose payload happens to start with 'MSBI' / 'SRBP'. Semantics are unchanged: mis-called helpers still silently no-op rather than crash, because TlsSocketSession.Shutdown currently routes fd-mode shutdown through the buffered SslStreamPal path (which invokes BioSetWriteWindow on the SSL*'s write BIO). That managed-layer misuse is what the guard catches today; the cleanup keeps the guard defensive until that layering is fixed as a follow-up.
Comment on lines
+39
to
+42
| private partial bool TryNextMessageViaTlsSession(ReadOnlySpan<byte> incomingBuffer, out ProtocolToken token, out int consumed) | ||
| { | ||
| EnsureTlsSession(); | ||
|
|
Contributor
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "dcc3d14f481aa62459a6208dfcfddac353897072",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e60457f69fcc587c47e9196552019630054f59f9",
"last_reviewed_commit": "dcc3d14f481aa62459a6208dfcfddac353897072",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e60457f69fcc587c47e9196552019630054f59f9",
"last_recorded_worker_run_id": "29685638027",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "dcc3d14f481aa62459a6208dfcfddac353897072",
"review_id": 4730708777
}
]
} |
Contributor
There was a problem hiding this comment.
Holistic Review
Motivation: Strong and real. Issue #128871 (api-approved) requests a caller-driven, non-blocking low-level TLS state machine so high-performance consumers (e.g. YARP, custom proxies, HTTP/3 stacks) can own buffer management and I/O scheduling instead of going through SslStream. The gap is genuine and the API was reviewed on the API-review board.
Approach: Introduces TlsContext (long-lived config) plus a TlsSession hierarchy (TlsBufferSession span-driven, TlsSocketSession socket-bound), all gated behind [Experimental("SYSLIB5007")]. The implementation reuses the existing per-platform SSPI/OpenSSL PAL and adds native BIO shims for zero/low-copy record handling and a socket-replay BIO for SNI peek. A separate concern: it also wedges the mainstream SslStream handshake through the new engine by default on Linux/FreeBSD/Windows.
Summary: @bartonjs on #128871 (namespaces, signatures, parameter names, SYSLIB5007 registered in list-of-diagnostics.md), and the new managed/native code is carefully written with disposal, overflow, and retry-flag handling that I could not fault by reading. My one substantive concern is scope/risk: this PR makes the experimental TlsSession the default SslStream handshake engine on three platforms with no kill-switch, which is a large regression surface the author's own description says is meant for a later integration step. A maintainer should confirm that is intended for this "initial submit." Because the PR cannot be built/tested in this worker, the correctness of the state machine and native BIOs also warrants human/CI verification.
Detailed Findings
✅ API approval — matches approved shape
The PR adds new public surface (ref/System.Net.Security.cs): TlsOperationStatus, TlsContext, TlsSession, TlsBufferSession, TlsSocketSession. Linked issue #128871 carries the api-approved label (applied by bartonjs 2026-07-07). The approved API posted in bartonjs's comment at label time matches the ref exactly — namespace System.Net.Security, [Experimental("SYSLIB5007")] on every type, identical method/property signatures and parameter names (including the CreateServer/CreateClient factories, SetContext, and the destination/bytesWritten out-parameter conventions). SYSLIB5007 is correctly reserved in docs/project/list-of-diagnostics.md for .NET 11. No extra or missing public surface detected. ✅ gate passes.
⚠️ Scope/risk — mainstream SslStream rerouted through the experimental engine by default
See the inline comment on SslStream.Protocol.cs line 790. SslStream.NextMessage now calls TryNextMessageViaTlsSession first, and SslStream.TlsSessionWedge.cs provides a real (non-stub) implementation on every platform except Android/Apple. So all ordinary SslStream handshakes on Linux, FreeBSD, and Windows run through the brand-new TlsBufferSession wedge, with no AppContext/env switch to revert to GenerateToken. This is the primary reason for the Needs-Human-Review verdict: it is the author's decision, but a maintainer should explicitly confirm the wedge is meant to be on-by-default in an initial API-exposure PR rather than opt-in / switch-gated, given how much SslStream behavior (renegotiation, TLS 1.3 post-handshake auth, client-cert reselection, SChannel ALPN re-entry, SslSessionsCache seeding, channel binding, custom alert tokens) now flows through untested-in-production code.
✅ Resource management and disposal
TlsSession.Dispose is idempotent (_disposed guard) and releases the socket/handle, session-local credentials, session-owned cert context (only when _ownsSessionCertificateContext), pending buffers, and pooled scratch arrays without double-freeing shared state. TlsContext.Dispose correctly skips disposing the options bag and native context in wedge mode (owned by SslStream) while still dropping the reference; the native SSL_CTX is described as reference-counted so disposing the context under live sessions is safe. EnsureCredentialsAcquired uses Interlocked.CompareExchange to install the shared SChannel credential and disposes the loser, avoiding a leak/race across concurrent sessions on one context. These are the highest-risk areas for this kind of API and they look correct on read.
✅ Native BIO shims (pal_bio.c)
The new ManagedSpanBio, SocketReplayBio, and BioPeekTlsFrame paths guard null/negative args, clear retry flags, translate EAGAIN/EWOULDBLOCK into BIO_set_retry_read, loop on EINTR, use MSG_NOSIGNAL where available, and include explicit INT32 overflow guards before sizing the spill buffer (remaining > INT32_MAX - spillLen). BioPeekTlsFrame bounds the record against prefixCap and rejects oversized frames. Allocation-failure paths return errors and free on the BioNewSocketReplay cleanup. I found no memory-safety defect by inspection, but this is exactly the kind of code that needs the CI native/sanitizer runs and human eyes — I could not build or run it here.
✅ Test coverage
TlsSessionTests.cs adds ~33 fact/theory methods exercising client/server handshakes, buffered and socket sessions, SNI-deferred context, cert validation callbacks, and platform-specific behavior (with appropriate OperatingSystem.Is* branching rather than blanket skips). Coverage looks proportionate to the new surface. Worth a human check that failure/renegotiation and post-handshake-auth edge cases are exercised, since those now also run in the SslStream wedge.
💡 Minor observations
SslStream.IO.csApple path now runsCertSelectionDelegateand throwsNotSupportedException(net_ssl_io_no_server_cert)when it returns null — this changes an Apple server-side handshake from a timeout to an immediate throw; behavior improvement, but a deliberate observable change worth noting to reviewers.- The
TlsContextXML doc claims it is safe to dispose the context while sessions are live "e.g. SChannel credentials handle is reference-counted"; on Windows the SSPICredHandlelifetime is managed viaSafeFreeCredentialsrefcounting in this repo, so the claim holds, but it is platform-nuanced and worth a maintainer's confirmation.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 291.8 AIC · ⌖ 16 AIC · ⊞ 10K
| // | ||
| internal ProtocolToken NextMessage(ReadOnlySpan<byte> incomingBuffer, out int consumed) | ||
| { | ||
| if (TryNextMessageViaTlsSession(incomingBuffer, out ProtocolToken wedged, out consumed)) |
Contributor
There was a problem hiding this comment.
SslStream handshakes through the new TlsSession wedge on Linux, FreeBSD, and Windows (SslStream.TlsSessionWedge.cs is compiled whenever UseAndroidCrypto/UseAppleCrypto are false). There is no AppContext/env kill-switch to fall back to the proven GenerateToken path. The PR description frames this as an initial submit to expose the approved API surface, with SslStream integration to follow "once this lands" — but as written this ships the wedge as the default engine for every SslStream user on those platforms, not just opt-in TlsContext/TlsSession consumers.
That is a very large regression surface for the flagship TLS type (renegotiation, TLS 1.3 post-handshake auth, client-cert selection, SChannel ALPN re-entry, session-cache seeding, channel binding, custom alert generation). Please confirm with the area maintainers whether the wedge is intended to be on-by-default in this PR, and consider gating it behind a switch (defaulting to the legacy path) so the experimental engine can bake without exposing all consumers to handshake regressions if a corner case is missed. This is a judgment call for a human reviewer, not a definitive defect.
rzikm
approved these changes
Jul 20, 2026
rzikm
left a comment
Member
There was a problem hiding this comment.
LGTM, modulo existing copilot comments
# Conflicts: # docs/project/list-of-diagnostics.md # src/libraries/Common/src/System/Experimentals.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/native/libs/System.Security.Cryptography.Native/pal_bio.c:736
- CryptoNative_BioNewSocketReplay allows prefixLen > 0 with prefix == NULL, but then unconditionally memcpy()s from prefix. This is an immediate null-deref risk if the native export is ever called with a null prefix pointer (even if current managed callers always pass a span). Add an explicit guard for (prefixLen > 0 && prefix == NULL).
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.TlsSessionWedge.cs:42 - TryNextMessageViaTlsSession unconditionally instantiates and drives the new TlsSession-based handshake wedge and then always returns true. Because SslStream.Protocol.NextMessage now checks this first, the wedge becomes the default handshake path on platforms where this file compiles (Linux/FreeBSD/Windows), which is a high-risk behavioral change for SslStream. This should be gated behind an AppContext switch (default-off) or moved out of this PR so the new public API surface can land independently of the SslStream rewrite.
private partial bool TryNextMessageViaTlsSession(ReadOnlySpan<byte> incomingBuffer, out ProtocolToken token, out int consumed)
{
EnsureTlsSession();
Address PR dotnet#130366 review feedback (discussion_r3598501248): CryptoNative_BioNewSocketReplay could memcpy from a NULL prefix when prefixLen > 0. Since this is a native export callable by any caller, defensively reject prefix == NULL when prefixLen > 0.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs:792
- SslStream.Protocol.NextMessage now routes all handshake token generation through TryNextMessageViaTlsSession first. On the supported build targets, the wedge implementation always returns true, making this a behavioral change to the existing SslStream handshake path by default. This conflicts with the PR description’s claim that SslStream integration will happen later; it should be gated (e.g., AppContext switch/feature flag) or moved to a separate PR so the new public API surface can land without changing SslStream behavior.
//
internal ProtocolToken NextMessage(ReadOnlySpan<byte> incomingBuffer, out int consumed)
{
if (TryNextMessageViaTlsSession(incomingBuffer, out ProtocolToken wedged, out consumed))
{
if (NetEventSource.Log.IsEnabled() && wedged.Failed)
src/libraries/System.Net.Security/src/System/Net/Security/TlsContext.cs:26
- The remarks state it is safe to dispose TlsContext while TlsSession instances created from it are still in use. However, session options are created via SslAuthenticationOptions.Clone(), which intentionally does not take ownership of (and currently shares) CertificateContext resources; disposing the context can therefore release certificate-context intermediates/platform resources that active sessions still reference. Either the implementation needs to ensure per-session options deep-copy/own any context-owned resources (so active sessions are independent), or the remarks/contract need to be changed to require the TlsContext to outlive all sessions.
/// Lifetime: it is safe to dispose the <see cref="TlsContext"/> while
/// <see cref="TlsSession"/> instances created from it are still in use.
/// The underlying native TLS context (e.g. OpenSSL <c>SSL_CTX</c>, SChannel
/// credentials handle) is reference-counted at the native layer, so each
/// live session keeps it alive until the session itself is disposed.
Comment on lines
+211
to
+244
| // Shallow copy of the configuration carried by this bag. Per-handle/per-stream | ||
| // state (SafeSslHandle, SslStream, RemoteCertificateValidator) is intentionally | ||
| // not propagated, and the clone does not take ownership of CertificateContext | ||
| // even if the source did. | ||
| internal SslAuthenticationOptions Clone() | ||
| { | ||
| SslAuthenticationOptions copy = new SslAuthenticationOptions | ||
| { | ||
| AllowRenegotiation = AllowRenegotiation, | ||
| TargetHost = TargetHost, | ||
| ClientCertificates = ClientCertificates, | ||
| ApplicationProtocols = ApplicationProtocols, | ||
| IsServer = IsServer, | ||
| CertificateContext = CertificateContext, | ||
| OwnsCertificateContext = false, | ||
| EnabledSslProtocols = EnabledSslProtocols, | ||
| CertificateRevocationCheckMode = CertificateRevocationCheckMode, | ||
| EncryptionPolicy = EncryptionPolicy, | ||
| RemoteCertRequired = RemoteCertRequired, | ||
| CheckCertName = CheckCertName, | ||
| CertValidationDelegate = CertValidationDelegate, | ||
| CertSelectionDelegate = CertSelectionDelegate, | ||
| ServerCertSelectionDelegate = ServerCertSelectionDelegate, | ||
| CipherSuitesPolicy = CipherSuitesPolicy, | ||
| UserState = UserState, | ||
| ServerOptionDelegate = ServerOptionDelegate, | ||
| CertificateChainPolicy = CertificateChainPolicy, | ||
| AllowTlsResume = AllowTlsResume, | ||
| AllowRsaPssPadding = AllowRsaPssPadding, | ||
| AllowRsaPkcs1Padding = AllowRsaPkcs1Padding, | ||
| ForceSyncPal = ForceSyncPal, | ||
| }; | ||
| return copy; | ||
| } |
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.
Introduce a caller-driven, non-blocking TLS state machine API for System.Net.Security, enabling high-performance scenarios where the caller controls buffer management and I/O scheduling.
fixes #128871
supports Linux, macOS, Windows & FreeBSD
This is initial submit (already too big) to expose the approved API surface. I will work on cleanup & integration with SslStream once this lands.