Skip to content

Commit

Permalink
sqlproxyccl: minor fixes around the proxy handler
Browse files Browse the repository at this point in the history 
In cockroachdb#65164, we migrated the sqlproxy in the CC code to the DB repository, and
there were a few buglets:
- sqlproxy crashes when the tenant ID supplied in the connection string is 0
- sqlproxy leaks internal parsing errors to the client

This patch hides internal parsing errors, and replaces them with friendly
user-facing errors (e.g. "Invalid cluster name"). We also add a bounds check
to the parsed tenant ID so that the process does not crash on an invalid
tenant ID. More tests were added as well.

Release note: None
  • Loading branch information
@jaylim-crl
jaylim-crl committed Jun 13, 2021
1 parent a7c486d commit 4834195
Show file tree
Hide file tree
Showing 2 changed files with 228 additions and 38 deletions.
18 changes: 11 additions & 7 deletions pkg/ccl/sqlproxyccl/proxy_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,8 @@ func (handler *proxyHandler) handle(ctx context.Context, proxyConn *conn) error
// we should be careful with the details that we want to expose.
backendStartupMsg, clusterName, tenID, err := clusterNameAndTenantFromParams(msg)
if err != nil {
clientErr := &codeError{codeParamsRoutingFailed, err}
log.Errorf(ctx, "unable to extract cluster name and tenant id: %s", clientErr.Error())
clientErr := newErrorf(codeParamsRoutingFailed, "Invalid cluster name")
log.Errorf(ctx, "unable to extract cluster name and tenant id: %s", err.Error())
updateMetricsAndSendErrToClient(clientErr, conn, handler.metrics)
return clientErr
}
Expand All @@ -212,8 +212,8 @@ func (handler *proxyHandler) handle(ctx context.Context, proxyConn *conn) error

ipAddr, _, err := net.SplitHostPort(conn.RemoteAddr().String())
if err != nil {
clientErr := &codeError{codeParamsRoutingFailed, err}
log.Errorf(ctx, "could not parse address: %v", clientErr.Error())
clientErr := newErrorf(codeParamsRoutingFailed, "Unexpected connection address")
log.Errorf(ctx, "could not parse address: %v", err.Error())
updateMetricsAndSendErrToClient(clientErr, conn, handler.metrics)
return clientErr
}
Expand Down Expand Up @@ -502,12 +502,13 @@ func clusterNameAndTenantFromParams(
}

sepIdx := strings.LastIndex(clusterNameFromDB, clusterTenantSep)

// Cluster name provided without a tenant ID in the end.
if sepIdx == -1 || sepIdx == len(clusterNameFromDB)-1 {
return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name %s", clusterNameFromDB)
return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name '%s'", clusterNameFromDB)
}
clusterNameSansTenant, tenantIDStr := clusterNameFromDB[:sepIdx], clusterNameFromDB[sepIdx+1:]

clusterNameSansTenant, tenantIDStr := clusterNameFromDB[:sepIdx], clusterNameFromDB[sepIdx+1:]
if !clusterNameRegex.MatchString(clusterNameSansTenant) {
return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name '%s'", clusterNameFromDB)
}
Expand All @@ -516,6 +517,9 @@ func clusterNameAndTenantFromParams(
if err != nil {
return msg, "", roachpb.MaxTenantID, errors.Wrapf(err, "cannot parse %s as uint64", tenantIDStr)
}
if tenID < roachpb.MinTenantID.ToUint64() {
return msg, "", roachpb.MaxTenantID, errors.Newf("cannot parse %s as a valid tenant ID", tenantIDStr)
}

// Make and return a copy of the startup msg so the original is not modified.
paramsOut := map[string]string{}
Expand Down Expand Up @@ -545,7 +549,7 @@ func parseDatabaseParam(databaseParam string) (clusterName, databaseName string,
return "", "", nil
}

parts := strings.SplitN(databaseParam, ".", 2)
parts := strings.Split(databaseParam, ".")

// Database param provided without cluster name.
if len(parts) <= 1 {
Expand Down
248 changes: 217 additions & 31 deletions pkg/ccl/sqlproxyccl/proxy_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func newSecureProxyServer(
const _ = `
openssl genrsa -out testdata/testserver.key 2048
openssl req -new -x509 -sha256 -key testdata/testserver.key -out testdata/testserver.crt \
-days 3650 -config testdata/testserver_config.cnf
-days 3650 -config testdata/testserver_config.cnf
`
opts.ListenKey = "testdata/testserver.key"
opts.ListenCert = "testdata/testserver.crt"
Expand Down Expand Up @@ -221,18 +221,27 @@ func TestFailedConnection(t *testing.T) {
)
}
require.Equal(t, int64(0), s.metrics.RoutingErrCount.Count())

// TenantID rejected as malformed.
ac.assertConnectErr(
ctx, t, u, "?options=--cluster=dim&sslmode=require",
codeParamsRoutingFailed, "invalid cluster name",
ctx, t, u, "?options=--cluster=dimdog&sslmode=require",
codeParamsRoutingFailed, "Invalid cluster name",
)
require.Equal(t, int64(1), s.metrics.RoutingErrCount.Count())
// No TenantID.

// No cluster name and TenantID.
ac.assertConnectErr(
ctx, t, u, "?sslmode=require",
codeParamsRoutingFailed, "missing cluster name",
codeParamsRoutingFailed, "Invalid cluster name",
)
require.Equal(t, int64(2), s.metrics.RoutingErrCount.Count())

// Bad TenantID. Ensure that we don't leak any parsing errors.
ac.assertConnectErr(
ctx, t, u, "?options=--cluster=dim-dog-foo3&sslmode=require",
codeParamsRoutingFailed, "Invalid cluster name",
)
require.Equal(t, int64(3), s.metrics.RoutingErrCount.Count())
}

func TestUnexpectedError(t *testing.T) {
Expand Down Expand Up @@ -449,32 +458,6 @@ func TestInsecureProxy(t *testing.T) {
require.NoError(t, runTestQuery(ctx, conn))
}

func TestInsecureDoubleProxy(t *testing.T) {
defer leaktest.AfterTest(t)()

ctx := context.Background()

sql, _, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: true})
defer sql.Stopper().Stop(ctx)
sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)

// Test multiple proxies: proxyB -> proxyA -> tc
_, proxyA := newProxyServer(ctx, t, sql.Stopper(),
&ProxyOptions{RoutingRule: sql.ServingSQLAddr(), Insecure: true},
)
_, proxyB := newProxyServer(ctx, t, sql.Stopper(),
&ProxyOptions{RoutingRule: proxyA, Insecure: true},
)

u := fmt.Sprintf("postgres://root:admin@%s/dim-dog-28.dim-dog-29.defaultdb?sslmode=disable", proxyB)
conn, err := pgx.Connect(ctx, u)
require.NoError(t, err)
defer func() {
require.NoError(t, conn.Close(ctx))
}()
require.NoError(t, runTestQuery(ctx, conn))
}

func TestErroneousFrontend(t *testing.T) {
defer leaktest.AfterTest(t)()

Expand Down Expand Up @@ -748,3 +731,206 @@ func TestDirectoryReconnect(t *testing.T) {
return err == nil
}, 1000*time.Second, 100*time.Millisecond)
}

func TestClusterNameAndTenantFromParams(t *testing.T) {
defer leaktest.AfterTest(t)()

testCases := []struct {
name string
params map[string]string
expectedClusterName string
expectedTenantID uint64
expectedParams map[string]string
expectedError string
}{
{
name: "empty params",
params: map[string]string{},
expectedError: "missing cluster name in connection string",
},
{
name: "cluster name is not provided",
params: map[string]string{
"database": "defaultdb",
"options": "--foo=bar",
},
expectedError: "missing cluster name in connection string",
},
{
name: "multiple similar cluster names",
params: map[string]string{
"database": "happy-koala-7.defaultdb",
"options": "--cluster=happy-koala",
},
expectedError: "multiple cluster names provided",
},
{
name: "multiple different cluster names",
params: map[string]string{
"database": "happy-koala-7.defaultdb",
"options": "--cluster=happy-tiger",
},
expectedError: "multiple cluster names provided",
},
{
name: "invalid cluster name in database param",
params: map[string]string{
// Cluster names need to be between 6 to 20 alphanumeric characters.
"database": "short-0.defaultdb",
},
expectedError: "invalid cluster name 'short-0'",
},
{
name: "invalid cluster name in options param",
params: map[string]string{
// Cluster names need to be between 6 to 20 alphanumeric characters.
"options": "--cluster=cockroachlabsdotcomfoobarbaz-0",
},
expectedError: "invalid cluster name 'cockroachlabsdotcomfoobarbaz-0'",
},
{
name: "invalid database param (1)",
params: map[string]string{"database": "."},
expectedError: "invalid database param",
},
{
name: "invalid database param (2)",
params: map[string]string{"database": "a."},
expectedError: "invalid database param",
},
{
name: "invalid database param (3)",
params: map[string]string{"database": ".b"},
expectedError: "invalid database param",
},
{
name: "invalid database param (4)",
params: map[string]string{"database": "a.b.c"},
expectedError: "invalid database param",
},
{
name: "multiple cluster flags",
params: map[string]string{
"database": "hello-world.defaultdb",
"options": "--cluster=foobar --cluster=barbaz --cluster=testbaz",
},
expectedError: "multiple cluster flags provided",
},
{
name: "invalid cluster flag",
params: map[string]string{
"database": "hello-world.defaultdb",
"options": "--cluster=",
},
expectedError: "invalid cluster flag",
},
{
name: "no tenant id",
params: map[string]string{"database": "happy2koala.defaultdb"},
expectedError: "invalid cluster name 'happy2koala'",
},
{
name: "missing tenant id",
params: map[string]string{"database": "happy2koala-.defaultdb"},
expectedError: "invalid cluster name 'happy2koala-'",
},
{
name: "missing cluster name",
params: map[string]string{"database": "-7.defaultdb"},
expectedError: "invalid cluster name '-7'",
},
{
name: "bad tenant id",
params: map[string]string{"database": "happy-koala-0-7a.defaultdb"},
expectedError: `cannot parse 7a as uint64: strconv.ParseUint: parsing "7a": invalid syntax`,
},
{
name: "zero tenant id",
params: map[string]string{"database": "happy-koala-0.defaultdb"},
expectedError: `cannot parse 0 as a valid tenant ID`,
},
{
name: "cluster name in database param",
params: map[string]string{
"database": "happy-koala-7.defaultdb",
"foo": "bar",
},
expectedClusterName: "happy-koala",
expectedTenantID: 7,
expectedParams: map[string]string{"database": "defaultdb", "foo": "bar"},
},
{
name: "valid cluster name with invalid arrangements",
params: map[string]string{
"database": "defaultdb",
"options": "-c --cluster=happy-koala-7 -c -c -c",
},
expectedClusterName: "happy-koala",
expectedTenantID: 7,
expectedParams: map[string]string{"database": "defaultdb"},
},
{
name: "short option: cluster name in options param",
params: map[string]string{
"database": "defaultdb",
"options": "-ccluster=happy-koala-7",
},
expectedClusterName: "happy-koala",
expectedTenantID: 7,
expectedParams: map[string]string{"database": "defaultdb"},
},
{
name: "short option with spaces: cluster name in options param",
params: map[string]string{
"database": "defaultdb",
"options": "-c cluster=happy-koala-7",
},
expectedClusterName: "happy-koala",
expectedTenantID: 7,
expectedParams: map[string]string{"database": "defaultdb"},
},
{
name: "long option: cluster name in options param",
params: map[string]string{
"database": "defaultdb",
"options": "--cluster=happy-koala-7\t--foo=test",
},
expectedClusterName: "happy-koala",
expectedTenantID: 7,
expectedParams: map[string]string{"database": "defaultdb"},
},
{
name: "leading 0s are ok",
params: map[string]string{"database": "happy-koala-0-07.defaultdb"},
expectedClusterName: "happy-koala-0",
expectedTenantID: 7,
expectedParams: map[string]string{"database": "defaultdb"},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
msg := &pgproto3.StartupMessage{Parameters: tc.params}

originalParams := make(map[string]string)
for k, v := range msg.Parameters {
originalParams[k] = v
}

outMsg, clusterName, tenantID, err := clusterNameAndTenantFromParams(msg)
if tc.expectedError == "" {
require.NoErrorf(t, err, "failed test case\n%+v", tc)

// When expectedError is specified, we always have a valid expectedTenantID.
require.Equal(t, roachpb.MakeTenantID(tc.expectedTenantID), tenantID)

require.Equal(t, tc.expectedClusterName, clusterName)
require.Equal(t, tc.expectedParams, outMsg.Parameters)
} else {
require.EqualErrorf(t, err, tc.expectedError, "failed test case\n%+v", tc)
}

// Check that the original parameters were not modified.
require.Equal(t, originalParams, msg.Parameters)
})
}
}

0 comments on commit 4834195

Please sign in to comment.