fix(server): downcast to MaybeTls in client_certificates to avoid panic#446
Merged
sunng87 merged 1 commit intoJun 29, 2026
Merged
Conversation
client_certificates() downcast the connection to TlsStream<TcpStream>, but process_socket wraps the negotiated stream in the MaybeTls enum, so the downcast always returned None and unwrap() panicked on every TLS connection -- making client-certificate auth unreachable from a StartupHandler. Downcast to MaybeTls and match the Tls arm instead; non-TLS variants return None (the Tls arm already implies a secure connection, so the is_secure() guard is no longer needed). Add a test that reconstructs the real MaybeTls-over-TcpStream wrapper process_socket produces and asserts client_certificates() returns None without panicking. Make the existing SNI test tolerant of an already-installed process-global crypto provider so both TLS tests can run in the same test binary. Fixes sunng87#445
sunng87
approved these changes
Jun 29, 2026
sunng87
left a comment
Owner
There was a problem hiding this comment.
LGTM as a backward-compatible fix.
In next minor release, I'm going to introduce a trait for the T so we can avoid downcast here.
Owner
|
Thank you @vladikbr for reporting and a quick fix! |
Owner
|
Released in 0.40.4 |
Contributor
Author
|
Thanks! |
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.
Fixes #445.
Problem
ClientInfo::client_certificates()panics on every TLS connection driven byprocess_socket.negotiate_tlswraps the negotiated stream in theMaybeTlsenum, butclient_certificates()downcast it to a bareTlsStream<TcpStream>:The runtime type is
MaybeTls, sodowncast_refreturnsNoneand.unwrap()panics. This makes peer certificates (and thus mTLS client-cert auth) unreachable from aStartupHandler.Fix
Downcast to
MaybeTlsand match theTlsarm; non-TLS variants returnNone. TheTlsarm already implies a secure connection, so the separateis_secure()guard is no longer needed.Tests
client_certificates_does_not_panic_under_maybe_tls: reconstructs the exactMaybeTls-over-TcpStreamwrapper thatprocess_socketproduces (over a real loopback TCP + TLS handshake) and assertsclient_certificates()returnsNone(the client presents no cert) instead of panicking. Verified it fails on the old code with the original panic and passes with the fix.server_name_metadata_is_set_from_tls_sni_in_memorytest tolerant of an already-installed process-global crypto provider (install_defaultis install-once), so both TLS tests can run in the same test binary without one'sunwrap()failing.Suite passes under both
server-api-ringandserver-api-aws-lc-rs.