IMPALA-14452: Fix impala-shell SSL with Python 3.12

Removes deprecated ImpalaHttpClient constructor that supported port and
path as it has been deprecated since at least 2020 and appears unused.

Removes cert_file and key_file as they were also never used, and if
required must now be passed in via ssl_context.

Updates TSSLSocket fixes for Thrift 0.16 and Python 3.12. _validate_cert
was removed by Thrift 0.16, but everything worked because Thrift used
ssl.match_hostname instead. With Python 3.12 ssl.match_hostname no
longer exists so we rely on OpenSSL to handle verification with
ssl.PROTOCOL_TLS_CLIENT.

Only uses ssl.PROTOCOL_TLS_CLIENT when match_hostname is unavailable to
avoid changing existing behavior. THRIFT-792 identifies that TSocket
suppresses connection errors, where we would otherwise see SSL hostname
verification errors like

    ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED]
    certificate verify failed: IP address mismatch, certificate is not
    valid for '::1'. (_ssl.c:1131)

Python 2.7.9 and 3.2 are minimum required versions; both have been EOL
for several years.

Testing:
- ran custom_cluster/{test_client_ssl.py,test_ipv6.py} on Ubuntu 24 with
  Python 3.12, OpenSSL 3.0.13.
- ran custom_cluster/test_client_ssl.py on RHEL 7.9 with Python 2.7.5
  and Python 3.6.8, OpenSSL 1.0.2k-fips.
- adds test that hostname checking is configured.

Change-Id: I046a9010ac4cb1f7d705935054b306cddaf8bdc7
Reviewed-on: http://gerrit.cloudera.org:8080/23519
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Reviewed-by: Csaba Ringhofer <csringhofer@cloudera.com>

setup.cfg

@@ -20,8 +20,9 @@
 # W503: ignored to allow line breaks before binary operators, as is becoming standard:
 #       https://github.com/python/peps/commit/7690a00d478866
 # E701: ignored to allow if statement body on same line as conditional
+# U101: ignore unused arguments prefixed with an underscore (_)
 # https://pycodestyle.readthedocs.io/en/latest/intro.html#error-codes
-ignore = E121,E125,E126,E127,E128,W503,E701
+ignore = E121,E125,E126,E127,E128,W503,E701,U101
 # Change from default to conform to Apache Impala line length.
 max-line-length = 90
 # Impala style guide uses 2-space indents.

shell/impala_shell/ImpalaHttpClient.py

@@ -23,9 +23,7 @@ import datetime
 from io import BytesIO
 import os
 import os.path
-import ssl
 import sys
-import warnings
 
 import six
 from six.moves import http_client, urllib
@@ -55,51 +53,29 @@ class ImpalaHttpClient(TTransportBase):
   # value was chosen to match curl's behavior. See Section 8.2.3 of RFC2616.
   MIN_REQUEST_SIZE_FOR_EXPECT = 1024
 
-  def __init__(self, uri_or_host, port=None, path=None, cafile=None, cert_file=None,
+  def __init__(self, uri_or_host, ssl_context=None, http_cookie_names=None,
-      key_file=None, ssl_context=None, http_cookie_names=None, socket_timeout_s=None,
+               socket_timeout_s=None, verbose=False):
-      verbose=False):
+    """To properly authenticate against an HTTPS server, provide an ssl_context created
-    """ImpalaHttpClient supports two different types of construction:
+    with ssl.create_default_context() to validate the server certificate.
 
-    ImpalaHttpClient(host, port, path) - deprecated
-    ImpalaHttpClient(uri, [port=<n>, path=<s>, cafile=<filename>, cert_file=<filename>,
-        key_file=<filename>, ssl_context=<context>], http_cookie_names=<cookienamelist>])
-
-    Only the second supports https.  To properly authenticate against the server,
-    provide the client's identity by specifying cert_file and key_file.  To properly
-    authenticate the server, specify either cafile or ssl_context with a CA defined.
-    NOTE: if both cafile and ssl_context are defined, ssl_context will override cafile.
     http_cookie_names is used to specify a comma-separated list of possible cookie names
     used for cookie-based authentication or session management. If a cookie with one of
     these names is returned in an http response by the server or an intermediate proxy
     then it will be included in each subsequent request for the same connection. If it
     is set as wildcards, all cookies in an http response will be preserved.
     """
-    if port is not None:
+    parsed = urllib.parse.urlparse(uri_or_host)
-      warnings.warn(
+    self.scheme = parsed.scheme
-          "Please use the ImpalaHttpClient('http{s}://host:port/path') constructor",
+    assert self.scheme in ('http', 'https')
-          DeprecationWarning,
+    if self.scheme == 'http':
-          stacklevel=2)
+      self.port = parsed.port or http_client.HTTP_PORT
-      self.host = uri_or_host
+    elif self.scheme == 'https':
-      self.port = port
+      self.port = parsed.port or http_client.HTTPS_PORT
-      assert path
+      self.context = ssl_context
-      self.path = path
+    self.host = parsed.hostname
-      self.scheme = 'http'
+    self.path = parsed.path
-    else:
+    if parsed.query:
-      parsed = urllib.parse.urlparse(uri_or_host)
+      self.path += '?%s' % parsed.query
-      self.scheme = parsed.scheme
-      assert self.scheme in ('http', 'https')
-      if self.scheme == 'http':
-        self.port = parsed.port or http_client.HTTP_PORT
-      elif self.scheme == 'https':
-        self.port = parsed.port or http_client.HTTPS_PORT
-        self.certfile = cert_file
-        self.keyfile = key_file
-        self.context = ssl.create_default_context(cafile=cafile) \
-            if (cafile and not ssl_context) else ssl_context
-      self.host = parsed.hostname
-      self.path = parsed.path
-      if parsed.query:
-        self.path += '?%s' % parsed.query
     try:
       proxy = urllib.request.getproxies()[self.scheme]
     except KeyError:
@@ -166,8 +142,6 @@ class ImpalaHttpClient(TTransportBase):
                                                timeout=self.__timeout)
     elif self.scheme == 'https':
       self.__http = http_client.HTTPSConnection(self.host, self.port,
-                                                key_file=self.keyfile,
-                                                cert_file=self.certfile,
                                                 timeout=self.__timeout,
                                                 context=self.context)
     if self.using_proxy():

shell/impala_shell/TSSLSocketWithFixes.py

@@ -0,0 +1,67 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import absolute_import, print_function, unicode_literals
+import ssl
+
+from thrift.transport.TSSLSocket import TSSLSocket
+
+
+def openssl_fallback(*_args):
+  return True
+
+
+class TSSLSocketWithFixes(TSSLSocket):
+  """
+  This is a subclass of Thrift 0.16.0's TSSLSocket
+  https://github.com/apache/thrift/blob/0.16.0/lib/py/src/transport/TSSLSocket.py
+  that fixes isOpen (THRIFT-5595) and adds support for Python 3.12+.
+
+  Requires Python 2.7.9+ or Python 3.2+. For Python 2.7 and 3.2-3.9, uses match_hostname
+  and PROTOCOL_TLS. For Python 3.10 and 3.11, uses match_hostname and PROTOCOL_TLS_CLIENT.
+  For Python 3.12+, relies solely on OpenSSL's built-in hostname validation, enabled by
+  PROTOCOL_TLS_CLIENT.
+  """
+  def __init__(self, host, port, cert_reqs, ca_certs=None):
+    # Implement Python 3.12 override from Thrift 0.22.0
+    # https://github.com/apache/thrift/commit/23e0e5ce75300451f49727ee438edbc76fcbb372
+    # Earlier versions continue to use ssl.match_hostname, which is available in
+    # Python 2.7.9+ and Python 3.2+.
+    ssl_version = ssl.PROTOCOL_SSLv23
+    try:
+      from ssl import match_hostname
+      validate_callback = match_hostname
+    except ImportError:
+      validate_callback = openssl_fallback
+      # ssl.PROTOCOL_TLS_CLIENT is available in Python 3.6+ and enables secure defaults
+      # (CERT_REQUIRED, check_hostname). Only use it when match_hostname is unavailable
+      # (i.e. Python 3.12+) to avoid regressing the clarity of error messages in earlier
+      # versions. See https://issues.apache.org/jira/browse/THRIFT-792 for future work.
+      assert hasattr(ssl, "PROTOCOL_TLS_CLIENT")
+      if cert_reqs == ssl.CERT_NONE:
+        # If no cert verification is requested, use the most compatible option.
+        ssl_version = ssl.PROTOCOL_TLS
+      else:
+        # This also enables CERT_REQUIRED and check_hostname by default.
+        ssl_version = ssl.PROTOCOL_TLS_CLIENT
+
+    TSSLSocket.__init__(self, host=host, port=port, cert_reqs=cert_reqs,
+                        ca_certs=ca_certs, ssl_version=ssl_version,
+                        validate_callback=validate_callback)
+
+  # THRIFT-5595: override TSocket.isOpen because it's broken for TSSLSocket
+  def isOpen(self):
+    return self.handle is not None

shell/impala_shell/TSSLSocketWithWildcardSAN.py

@@ -1,170 +0,0 @@
-#!/usr/bin/env python
-#
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements.  See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership.  The ASF licenses this file
-# to you under the Apache License, Version 2.0 (the
-# "License"); you may not use this file except in compliance
-# with the License.  You may obtain a copy of the License at
-#
-#   http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing,
-# software distributed under the License is distributed on an
-# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-# KIND, either express or implied.  See the License for the
-# specific language governing permissions and limitations
-# under the License.
-from __future__ import absolute_import, print_function, unicode_literals
-import re
-import ssl
-
-from thrift.transport import TSSLSocket
-from thrift.transport.TTransport import TTransportException
-
-
-class CertificateError(ValueError):
-  """Convenience class to raise errors"""
-  pass
-
-
-class TSSLSocketWithWildcardSAN(TSSLSocket.TSSLSocket):
-  """
-  This is a subclass of thrift's TSSLSocket which has been extended to add the missing
-  functionality of validating wildcard certificates and certificates with SANs
-  (subjectAlternativeName).
-
-  The core of the validation logic is based on the python-ssl library:
-  See <https://svn.python.org/projects/python/tags/r32/Lib/ssl.py>
-  """
-  def __init__(self,
-      host='localhost',
-      port=9090,
-      validate=True,
-      ca_certs=None,
-      unix_socket=None):
-    cert_reqs = ssl.CERT_REQUIRED if validate else ssl.CERT_NONE
-    # Set client protocol choice to be very permissive, as we rely on servers to enforce
-    # good protocol selection. This value is forwarded to the ssl.wrap_socket() API during
-    # open(). See https://docs.python.org/2/library/ssl.html#socket-creation for a table
-    # that shows a better option is not readily available for sockets that use
-    # wrap_socket().
-    # THRIFT-3505 changes transport/TSSLSocket.py. The SSL_VERSION is passed to TSSLSocket
-    # via a parameter.
-    TSSLSocket.TSSLSocket.__init__(self, host=host, port=port, cert_reqs=cert_reqs,
-                                   ca_certs=ca_certs, unix_socket=unix_socket,
-                                   ssl_version=ssl.PROTOCOL_SSLv23)
-
-  # THRIFT-5595: override TSocket.isOpen because it's broken for TSSLSocket
-  def isOpen(self):
-    return self.handle is not None
-
-  def _validate_cert(self):
-    cert = self.handle.getpeercert()
-    self.peercert = cert
-    if 'subject' not in cert:
-      raise TTransportException(
-        type=TTransportException.NOT_OPEN,
-        message='No SSL certificate found from %s:%s' % (self.host, self.port))
-    try:
-      self._match_hostname(cert, self.host)
-      self.is_valid = True
-      return
-    except CertificateError as ce:
-      raise TTransportException(
-        type=TTransportException.UNKNOWN,
-        message='Certificate error with remote host: %s' % (ce))
-    raise TTransportException(
-      type=TTransportException.UNKNOWN,
-      message='Could not validate SSL certificate from '
-              'host "%s".  Cert=%s' % (self.host, cert))
-
-  def _match_hostname(self, cert, hostname):
-    """Verify that *cert* (in decoded format as returned by
-    SSLSocket.getpeercert()) matches the *hostname*.  RFC 2818 and RFC 6125
-    rules are followed, but IP addresses are not accepted for *hostname*.
-
-    CertificateError is raised on failure. On success, the function
-    returns nothing.
-    """
-    dnsnames = []
-    san = cert.get('subjectAltName', ())
-    for key, value in san:
-      if key == 'DNS':
-        if self._dnsname_match(value, hostname):
-          return
-        dnsnames.append(value)
-    if not dnsnames:
-      # The subject is only checked when there is no dNSName entry
-      # in subjectAltName
-      for sub in cert.get('subject', ()):
-        for key, value in sub:
-          # XXX according to RFC 2818, the most specific Common Name
-          # must be used.
-          if key == 'commonName':
-            if self._dnsname_match(value, hostname):
-              return
-            dnsnames.append(value)
-    if len(dnsnames) > 1:
-      raise CertificateError("hostname %r "
-        "doesn't match either of %s"
-        % (hostname, ', '.join(map(repr, dnsnames))))
-    elif len(dnsnames) == 1:
-      raise CertificateError("hostname %r "
-        "doesn't match %r"
-        % (hostname, dnsnames[0]))
-    else:
-      raise CertificateError("no appropriate commonName or "
-        "subjectAltName fields were found")
-
-  def _dnsname_match(self, dn, hostname, max_wildcards=1):
-    """Matching according to RFC 6125, section 6.4.3
-    http://tools.ietf.org/html/rfc6125#section-6.4.3
-    """
-    pats = []
-    if not dn:
-      return False
-
-    # Ported from python3-syntax:
-    # leftmost, *remainder = dn.split(r'.')
-    parts = dn.split(r'.')
-    leftmost = parts[0]
-    remainder = parts[1:]
-
-    wildcards = leftmost.count('*')
-    if wildcards > max_wildcards:
-      # Issue #17980: avoid denials of service by refusing more
-      # than one wildcard per fragment.  A survey of established
-      # policy among SSL implementations showed it to be a
-      # reasonable choice.
-      raise CertificateError(
-        "too many wildcards in certificate DNS name: " + repr(dn))
-
-    # speed up common case w/o wildcards
-    if not wildcards:
-      return dn.lower() == hostname.lower()
-
-    # RFC 6125, section 6.4.3, subitem 1.
-    # The client SHOULD NOT attempt to match a presented identifier in which
-    # the wildcard character comprises a label other than the left-most label.
-    if leftmost == '*':
-      # When '*' is a fragment by itself, it matches a non-empty dotless
-      # fragment.
-      pats.append('[^.]+')
-    elif leftmost.startswith('xn--') or hostname.startswith('xn--'):
-      # RFC 6125, section 6.4.3, subitem 3.
-      # The client SHOULD NOT attempt to match a presented identifier
-      # where the wildcard character is embedded within an A-label or
-      # U-label of an internationalized domain name.
-      pats.append(re.escape(leftmost))
-    else:
-      # Otherwise, '*' matches any dotless string, e.g. www*
-      pats.append(re.escape(leftmost).replace(r'\*', '[^.]*'))
-
-    # add the remaining fragments, ignore any wildcards
-    for frag in remainder:
-      pats.append(re.escape(frag))
-
-    pat = re.compile(r'\A' + r'\.'.join(pats) + r'\Z', re.IGNORECASE)
-    return pat.match(hostname)

shell/impala_shell/impala_client.py

@@ -50,6 +50,7 @@ from impala_shell.shell_exceptions import (
     RPCException,
 )
 from impala_shell.thrift_printer import ThriftPrettyPrinter
+from impala_shell.TSSLSocketWithFixes import TSSLSocketWithFixes
 from impala_shell.value_converter import HS2ValueConverter
 from impala_thrift_gen.beeswax import BeeswaxService
 from impala_thrift_gen.beeswax.BeeswaxService import QueryState
@@ -493,11 +494,6 @@ class ImpalaClient(object):
        is used.
        This function returns the socket and the transport object.
     """
-    if self.use_ssl:
-      # TSSLSocket needs the ssl module, which may not be standard on all Operating
-      # Systems. Only attempt to import TSSLSocket if the user wants an SSL connection.
-      from impala_shell.TSSLSocketWithWildcardSAN import TSSLSocketWithWildcardSAN
-
     # The kerberos_host_fqdn option exposes the SASL client's hostname attribute to
     # the user. impala-shell checks to ensure this host matches the host in the kerberos
     # principal. So when a load balancer is configured to be used, its hostname is
@@ -515,10 +511,10 @@ class ImpalaClient(object):
     if self.use_ssl:
       if self.ca_cert is None:
         # No CA cert means don't try to verify the certificate
-        sock = TSSLSocketWithWildcardSAN(sock_host, sock_port, validate=False)
+        sock = TSSLSocketWithFixes(sock_host, sock_port, cert_reqs=ssl.CERT_NONE)
       else:
-        sock = TSSLSocketWithWildcardSAN(
+        sock = TSSLSocketWithFixes(
-            sock_host, sock_port, validate=True, ca_certs=self.ca_cert)
+            sock_host, sock_port, cert_reqs=ssl.CERT_REQUIRED, ca_certs=self.ca_cert)
     else:
       sock = TSocket(sock_host, sock_port)
 

shell/impala_shell/impala_shell.py

@@ -168,7 +168,7 @@ class ImpalaShell(cmd.Cmd, object):
   # Message to display when the connection failed and it is reconnecting.
   CONNECTION_LOST_MESSAGE = 'Connection lost, reconnecting...'
   # Message to display when there is an exception when connecting.
-  ERROR_CONNECTING_MESSAGE = "Error connecting"
+  ERROR_CONNECTING_MESSAGE = "Error connecting "
   # Message to display when there is a socket error.
   SOCKET_ERROR_MESSAGE = "Socket error"
   # Message to display upon successful connection to an Impalad

tests/custom_cluster/test_client_ssl.py

@@ -40,6 +40,10 @@ from tests.shell.util import run_impala_shell_cmd, run_impala_shell_cmd_no_expec
 
 CERT_DIR = "%s/be/src/testutil" % os.environ['IMPALA_HOME']
 
+# Due to THRIFT-792, SSL errors are suppressed when using OpenSSL hostname verification.
+# This is the only option on Python 3.12+, using ssl.PROTOCOL_TLS_CLIENT.
+CERT_ERR = ["doesn't match", "certificate verify failed", "Could not connect"]
+
 
 class TestClientSsl(CustomClusterTestSuite):
   """Tests for a client using SSL (particularly, the Impala Shell) """
@@ -85,7 +89,7 @@ class TestClientSsl(CustomClusterTestSuite):
   @CustomClusterTestSuite.with_args(impalad_args=SSL_ARGS, statestored_args=SSL_ARGS,
                                     catalogd_args=SSL_ARGS)
   def test_ssl(self, vector):
-    self._verify_negative_cases(vector)
+    self._verify_negative_cases(vector, "%s/server-cert.pem" % CERT_DIR)
     # TODO: This is really two different tests, but the custom cluster takes too long to
     # start. Make it so that custom clusters can be specified across test suites.
     self._validate_positive_cases(vector, "%s/server-cert.pem" % CERT_DIR)
@@ -170,7 +174,7 @@ class TestClientSsl(CustomClusterTestSuite):
                                     catalogd_args=TLS_ECDH_ARGS)
   @pytest.mark.skipif(SKIP_SSL_MSG is not None, reason=SKIP_SSL_MSG)
   def test_tls_ecdh(self, vector):
-    self._verify_negative_cases(vector)
+    self._verify_negative_cases(vector, "%s/server-cert.pem" % CERT_DIR)
     self._validate_positive_cases(vector, "%s/server-cert.pem" % CERT_DIR)
     self._verify_ssl_webserver()
 
@@ -199,7 +203,8 @@ class TestClientSsl(CustomClusterTestSuite):
     """ Test for IMPALA-3159: Test with a certificate which has a wildcard for the
     CommonName.
     """
-    self._verify_negative_cases(vector, host="ip4.impala.test")
+    self._verify_negative_cases(vector, "%s/server-cert.pem" % CERT_DIR,
+                                host="ip4.impala.test")
 
     self._validate_positive_cases(vector, "%s/wildcardCA.pem" % CERT_DIR,
                                   host="ip4.impala.test")
@@ -221,14 +226,27 @@ class TestClientSsl(CustomClusterTestSuite):
           "cannot retrieve SAN from certificate: "
           "https://bugzilla.redhat.com/show_bug.cgi?id=928390")
 
-    self._verify_negative_cases(vector, host="ip4.impala.test")
+    self._verify_negative_cases(vector, "%s/server-cert.pem" % CERT_DIR,
+                                host="ip4.impala.test")
 
     self._validate_positive_cases(vector, "%s/wildcardCA.pem" % CERT_DIR,
                                   host="ip4.impala.test")
     self.check_connections()
 
+  def _is_cert_error(self, stderr):
+    for err in CERT_ERR:
+      if err in stderr:
+        return True
+    return False
+
+  def _verify_negative_cases(self, vector, ca_cert, host=""):
+    # Expect the shell to not start successfully if we connect to an endpoint that
+    # doesn't match the certificate.
+    invalid_host = "localhost" if host else "127.0.0.1"
+    args = ["--ssl", "-q", "select 1 + 2", "-i", invalid_host, "--ca_cert=%s" % ca_cert]
+    result = run_impala_shell_cmd(vector, args, expect_success=False)
+    assert self._is_cert_error(result.stderr), result.stderr
 
-  def _verify_negative_cases(self, vector, host=""):
     # Expect the shell to not start successfully if we point --ca_cert to an incorrect
     # certificate.
     args = ["--ssl", "-q", "select 1 + 2",

tests/custom_cluster/test_ipv6.py

@@ -66,7 +66,9 @@ WEBUI_PORTS = [25000, 25010, 25020]
 
 # Error text can depend on both protocol and python version.
 CONN_ERR = ["Could not connect", "Connection refused"]
-CERT_ERR = ["doesn't match", "certificate verify failed"]
+# Due to THRIFT-792, SSL errors are suppressed when using OpenSSL hostname verification.
+# This is the only option on Python 3.12+, using ssl.PROTOCOL_TLS_CLIENT.
+CERT_ERR = ["doesn't match", "certificate verify failed", "Could not connect"]
 WEB_CERT_ERR = ("CertificateError" if sys.version_info.major < 3
                 else "SSLCertVerificationError")