Merge cockroachdb#79065

79065: cert: Extend tls code to authenticate tenant scoped client cert r=rimadeodhar a=rimadeodhar

This PR extends the TLS code to use a tenant scoped
client cert to authenticate a client for specific tenant.

Release note (security update): We introduce a new
tenant scoped client certificate to authenticate a client
on a specific tenant. A tenant scoped client certificate contains the client
name within the CN and the tenant ID, to which the
certificate is being scoped to, as the SAN. The tenant ID
is embedded within the URI section with the format
"crdb://tenant/<tenant_id>". For example, a root
client certificate scoped to a tenant with ID 123
will contain "root" in the CN field and the URI
"crdb://tenant/123" in the URI section of the
certificate. This certificate will authorize the root
client on the tenant with the ID 123. It will result
in an authorization error if used to authenticate the
root client on any other tenant.

Informs cockroachdb#77958

Please ignore commit 1 in this PR, that will be reviewed in cockroachdb#79064. 

Co-authored-by: rimadeodhar <[email protected]>

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
+}