fix: root CA support regression

The switch from the deprecated NewTBinaryProtocol to the recommended replacement
NewTBinaryProtocolConf had an unintended side-effect - TLS configuration was
propagated from the protocol to the transport, erasing the previously configured RootCAPool.

This PR fixes that and extends integration tests to cover TLS.

Closes #9

compose/quickstart.yml

@@ -22,7 +22,6 @@
 # will be stored in that volume by default.
 #
 # See README.md in this directory for usage instructions.
-version: "3"
 services:
   hms:
     image: ${IMPALA_QUICKSTART_IMAGE_PREFIX:-}impala_quickstart_hms
@@ -79,18 +78,23 @@ services:
       - "${QUICKSTART_LISTEN_ADDR:?Please set QUICKSTART_LISTEN_ADDR environment variable}:25000:25000"
       # HS2 over HTTP endpoint.
       - "${QUICKSTART_LISTEN_ADDR:?Please set QUICKSTART_LISTEN_ADDR environment variable}:28000:28000"
-    command: [ "-v=1",
-              "-redirect_stdout_stderr=false", "-logtostderr",
-              "-kudu_master_hosts=kudu-master-1:7051",
-              "-mt_dop_auto_fallback=true",
-              "-default_query_options=mt_dop=4,default_file_format=parquet,default_transactional_type=insert_only",
-              "-mem_limit=4gb"]
+    command:
+      - "-v=1"
+      - "-redirect_stdout_stderr=false"
+      - "-logtostderr"
+      #      - "-kudu_master_hosts=kudu-master-1:7051"
+      - "-mt_dop_auto_fallback=true"
+      - "-default_query_options=mt_dop=4,default_file_format=parquet,default_transactional_type=insert_only"
+      - "-mem_limit=4gb"
+      - "-ssl_server_certificate=/ssl/localhost.crt"
+      - "-ssl_private_key=/ssl/localhost.key"
     environment:
       # Keep the Java heap small to preserve memory for query execution.
       - JAVA_TOOL_OPTIONS="-Xmx1g"
     volumes:
       - impala-quickstart-warehouse:/user/hive/warehouse
       - ./quickstart_conf:/opt/impala/conf:ro
+      - ./testssl:/ssl:ro
     networks:
       - quickstart-network
 volumes:

compose/testssl/localhost.crt

@@ -0,0 +1,30 @@
+-----BEGIN CERTIFICATE-----
+MIIFGzCCAwOgAwIBAgIUAahWdKX/OerjdaZlvpBbH/aMTdswDQYJKoZIhvcNAQEL
+BQAwEjEQMA4GA1UEAwwHbXVyZmZmaTAeFw0yNTAxMTkxMjUwNTNaFw0zNTAxMTcx
+MjUwNTNaMBIxEDAOBgNVBAMMB211cmZmZmkwggIiMA0GCSqGSIb3DQEBAQUAA4IC
+DwAwggIKAoICAQC8b+sRlY7tSXAKhhIs+qFpAnKjjsGH6HUP0iBNLR3x3/dfe2Do
+KnGwRWpPcX8cu/2+/izwy0APsMVlfWZ/5L3BJTLxTjogw19ZaEYy9ZSwwEGflMQV
+E3u8pxSJaRCRxOKhkk+fG4RKkU/5dI3QdAWwXn03mqZlplqZ6Oh96hyPARtcMS3i
+68D+lHdk3QUE27eaeMkuakcUJRVkWRZ0LHJEXlzL81lTULDxLDnLlxUfuPvxuGcF
+/PGyjiR6dH80uqYEmeb8V0RyPxbYEplDf2/3xpUgFnatiBXlLvH71k/t/1Q4A1yF
+IHwSVD62hpjfA9xavLjjnYJ1w1SzClbo9eq3dm14vSqIxgyxCILeUvxs8b7K9lh2
+lEGfsiTTtu1zaBDlh0p3jJtDe8r5tXC0YMI/klH6gKWkbKpUtls5Tt1hfQxY+mWS
+rcDyA7o3gtyL9nNEgx7lBVehvUB0jZgSkiodJoW0C/X4GNqpqDSySXN4d0GeNHRJ
+XRtzbnyKdVGKnVebrwAdeSyq1d+OHGSCxq+Fvin9qKjIznaJjyngo4c58iJ+XACF
+t0LWl24EHDfCYTPWQC9FYnJEE2zRKeCegVQjqjPUN5dZl2WK4ghQS4W6y80CvvFB
+UU0vH4g8bZzEYLp0nDMDr/yVwx6oL+5XgocGNWOOndz9VCViE8G51udadwIDAQAB
+o2kwZzAdBgNVHQ4EFgQUdoscQlYqX44WF2lb+ITcsEvVNUcwHwYDVR0jBBgwFoAU
+doscQlYqX44WF2lb+ITcsEvVNUcwDwYDVR0TAQH/BAUwAwEB/zAUBgNVHREEDTAL
+gglsb2NhbGhvc3QwDQYJKoZIhvcNAQELBQADggIBADY1AMTrM0hRV7sGjuQCEwkH
+5jMW1qo7i7QH9RNXb2PnWNz7IUGFIWXEk5H1K7xn5HTspQ8HZHKMw1HD9IhAYXCb
+w+XWymULqJXKEJeM9a2mqIJJlVNkVq5mbKJr3OBL3w7SMZwSnIO+zgU4jrUjC759
+BLIOoPiSYL145+piiE5xZa3RGUSh4RymQefkE0EjzyARUfSeoUl83TZYTvHNrrOm
+3bnKeYVzL3V7bDillLqLFDyb0/HCxyiLSkOAJ/NjdH9L1NcdJmiMnF8C3qv/v/Mw
+VCrmEwxPstmSNUtqiGCtqJ+MaWIZ7xZZaeQJr7/guJ3/ynBmtxheAz/1KP9vr48g
+cV3XP4PHqQZPfF3JewqTkv+CeKMRcCGSvivw20I9TURgBnXfqmJO3IzmwT8exrWa
+tYr5Al5E4yjK18Y6znvbaYnJUu01N5nQYrEwfNZrvsn+mN+XgDUmWqfrH4yTmiuI
+gIfsk5haBtrrirH+Mqb4VTE/ijZJUPvEccz9AHXhD6ejMynUX2nQkUct/XPYqneO
+d9HYE7W038CYL8FxeyuFkJH4jcyAZwzXaqqCJnLyWeHxlUjQP4o2DyIr/xz4mrSP
+zHi2ICVplrlMsKMg9e7OsbFWHt/d+3YQy7AIifjtoydzVEAvZ+J1p0tOcOoclZPM
+YZudhFMVY9Q8wEx8iCtv
+-----END CERTIFICATE-----

compose/testssl/localhost.ext

@@ -0,0 +1,7 @@
+authorityKeyIdentifier=keyid,issuer
+basicConstraints=CA:FALSE
+keyUsage = digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment
+subjectAltName = @alt_names
+
+[alt_names]
+DNS.1 = localhost

compose/testssl/localhost.key

@@ -0,0 +1,52 @@
+-----BEGIN PRIVATE KEY-----
+MIIJQgIBADANBgkqhkiG9w0BAQEFAASCCSwwggkoAgEAAoICAQC8b+sRlY7tSXAK
+hhIs+qFpAnKjjsGH6HUP0iBNLR3x3/dfe2DoKnGwRWpPcX8cu/2+/izwy0APsMVl
+fWZ/5L3BJTLxTjogw19ZaEYy9ZSwwEGflMQVE3u8pxSJaRCRxOKhkk+fG4RKkU/5
+dI3QdAWwXn03mqZlplqZ6Oh96hyPARtcMS3i68D+lHdk3QUE27eaeMkuakcUJRVk
+WRZ0LHJEXlzL81lTULDxLDnLlxUfuPvxuGcF/PGyjiR6dH80uqYEmeb8V0RyPxbY
+EplDf2/3xpUgFnatiBXlLvH71k/t/1Q4A1yFIHwSVD62hpjfA9xavLjjnYJ1w1Sz
+Clbo9eq3dm14vSqIxgyxCILeUvxs8b7K9lh2lEGfsiTTtu1zaBDlh0p3jJtDe8r5
+tXC0YMI/klH6gKWkbKpUtls5Tt1hfQxY+mWSrcDyA7o3gtyL9nNEgx7lBVehvUB0
+jZgSkiodJoW0C/X4GNqpqDSySXN4d0GeNHRJXRtzbnyKdVGKnVebrwAdeSyq1d+O
+HGSCxq+Fvin9qKjIznaJjyngo4c58iJ+XACFt0LWl24EHDfCYTPWQC9FYnJEE2zR
+KeCegVQjqjPUN5dZl2WK4ghQS4W6y80CvvFBUU0vH4g8bZzEYLp0nDMDr/yVwx6o
+L+5XgocGNWOOndz9VCViE8G51udadwIDAQABAoICACBHgbqU5pNySY2b8uAenOjK
+oQ2whil8YhUwNxVRXbhR1+PU1EBhCPGbqn7qX85xy6FclCE5iANIEkB070qIShMc
+hrH8Zwu2vA4qRSA0nmB6xqMKDl7X5BHogyWYyFxYIERyNk54nbxSUSNX2nUzcsgI
+H6YXSVrPWOTVl4H+SR8cHu43hHdDUF+r2hn+nH39D2IYP8pCLWPfiF8Uvbpv5LEe
+gwHy8UHK6Eyf+VSNUHKLMylwn8FQV6zxGYGalDHZqVfPb1iZsGFrukOQSA3edjU5
+INGK15CZZ7rHRG+SAkYwP2KK/N3XNznCka2dt/E4DZbDjl57XF5E9CzpYALvIWI5
+2RmXC37Mz6FKa3qHR3b5KWZ73A5ZrHKhdENWdDEQEWBE4bQpYR9QWBN6iG3KgGnl
+pyCEXDz8U1brlY1gaZLbD960Qa+62hgZdIqIE16/TgztaoWlq5sZyPwtc2l9aWuL
+QentPIwSo7CAA8wlOFDqrrmbFhbYg5crx1dBAGeI4ZpohCWdjygawx8LpR4eGo9M
+9uysnfo6oXIu4rcgthsYqQVUY348Oxi9Dr7Nb9vdoAmR43TvsoPQov4ouY+sVku1
+hfQtF7RiM5APAPx/cVz74NQSi2N/zrOmp7KdpZKtaAUbyJzWUdf1KHW/PmB/OMsi
+YNe/pYIS7EGvB1Cg68LZAoIBAQDcB8tTsBMfw3zD9squ7lHN9Oih7CNg3vG/y5ol
+jLWhtDOSHPG0eezXf3KAqBgDvfwide9XErD1WgANEDHwYvD/xgHHEcbpJl2ySXR3
+b+MeftePzCV8ypOHO6PjcCWQ1XYBbclxiXUVjhH48jXDzr/uoTiYXgUkMd88YT27
+PDYKgwwUhf4Q29WTigvGnGXMP3w2u7cP1oftfGb3YiLCX5EdIet6vC4R/dVF0JHd
+ZybRbsFnhzvsPAgz7SX9774iog9Armdz2BS5wxAsKPy80srpKOyDwy2Rk2IoLSr6
+OUanPjUbr7RBFH2MMoAQCsN3nq8CNn8U23ClEK9gjG6rAKGJAoIBAQDbPfSlWJ+I
+MuClt4Gu4wN9cTXosEYSfxjCUOgNjU9Mg7/T3OyJWaKWrDHln92SFh7hxbck81f4
+PFC31KtzQMxZUgVs5X9L7vDdHkKgNML17Dvj4voWbTo8LHh/YvQFbQfQoaHP4Al9
+jXENJSKTEGIKjk6yYlERznBaIyqDqO9AohFuP75XTGH30ceMT9IE5ne/aE8/9C07
+PVFMM44NLrudpROA3tqcT82b62nfWHwmi/dlzroAI1dWBc05Jn0zfuWc3sk4F3vS
+1q5qR12sGG+LIGk0Fs40xRyqqSDy9O8RenUpkhtXRFmXW+rWGz8tpEDq0nAbX7XV
+dmVYWVGwFRv/AoIBAQC4c+BD++tMSXkiXpVzKF5zpcgPVgIih0NqYaiLqfXp5UMG
+TjVh8oRV84VtzXy0RmRED1HhUM4AIL7CNL1oo8kc4kVDOOfjNEJ/34w/RBDF09ep
+uJ4Ei145lAnD9JPJYulWMU8aWv1IYJ04cPQZfgzwYz8qnEb7HMPjvjEd8U/saAeM
+fPeL8n6M/MD6csnz+5SJ3buND29L62n2INK615qzLWhWr8J/Wqebq0lcrcig8ZQ4
+0emuFHVb8oZS5tQh/HGE66/WMWLOh3PbUVDuileINsJvgwOEcmVrANJyiels8n1b
+BGS72g2VphAtYpiSgvh7hmvqdDtuZRpgedmciR7hAoIBAEmQQlRwpibnG2W/ay4p
+UfR3ViVbcEeoicA5sYNKlP8Rff4ytNNpVhrEcIGh4JCyB26uDsPBOxU598OXAmhL
+p8WYteVSJCJwbDVlf+mNipVFzNqsQEniIJYsPcpQg5yGdwbUFR1RCR4tpW9JtI4w
+AxWTpmhIgPc+k4hLIGrI9m5zcvHRRaa5JG4o35H+/nNSeX+qGl1VKxAjhPHSoqld
+suo0h5TIMgYc6NqWZRHh+NZeRIbOlK6TLSzaCYBRn9T1kUnPnyjJCG04R5G1nIKw
+OE0wdNxbgplGQlYpJbsf4E+3DTUDp1TixET7PJLW1Z4U7QtdzOwCc1rKvTqlzbkN
+fDUCggEATJSwO0dkvhxBrO86YSVY4wxboSacaOWrQNuFeiD7pZcWFvki6W3LUUOl
+hykOcsL+Dwbc7auVdhjHK49jpa4lNB/1rhuAn9oD5S9hBG+Zfl6rs8jyqIQszm51
+5b2BtPIt/YJY5YOtihluS2XVEqE4uVisFAcJ4ikG02W/OXUmY9SvEzwA3MH37yES
+oJmG6kvf11AH/9Pc4QMCDhvHsD0JfZ+I/ZCJmmQ1aumUu8xgnllgwHGemqVdUJBL
+fN5eobJRcgnDnWMUiUAy2YQiUQ5x/0E0Z7IVEzT6YbHGJAIE3wR40Vivr4f/nTAF
+takD7d5PmjrHQ14SgwhskL32WUtJUQ==
+-----END PRIVATE KEY-----

compose/testssl/regen.sh

@@ -0,0 +1,8 @@
+rm -- *.key *.crt *.csr
+openssl req -x509 -newkey rsa:4096 -sha256 -days 3650 -nodes -keyout localhost.key -out localhost.crt -subj "/CN=murfffi" -addext "subjectAltName=DNS:localhost"
+
+#with CA cart:
+#openssl req -x509 -newkey rsa:4096 -sha256 -days 3650 -nodes -keyout ca.key -out ca.crt -subj "/CN=murfffi"
+#openssl req -new -newkey rsa:4096 -sha256 -nodes -keyout localhost.key -out localhost.csr -subj "/CN=localhost" -addext "subjectAltName=DNS:localhost"
+#openssl x509 -req -in localhost.csr -CA ca.crt -CAkey ca.key \
+# -CAcreateserial -out localhost.crt -days 825 -sha256 -extfile localhost.ext

driver.go

@@ -160,7 +160,7 @@ func NewConnector(opts *Options) driver.Connector {
 
 // Connect implements driver.Connector
 func (c *connector) Connect(context.Context) (driver.Conn, error) {
-	// Strangely, TTransport.Open doesn't support context, so we don't use it here
+	// TTransport.Open doesn't support context. In general, Thrift almost always doesn't accept or ignores context.
 	return connect(c.opts)
 }
 
@@ -174,28 +174,25 @@ func connect(opts *Options) (*isql.Conn, error) {
 	addr := net.JoinHostPort(opts.Host, opts.Port)
 
 	var socket thrift.TTransport
+	var tlsConf *tls.Config
 	if opts.UseTLS {
 
-		if opts.CACertPath == "" {
-			return nil, errors.New("Please provide CA certificate path")
+		certPath := opts.CACertPath
+		if certPath == "" {
+			return nil, errors.New("impala: please provide CA certificate path")
 		}
 
-		caCert, certErr := os.ReadFile(opts.CACertPath)
-		if certErr != nil {
-			return nil, certErr
+		caCertPool, err := readCert(certPath)
+		if err != nil {
+			return nil, fmt.Errorf("impala: failed to read CA certificate: %w", err)
 		}
 
-		caCertPool := x509.NewCertPool()
-		caCertPool.AppendCertsFromPEM(caCert)
-
-		tlsConf := &tls.Config{
+		tlsConf = &tls.Config{
 			RootCAs: caCertPool,
 		}
-		socket = thrift.NewTSSLSocketConf(addr, &thrift.TConfiguration{
-			TLSConfig: tlsConf,
-		})
+		// Configuration applied in protocol below
+		socket = thrift.NewTSSLSocketConf(addr, &thrift.TConfiguration{})
 	} else {
-		// TODO SocketTimeout, ConnectTimeout Github #34
 		socket = thrift.NewTSocketConf(addr, &thrift.TConfiguration{})
 	}
 
@@ -225,12 +222,15 @@ func connect(opts *Options) (*isql.Conn, error) {
 	}
 
 	protocol := thrift.NewTBinaryProtocolConf(transport, &thrift.TConfiguration{
+		// The following configuration is propagated to Transport / Socket
 		TBinaryStrictRead:  lo.ToPtr(false),
 		TBinaryStrictWrite: lo.ToPtr(true),
+		TLSConfig:          tlsConf,
+		// TODO SocketTimeout, ConnectTimeout Github #34
 	})
 
 	if err := transport.Open(); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("impala: failed to open connection: %w", err)
 	}
 
 	logger := log.New(opts.LogOut, "impala: ", log.LstdFlags)
@@ -244,3 +244,17 @@ func connect(opts *Options) (*isql.Conn, error) {
 
 	return isql.NewConn(client, transport, logger), nil
 }
+
+func readCert(certPath string) (*x509.CertPool, error) {
+	caCert, certErr := os.ReadFile(certPath)
+	if certErr != nil {
+		return nil, certErr
+	}
+
+	caCertPool := x509.NewCertPool()
+	ok := caCertPool.AppendCertsFromPEM(caCert)
+	if !ok {
+		return nil, errors.New("failed to parse CA certificate")
+	}
+	return caCertPool, nil
+}

internal/isql/connection_test.go

@@ -8,6 +8,7 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"path/filepath"
 	"slices"
 	"testing"
 	"time"
@@ -34,12 +35,14 @@ func TestIntegration_FromEnv(t *testing.T) {
 	runSuite(t, dsn)
 }
 
+// TestIntegration_Impala3 covers integration with Impala 3.x and no TLS - plain TCP
 func TestIntegration_Impala3(t *testing.T) {
 	fi.SkipLongTest(t)
-	dsn := startImpala(t)
+	dsn := startImpala3(t)
 	runSuite(t, dsn)
 }
 
+// TestIntegration_Impala4 covers integration with Impala 4.x and TLS
 func TestIntegration_Impala4(t *testing.T) {
 	fi.SkipLongTest(t)
 	dsn := startImpala4(t)
@@ -206,7 +209,7 @@ func runImpala4SpecificTests(t *testing.T, dsn string) {
 	})
 }
 
-func startImpala(t *testing.T) string {
+func startImpala3(t *testing.T) string {
 	ctx := context.Background()
 	c := fi.NoError(Setup(ctx)).Require(t)
 	dsn := GetDsn(ctx, t, c)
@@ -233,7 +236,10 @@ func startImpala4(t *testing.T) string {
 	require.NoError(t, compose.WaitForService("impalad-1", waitRule).Up(ctx, tc.Wait(true)))
 	c, err := compose.ServiceContainer(ctx, "impalad-1")
 	require.NoError(t, err)
-	return GetDsn(ctx, t, c)
+	dsn := GetDsn(ctx, t, c)
+	certPath := filepath.Join("..", "..", "compose", "testssl", "localhost.crt")
+	dsn += "&tls=true&ca-cert=" + fi.NoError(filepath.Abs(certPath)).Require(t)
+	return dsn
 }
 
 func testPinger(t *testing.T, db *sql.DB) {