THRIFT-6233: Make the peer-address check on TSSLServerSocket opt-in - #3839
Draft
slachiewicz wants to merge 1 commit into
Draft
THRIFT-6233: Make the peer-address check on TSSLServerSocket opt-in#3839slachiewicz wants to merge 1 commit into
slachiewicz wants to merge 1 commit into
Conversation
Client: py The default validate_callback matched a client certificate against the address the connection arrived from, which no other binding does and which a client certificate often cannot satisfy behind NAT or a proxy. The default is now None; pass validate_callback=match_peer_ipaddress to keep the check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
slachiewicz
force-pushed
the
THRIFT-6233
branch
from
September 14, 2026 09:44
79be3ca to
42d6fb0
Compare
Member
Author
|
Rebased on master (42d6fb0) to pick up the TNonblockingServerTest fix from THRIFT-6244 behind the AppVeyor failure. The diff is unchanged (same patch-id). |
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
1 participant
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 agreed on #3818:
TSSLServerSocketno longer matches a client certificate against the address the connection arrived from unless the caller asks for it.validate_callbacknow defaults toNone; OpenSSL still verifies the certificate againstca_certswhencert_reqsasks for one, andvalidate_callback=thrift.transport.sslcompat.match_peer_ipaddressbrings the address check back.TSSLSocketandsslcompat._match_hostnameare unchanged. The_match_has_ipaddressguard inTSSLServerSocket.__init__goes with the default. Both readmes carry the note for servers on Python 3.11 or earlier that relied on the check.Two choices worth a look:
test/py/TestServer.pyopts in withvalidate_callback=match_peer_ipaddress, so the cross-test keeps exercising the address path, including the IPv4-mapped form THRIFT-6201: Reduce IPv4-mapped addresses before matching a peer certificate #3818 fixes. Dropping it would leave that path with unit coverage only.ssl_version=ssl.PROTOCOL_TLS_SERVER, asTestServer.pyalready does. ATSSLServerSocketbuilt withoutssl_versiongets the class defaultPROTOCOL_TLS_CLIENT, whose context requires aserver_hostname, andaccept()fails withValueError: check_hostname requires server_hostname. That is unchanged by this PR. The default dates from 7ec4177 (October 2024, first released in 0.22.0);TSSLSocketTestwas already skipped in April 2023, so the two are unrelated. It needs a ticket of its own, which I can file.Verified:
python test/test_sslsocket.py→ 20 tests, 3 new, OK with the 12 skips as before; with the library change stashed,test_client_cert_without_address_accepted_by_defaultfails.This change was created with AI assistance.