Don't ignore TLS config's InsecureSkipVerify

tls.Config by default verifies certificates, gocql.SslOptions does
not. If one only provided the tls.Config with InsecureSkipVerify=false,
callers expect that the host will be verified, but gocql resets the
InsecureSkipVerify to true.

It's safer to explicitly disable host verification than to explicitly
enable it, so if the tls.Config is provided, let's honor it's
security settings.

If a TLS config with InsecureSkipVerify=true is provided at the same
time as EnableHostVerification=true is provided, this is a conflict
in settings. We could either return an error or fall back to verify
the host. We chose to verify the host.

The issue is in gocql codebase since commit
6495810, when the tls.Config was
embedded to SslOptions struct.

conn.go

@@ -80,6 +80,19 @@ func (p PasswordAuthenticator) Success(data []byte) error {
 	return nil
 }
 
+// SslOptions configures TLS use.
+//
+// Warning: Due to historical reasons, the SslOptions is insecure by default, so you need to set EnableHostVerification
+// to true if no Config is set. Most users should set SslOptions.Config to a *tls.Config.
+// SslOptions and Config.InsecureSkipVerify interact as follows:
+//
+//  Config.InsecureSkipVerify | EnableHostVerification | Result
+//  Config is nil             | false                  | do not verify host
+//  Config is nil             | true                   | verify host
+//  false                     | false                  | verify host
+//  true                      | false                  | do not verify host
+//  false                     | true                   | verify host
+//  true                      | true                   | verify host
 type SslOptions struct {
 	*tls.Config
 
@@ -89,9 +102,12 @@ type SslOptions struct {
 	CertPath string
 	KeyPath  string
 	CaPath   string //optional depending on server config
-	// If you want to verify the hostname and server cert (like a wildcard for cass cluster) then you should turn this on
-	// This option is basically the inverse of InSecureSkipVerify
-	// See InSecureSkipVerify in http://golang.org/pkg/crypto/tls/ for more info
+	// If you want to verify the hostname and server cert (like a wildcard for cass cluster) then you should turn this
+	// on.
+	// This option is basically the inverse of tls.Config.InsecureSkipVerify.
+	// See InsecureSkipVerify in http://golang.org/pkg/crypto/tls/ for more info.
+	//
+	// See SslOptions documentation to see how EnableHostVerification interacts with the provided tls.Config.
 	EnableHostVerification bool
 }
 

connectionpool.go

@@ -28,22 +28,39 @@ type SetPartitioner interface {
 }
 
 func setupTLSConfig(sslOpts *SslOptions) (*tls.Config, error) {
+	//  Config.InsecureSkipVerify | EnableHostVerification | Result
+	//  Config is nil             | true                   | verify host
+	//  Config is nil             | false                  | do not verify host
+	//  false                     | false                  | verify host
+	//  true                      | false                  | do not verify host
+	//  false                     | true                   | verify host
+	//  true                      | true                   | verify host
+	var tlsConfig *tls.Config
 	if sslOpts.Config == nil {
-		sslOpts.Config = &tls.Config{}
+		tlsConfig = &tls.Config{
+			InsecureSkipVerify: !sslOpts.EnableHostVerification,
+		}
+	} else {
+		// use clone to avoid race.
+		tlsConfig = sslOpts.Config.Clone()
+	}
+
+	if tlsConfig.InsecureSkipVerify && sslOpts.EnableHostVerification {
+		tlsConfig.InsecureSkipVerify = false
 	}
 
 	// ca cert is optional
 	if sslOpts.CaPath != "" {
-		if sslOpts.RootCAs == nil {
-			sslOpts.RootCAs = x509.NewCertPool()
+		if tlsConfig.RootCAs == nil {
+			tlsConfig.RootCAs = x509.NewCertPool()
 		}
 
 		pem, err := ioutil.ReadFile(sslOpts.CaPath)
 		if err != nil {
 			return nil, fmt.Errorf("connectionpool: unable to open CA certs: %v", err)
 		}
 
-		if !sslOpts.RootCAs.AppendCertsFromPEM(pem) {
+		if !tlsConfig.RootCAs.AppendCertsFromPEM(pem) {
 			return nil, errors.New("connectionpool: failed parsing or CA certs")
 		}
 	}
@@ -53,13 +70,10 @@ func setupTLSConfig(sslOpts *SslOptions) (*tls.Config, error) {
 		if err != nil {
 			return nil, fmt.Errorf("connectionpool: unable to load X509 key pair: %v", err)
 		}
-		sslOpts.Certificates = append(sslOpts.Certificates, mycert)
+		tlsConfig.Certificates = append(tlsConfig.Certificates, mycert)
 	}
 
-	sslOpts.InsecureSkipVerify = !sslOpts.EnableHostVerification
-
-	// return clone to avoid race
-	return sslOpts.Config.Clone(), nil
+	return tlsConfig, nil
 }
 
 type policyConnPool struct {

connectionpool_test.go

@@ -0,0 +1,85 @@
+// +build all unit
+
+package gocql
+
+import (
+	"crypto/tls"
+	"testing"
+)
+
+func TestSetupTLSConfig(t *testing.T) {
+	tests := []struct {
+		name string
+		opts *SslOptions
+		expectedInsecureSkipVerify bool
+	}{
+		{
+			name: "Config nil, EnableHostVerification false",
+			opts: &SslOptions{
+				EnableHostVerification: false,
+			},
+			expectedInsecureSkipVerify: true,
+		},
+		{
+			name: "Config nil, EnableHostVerification true",
+			opts: &SslOptions{
+				EnableHostVerification: true,
+			},
+			expectedInsecureSkipVerify: false,
+		},
+		{
+			name: "Config.InsecureSkipVerify false, EnableHostVerification false",
+			opts: &SslOptions{
+				EnableHostVerification: false,
+				Config: &tls.Config{
+					InsecureSkipVerify: false,
+				},
+			},
+			expectedInsecureSkipVerify: false,
+		},
+		{
+			name: "Config.InsecureSkipVerify true, EnableHostVerification false",
+			opts: &SslOptions{
+				EnableHostVerification: false,
+				Config: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedInsecureSkipVerify: true,
+		},
+		{
+			name: "Config.InsecureSkipVerify false, EnableHostVerification true",
+			opts: &SslOptions{
+				EnableHostVerification: true,
+				Config: &tls.Config{
+					InsecureSkipVerify: false,
+				},
+			},
+			expectedInsecureSkipVerify: false,
+		},
+		{
+			name: "Config.InsecureSkipVerify true, EnableHostVerification true",
+			opts: &SslOptions{
+				EnableHostVerification: true,
+				Config: &tls.Config{
+					InsecureSkipVerify: true,
+				},
+			},
+			expectedInsecureSkipVerify: false,
+		},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			tlsConfig, err := setupTLSConfig(test.opts)
+			if err != nil {
+				t.Fatalf("unexpected error %q", err.Error())
+			}
+			if tlsConfig.InsecureSkipVerify != test.expectedInsecureSkipVerify {
+				t.Fatalf("got %v, but expected %v", tlsConfig.InsecureSkipVerify,
+					test.expectedInsecureSkipVerify)
+			}
+		})
+	}
+}
+

doc.go

@@ -65,9 +65,19 @@
 // To use TLS, set the ClusterConfig.SslOpts field. SslOptions embeds *tls.Config so you can set that directly.
 // There are also helpers to load keys/certificates from files.
 //
-// Warning: Due to backward-compatibility reasons, the tls.Config's InsecureSkipVerify is set to
-// !SslOptions.EnableHostVerification, so by default host verification is disabled. Most users using TLS should set
-// SslOptions.EnableHostVerification to true.
+// Warning: Due to historical reasons, the SslOptions is insecure by default, so you need to set EnableHostVerification
+// to true if no Config is set. Most users should set SslOptions.Config to a *tls.Config.
+// SslOptions and Config.InsecureSkipVerify interact as follows:
+//
+//  Config.InsecureSkipVerify | EnableHostVerification | Result
+//  Config is nil             | false                  | do not verify host
+//  Config is nil             | true                   | verify host
+//  false                     | false                  | verify host
+//  true                      | false                  | do not verify host
+//  false                     | true                   | verify host
+//  true                      | true                   | verify host
+//
+// For example:
 //
 //  cluster := gocql.NewCluster("192.168.1.1", "192.168.1.2", "192.168.1.3")
 //  cluster.SslOpts = &gocql.SslOptions{