server: add end-to-end tenant authentication test

pkg/base/test_server_args.go

@@ -218,4 +218,8 @@ type TestTenantArgs struct {
 	// AllowSettingClusterSettings, if true, allows the tenant to set in-memory
 	// cluster settings.
 	AllowSettingClusterSettings bool
+
+	// TenantIDCodecOverride overrides the tenant ID used to construct the SQL
+	// server's codec, but nothing else (e.g. its certs). Used for testing.
+	TenantIDCodecOverride roachpb.TenantID
 }

pkg/ccl/kvccl/kvtenantccl/proxy.go

@@ -25,14 +25,13 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
+	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil/singleflight"
 	"github.com/cockroachdb/errors"
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func init() {
@@ -297,7 +296,7 @@ func (p *Proxy) RangeLookup(
 		})
 		if err != nil {
 			log.Warningf(ctx, "error issuing RangeLookup RPC: %v", err)
-			if status.Code(err) == codes.Unauthenticated {
+			if grpcutil.IsAuthenticationError(err) {
 				// Authentication error. Propagate.
 				return nil, nil, err
 			}

pkg/ccl/serverccl/server_sql_test.go

@@ -77,3 +77,20 @@ func TestTenantCannotSetClusterSetting(t *testing.T) {
 	require.True(t, ok, "expected err to be a *pq.Error but is of type %T. error is: %v", err)
 	require.Equal(t, pq.ErrorCode(pgcode.InsufficientPrivilege.String()), pqErr.Code, "err %v has unexpected code", err)
 }
+
+func TestTenantUnauthenticatedAccess(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	ctx := context.Background()
+
+	tc := serverutils.StartTestCluster(t, 1, base.TestClusterArgs{})
+	defer tc.Stopper().Stop(ctx)
+
+	_, err := tc.Server(0).StartTenant(base.TestTenantArgs{
+		TenantID: roachpb.MakeTenantID(security.EmbeddedTenantIDs()[0]),
+		// Configure the SQL server to access the wrong tenant keyspace.
+		TenantIDCodecOverride: roachpb.MakeTenantID(security.EmbeddedTenantIDs()[1]),
+	})
+	require.Error(t, err)
+	require.Regexp(t, `Unauthenticated desc = requested key /Tenant/11/System/"system-version/" not fully contained in tenant keyspace /Tenant/1{0-1}`, err)
+}

pkg/kv/kvclient/kvcoord/dist_sender.go

@@ -1007,7 +1007,7 @@ func (ds *DistSender) detectIntentMissingDueToIntentResolution(
 		// We weren't able to determine whether the intent missing error is
 		// due to intent resolution or not, so it is still ambiguous whether
 		// the commit succeeded.
-		return false, roachpb.NewAmbiguousResultError(fmt.Sprintf("error=%s [intent missing]", pErr))
+		return false, roachpb.NewAmbiguousResultErrorf("error=%s [intent missing]", pErr)
 	}
 	respTxn := &br.Responses[0].GetQueryTxn().QueriedTxn
 	switch respTxn.Status {
@@ -1032,7 +1032,7 @@ func (ds *DistSender) detectIntentMissingDueToIntentResolution(
 		// to further isolates the ambiguity caused by the loss of information
 		// during intent resolution. If this error becomes a problem, we can explore
 		// this option.
-		return false, roachpb.NewAmbiguousResultError("intent missing and record aborted")
+		return false, roachpb.NewAmbiguousResultErrorf("intent missing and record aborted")
 	default:
 		// The transaction has not been finalized yet, so the missing intent
 		// error must have been caused by a real missing intent. Propagate the
@@ -1433,6 +1433,9 @@ func (ds *DistSender) sendPartialBatch(
 				// We set pErr if we encountered an error getting the descriptor in
 				// order to return the most recent error when we are out of retries.
 				pErr = roachpb.NewError(err)
+				if !isRangeLookupErrorRetryable(err) {
+					return response{pErr: roachpb.NewError(err)}
+				}
 				continue
 			}
 
@@ -1676,7 +1679,7 @@ func fillSkippedResponses(
 // the error that the last attempt to execute the request returned.
 func noMoreReplicasErr(ambiguousErr, lastAttemptErr error) error {
 	if ambiguousErr != nil {
-		return roachpb.NewAmbiguousResultError(fmt.Sprintf("error=%s [exhausted]", ambiguousErr))
+		return roachpb.NewAmbiguousResultErrorf("error=%s [exhausted]", ambiguousErr)
 	}
 
 	// TODO(bdarnell): The error from the last attempt is not necessarily the best
@@ -1805,6 +1808,14 @@ func (ds *DistSender) sendToReplicas(
 		br, err = transport.SendNext(ctx, ba)
 
 		if err != nil {
+			if grpcutil.IsAuthenticationError(err) {
+				// Authentication error. Propagate.
+				if ambiguousError != nil {
+					return nil, roachpb.NewAmbiguousResultErrorf("error=%s [propagate]", ambiguousError)
+				}
+				return nil, err
+			}
+
 			// For most connection errors, we cannot tell whether or not the request
 			// may have succeeded on the remote server (exceptions are captured in the
 			// grpcutil.RequestDidNotStart function). We'll retry the request in order
@@ -1940,7 +1951,7 @@ func (ds *DistSender) sendToReplicas(
 				}
 			default:
 				if ambiguousError != nil {
-					return nil, roachpb.NewAmbiguousResultError(fmt.Sprintf("error=%s [propagate]", ambiguousError))
+					return nil, roachpb.NewAmbiguousResultErrorf("error=%s [propagate]", ambiguousError)
 				}
 
 				// The error received is likely not specific to this
@@ -1957,7 +1968,7 @@ func (ds *DistSender) sendToReplicas(
 			reportedErr := errors.Wrap(ctx.Err(), "context done during DistSender.Send")
 			log.Eventf(ctx, "%v", reportedErr)
 			if ambiguousError != nil {
-				return nil, roachpb.NewAmbiguousResultError(reportedErr.Error())
+				return nil, roachpb.NewAmbiguousResultErrorf(reportedErr.Error())
 			}
 			// Don't consider this a sendError, because sendErrors indicate that we
 			// were unable to reach a replica that could serve the request, and they

pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
+	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -137,6 +138,9 @@ func (ds *DistSender) partialRangeFeed(
 			ri, err := ds.getRoutingInfo(ctx, rangeInfo.rs.Key, EvictionToken{}, false)
 			if err != nil {
 				log.VErrEventf(ctx, 1, "range descriptor re-lookup failed: %s", err)
+				if !isRangeLookupErrorRetryable(err) {
+					return err
+				}
 				continue
 			}
 			rangeInfo.token = ri
@@ -253,6 +257,10 @@ func (ds *DistSender) singleRangeFeed(
 		stream, err := client.RangeFeed(clientCtx, &args)
 		if err != nil {
 			log.VErrEventf(ctx, 2, "RPC error: %s", err)
+			if grpcutil.IsAuthenticationError(err) {
+				// Authentication error. Propagate.
+				return args.Timestamp, err
+			}
 			continue
 		}
 		for {

pkg/kv/kvclient/kvcoord/range_cache.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/cache"
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
+	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -1261,3 +1262,10 @@ func (e *rangeCacheEntry) updateLease(l *roachpb.Lease) (updated bool, newEntry
 		lease: *l,
 	}
 }
+
+// isRangeLookupErrorRetryable returns whether the provided range lookup error
+// can be retried or whether it should be propagated immediately.
+func isRangeLookupErrorRetryable(err error) bool {
+	// For now, all errors are retryable except authentication errors.
+	return !grpcutil.IsAuthenticationError(err)
+}

pkg/kv/kvclient/kvcoord/range_iter.go

@@ -183,6 +183,9 @@ func (ri *RangeIterator) Seek(ctx context.Context, key roachpb.RKey, scanDir Sca
 		// for before reaching this point.
 		if err != nil {
 			log.VEventf(ctx, 1, "range descriptor lookup failed: %s", err)
+			if !isRangeLookupErrorRetryable(err) {
+				break
+			}
 			continue
 		}
 		if log.V(2) {

pkg/roachpb/errors.go

@@ -461,6 +461,12 @@ func NewAmbiguousResultError(msg string) *AmbiguousResultError {
 	return &AmbiguousResultError{Message: msg}
 }
 
+// NewAmbiguousResultErrorf initializes a new AmbiguousResultError with
+// an explanatory format and set of arguments.
+func NewAmbiguousResultErrorf(format string, args ...interface{}) *AmbiguousResultError {
+	return NewAmbiguousResultError(fmt.Sprintf(format, args...))
+}
+
 func (e *AmbiguousResultError) Error() string {
 	return e.message(nil)
 }

pkg/rpc/context.go

@@ -710,14 +710,13 @@ func (ctx *Context) grpcDialOptions(
 	if ctx.Config.Insecure {
 		dialOpts = append(dialOpts, grpc.WithInsecure())
 	} else {
-		var err error
 		var tlsConfig *tls.Config
+		var err error
 		if ctx.tenID == roachpb.SystemTenantID {
 			tlsConfig, err = ctx.GetClientTLSConfig()
 		} else {
 			tlsConfig, err = ctx.GetTenantClientTLSConfig()
 		}
-
 		if err != nil {
 			return nil, err
 		}

pkg/server/config.go

@@ -332,6 +332,12 @@ type SQLConfig struct {
 	//
 	// Only applies when the SQL server is deployed individually.
 	TenantKVAddrs []string
+
+	// TenantIDCodecOverride overrides the tenant ID used to construct the SQL
+	// server's codec, but nothing else (e.g. its certs). Used for testing.
+	//
+	// Only applies when the SQL server is deployed individually.
+	TenantIDCodecOverride roachpb.TenantID
 }
 
 // MakeSQLConfig returns a SQLConfig with default values.

pkg/server/server_sql.go

@@ -187,6 +187,9 @@ type sqlServerArgs struct {
 func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) {
 	execCfg := &sql.ExecutorConfig{}
 	codec := keys.MakeSQLCodec(cfg.SQLConfig.TenantID)
+	if override := cfg.SQLConfig.TenantIDCodecOverride; override != (roachpb.TenantID{}) {
+		codec = keys.MakeSQLCodec(override)
+	}
 
 	// Create blob service for inter-node file sharing.
 	blobService, err := blobs.NewBlobService(cfg.Settings.ExternalIODir)

pkg/server/testserver.go

@@ -616,13 +616,14 @@ func (ts *TestServer) StartTenant(params base.TestTenantArgs) (pgAddr string, _
 
 	st := cluster.MakeTestingClusterSettings()
 	sqlCfg := makeTestSQLConfig(st, params.TenantID)
+	sqlCfg.TenantKVAddrs = []string{ts.ServingTenantAddr()}
+	sqlCfg.TenantIDCodecOverride = params.TenantIDCodecOverride
 	baseCfg := makeTestBaseConfig(st)
 	if params.AllowSettingClusterSettings {
 		baseCfg.TestingKnobs.TenantTestingKnobs = &sql.TenantTestingKnobs{
 			ClusterSettingsUpdater: st.MakeUpdater(),
 		}
 	}
-	sqlCfg.TenantKVAddrs = []string{ts.ServingTenantAddr()}
 	return StartTenant(
 		ctx,
 		ts.Stopper(),

pkg/util/grpcutil/grpc_util.go

@@ -88,6 +88,15 @@ func IsClosedConnection(err error) bool {
 	return netutil.IsClosedConnection(err)
 }
 
+// IsAuthenticationError returns true if err's Cause is an error produced by
+// gRPC due to invalid authentication credentials for the operation.
+func IsAuthenticationError(err error) bool {
+	if s, ok := status.FromError(errors.UnwrapAll(err)); ok {
+		return s.Code() == codes.Unauthenticated
+	}
+	return false
+}
+
 // RequestDidNotStart returns true if the given error from gRPC
 // means that the request definitely could not have started on the
 // remote server.