From c50bd93fe35e97cf00e2165520448cdc99bbf78a Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Thu, 23 Feb 2023 20:18:31 +0100 Subject: [PATCH] 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 | 6 ++-- pkg/cli/context.go | 15 +++++++- pkg/cli/flags.go | 3 +- pkg/cmd/roachtest/tests/BUILD.bazel | 1 - pkg/cmd/roachtest/tests/multitenant_utils.go | 34 ------------------- pkg/roachprod/install/cluster_synced.go | 24 ++----------- .../securitytest/test_certs/regenerate.sh | 6 ++-- 7 files changed, 24 insertions(+), 65 deletions(-) diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go index e028b470214a..57e845515a44 100644 --- a/pkg/cli/cliflags/flags.go +++ b/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{ diff --git a/pkg/cli/context.go b/pkg/cli/context.go index b97de80a087d..4103bc8cf933 100644 --- a/pkg/cli/context.go +++ b/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{ diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index fa8dbc23a29a..d30325c381a1 100644 --- a/pkg/cli/flags.go +++ b/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) diff --git a/pkg/cmd/roachtest/tests/BUILD.bazel b/pkg/cmd/roachtest/tests/BUILD.bazel index 444c541f4d65..61d976e7d765 100644 --- a/pkg/cmd/roachtest/tests/BUILD.bazel +++ b/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", diff --git a/pkg/cmd/roachtest/tests/multitenant_utils.go b/pkg/cmd/roachtest/tests/multitenant_utils.go index 686feb9d78a7..35603c449a53 100644 --- a/pkg/cmd/roachtest/tests/multitenant_utils.go +++ b/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 diff --git a/pkg/roachprod/install/cluster_synced.go b/pkg/roachprod/install/cluster_synced.go index e08fb8c182b4..dd4f7c21190f 100644 --- a/pkg/roachprod/install/cluster_synced.go +++ b/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. diff --git a/pkg/security/securitytest/test_certs/regenerate.sh b/pkg/security/securitytest/test_certs/regenerate.sh index 67d7e2fa903e..135764f4ed70 100755 --- a/pkg/security/securitytest/test_certs/regenerate.sh +++ b/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