Add more test coverage for TlsSession APIs - #133159
Open
rzikm wants to merge 10 commits into
Open
Conversation
…tionException RequestClientCertificate is a server-only post-handshake operation. Add a negative test asserting a client TlsBufferSession throws InvalidOperationException, covering the server-only guard that was previously untested. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89084543-7d70-4562-8776-d4ca1baa3c7c
Socket analog of TlsSession_OperationsBeforeHandshake_Throw. Asserts TlsSocketSession.Read/Write throw InvalidOperationException before the handshake completes, closing the socket-path gap for the operations-before-handshake guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89084543-7d70-4562-8776-d4ca1baa3c7c
TwoSessions_HandshakeAndPingPong_InMemory_Succeeds now asserts both endpoints agree on the negotiated cipher suite and that a real (non-default) suite was selected. NegotiatedCipherSuite previously had no assertion in the test suite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89084543-7d70-4562-8776-d4ca1baa3c7c
ServerSession_ChannelBinding_MatchesSslStreamClient now also verifies the tls-server-end-point (Endpoint) binding matches the SslStream client. Previously only ChannelBindingKind.Unique was tested; Endpoint is the kind used for HTTP channel-binding-token auth. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89084543-7d70-4562-8776-d4ca1baa3c7c
Socket analog of ServerSession_Shutdown_DeliversCloseNotifyToSslStreamClient. Drives a socket-bound handshake to completion, calls Shutdown() until Closed, and asserts the SslStream peer observes a clean EOF. The socket Shutdown() path was previously only exercised inside an unrelated stress test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89084543-7d70-4562-8776-d4ca1baa3c7c
Exercises the previously-untested TargetHostName setter on both client and server sessions, verifying override and null-clears-to-null behavior on each role branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89084543-7d70-4562-8776-d4ca1baa3c7c
|
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
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The fd-mode post-handshake auth driver can block write-progress (WANT_WRITE) by gating re-entry only on socket readability, risking a second-handshake hang under backpressure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR completes post-handshake client certificate authentication for the experimental TlsSession API family and expands functional test coverage, including OpenSSL “fd-mode” (socket-bound) sessions.
Changes:
- Implement full post-handshake client-auth flow so
RequestClientCertificatere-arms the handshake and drives the second handshake throughHandshake()to completion (including re-surfacingNeedsCertificateValidation). - Add OpenSSL fd-mode support for post-handshake auth and apply external-validation gating consistently across fd-mode handshake/read/write paths.
- Expand
System.Net.Securityfunctional tests to cover post-handshake negotiation completion, socket-bound scenarios, channel binding/cipher suite properties, and additional API behaviors.
File summaries
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native/pal_ssl.c | Adds a SSL_peek nudge in the fd-mode handshake entrypoint to kick renegotiation / post-handshake auth processing. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSession.cs | Re-arms handshake state for post-handshake client auth, adjusts socket-bound RequestClientCertificate path, and aligns rejected-certificate visibility. |
| src/libraries/System.Net.Security/src/System/Net/Security/TlsSession.OpenSsl.cs | Implements fd-mode post-handshake client-auth driving and applies external-validation guards to fd-mode fast paths. |
| src/libraries/System.Net.Security/tests/FunctionalTests/TlsSessionTests.cs | Adds broad functional coverage for second-handshake completion (buffered + socket-bound), validation behavior, and related API properties. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+280
to
+284
| bool flushingRequest = !_fdPostHandshakeRequestSent; | ||
| if (!flushingRequest && !PeerHasPendingData()) | ||
| { | ||
| return TlsOperationStatus.NeedMoreData; | ||
| } |
| } | ||
| inUsed += r; | ||
| continue; | ||
| // Do not dispose: GetRemoteCertificate returns the session-owned instance. |
rzikm
changed the title
This was referenced Sep 3, 2026
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.
Add more test coverage and fix some behavior inconsistencies.
Note
This pull request was authored with assistance from GitHub Copilot.