THRIFT-6233: Use the peer-address matcher in TSSLServerSocket on every Python

lib/py/README.md

@@ -63,9 +63,20 @@ 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
+arrived from with that same matcher on every Python version, where it previously
+used ssl.match_hostname on 3.11 and earlier. The check stays on whenever cert_reqs
+asks for a client certificate, and covers IP subjectAltName records only: an
+IPv4-mapped peer such as ::ffff:127.0.0.1 now matches a certificate carrying
+127.0.0.1 on every version, and a certificate without an IP subjectAltName is
+refused, where ssl.match_hostname fell back to comparing the commonName with the
+address. DNS records are not matched, since a server has no name for its client.
+Which subjects may connect is the application's policy: pass a validate_callback,
+which receives the certificate as getpeercert() returns it and the peer address.
+The backports.ssl_match_hostname dependency, only reachable below Python 3.5, is
+gone from setup.py.
 
 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

lib/py/setup.py

@@ -104,8 +104,6 @@ def run_setup(with_binary):
         extensions = dict()
 
     ssl_deps = []
-    if sys.hexversion < 0x03050000:
-        ssl_deps.append('backports.ssl_match_hostname>=3.5')
     tornado_deps = ['tornado>=6.3.0']
     twisted_deps = ['twisted>=24.3.0', 'zope.interface>=6.1']
 

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,23 @@ 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 with the client certificate, as
+              ``getpeercert()`` returns it, and the address the connection
+              arrived from, whenever ``cert_reqs`` asks for a certificate.
+              Raise to refuse the connection. The default,
+              ``thrift.transport.sslcompat.match_peer_ipaddress``, requires
+              the peer address among the certificate's IP subjectAltName
+              records on every Python version; DNS records are not matched,
+              since a server has no name for its client. Which subjects may
+              connect is the application's policy and goes here, 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 +392,14 @@ def __init__(self, host=None, port=9090, *args, **kwargs):
                 kwargs['certfile'] = 'cert.pem'
 
         unix_socket = kwargs.pop('unix_socket', None)
+        # The server only ever matches the address the connection arrived
+        # from, so the IP matcher is the default on every Python version
+        # rather than 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

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, and there only an IP address can still be checked; a name is
+        # refused rather than 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:

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.
+
+    Kept out of TSSLSocketTest, which is skipped wholesale, and driven through
+    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.
+        # Before the IP matcher became the default on every version,
+        # ssl.match_hostname refused that peer on Python 3.11 and earlier
+        # unless the certificate listed the mapped address itself, which is
+        # why client_v3.crt carries one. The client therefore presents
+        # server.crt here: 127.0.0.1 and ::1, no mapped entry, the shape a
+        # certificate normally has.
+        try:
+            acc = self._serve(host=None, ca_certs=SERVER_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, SERVER_CERT, SERVER_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()