Merge cockroachdb#129216

129216: cli, security: add --tenant-name-scope CLI flag r=cthumuluru-crdb a=cthumuluru-crdb

Currently client certificates can be generated with tenant scope using `cert create-client` command with `--tenant-scope` flag. Certificates generated with tenant scope restrict access to only those tenants. Tenant ID is an internal identifier to identify a specific tenant and it is not a great user experience to use it in customer facing interfaces (CLI in this case). The goal of the ticket [CRDB-28992](https://cockroachlabs.atlassian.net/browse/CRDB-28992) is to allow users to generate certificates with tenant scope using tenant names and use those certificates to authenticate. 

To complete this ticket, following changes are needed:
1.  Allow users to generate tenant scoped certs using tenant names. 
2. Use those certificates to authenticate. 

This PR addresses the first part of the changes. I'll send a followup PR for the second part of the changes. 

Changes in this PR allows users to use both `tenant-scope` and `tenant-name-scope` flags to specify the scopes. IMO, this will be useful for backward compatibility. We can eventually deprecate supporting `tenant-scope` flag. Another option is to make `tenant-scope`, `tenant-name-scope` flags mutually exclusive.

Informs: cockroachdb#105340
EPIC: CRDB-39093
Release note: None

Co-authored-by: Chandra Thumuluru <[email protected]>

pkg/acceptance/cluster/certs.go

@@ -63,14 +63,16 @@ func GenerateCerts(ctx context.Context) func() {
 	userScopes := []roachpb.TenantID{roachpb.SystemTenantID, roachpb.MustMakeTenantID(5)}
 	maybePanic(security.CreateClientPair(
 		certsDir, filepath.Join(certsDir, certnames.EmbeddedCAKey),
-		keyLen, 48*time.Hour, false, username.RootUserName(), userScopes, true /* generate pk8 key */))
+		keyLen, 48*time.Hour, false, username.RootUserName(), userScopes,
+		nil /* tenantNames */, true /* generate pk8 key */))
 
 	// Test user.
 	// Scope test user to system tenant and tenant ID 5 which is what we use by default for acceptance
 	// tests.
 	maybePanic(security.CreateClientPair(
 		certsDir, filepath.Join(certsDir, certnames.EmbeddedCAKey),
-		keyLen, 48*time.Hour, false, username.TestUserName(), userScopes, true /* generate pk8 key */))
+		keyLen, 48*time.Hour, false, username.TestUserName(), userScopes,
+		nil /* tenantNames */, true /* generate pk8 key */))
 
 	// Certs for starting a cockroach server. Key size is from cli/cert.go:defaultKeySize.
 	maybePanic(security.CreateNodePair(

pkg/cli/cert.go

@@ -193,6 +193,7 @@ func runCreateClientCert(cmd *cobra.Command, args []string) error {
 			certCtx.overwriteFiles,
 			user,
 			certCtx.tenantScope,
+			certCtx.tenantNameScope,
 			certCtx.generatePKCS8Key),
 		"failed to generate client certificate and key")
 }

pkg/cli/cert_test.go

@@ -17,13 +17,19 @@ func Example_cert() {
 	c.RunWithCAArgs([]string{"cert", "create-client", "foo"})
 	c.RunWithCAArgs([]string{"cert", "create-client", "Ομηρος"})
 	c.RunWithCAArgs([]string{"cert", "create-client", "0foo"})
+	c.RunWithCAArgs([]string{"cert", "create-client", "foo-1", "--tenant-scope", "1"})
+	c.RunWithCAArgs([]string{"cert", "create-client", "foo-tenant2", "--tenant-name-scope", "tenant2"})
+	c.RunWithCAArgs([]string{"cert", "create-client", "foo-1-tenant2", "--tenant-scope", "1", "--tenant-name-scope", "tenant2"})
 	c.RunWithCAArgs([]string{"cert", "create-client", ",foo"})
 	c.RunWithCAArgs([]string{"cert", "create-client", "--disable-username-validation", ",foo"})
 
 	// Output:
 	// cert create-client foo
 	// cert create-client Ομηρος
 	// cert create-client 0foo
+	// cert create-client foo-1 --tenant-scope 1
+	// cert create-client foo-tenant2 --tenant-name-scope tenant2
+	// cert create-client foo-1-tenant2 --tenant-scope 1 --tenant-name-scope tenant2
 	// cert create-client ,foo
 	// ERROR: failed to generate client certificate and key: username is invalid
 	// HINT: Usernames are case insensitive, must start with a letter, digit or underscore, may contain letters, digits, dashes, periods, or underscores, and must not exceed 63 characters.

pkg/cli/cliflags/flags.go

@@ -843,6 +843,14 @@ This flag is optional. When omitted, the certificate is not scoped; i.e.
 it can be used with all tenants.`,
 	}
 
+	TenantScopeByNames = FlagInfo{
+		Name: "tenant-name-scope",
+		Description: `Assign a tenant scope using tenant names to the certificate.
+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{
 		Name:        "also-generate-pkcs8-key",
 		Description: `Also write the key in pkcs8 format to <certs-dir>/client.<username>.key.pk8.`,

pkg/cli/context.go

@@ -250,9 +250,12 @@ var certCtx struct {
 	certPrincipalMap []string
 	// tenantScope indicates a tenantID(s) that a certificate is being
 	// scoped to. By creating a tenant-scoped certicate, the usage of that certificate
-	// is restricted to a specific tenant.
+	// is restricted to a specific tenant(s).
 	tenantScope []roachpb.TenantID
-
+	// tenantNameScope indicates a tenantName(s) that a certificate is being scoped to.
+	// By creating a tenant-scoped certificate, the usage of that certificate is
+	// restricted to a specific tenant(s).
+	tenantNameScope []roachpb.TenantName
 	// disableUsernameValidation removes the username syntax check on
 	// the input.
 	disableUsernameValidation bool
@@ -269,9 +272,9 @@ func setCertContextDefaults() {
 	certCtx.generatePKCS8Key = false
 	certCtx.disableUsernameValidation = false
 	certCtx.certPrincipalMap = nil
-	// 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: we set tenantScope and tenantNameScope 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
@@ -283,6 +286,7 @@ func setCertContextDefaults() {
 	// other, defaulting to certs that are valid on every tenant is a
 	// good choice.
 	certCtx.tenantScope = nil
+	certCtx.tenantNameScope = nil
 }
 
 var sqlExecCtx = clisqlexec.Context{

pkg/cli/democluster/demo_cluster.go

@@ -1486,6 +1486,7 @@ func (c *transientCluster) generateCerts(ctx context.Context, certsDir string) (
 			true, /* overwrite */
 			username.RootUserName(),
 			nil,  /* tenantIDs - this makes it valid for all tenants */
+			nil,  /* tenantNames - this makes it valid for all tenants */
 			true, /* generatePKCS8Key */
 		); err != nil {
 			return err
@@ -1502,6 +1503,7 @@ func (c *transientCluster) generateCerts(ctx context.Context, certsDir string) (
 			true, /* overwrite */
 			demoUser,
 			nil,  /* tenantIDs - this makes it valid for all tenants */
+			nil,  /* tenantNames - this makes it valid for all tenants */
 			true, /* generatePKCS8Key */
 		); err != nil {
 			return err

pkg/cli/flags.go

@@ -191,6 +191,41 @@ func (t tenantIDSetter) Set(v string) error {
 	return nil
 }
 
+// tenantNameSetter wraps a list of roachpb.TenantNames and enables setting
+// them via a command-line flag.
+type tenantNameSetter struct {
+	tenantNames *[]roachpb.TenantName
+}
+
+// String implements the pflag.Value interface.
+func (t tenantNameSetter) String() string {
+	var tenantString strings.Builder
+	separator := ""
+	for _, tName := range *t.tenantNames {
+		tenantString.WriteString(separator)
+		tenantString.WriteString(string(tName))
+		separator = ","
+	}
+	return tenantString.String()
+}
+
+// Type implements the pflag.Value interface.
+func (t tenantNameSetter) Type() string { return "<[]TenantName>" }
+
+// Set implements the pflag.Value interface.
+func (t tenantNameSetter) Set(v string) error {
+	*t.tenantNames = []roachpb.TenantName{}
+	tenantScopes := strings.Split(v, "," /* separator */)
+	for _, tenantScope := range tenantScopes {
+		tenant := roachpb.TenantName(tenantScope)
+		if err := tenant.IsValid(); err != nil {
+			return err
+		}
+		*t.tenantNames = append(*t.tenantNames, roachpb.TenantName(tenantScope))
+	}
+	return nil
+}
+
 // Set implements the pflag.Value interface.
 func (a clusterNameSetter) Set(v string) error {
 	if v == "" {
@@ -617,6 +652,7 @@ func init() {
 
 		if cmd == createClientCertCmd {
 			cliflagcfg.VarFlag(f, &tenantIDSetter{tenantIDs: &certCtx.tenantScope}, cliflags.TenantScope)
+			cliflagcfg.VarFlag(f, &tenantNameSetter{tenantNames: &certCtx.tenantNameScope}, cliflags.TenantScopeByNames)
 
 			// PKCS8 key format is only available for the client cert command.
 			cliflagcfg.BoolFlag(f, &certCtx.generatePKCS8Key, cliflags.GeneratePKCS8Key)

pkg/cli/flags_test.go

@@ -1529,6 +1529,34 @@ func TestTenantID(t *testing.T) {
 	}
 }
 
+func TestTenantName(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	tests := []struct {
+		name        string
+		arg         string
+		errContains string
+	}{
+		{"empty tenant name text", "", "invalid tenant name: \"\""},
+		{"tenant name not valid", "a+bc", "invalid tenant name: \"a+bc\""},
+		{"tenant name \"abc\" is valid", "abc", ""},
+		{"tenant name \"system\" is valid", "system", ""},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tns := tenantNameSetter{tenantNames: &[]roachpb.TenantName{}}
+			err := tns.Set(tt.arg)
+			if tt.errContains == "" {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.arg, tns.String())
+			} else {
+				assert.True(t, strings.Contains(err.Error(), tt.errContains))
+			}
+		})
+	}
+}
+
 func TestTenantIDFromFile(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/security/auth.go

@@ -97,8 +97,9 @@ func UnsetNodeSubject() {
 // GetCertificateUserScope function expands a cert into a set of "scopes" with
 // each possible username (and tenant ID).
 type CertificateUserScope struct {
-	Username string
-	TenantID roachpb.TenantID
+	Username   string
+	TenantID   roachpb.TenantID
+	TenantName roachpb.TenantName
 	// global is set to true to indicate that the certificate unscoped to
 	// any tenant is a global client certificate which can authenticate
 	// on any tenant. This is ONLY for backward compatibility with old
@@ -212,15 +213,26 @@ func GetCertificateUserScope(
 	peerCert *x509.Certificate,
 ) (userScopes []CertificateUserScope, _ error) {
 	for _, uri := range peerCert.URIs {
-		uriString := uri.String()
-		if URISANHasCRDBPrefix(uriString) {
-			tenantID, user, err := ParseTenantURISAN(uriString)
-			if err != nil {
-				return nil, err
-			}
-			scope := CertificateUserScope{
-				Username: user,
-				TenantID: tenantID,
+		if isCRDBSANURI(uri) {
+			var scope CertificateUserScope
+			if isTenantNameSANURI(uri) {
+				tenantName, user, err := parseTenantNameURISAN(uri)
+				if err != nil {
+					return nil, err
+				}
+				scope = CertificateUserScope{
+					Username:   user,
+					TenantName: tenantName,
+				}
+			} else {
+				tenantID, user, err := parseTenantURISAN(uri)
+				if err != nil {
+					return nil, err
+				}
+				scope = CertificateUserScope{
+					Username: user,
+					TenantID: tenantID,
+				}
 			}
 			userScopes = append(userScopes, scope)
 		}

pkg/security/auth_test.go

@@ -249,6 +249,50 @@ func TestGetCertificateUserScope(t *testing.T) {
 			require.True(t, userScopes[0].Global)
 		}
 	})
+
+	t.Run("extracts username, tenantName from tenant-name URI SAN", func(t *testing.T) {
+		state := makeFakeTLSState(t, "(CN=foo)uri:crdb://tenant-name/tenant10/user/foo;(CN=CA)")
+		cert := state.PeerCertificates[0]
+		if userScopes, err := security.GetCertificateUserScope(cert); err != nil {
+			t.Error(err)
+		} else {
+			require.Equal(t, 1, len(userScopes))
+			require.Equal(t, "foo", userScopes[0].Username)
+			require.Equal(t, roachpb.TenantName("tenant10"), userScopes[0].TenantName)
+			require.False(t, userScopes[0].Global)
+		}
+	})
+
+	t.Run("extracts username, tenantName from tenant-name URI SAN with URI scheme in upper case", func(t *testing.T) {
+		state := makeFakeTLSState(t, "(CN=foo)uri:CRDB://tenant-name/tenant10/user/foo;(CN=CA)")
+		cert := state.PeerCertificates[0]
+		if userScopes, err := security.GetCertificateUserScope(cert); err != nil {
+			t.Error(err)
+		} else {
+			require.Equal(t, 1, len(userScopes))
+			require.Equal(t, "foo", userScopes[0].Username)
+			require.Equal(t, roachpb.TenantName("tenant10"), userScopes[0].TenantName)
+			require.False(t, userScopes[0].Global)
+		}
+	})
+
+	t.Run("extracts both tenant URI SAN and tenant name URI SAN when both are present", func(t *testing.T) {
+		state := makeFakeTLSState(t, "(CN=foo)uri:crdb://tenant-name/tenant10/user/bar,uri:crdb://tenant/123/user/foo;(CN=CA)")
+		cert := state.PeerCertificates[0]
+		if userScopes, err := security.GetCertificateUserScope(cert); err != nil {
+			t.Error(err)
+		} else {
+			require.Equal(t, 2, len(userScopes))
+
+			require.Equal(t, "bar", userScopes[0].Username)
+			require.Equal(t, roachpb.TenantName("tenant10"), userScopes[0].TenantName)
+			require.False(t, userScopes[0].Global)
+
+			require.Equal(t, "foo", userScopes[1].Username)
+			require.Equal(t, roachpb.MustMakeTenantID(123), userScopes[1].TenantID)
+			require.False(t, userScopes[1].Global)
+		}
+	})
 }
 
 func TestSetCertPrincipalMap(t *testing.T) {

pkg/security/certificate_manager_test.go

@@ -96,7 +96,13 @@ func TestManagerWithPrincipalMap(t *testing.T) {
 		certsDir, caKey, testKeySize, time.Hour*96, true, true,
 	))
 	require.NoError(t, security.CreateClientPair(
-		certsDir, caKey, testKeySize, time.Hour*48, true, username.TestUserName(), []roachpb.TenantID{roachpb.SystemTenantID}, false,
+		certsDir, caKey, testKeySize, time.Hour*48, true, username.TestUserName(),
+		[]roachpb.TenantID{roachpb.SystemTenantID}, nil /* tenantNameScope */, false,
+	))
+	require.NoError(t, security.CreateClientPair(
+		certsDir, caKey, testKeySize, time.Hour*48, true, username.TestUserName(),
+		nil /* tenantScope */, []roachpb.TenantName{roachpb.TenantName(roachpb.SystemTenantID.String())},
+		false,
 	))
 	require.NoError(t, security.CreateNodePair(
 		certsDir, caKey, testKeySize, time.Hour*48, true, []string{"127.0.0.1", "foo"},

pkg/security/certs.go

@@ -375,6 +375,7 @@ func CreateClientPair(
 	overwrite bool,
 	user username.SQLUsername,
 	tenantIDs []roachpb.TenantID,
+	tenantNames []roachpb.TenantName,
 	wantPKCS8Key bool,
 ) error {
 	if len(caKeyPath) == 0 {
@@ -411,7 +412,7 @@ func CreateClientPair(
 		return errors.Wrap(err, "could not generate new client key")
 	}
 
-	clientCert, err := GenerateClientCert(caCert, caPrivateKey, clientKey.Public(), lifetime, user, tenantIDs)
+	clientCert, err := GenerateClientCert(caCert, caPrivateKey, clientKey.Public(), lifetime, user, tenantIDs, tenantNames)
 	if err != nil {
 		return errors.Wrap(err, "error creating client certificate and key")
 	}