Skip peer certificate revalidation on resumed TLS sessions by default - #133174
Skip peer certificate revalidation on resumed TLS sessions by default#133174rzikm wants to merge 10 commits into
Conversation
On a resumed (abbreviated) TLS handshake the peer does not resend its certificate; its identity was established during the original full handshake. Common TLS stacks (OpenSSL, SChannel) do not re-run certificate verification on resumption. Match that behavior: by default SslStream now adopts the cached peer certificate without rebuilding the chain or invoking the user validation callback on a resumed session. Add the opt-in System.Net.Security.RevalidateCertificateOnTlsResume AppContext switch (env DOTNET_SYSTEM_NET_SECURITY_REVALIDATECERTIFICATEONTLSRESUME) to restore the previous revalidate-on-resume behavior. Wire up TlsResumed detection across platforms: - Windows (SChannel) / Linux (OpenSSL): un-guard the existing TlsResumed computation from DEBUG-only builds. - Apple SecureTransport: new AppleCryptoNative_SslGetSessionResumed export using SSLGetResumableSessionInfo, plumbed through Interop.Ssl and SslConnectionInfo.OSX. - Network Framework and Android expose no reliable resumption signal and keep the safe fallback of always revalidating (documented in code). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
|
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. |
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
There was a problem hiding this comment.
🟡 Changes recommended
The resumed-session validation skip can incorrectly apply during renegotiation/TLS 1.3 post-handshake auth, potentially accepting newly supplied certificates without validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR changes SslStream certificate-validation behavior on resumed (abbreviated) TLS handshakes: when a backend can reliably detect resumption, it can skip rebuilding the certificate chain and skip invoking the user validation callback by default, with an AppContext/env switch to restore the prior behavior. It also makes TlsResumed detection available cross-platform (Windows/Linux unconditional, Apple SecureTransport via a new native export; Android/Network Framework keep the “safe fallback” of always revalidating due to lack of a reliable signal).
Changes:
- Add
System.Net.Security.RevalidateCertificateOnTlsResume/DOTNET_SYSTEM_NET_SECURITY_REVALIDATECERTIFICATEONTLSRESUMEto opt back into revalidation on session resumption. - Detect
TlsResumedon Windows/Linux unconditionally and on Apple SecureTransport viaAppleCryptoNative_SslGetSessionResumed. - Skip managed chain build + user validation callback when
TlsResumedis true and the opt-out switch is not enabled.
File summaries
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.h | Declares a new native export to query whether a SecureTransport session was resumed. |
| src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.c | Implements AppleCryptoNative_SslGetSessionResumed via SSLGetResumableSessionInfo. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs | Adds the resumption fast-path to skip remote cert revalidation (chain build + user callback) by default. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Windows.cs | Makes TlsResumed detection run in non-Debug builds (SChannel session-info flag). |
| src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.OSX.cs | Adds SecureTransport-based TlsResumed detection and documents Network Framework’s lack of signal. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Linux.cs | Makes TlsResumed detection run in non-Debug builds (OpenSSL SSL_session_reused). |
| src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.cs | Makes TlsResumed a regular field/property (not Debug-only) with clarifying comment. |
| src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Android.cs | Documents lack of a reliable resumption signal; keeps TlsResumed false (always revalidate). |
| src/libraries/System.Net.Security/src/System/Net/Security/LocalAppContextSwitches.cs | Adds the new switch plumbing for RevalidateCertificateOnTlsResume. |
| src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs | Adds managed LibraryImport for AppleCryptoNative_SslGetSessionResumed. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
bartonjs
left a comment
There was a problem hiding this comment.
I'd expect a test, using RemoteExecutor, to confirm the default behavior and that the AppContext works as expected...
…Apple API
Address review feedback and fix Apple CI build:
- Only skip peer certificate revalidation on the *initial* resumed
handshake. During renegotiation / TLS 1.3 post-handshake auth the peer
can present a new certificate, which must always be validated. Thread
an isInitialHandshake flag (SslStream passes !_isRenego; the TlsSession
external-validation path passes false) into VerifyRemoteCertificateCore
and gate the resumption shortcut on it.
- Revert the SecureTransport TlsResumed wiring: SSLGetResumableSessionInfo
has been removed from current Apple SDKs and fails to compile ("call to
undeclared function") across all Apple targets. Apple now leaves
TlsResumed unset, matching the Network Framework and Android safe
fallback of always revalidating on resumption.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🟡 Changes recommended
The new resumed-handshake fast-path needs targeted coverage for the new default + opt-out behavior and should address the resource-lifetime implications of skipping the user callback while still collecting chain intermediates.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
…itch tests On a resumed handshake the certificate validation callback is skipped by default, so the outer VerifyRemoteCertificate no longer had anything adopt the peer-sent intermediate certificates that GetRemoteCertificate appends to the chain's ExtraStore. Thread a certificateValidationSkippedOnResume flag out of VerifyRemoteCertificateCore so those intermediates are disposed even when a RemoteCertificateValidationCallback is configured, avoiding leaked X509Certificate2 handles across repeated resumptions. Add a RemoteExecutor-based test that verifies the client validation callback is not invoked on a resumed handshake by default and that setting RevalidateCertificateOnTlsResume restores callback invocation on resumption. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-relevant certificate validation semantics on resumed TLS handshakes and still needs broader test coverage (notably server-side client-cert validation) and careful human review for compatibility impact.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
…validate test Parametrize the revalidate-switch resume test over client-side (server cert) and server-side (client cert) validation so the shared skip default is exercised in both directions. When the environment cannot establish session resumption, signal a skip to the parent process via a marker file instead of hard-failing, matching the SkipTestException pattern used elsewhere in this file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🟡 Changes recommended
It changes a security-relevant default to skip certificate validation on resumed sessions (and introduces a Release-path perf regression on Windows via Enum.HasFlag boxing) which needs addressing/explicit opt-in alignment before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Windows.cs:88
- Using Enum.HasFlag here boxes the enum value and can allocate; this code now runs in Release for every handshake, so it’s worth avoiding the overhead. Prefer a simple bit-test against SSL_SESSION_RECONNECT.
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs:1262
- This introduces a default-path validation bypass: on resumed initial handshakes, the user RemoteCertificateValidationCallback and chain build are skipped unless an AppContext switch opts out. This conflicts with the System.Net.Security guidance that certificate validation bypass must require explicit opt-in (i.e., keep the existing secure default and allow opting into the optimization).
if (certificate != null &&
isInitialHandshake &&
connectionInfo.TlsResumed &&
!LocalAppContextSwitches.RevalidateCertificateOnTlsResume)
{
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Add explicit assertions after the field/type/property reflection lookups so a future rename of the internal members produces a clear diagnostic instead of a NullReferenceException. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
TlsResumed is now evaluated on every handshake in Release builds, so replace the boxing Enum.HasFlag call with a direct bit-test against SSL_SESSION_RECONNECT to avoid the per-handshake allocation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
|
Addressed the Copilot review feedback in the latest commits:
On the note about skipping certificate re-validation on resumption being a security-relevant default change: that is the intended behavior of this PR — matching what mainstream TLS stacks (OpenSSL, SChannel, etc.) do, which don't re-invoke user validation on an abbreviated handshake. The previous behavior remains available via the Note This comment was drafted with GitHub Copilot. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes a security-sensitive default (skipping user certificate validation callbacks on resumed sessions) and warrants careful human review of compatibility and security implications across platforms and scenarios.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/System.Net.Security/src/System/Net/Security/LocalAppContextSwitches.cs:27
- The switch comment says the peer certificate is not re-validated on resumed TLS handshakes "by default", but the implementation only skips validation when resumption can be detected (currently Windows/SChannel and Linux/OpenSSL). On Apple/Android, TlsResumed remains false and validation still runs, so the comment is misleading.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Session resumption is only detected on Windows and Linux; on other platforms TlsResumed stays false and the peer certificate is always re-validated. Note this in the switch comment so the default behavior is not read as universal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🟡 Changes recommended
The new test infrastructure has a concrete temp-marker correctness issue and the new behavior should be validated in non-Debug configurations (the current test file is #if DEBUG).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
TlsResumed is no longer DEBUG-only, so drop the file-level #if DEBUG guard so the resumption/revalidation tests are also exercised in Release. Reserve a unique skip-marker path via Path.GetTempFileName() (deleted up-front) so the child creates it only to signal a skip, avoiding the GetRandomFileName collision case. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive certificate validation defaults on resumed TLS handshakes and warrants maintainer review (including breaking-change/process considerations) before merging.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs:39
CheckResumeFlagrelies on theconnectionInfoandtlsResumedstatic reflection lookups being non-null. If any of these internal members are renamed or removed, the test will currently fail with aNullReferenceExceptioninstead of a clear diagnostic. Adding explicit asserts here makes failures easier to diagnose and keeps the test robust to internal refactors.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add explicit assertions so the static field/property reflection lookups used by CheckResumeFlag produce a clear diagnostic instead of a NullReferenceException if the internals are renamed or removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🟡 Changes recommended
The updated tests still have a brittle reflection static initializer risk, and the PR lacks the breaking-change documentation implied as necessary by the PR description for a security-relevant default behavior change.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/System.Net.Security/src/System/Net/Security/LocalAppContextSwitches.cs:35
- The PR description calls out that this is a security-relevant default flip and likely needs a breaking-change doc before merge, but there are no corresponding breaking-change / compatibility notes in the PR changes. Please add the required breaking-change documentation (and any discoverability note for the new AppContext/env switch) so downstream users understand why their validation callback stops firing on resumed initial handshakes by default.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
The static initializer chained GetProperty on the result of GetType, which would throw a TypeInitializationException before the asserts in CheckResumeFlag could run if the type name ever changed. Use ?. so the field becomes null and the existing Assert.True gives a clear diagnostic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fc01286-cba3-4fbb-b19f-c323b7b4d964
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a security-relevant behavioral default change but does not include the corresponding breaking-change documentation for discoverability and release/process compliance.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs:1262
- This change alters SslStream’s security-relevant default by skipping chain build + RemoteCertificateValidationCallback on resumed initial handshakes. Per repo process, this kind of behavioral change should ship with an explicit breaking-change entry (and/or other official documentation) describing the new default and the opt-out switch so downstream consumers can discover it.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
CI failure analysis (runtime build 1582740)The failed
None of these are caused by this PR:
No code change is warranted for these failures. I have not retriggered CI (per the agent-merge workflow, reruns are not used to classify failures). None of these test names currently have a matching Known Build Error / open tracking issue. Note This comment was generated by GitHub Copilot. |
Summary
On a resumed (abbreviated) TLS handshake the peer does not resend its certificate — its identity was established and validated during the original full handshake that produced the session ticket / session id. Common TLS stacks (OpenSSL, SChannel) do not re-run certificate verification on resumption.
Today
SslStreamstill rebuilds the chain and invokes the user validation callback on every resumed session. This PR changes the default so that, on a resumed initial handshake,SslStreamadopts the cached peer certificate for theRemoteCertificateproperty but skips the chain build and the user validation callback — matching the underlying TLS libraries.The shortcut is gated to the initial handshake only: during renegotiation or TLS 1.3 post-handshake authentication the peer can present a new certificate, which is always fully validated.
Opt-out switch
A new AppContext switch restores the previous behavior:
System.Net.Security.RevalidateCertificateOnTlsResume(config)DOTNET_SYSTEM_NET_SECURITY_REVALIDATECERTIFICATEONTLSRESUME=1(env)When set, the peer certificate is re-validated on every successful resumption as before.
Cross-platform
TlsResumeddetectionThe skip only fires when the backend can reliably report resumption:
SECPKG_ATTR_SESSION_INFO/SSL_SESSION_RECONNECTSSL_session_reusedThe Windows/Linux
TlsResumedcomputation was previously#if DEBUG-only and is now unconditional. SecureTransport'sSSLGetResumableSessionInfohas been removed from current Apple SDKs (it fails to compile as an undeclared function), so Apple — like Network Framework and Android — leavesTlsResumedunset and always revalidates on resumption (documented inline). The optimization therefore currently applies on Windows and Linux.Performance
dotnet/performance
ResumedHandshakebenchmark, Windows x64, across a cert × protocol matrix. Revalidate = old behavior (RevalidateCertificateOnTlsResume=1); Skip = new default. Stabilized run (warmup 12 / 30 iterations / 500 ms,GCgen0size=256MB).Notes / open questions
Note
This PR (including its description) was created with the assistance of GitHub Copilot.