release-21.2: sqlproxyccl: support secure connections to SQL backends

pkg/ccl/sqlproxyccl/BUILD.bazel

@@ -28,6 +28,7 @@ go_library(
         "//pkg/util/httputil",
         "//pkg/util/log",
         "//pkg/util/metric",
+        "//pkg/util/netutil/addr",
         "//pkg/util/retry",
         "//pkg/util/stop",
         "//pkg/util/syncutil",
@@ -72,6 +73,7 @@ go_test(
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",
         "//pkg/util/log",
+        "//pkg/util/netutil/addr",
         "//pkg/util/randutil",
         "//pkg/util/stop",
         "//pkg/util/syncutil",

pkg/ccl/sqlproxyccl/proxy_handler.go

@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/certmgr"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -228,7 +229,9 @@ func (handler *proxyHandler) handle(ctx context.Context, incomingConn *proxyConn
 	ctx = logtags.AddTag(ctx, "cluster", clusterName)
 	ctx = logtags.AddTag(ctx, "tenant", tenID)
 
-	ipAddr, _, err := net.SplitHostPort(conn.RemoteAddr().String())
+	// Use an empty string as the default port as we only care about the
+	// correctly parsing the IP address here.
+	ipAddr, _, err := addr.SplitHostPort(conn.RemoteAddr().String(), "")
 	if err != nil {
 		clientErr := newErrorf(codeParamsRoutingFailed, "unexpected connection address")
 		log.Errorf(ctx, "could not parse address: %v", err.Error())
@@ -264,11 +267,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 +312,37 @@ 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. InsecureSkipVerify will provide an encrypted connection but
+		// not verify that the connection recipient is a trusted party.
+		var tlsConf *tls.Config
+		if !handler.Insecure {
+			// Use an empty string as the default port as we only care about the
+			// correctly parsing the outgoingHost/IP here.
+			outgoingHost, _, err := addr.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

@@ -34,6 +34,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -101,7 +102,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)
@@ -118,12 +119,12 @@ func TestFailedConnection(t *testing.T) {
 
 	stopper := stop.NewStopper()
 	defer stopper.Stop(ctx)
-	s, addr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{RoutingRule: "undialable%$!@$"})
+	s, proxyAddr := newSecureProxyServer(ctx, t, stopper, &ProxyOptions{RoutingRule: "undialable%$!@$"})
 
 	// TODO(asubiotto): consider using datadriven for these, especially if the
 	// proxy becomes more complex.
 
-	_, p, err := net.SplitHostPort(addr)
+	_, p, err := addr.SplitHostPort(proxyAddr, "")
 	require.NoError(t, err)
 	u := fmt.Sprintf("postgres://unused:unused@localhost:%s/", p)
 
@@ -228,6 +229,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 := addr.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

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.",