cli: don't scope TLS client certs to a specific tenant by default

This commit changes the default for `--tenant-scope` from "only the
system tenant" to "cert valid for all tenants".

Note that the scoping is generally useful for security, and it is used
in CockroachCloud. However, CockroachCloud does not use our CLI code
to generate certs and sets its cert tenant scopes on its own.

Given that our CLI code is provided for convenience and developer
productivity, and we don't expect certs generated here to be used in
multi-tenant deployments where tenants are adversarial to each other,
defaulting to certs that are valid on every tenant is a good choice.

Release note: None

pkg/cli/cliflags/flags.go

@@ -793,9 +793,9 @@ Note: that --external-io-disable-http or --external-io-disable-implicit-credenti
 	TenantScope = FlagInfo{
 		Name: "tenant-scope",
 		Description: `Assign a tenant scope to the certificate.
-This will allow for the certificate to only be used specifically for a particular
-tenant. This flag is optional, when omitted, the certificate is scoped to the
-system tenant.`,
+This will restrict the certificate to only be valid for the specified tenants.
+This flag is optional. When omitted, the certificate is not scoped; i.e.
+it can be used with all tenants.`,
 	}
 
 	GeneratePKCS8Key = FlagInfo{

pkg/cli/context.go

@@ -265,7 +265,20 @@ func setCertContextDefaults() {
 	certCtx.generatePKCS8Key = false
 	certCtx.disableUsernameValidation = false
 	certCtx.certPrincipalMap = nil
-	certCtx.tenantScope = []roachpb.TenantID{roachpb.SystemTenantID}
+	// Note: we set tenantScope to nil so that by default, client certs
+	// are not scoped to a specific tenant and can be used to connect to
+	// any tenant.
+	//
+	// Note that the scoping is generally useful for security, and it is
+	// used in CockroachCloud. However, CockroachCloud does not use our
+	// CLI code to generate certs and sets its tenant scopes on its own.
+	//
+	// Given that our CLI code is provided for convenience and developer
+	// productivity, and we don't expect certs generated here to be used
+	// in multi-tenant deployments where tenants are adversarial to each
+	// other, defaulting to certs that are valid on every tenant is a
+	// good choice.
+	certCtx.tenantScope = nil
 }
 
 var sqlExecCtx = clisqlexec.Context{

pkg/cli/flags.go

@@ -580,7 +580,6 @@ func init() {
 		cliflagcfg.StringFlag(f, &certCtx.caKey, cliflags.CAKey)
 		cliflagcfg.IntFlag(f, &certCtx.keySize, cliflags.KeySize)
 		cliflagcfg.BoolFlag(f, &certCtx.overwriteFiles, cliflags.OverwriteFiles)
-		cliflagcfg.VarFlag(f, &tenantIDSetter{tenantIDs: &certCtx.tenantScope}, cliflags.TenantScope)
 
 		if strings.HasSuffix(cmd.Name(), "-ca") {
 			// CA-only commands.
@@ -596,6 +595,8 @@ func init() {
 		}
 
 		if cmd == createClientCertCmd {
+			cliflagcfg.VarFlag(f, &tenantIDSetter{tenantIDs: &certCtx.tenantScope}, cliflags.TenantScope)
+
 			// PKCS8 key format is only available for the client cert command.
 			cliflagcfg.BoolFlag(f, &certCtx.generatePKCS8Key, cliflags.GeneratePKCS8Key)
 			cliflagcfg.BoolFlag(f, &certCtx.disableUsernameValidation, cliflags.DisableUsernameValidation)

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -205,7 +205,6 @@ go_library(
         "//pkg/roachprod/install",
         "//pkg/roachprod/logger",
         "//pkg/roachprod/prometheus",
-        "//pkg/security/username",
         "//pkg/server",
         "//pkg/server/serverpb",
         "//pkg/sql",

pkg/cmd/roachtest/tests/multitenant_utils.go

@@ -26,7 +26,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/config"
-	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/stretchr/testify/require"
@@ -100,26 +99,10 @@ func createTenantNode(
 		node:       node,
 		sqlPort:    sqlPort,
 	}
-	if tn.cockroachBinSupportsTenantScope(ctx, c) {
-		err := tn.recreateClientCertsWithTenantScope(ctx, c, createOptions.otherTenantIDs)
-		require.NoError(t, err)
-	}
 	tn.createTenantCert(ctx, t, c, createOptions.certNodes)
 	return tn
 }
 
-// cockroachBinSupportsTenantScope is a hack to figure out if the version of
-// cockroach on the node supports tenant scoped certificates. We can't use a
-// version comparison here because we need to compare alpha build versions which
-// are compared lexicographically. This is a problem because our alpha versions
-// contain an integer count of commits, which does not sort correctly.  Once
-// this feature ships in a release, it will be easier to do a version comparison
-// on whether this command line flag is supported.
-func (tn *tenantNode) cockroachBinSupportsTenantScope(ctx context.Context, c cluster.Cluster) bool {
-	err := c.RunE(ctx, c.Node(tn.node), "./cockroach cert create-client --help | grep '\\--tenant-scope'")
-	return err == nil
-}
-
 func (tn *tenantNode) createTenantCert(
 	ctx context.Context, t test.Test, c cluster.Cluster, certNodes option.NodeListOption,
 ) {
@@ -145,23 +128,6 @@ func (tn *tenantNode) createTenantCert(
 	c.Run(ctx, c.Node(tn.node), cmd)
 }
 
-func (tn *tenantNode) recreateClientCertsWithTenantScope(
-	ctx context.Context, c cluster.Cluster, otherIDs []int,
-) error {
-	tenantArgs := fmt.Sprintf("1,%d", tn.tenantID)
-	for _, id := range otherIDs {
-		tenantArgs = fmt.Sprintf("%s,%d", tenantArgs, id)
-	}
-
-	for _, user := range []username.SQLUsername{username.RootUserName(), username.TestUserName()} {
-		cmd := fmt.Sprintf(
-			"./cockroach cert create-client %s --certs-dir=certs --ca-key=certs/ca.key --tenant-scope %s --overwrite",
-			user.Normalized(), tenantArgs)
-		c.Run(ctx, c.Node(tn.node), cmd)
-	}
-	return c.RefetchCertsFromNode(ctx, tn.node)
-}
-
 func (tn *tenantNode) stop(ctx context.Context, t test.Test, c cluster.Cluster) {
 	if tn.errCh == nil {
 		return

pkg/roachprod/install/cluster_synced.go

@@ -1199,8 +1199,8 @@ func (c *SyncedCluster) DistributeCerts(ctx context.Context, l *logger.Logger) e
 rm -fr certs
 mkdir -p certs
 %[1]s cert create-ca --certs-dir=certs --ca-key=certs/ca.key
-%[1]s cert create-client root --certs-dir=certs --ca-key=certs/ca.key --tenant-scope 1,2,3,4
-%[1]s cert create-client testuser --certs-dir=certs --ca-key=certs/ca.key --tenant-scope 1,2,3,4
+%[1]s cert create-client root --certs-dir=certs --ca-key=certs/ca.key
+%[1]s cert create-client testuser --certs-dir=certs --ca-key=certs/ca.key
 %[1]s cert create-node %[2]s --certs-dir=certs --ca-key=certs/ca.key
 # Pre-create a few tenant-client
 %[1]s cert create-tenant-client 2 %[2]s --certs-dir=certs --ca-key=certs/ca.key
@@ -1284,9 +1284,6 @@ func (c *SyncedCluster) createTenantCertBundle(
 		node := c.Nodes[i]
 
 		var tenantScopeArg string
-		if c.cockroachBinSupportsTenantScope(l, ctx, node) {
-			tenantScopeArg = fmt.Sprintf("--tenant-scope %d", tenantID)
-		}
 
 		cmd := "set -e;"
 		if c.IsLocal() {
@@ -1327,23 +1324,6 @@ tar cvf %[5]s $CERT_DIR
 	}, DefaultSSHRetryOpts)
 }
 
-// cockroachBinSupportsTenantScope is a hack to figure out if the version of
-// cockroach on the node supports tenant scoped certificates. We can't use a
-// version comparison here because we need to compare alpha build versions which
-// are compared lexicographically. This is a problem because our alpha versions
-// contain an integer count of commits, which does not sort correctly.  Once
-// this feature ships in a release, it will be easier to do a version comparison
-// on whether this command line flag is supported.
-func (c *SyncedCluster) cockroachBinSupportsTenantScope(
-	l *logger.Logger, ctx context.Context, node Node,
-) bool {
-	cmd := fmt.Sprintf("%s cert create-client --help | grep '\\--tenant-scope'", cockroachNodeBinary(c, node))
-	sess := c.newSession(l, node, cmd)
-	defer sess.Close()
-
-	return sess.Run(ctx) == nil
-}
-
 // getFile retrieves the given file from the first node in the cluster. The
 // filename is assumed to be relative from the home directory of the node's
 // user.

pkg/security/securitytest/test_certs/regenerate.sh

@@ -7,9 +7,9 @@ rm -f "${dir_n}"/*.{crt,key}
 ./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-node 127.0.0.1 ::1 localhost *.local
 
 # Create client certs with tenant scopes.
-./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-client root --tenant-scope 1,2,10,11,20
-./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-client testuser --tenant-scope 1,2,10,11,20
-./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-client testuser2 --tenant-scope 1,2,10,11,20
+./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-client root
+./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-client testuser
+./cockroach cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca.key" create-client testuser2
 
 # Tenant certs
 ./cockroach mt cert --certs-dir="${dir_n}" --ca-key="${dir_n}/ca-client-tenant.key" create-tenant-client-ca