sqlproxyccl: support secure connections to SQL backends

    Previously, when establishing a TLS connection to the SQL backend,
    the sqlproxy failed to set .ServerName on the tls.Config. The
    result was the error `tls: either ServerName or InsecureSkipVerify
    must be specified in the tls.Config` whenever .SkipVerify was false.
    This behavior made it impossible to establish verified secure
    connections to SQL backends. This commit properly sets .ServerName
    based on the outgoingAddress returned by the tenantdir service.

Release note: None

Release justification: Having a verified TLS connection betweeen the
SQLProxy and SQL Pods in Cockroach Serverless is a requirement for the
beta release. This code change enables that secure connection without
making any changes to CockroachDB, itself.

pkg/ccl/sqlproxyccl/proxy_handler.go

@@ -264,11 +264,6 @@ func (handler *proxyHandler) handle(ctx context.Context, incomingConn *proxyConn
 		return err
 	}
 
-	var TLSConf *tls.Config
-	if !handler.Insecure {
-		TLSConf = &tls.Config{InsecureSkipVerify: handler.SkipVerify}
-	}
-
 	var crdbConn net.Conn
 	var outgoingAddress string
 
@@ -314,8 +309,34 @@ func (handler *proxyHandler) handle(ctx context.Context, incomingConn *proxyConn
 			break
 		}
 
+		// NB: TLS options for the proxy are split into Insecure and
+		// SkipVerify. In insecure mode, tlsConf is expected to be nil. This
+		// will cause BackendDial to skip TLS entirely. If SkipVerify is true,
+		// tlsConf will be set to a non-nil config with InsecureSkipVerify set
+		// to true.
+		var tlsConf *tls.Config
+		if !handler.Insecure {
+			outgoingHost, _, err := net.SplitHostPort(outgoingAddress)
+			if err != nil {
+				log.Errorf(ctx, "could not split outgoing address '%s' into host and port: %v", outgoingAddress, err.Error())
+				// Remap error for external consumption.
+				clientErr := newErrorf(
+					codeParamsRoutingFailed, "cluster %s-%d not found", clusterName, tenID.ToUint64())
+				updateMetricsAndSendErrToClient(clientErr, conn, handler.metrics)
+				return clientErr
+			}
+
+			tlsConf = &tls.Config{
+				// Always set ServerName, if SkipVerify is true, it will be
+				// ignored. When SkipVerify is false, it is required to
+				// establish a TLS connection.
+				ServerName:         outgoingHost,
+				InsecureSkipVerify: handler.SkipVerify,
+			}
+		}
+
 		// Now actually dial the backend server.
-		crdbConn, err = BackendDial(backendStartupMsg, outgoingAddress, TLSConf)
+		crdbConn, err = BackendDial(backendStartupMsg, outgoingAddress, tlsConf)
 
 		// If we get a backend down error, retry the connection.
 		var codeErr *codeError

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -101,7 +101,7 @@ func TestBackendDownRetry(t *testing.T) {
 
 	stopper := stop.NewStopper()
 	defer stopper.Stop(ctx)
-	_, addr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{RoutingRule: "undialable%$!@$"})
+	_, addr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{RoutingRule: "undialable%$!@$:1234"})
 
 	// Valid connection, but no backend server running.
 	pgurl := fmt.Sprintf("postgres://unused:unused@%s/db?options=--cluster=dim-dog-28&sslmode=require", addr)
@@ -228,6 +228,86 @@ func TestProxyAgainstSecureCRDB(t *testing.T) {
 	require.Equal(t, int64(2), s.metrics.AuthFailedCount.Count())
 }
 
+func TestProxyTLSConf(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	t.Run("insecure", func(t *testing.T) {
+		ctx := context.Background()
+		te := newTester()
+		defer te.Close()
+
+		defer testutils.TestingHook(&BackendDial, func(
+			_ *pgproto3.StartupMessage, _ string, tlsConf *tls.Config,
+		) (net.Conn, error) {
+			require.Nil(t, tlsConf)
+			return nil, newErrorf(codeParamsRoutingFailed, "boom")
+		})()
+
+		stopper := stop.NewStopper()
+		defer stopper.Stop(ctx)
+		_, addr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{
+			Insecure:    true,
+			RoutingRule: "{{clusterName}}-0.cockroachdb:26257",
+		})
+
+		pgurl := fmt.Sprintf("postgres://unused:unused@%s/%s?options=--cluster=dim-dog-28&sslmode=require", addr, "defaultdb")
+		te.TestConnectErr(ctx, t, pgurl, codeParamsRoutingFailed, "boom")
+	})
+
+	t.Run("skip-verify", func(t *testing.T) {
+		ctx := context.Background()
+		te := newTester()
+		defer te.Close()
+
+		defer testutils.TestingHook(&BackendDial, func(
+			_ *pgproto3.StartupMessage, _ string, tlsConf *tls.Config,
+		) (net.Conn, error) {
+			require.True(t, tlsConf.InsecureSkipVerify)
+			return nil, newErrorf(codeParamsRoutingFailed, "boom")
+		})()
+
+		stopper := stop.NewStopper()
+		defer stopper.Stop(ctx)
+		_, addr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{
+			Insecure:    false,
+			SkipVerify:  true,
+			RoutingRule: "{{clusterName}}-0.cockroachdb:26257",
+		})
+
+		pgurl := fmt.Sprintf("postgres://unused:unused@%s/%s?options=--cluster=dim-dog-28&sslmode=require", addr, "defaultdb")
+		te.TestConnectErr(ctx, t, pgurl, codeParamsRoutingFailed, "boom")
+	})
+
+	t.Run("no-skip-verify", func(t *testing.T) {
+		ctx := context.Background()
+		te := newTester()
+		defer te.Close()
+
+		defer testutils.TestingHook(&BackendDial, func(
+			_ *pgproto3.StartupMessage, outgoingAddress string, tlsConf *tls.Config,
+		) (net.Conn, error) {
+			outgoingHost, _, err := net.SplitHostPort(outgoingAddress)
+			require.NoError(t, err)
+
+			require.False(t, tlsConf.InsecureSkipVerify)
+			require.Equal(t, tlsConf.ServerName, outgoingHost)
+			return nil, newErrorf(codeParamsRoutingFailed, "boom")
+		})()
+
+		stopper := stop.NewStopper()
+		defer stopper.Stop(ctx)
+		_, addr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{
+			Insecure:    false,
+			SkipVerify:  false,
+			RoutingRule: "{{clusterName}}-0.cockroachdb:26257",
+		})
+
+		pgurl := fmt.Sprintf("postgres://unused:unused@%s/%s?options=--cluster=dim-dog-28&sslmode=require", addr, "defaultdb")
+		te.TestConnectErr(ctx, t, pgurl, codeParamsRoutingFailed, "boom")
+	})
+
+}
+
 func TestProxyTLSClose(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	// NB: The leaktest call is an important part of this test. We're
@@ -998,6 +1078,12 @@ func newProxyServer(
 	ln, err := net.Listen("tcp", listenAddress)
 	require.NoError(t, err)
 
+	// If routing rule is not specified, default to something that will
+	// return a non empty string with a port.
+	if opts.RoutingRule == "" {
+		opts.RoutingRule = "{{clusterName}}:1234"
+	}
+
 	server, err = NewServer(ctx, stopper, *opts)
 	require.NoError(t, err)
 

pkg/cli/cliflags/flags_mt.go

@@ -71,6 +71,8 @@ This rule must include the port of the SQL pod.`,
 		Description: "Directory address of the service doing resolution from backend id to IP.",
 	}
 
+	// TODO(chrisseto): Remove skip-verify as a CLI option. It should only be
+	// set internally for testing, rather than being exposed to consumers.
 	SkipVerify = FlagInfo{
 		Name:        "skip-verify",
 		Description: "If true, skip identity verification of backend. For testing only.",