From b94d0a188515314b28608669e6332fa11d3a91b3 Mon Sep 17 00:00:00 2001 From: Chris Seto Date: Thu, 9 Sep 2021 14:13:31 +0000 Subject: [PATCH] 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 | 33 +++++++-- pkg/ccl/sqlproxyccl/proxy_handler_test.go | 88 ++++++++++++++++++++++- pkg/cli/cliflags/flags_mt.go | 2 + 3 files changed, 116 insertions(+), 7 deletions(-) diff --git a/pkg/ccl/sqlproxyccl/proxy_handler.go b/pkg/ccl/sqlproxyccl/proxy_handler.go index 7f53ebc2a2e7..17f9d5c34d03 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler.go +++ b/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 diff --git a/pkg/ccl/sqlproxyccl/proxy_handler_test.go b/pkg/ccl/sqlproxyccl/proxy_handler_test.go index 06747e28dcaa..2a4c27ebeffd 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler_test.go +++ b/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) diff --git a/pkg/cli/cliflags/flags_mt.go b/pkg/cli/cliflags/flags_mt.go index 830fe381741b..e611bf3c7f92 100644 --- a/pkg/cli/cliflags/flags_mt.go +++ b/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.",