cert: Extend tls code to authenticate tenant scoped client cert

pkg/cli/interactive_tests/test_cert_advisory_validation.tcl

@@ -78,23 +78,3 @@ send "$argv cert list --certs-dir=$certs_dir --cert-principal-map=foo.bar:node\r
 eexpect "Certificate directory:"
 expect $prompt
 end_test
-
-start_test "Check that 'cert create-client' can utilize cert principal map."
-send "$argv cert create-client root.crdb.io --certs-dir=$certs_dir --ca-key=$certs_dir/ca.key --cert-principal-map=foo.bar:node\r"
-eexpect $prompt
-send "mv $certs_dir/client.root.crdb.io.crt $certs_dir/client.root.crt; mv $certs_dir/client.root.crdb.io.key $certs_dir/client.root.key\r"
-eexpect $prompt
-end_test
-
-start_test "Check that the client commands can use cert principal map."
-system "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root --advertise-addr=localhost --background >>expect-cmd.log 2>&1"
-send "$argv sql --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root -e \"select 'hello'\"\r"
-eexpect "hello"
-expect $prompt
-send "$argv node ls --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root\r"
-eexpect "1 row"
-expect $prompt
-send "$argv quit --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root\r"
-eexpect "ok"
-expect $prompt
-end_test

pkg/rpc/auth.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
+	"github.com/cockroachdb/errors"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials"
@@ -114,11 +115,6 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
 		return roachpb.TenantID{}, errTLSInfoMissing
 	}
 
-	certUsers, err := security.GetCertificateUsers(&tlsInfo.State)
-	if err != nil {
-		return roachpb.TenantID{}, err
-	}
-
 	clientCert := tlsInfo.State.PeerCertificates[0]
 	if a.tenant.tenantID == roachpb.SystemTenantID {
 		// This node is a KV node.
@@ -153,16 +149,55 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
 	//   gateway uses a connection dialed as the node user.
 	//
 	// In both cases, we must check that the client cert is either root
-	// or node.
+	// or node. We also need to check that the tenant scope for the cert
+	// is either the system tenant ID or matches the tenant ID of the server.
 
 	// TODO(benesch): the vast majority of RPCs should be limited to just
 	// NodeUser. This is not a security concern, as RootUser has access to
 	// read and write all data, merely good hygiene. For example, there is
 	// no reason to permit the root user to send raw Raft RPCs.
-	if !security.Contains(certUsers, username.NodeUser) &&
-		!security.Contains(certUsers, username.RootUser) {
-		return roachpb.TenantID{}, authErrorf("user %s is not allowed to perform this RPC", certUsers)
+	certUserScope, err := security.GetCertificateUserScope(&tlsInfo.State)
+	if err != nil {
+		return roachpb.TenantID{}, err
+	}
+
+	// Confirm that the user scope is node/root. Otherwise, return an authentication error.
+	_, err = getActiveNodeOrUserScope(certUserScope, username.RootUser, a.tenant.tenantID)
+	if err != nil {
+		return roachpb.TenantID{}, authErrorf("client certificate %s cannot be used to perform RPC on tenant %d", clientCert.Subject, a.tenant.tenantID)
 	}
 
+	// User is node/root user authorized for this tenant, return success.
 	return roachpb.TenantID{}, nil
 }
+
+// getActiveNodeOrUserScope returns a node user scope if one is present in the set of certificate user scopes. If node
+// user scope is not present, it returns the user scope corresponding to the username parameter. The node user scope
+// will always override the user scope for authentication.
+func getActiveNodeOrUserScope(
+	certUserScope []security.CertificateUserScope, user string, serverTenantID roachpb.TenantID,
+) (security.CertificateUserScope, error) {
+	var userScope security.CertificateUserScope
+	for _, scope := range certUserScope {
+		// If we get a scope that matches the Node user, immediately return.
+		if scope.Username == username.NodeUser {
+			if scope.Global {
+				return scope, nil
+			}
+			// Confirm that the certificate scope and serverTenantID are the system tenant.
+			if scope.TenantID == roachpb.SystemTenantID && serverTenantID == roachpb.SystemTenantID {
+				return scope, nil
+			}
+		}
+		if scope.Username == user && (scope.TenantID == serverTenantID || scope.Global) {
+			userScope = scope
+		}
+	}
+	// Double check we're not returning an empty scope
+	if userScope.Username == "" {
+		return userScope, errors.New("could not find active user scope for client certificate")
+	}
+
+	// Only return userScope if we haven't found a NodeUser.
+	return userScope, nil
+}

pkg/rpc/auth_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/stretchr/testify/require"
@@ -95,10 +96,11 @@ func TestTenantFromCert(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	correctOU := []string{security.TenantsOU}
 	for _, tc := range []struct {
-		ous        []string
-		commonName string
-		expTenID   roachpb.TenantID
-		expErr     string
+		ous         []string
+		commonName  string
+		expTenID    roachpb.TenantID
+		expErr      string
+		tenantScope uint64
 	}{
 		{ous: correctOU, commonName: "10", expTenID: roachpb.MakeTenantID(10)},
 		{ous: correctOU, commonName: roachpb.MinTenantID.String(), expTenID: roachpb.MinTenantID},
@@ -109,9 +111,11 @@ func TestTenantFromCert(t *testing.T) {
 		{ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
 		{ous: correctOU, commonName: "root", expErr: `could not parse tenant ID from Common Name \(CN\)`},
 		{ous: correctOU, commonName: "other", expErr: `could not parse tenant ID from Common Name \(CN\)`},
-		{ous: []string{"foo"}, commonName: "other", expErr: `user \[other\] is not allowed to perform this RPC`},
-		{ous: nil, commonName: "other", expErr: `user \[other\] is not allowed to perform this RPC`},
+		{ous: []string{"foo"}, commonName: "other", expErr: `client certificate CN=other,OU=foo cannot be used to perform RPC on tenant {1}`},
+		{ous: nil, commonName: "other", expErr: `client certificate CN=other cannot be used to perform RPC on tenant {1}`},
 		{ous: append([]string{"foo"}, correctOU...), commonName: "other", expErr: `could not parse tenant ID from Common Name`},
+		{ous: nil, commonName: "root"},
+		{ous: nil, commonName: "root", tenantScope: 10, expErr: "client certificate CN=root cannot be used to perform RPC on tenant {1}"},
 	} {
 		t.Run(tc.commonName, func(t *testing.T) {
 			cert := &x509.Certificate{
@@ -120,6 +124,11 @@ func TestTenantFromCert(t *testing.T) {
 					OrganizationalUnit: tc.ous,
 				},
 			}
+			if tc.tenantScope > 0 {
+				tenantSANs, err := security.MakeTenantURISANs(username.MakeSQLUsernameFromPreNormalizedString(tc.commonName), []roachpb.TenantID{roachpb.MakeTenantID(tc.tenantScope)})
+				require.NoError(t, err)
+				cert.URIs = append(cert.URIs, tenantSANs...)
+			}
 			tlsInfo := credentials.TLSInfo{
 				State: tls.ConnectionState{
 					PeerCertificates: []*x509.Certificate{cert},

pkg/security/auth.go

@@ -17,6 +17,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/password"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -28,6 +29,21 @@ var certPrincipalMap struct {
 	m map[string]string
 }
 
+// CertificateUserScope indicates the scope of a user certificate i.e.
+// which tenant the user is allowed to authenticate on. Older client certificates
+// without a tenant scope are treated as global certificates which can
+// authenticate on any tenant strictly for backward compatibility with the
+// older certificates.
+type CertificateUserScope struct {
+	Username string
+	TenantID roachpb.TenantID
+	// 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
+	// client certificates without a tenant scope.
+	Global bool
+}
+
 // UserAuthHook authenticates a user based on their username and whether their
 // connection originates from a client or another node in the cluster. It
 // returns an optional func that is run at connection close.
@@ -83,8 +99,10 @@ func getCertificatePrincipals(cert *x509.Certificate) []string {
 	return results
 }
 
-// GetCertificateUsers extract the users from a client certificate.
-func GetCertificateUsers(tlsState *tls.ConnectionState) ([]string, error) {
+// GetCertificateUserScope extracts the certificate scopes from a client certificate.
+func GetCertificateUserScope(
+	tlsState *tls.ConnectionState,
+) (userScopes []CertificateUserScope, _ error) {
 	if tlsState == nil {
 		return nil, errors.Errorf("request is not using TLS")
 	}
@@ -95,7 +113,28 @@ func GetCertificateUsers(tlsState *tls.ConnectionState) ([]string, error) {
 	// any following certificates as intermediates. See:
 	// https://github.com/golang/go/blob/go1.8.1/src/crypto/tls/handshake_server.go#L723:L742
 	peerCert := tlsState.PeerCertificates[0]
-	return getCertificatePrincipals(peerCert), nil
+	for _, uri := range peerCert.URIs {
+		tenantID, user, err := ParseTenantURISAN(uri.String())
+		if err != nil {
+			return nil, err
+		}
+		scope := CertificateUserScope{
+			Username: user,
+			TenantID: tenantID,
+		}
+		userScopes = append(userScopes, scope)
+	}
+	if len(userScopes) == 0 {
+		users := getCertificatePrincipals(peerCert)
+		for _, user := range users {
+			scope := CertificateUserScope{
+				Username: user,
+				Global:   true,
+			}
+			userScopes = append(userScopes, scope)
+		}
+	}
+	return userScopes, nil
 }
 
 // Contains returns true if the specified string is present in the given slice.
@@ -110,12 +149,13 @@ func Contains(sl []string, s string) bool {
 
 // UserAuthCertHook builds an authentication hook based on the security
 // mode and client certificate.
-func UserAuthCertHook(insecureMode bool, tlsState *tls.ConnectionState) (UserAuthHook, error) {
-	var certUsers []string
-
+func UserAuthCertHook(
+	insecureMode bool, tlsState *tls.ConnectionState, tenantID roachpb.TenantID,
+) (UserAuthHook, error) {
+	var certUserScope []CertificateUserScope
 	if !insecureMode {
 		var err error
-		certUsers, err = GetCertificateUsers(tlsState)
+		certUserScope, err = GetCertificateUserScope(tlsState)
 		if err != nil {
 			return nil, err
 		}
@@ -143,12 +183,10 @@ func UserAuthCertHook(insecureMode bool, tlsState *tls.ConnectionState) (UserAut
 			return errors.Errorf("using tenant client certificate as user certificate is not allowed")
 		}
 
-		// The client certificate user must match the requested user.
-		if !Contains(certUsers, systemIdentity.Normalized()) {
-			return errors.Errorf("requested user is %s, but certificate is for %s", systemIdentity, certUsers)
+		if ValidateUserScope(certUserScope, systemIdentity.Normalized(), tenantID) {
+			return nil
 		}
-
-		return nil
+		return errors.Errorf("requested user %s is not authorized for tenant %d", systemIdentity, tenantID)
 	}, nil
 }
 
@@ -223,3 +261,23 @@ func (i *PasswordUserAuthError) Format(s fmt.State, verb rune) { errors.FormatEr
 func (i *PasswordUserAuthError) FormatError(p errors.Printer) error {
 	return i.err
 }
+
+// ValidateUserScope returns true if the user is a valid user for the tenant based on the certificate
+// user scope. It also returns true if the certificate is a global certificate. A client certificate
+// is considered global only when it doesn't contain a tenant SAN which is only possible for older
+// client certificates created prior to introducing tenant based scoping for the client.
+func ValidateUserScope(
+	certUserScope []CertificateUserScope, user string, tenantID roachpb.TenantID,
+) bool {
+	for _, scope := range certUserScope {
+		if scope.Username == user {
+			// If username matches, allow authentication to succeed if the tenantID is a match
+			// or if the certificate scope is global.
+			if scope.TenantID == tenantID || scope.Global {
+				return true
+			}
+		}
+	}
+	// No user match, return false.
+	return false
+}