From e2cd212e708fc76a308e411955554f5654e5de61 Mon Sep 17 00:00:00 2001 From: Sylwester Lachiewicz Date: Wed, 16 Sep 2026 15:49:07 +0200 Subject: [PATCH] THRIFT-6233: Use the peer-address matcher in TSSLServerSocket on every Python Client: py TSSLServerSocket checks a client certificate against the address the connection came from. On Python 3.12 and later it used sslcompat.match_peer_ipaddress. On 3.11 and earlier it used ssl.match_hostname. The two disagree. Only the former reduces an IPv4-mapped peer (THRIFT-6201). Only the latter falls back to the commonName when the certificate has no subjectAltName. A dual-stack listener reports an IPv4 client as ::ffff:127.0.0.1, so a certificate that carries 127.0.0.1 was accepted on 3.12 and refused on 3.10. THRIFT-3660 hid that in 2016 by listing the mapped address in client_v3.crt. THRIFT-6275 removed that entry, because Go 1.27 refuses to load a certificate with one. Since then the cross-test Python server, which listens dual-stack with client_v3.crt as its CA, refuses every IPv4 client on 3.11 and earlier. The server only matches an IP address, so match_peer_ipaddress is now its default on every version. The check runs whenever cert_reqs asks for a client certificate. It looks only at IP subjectAltName records. Which subjects may connect is the application's decision, made in a validate_callback; the docstring now shows one. TSSLSocket on the client side is unchanged. The backports.ssl_match_hostname branch in sslcompat, and the ValueError that named it, are removed. THRIFT-6265 already dropped the setup.py dependency. The library requires Python 3.10, so nothing below 3.5 can reach them. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5.1 --- lib/py/README.md | 18 ++++- lib/py/src/transport/TSSLSocket.py | 30 ++++++-- lib/py/src/transport/sslcompat.py | 21 +---- lib/py/test/test_sslsocket.py | 118 ++++++++++++++++++++++++++++- 4 files changed, 158 insertions(+), 29 deletions(-) diff --git a/lib/py/README.md b/lib/py/README.md index 44f676dd1ae..fc704f5e701 100644 --- a/lib/py/README.md +++ b/lib/py/README.md @@ -63,9 +63,21 @@ switches verification off altogether, as before. The default validate_callback on Python 3.12 and later, which stood in for the ssl.match_hostname removed in that release, now checks a peer certificate against an IP address and raises for a name rather than reporting a success it cannot back. -Name matching belongs to OpenSSL on those versions. The only caller left in the -library is TSSLServerSocket, which validates a client certificate against the -address the connection arrived from. +Name matching belongs to OpenSSL on those versions. + +TSSLServerSocket checks a client certificate against the address the connection +came from. It now uses that same matcher on every Python version. Before, it used +ssl.match_hostname on Python 3.11 and earlier. The check runs whenever cert_reqs +asks for a client certificate, and it looks only at the IP subjectAltName records +of the certificate. A dual-stack listener reports an IPv4 client as +::ffff:127.0.0.1; that peer now matches a certificate that carries 127.0.0.1 on +every version. A certificate without an IP subjectAltName is refused, where +ssl.match_hostname compared the commonName with the address instead. DNS records +are not matched, because a server has no name for its client. To decide which +subjects may connect, pass a validate_callback. It receives the certificate as +getpeercert() returns it, and the peer address. The backports.ssl_match_hostname +fallback in sslcompat can only run below Python 3.5 and is removed, as is the +setup.py dependency on it (THRIFT-6265). THttpServer now checks a request's Content-Length before it reads the body: a missing length is answered with 411, one that is not a non-negative number with diff --git a/lib/py/src/transport/TSSLSocket.py b/lib/py/src/transport/TSSLSocket.py index 84b50c5884a..81bbab5668e 100644 --- a/lib/py/src/transport/TSSLSocket.py +++ b/lib/py/src/transport/TSSLSocket.py @@ -24,7 +24,7 @@ import sys import warnings -from .sslcompat import _match_has_ipaddress, _match_hostname +from .sslcompat import _match_hostname, match_peer_ipaddress from thrift.transport import TSocket from thrift.transport.TTransport import TTransportException @@ -358,9 +358,22 @@ def __init__(self, host=None, port=9090, *args, **kwargs): ``server_hostname``: Passed to SSLContext.wrap_socket Common keyword argument: - ``validate_callback`` (cert, hostname) -> None: - Called after SSL handshake. Can raise when hostname does not - match the cert. + ``validate_callback`` (cert, peer_address) -> None: + Called after the SSL handshake whenever ``cert_reqs`` asks for + a client certificate. It receives the certificate as + ``getpeercert()`` returns it, and the address the connection + came from. Raise to refuse the connection. The default, + ``thrift.transport.sslcompat.match_peer_ipaddress``, requires + the peer address among the IP subjectAltName records of the + certificate, on every Python version. It does not match DNS + records, because a server has no name for its client. Use this + callback to decide which subjects may connect. For example:: + + def only_thrift_clients(cert, peer_address): + subject = dict(x[0] for x in cert.get('subject', ())) + if subject.get('organizationalUnitName') != 'Apache Thrift': + raise TTransportException( + message='client certificate not allowed') """ if args: if len(args) > 3: @@ -378,13 +391,14 @@ def __init__(self, host=None, port=9090, *args, **kwargs): kwargs['certfile'] = 'cert.pem' unix_socket = kwargs.pop('unix_socket', None) + # The server only matches the address the connection came from, so + # the IP matcher is the default on every Python version, not + # ssl.match_hostname where that still exists. The two disagree on an + # IPv4-mapped peer and on the commonName fallback. self._validate_callback = \ - kwargs.pop('validate_callback', _match_hostname) + kwargs.pop('validate_callback', match_peer_ipaddress) TSSLBase.__init__(self, True, None, kwargs) TSocket.TServerSocket.__init__(self, host, port, unix_socket) - if self._should_verify and not _match_has_ipaddress: - raise ValueError('Need ipaddress and backports.ssl_match_hostname ' - 'module to verify client certificate') def setCertfile(self, certfile): """Set or change the server certificate file used to wrap new diff --git a/lib/py/src/transport/sslcompat.py b/lib/py/src/transport/sslcompat.py index 1d505d0c37a..50de6fc02cb 100644 --- a/lib/py/src/transport/sslcompat.py +++ b/lib/py/src/transport/sslcompat.py @@ -134,20 +134,6 @@ def _optional_dependencies(): logger.warning('ipaddress module is unavailable') ipaddr = False - if sys.hexversion < 0x030500F0: - try: - from backports.ssl_match_hostname import match_hostname, __version__ as ver - ver = list(map(int, ver.split('.'))) - logger.debug('backports.ssl_match_hostname module is available') - match = match_hostname - if ver[0] * 10 + ver[1] >= 35: - return ipaddr, match - else: - logger.warning('backports.ssl_match_hostname module is too old') - ipaddr = False - except ImportError: - logger.warning('backports.ssl_match_hostname is unavailable') - ipaddr = False try: from ssl import match_hostname logger.debug('ssl.match_hostname is available') @@ -160,9 +146,10 @@ def _optional_dependencies(): # # OpenSSL performs it only when the context has check_hostname set, # which TSSLSocket now does for the contexts it builds. What is left - # for this function is TSSLServerSocket, which matches a client - # certificate against the address the connection arrived from -- an IP - # address, never a name -- so that is what the replacement covers. + # for this function is a client whose caller-supplied context has it + # off. There, only an IP address can still be checked; a name is + # refused, not passed. TSSLServerSocket uses match_peer_ipaddress + # directly on every Python version. if sys.version_info[0] > 3 or (sys.version_info[0] == 3 and sys.version_info[1] >= 12): match = match_peer_ipaddress else: diff --git a/lib/py/test/test_sslsocket.py b/lib/py/test/test_sslsocket.py index e03e328934e..3d6e21f87df 100644 --- a/lib/py/test/test_sslsocket.py +++ b/lib/py/test/test_sslsocket.py @@ -254,6 +254,7 @@ def test_set_server_cert(self): self._assert_connection_success(server, cert_reqs=ssl.CERT_REQUIRED, ca_certs=SERVER_CERT) def test_client_cert(self): + from thrift.transport.sslcompat import _match_has_ipaddress if not _match_has_ipaddress: print('skipping test_client_cert') return @@ -466,6 +467,121 @@ def test_peer_address_matcher_unmaps_ipv4(self): match_peer_ipaddress(plain, '::1') +class TSSLServerSocketClientCertTest(unittest.TestCase): + """A client certificate is matched against the peer address by default. + + These tests are kept out of TSSLSocketTest, which is skipped as a whole. + They drive TSSLServerSocket.accept() under the running interpreter, so + every Python in the matrix exercises the same default. Both client + certificates are self-signed, so the server trusts each one directly. + """ + + def _serve(self, host='127.0.0.1', **server_kwargs): + from thrift.transport.TSSLSocket import TSSLServerSocket + # The server protocol is named explicitly, as test/py/TestServer.py + # does: the class default is the client protocol, whose context + # insists on a server_hostname that a listener cannot supply. + server = TSSLServerSocket(host=host, port=0, + cert_reqs=ssl.CERT_REQUIRED, + certfile=SERVER_CERT, keyfile=SERVER_KEY, + ssl_version=ssl.PROTOCOL_TLS_SERVER, + **server_kwargs) + acc = ServerAcceptor(server, expect_failure=True) + acc.start() + acc.await_listening() + self.addCleanup(acc.close) + return acc + + def _exchange(self, port, certfile, keyfile): + """Return the server's reply, or None when it dropped the connection.""" + from thrift.transport.TSSLSocket import TSSLSocket + client = TSSLSocket('127.0.0.1', port, cert_reqs=ssl.CERT_REQUIRED, + ca_certs=SERVER_CERT, server_hostname='localhost', + certfile=certfile, keyfile=keyfile) + client.setTimeout(2000) + try: + client.open() + client.write(b"hello") + return client.read(5) + except Exception: + return None + finally: + try: + client.close() + except Exception: + pass + + @contextmanager + def _quiet(self): + # A refused client is logged as a warning with the traceback. + logging.disable(logging.CRITICAL) + try: + yield + finally: + logging.disable(logging.NOTSET) + + def test_default_is_the_peer_address_matcher_on_every_version(self): + from thrift.transport.TSSLSocket import TSSLServerSocket + from thrift.transport.sslcompat import match_peer_ipaddress + server = TSSLServerSocket(host='127.0.0.1', port=0, + certfile=SERVER_CERT, keyfile=SERVER_KEY, + ssl_version=ssl.PROTOCOL_TLS_SERVER) + self.assertIs(server._validate_callback, match_peer_ipaddress) + + def test_client_cert_with_address_accepted(self): + acc = self._serve(ca_certs=CLIENT_CERT) + self.assertEqual( + self._exchange(acc.port, CLIENT_CERT, CLIENT_KEY), b"there") + self.assertIsNotNone(acc.client) + + def test_client_cert_without_address_refused(self): + acc = self._serve(ca_certs=CLIENT_CERT_NO_IP) + with self._quiet(): + self.assertIsNone( + self._exchange(acc.port, CLIENT_CERT_NO_IP, CLIENT_KEY_NO_IP)) + self.assertIsNone(acc.client) + + def test_ipv4_mapped_peer_matches_the_plain_address(self): + # A dual-stack listener reports an IPv4 client as ::ffff:127.0.0.1. + # ssl.match_hostname refused that peer on Python 3.11 and earlier + # unless the certificate listed the mapped address itself. + # client_v3.crt carried that entry for this reason until THRIFT-6275 + # removed it, because Go 1.27 refuses to load a certificate with one. + # The cross-test Python server listens like this, with client_v3.crt + # as its CA. + try: + acc = self._serve(host=None, ca_certs=CLIENT_CERT) + except OSError as ex: + self.skipTest('no dual-stack listener here: %s' % ex) + if acc._server.handle.family != socket.AF_INET6: + self.skipTest('listener is not dual-stack') + self.assertEqual( + self._exchange(acc.port, CLIENT_CERT, CLIENT_KEY), b"there") + self.assertIsNotNone(acc.client) + + def test_custom_callback_decides_on_the_subject(self): + from thrift.transport.TTransport import TTransportException + seen = [] + + def refuse_thrift_clients(cert, peer_address): + seen.append((cert, peer_address)) + subject = dict(x[0] for x in cert.get('subject', ())) + if subject.get('organizationalUnitName') == 'Apache Thrift': + raise TTransportException( + message='client certificate not allowed') + + acc = self._serve(ca_certs=CLIENT_CERT, + validate_callback=refuse_thrift_clients) + with self._quiet(): + self.assertIsNone( + self._exchange(acc.port, CLIENT_CERT, CLIENT_KEY)) + self.assertIsNone(acc.client) + self.assertEqual(len(seen), 1) + cert, peer_address = seen[0] + self.assertIn('subject', cert) + self.assertEqual(peer_address, '127.0.0.1') + + # Add a dummy test because starting from python 3.12, if all tests in a test # file are skipped that's considered an error. class DummyTest(unittest.TestCase): @@ -475,7 +591,7 @@ def test_dummy(self): if __name__ == '__main__': logging.basicConfig(level=logging.WARN) - from thrift.transport.TSSLSocket import TSSLSocket, TSSLServerSocket, _match_has_ipaddress + from thrift.transport.TSSLSocket import TSSLSocket, TSSLServerSocket from thrift.transport.TTransport import TTransportException unittest.main()