rpc,security: use the tenant client cert for pod-pod communication

As of this patch, we have the following file usage:

- KV nodes on host cluster:
  - ui.crt (optional):
    - used as server cert for HTTP
  - ui-ca.crt (optional):
    - used in unit tests to verify the server's identity for HTTP conns
  - node.crt:
    - used as client cert for node-to-node comms
    - used as server cert for node-to-node comms
    - used as server cert for SQL clients
    - used as server cert for incoming conns from SQL tenant servers
    - used as server cert for HTTP, if ui.crt doesn't exist
  - tenant-client-ca.crt (optional):
    - used to verify client certs for SQL tenant servers
  - client-ca.crt (optional);
    - used to verify client certs for SQL clients
    - used to verify client certs for SQL tenant servers, if tenant-client-ca.crt doesn't exist
  - ca.crt:
    - used to verify other node client certs for node-to-node comms
    - used in unit tests to verify the server's identity for SQL and RPC conns
    - used to verify client certs for SQL clients, if client-ca.crt doesn't exist
    - used to verify client certs for SQL tenant servers, if neither tenant-client.ca.crt nor client-ca.crt exist

- SQL servers:
  - ui.crt (optional):
    - used as server cert for HTTP
  - ui-ca.crt (optional):
    - used in unit tests to verify the server's identity for HTTP conns
  - client-tenant.NN.crt:
    - used as client cert for node-to-node comms (SQL server to SQL server)
    - used as server cert for node-to-node comms (SQL server to SQL server)
    - used as client cert for conns to KV nodes
    - used as server cert for SQL clients
    - used as server cert for HTTP, if ui.crt doesn't exist
  - tenant-client-ca.crt (optional):
    - used to verify client certs for SQL tenant servers
  - client-ca.crt (optional);
    - used to verify client certs for SQL clients
    - used to verify client certs for SQL tenant servers, if tenant-client-ca.crt doesn't exist
  - ca.crt:
    - used to verify other SQL server certs for node-to-node comms, if tenant-client-ca.crt doens't exist
    - used to verify client certs for SQL clients, if client-ca.crt doesn't exist
    - used to verify client certs for SQL tenant servers, if neither tenant-client.ca.crt nor client-ca.crt exist
    - used in unit tests to verify the server's identity for SQL and  RPC conns

Release note (security update): Multitenant SQL servers now reuse
the tenant client certificate (`client-tenant.NN.crt`) for SQL-to-SQL
communication. Existing deployments must regenerate the certificates
with dual purpose (client and server authentication).

pkg/cli/cert.go

@@ -290,10 +290,10 @@ func runListCerts(cmd *cobra.Command, args []string) error {
 var certCmds = []*cobra.Command{
 	createCACertCmd,
 	createClientCACertCmd,
-	mtCreateTenantClientCACertCmd,
+	mtCreateTenantCACertCmd,
 	createNodeCertCmd,
 	createClientCertCmd,
-	mtCreateTenantClientCertCmd,
+	mtCreateTenantCertCmd,
 	listCertsCmd,
 }
 

pkg/cli/mt.go

@@ -19,8 +19,8 @@ func init() {
 	mtCmd.AddCommand(mtTestDirectorySvr)
 
 	mtCertsCmd.AddCommand(
-		mtCreateTenantClientCACertCmd,
-		mtCreateTenantClientCertCmd,
+		mtCreateTenantCACertCmd,
+		mtCreateTenantCertCmd,
 	)
 
 	mtCmd.AddCommand(mtCertsCmd)

pkg/cli/mt_cert.go

@@ -19,9 +19,9 @@ import (
 	"github.com/spf13/cobra"
 )
 
-// mtCreateTenantClientCACertCmd generates a tenant CA certificate and stores it
+// mtCreateTenantCACertCmd generates a tenant CA certificate and stores it
 // in the cert directory.
-var mtCreateTenantClientCACertCmd = &cobra.Command{
+var mtCreateTenantCACertCmd = &cobra.Command{
 	Use:   "create-tenant-client-ca --certs-dir=<path to cockroach certs dir> --ca-key=<path>",
 	Short: "create tenant client CA certificate and key",
 	Long: `
@@ -34,7 +34,7 @@ If the CA certificate exists and --overwrite is true, the new CA certificate is
 	Args: cobra.NoArgs,
 	RunE: clierrorplus.MaybeDecorateError(func(cmd *cobra.Command, args []string) error {
 		return errors.Wrap(
-			security.CreateTenantClientCAPair(
+			security.CreateTenantCAPair(
 				certCtx.certsDir,
 				certCtx.caKey,
 				certCtx.keySize,
@@ -47,8 +47,8 @@ If the CA certificate exists and --overwrite is true, the new CA certificate is
 
 // A createClientCert command generates a client certificate and stores it
 // in the cert directory under <username>.crt and key under <username>.key.
-var mtCreateTenantClientCertCmd = &cobra.Command{
-	Use:   "create-tenant-client --certs-dir=<path to cockroach certs dir> --ca-key=<path-to-ca-key> <tenant-id>",
+var mtCreateTenantCertCmd = &cobra.Command{
+	Use:   "create-tenant-client --certs-dir=<path to cockroach certs dir> --ca-key=<path-to-ca-key> <tenant-id> <host 1> <host 2> ... <host N>",
 	Short: "create tenant client certificate and key",
 	Long: `
 Generate a tenant client certificate "<certs-dir>/client-tenant.<tenant-id>.crt" and key
@@ -60,27 +60,30 @@ Requires a CA cert in "<certs-dir>/ca-client-tenant.crt" and matching key in "--
 If "ca-client-tenant.crt" contains more than one certificate, the first is used.
 Creation fails if the CA expiration time is before the desired certificate expiration.
 `,
-	Args: cobra.ExactArgs(1),
+	Args: cobra.MinimumNArgs(2),
 	RunE: clierrorplus.MaybeDecorateError(
 		func(cmd *cobra.Command, args []string) error {
-			tenantID, err := strconv.ParseUint(args[0], 10, 64)
+			tenantIDs := args[0]
+			hostAddrs := args[1:]
+			tenantID, err := strconv.ParseUint(tenantIDs, 10, 64)
 			if err != nil {
-				return errors.Wrapf(err, "%s is invalid uint64", args[0])
+				return errors.Wrapf(err, "%s is invalid uint64", tenantIDs)
 			}
-			cp, err := security.CreateTenantClientPair(
+			cp, err := security.CreateTenantPair(
 				certCtx.certsDir,
 				certCtx.caKey,
 				certCtx.keySize,
 				certCtx.certificateLifetime,
 				tenantID,
+				hostAddrs,
 			)
 			if err != nil {
 				return errors.Wrap(
 					err,
 					"failed to generate tenant client certificate and key")
 			}
 			return errors.Wrap(
-				security.WriteTenantClientPair(certCtx.certsDir, cp, certCtx.overwriteFiles),
+				security.WriteTenantPair(certCtx.certsDir, cp, certCtx.overwriteFiles),
 				"failed to write tenant client certificate and key")
 		}),
 }

pkg/rpc/auth.go

@@ -121,21 +121,34 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
 		return roachpb.TenantID{}, err
 	}
 
-	subj := tlsInfo.State.PeerCertificates[0].Subject
-	if security.Contains(subj.OrganizationalUnit, security.TenantsOU) {
-		// Tenant authentication.
-		return tenantFromCommonName(subj.CommonName)
-	}
-
-	// KV auth.
+	if a.tenant.tenantID == roachpb.SystemTenantID {
+		// This node is a KV node.
+		//
+		//
+		// Is this a connection from a SQL tenant server?
+		clientCert := tlsInfo.State.PeerCertificates[0]
+		if security.IsTenantCertificate(clientCert) {
+			// Incoming connection originating from a tenant SQL server,
+			// into a KV node.
+			return tenantFromCommonName(clientCert.Subject.CommonName)
+		}
 
-	// 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, security.NodeUser) &&
-		!security.Contains(certUsers, security.RootUser) {
-		return roachpb.TenantID{}, authErrorf("user %s is not allowed to perform this RPC", certUsers)
+		// Connection is from another KV node.
+		//
+		// 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, security.NodeUser) &&
+			!security.Contains(certUsers, security.RootUser) {
+			return roachpb.TenantID{}, authErrorf("user %s is not allowed to perform this RPC", certUsers)
+		}
+	} else {
+		// This node is a SQL tenant server.
+		// Is this a connection from another SQL tenant server?
+		if !security.IsTenantCertificate(tlsInfo.State.PeerCertificates[0]) {
+			return roachpb.TenantID{}, authErrorf("user %s is not allowed to perform this RPC", certUsers)
+		}
 	}
 	return roachpb.TenantID{}, nil
 }

pkg/rpc/auth_tenant.go

@@ -24,6 +24,8 @@ import (
 // server, that is, it ensures that the request only accesses resources
 // available to the tenant.
 type tenantAuthorizer struct {
+	// tenantID is the tenant ID for the current node.
+	// Equals SystemTenantID when running a KV node.
 	tenantID roachpb.TenantID
 }
 

pkg/rpc/auth_test.go

@@ -127,7 +127,7 @@ func TestTenantFromCert(t *testing.T) {
 			p := peer.Peer{AuthInfo: tlsInfo}
 			ctx := peer.NewContext(context.Background(), &p)
 
-			tenID, err := kvAuth{}.authenticate(ctx)
+			tenID, err := kvAuth{tenant: tenantAuthorizer{tenantID: roachpb.SystemTenantID}}.authenticate(ctx)
 
 			if tc.expErr == "" {
 				require.Equal(t, tc.expTenID, tenID)

pkg/rpc/context.go

@@ -729,7 +729,7 @@ func (ctx *Context) grpcDialOptions(
 		if ctx.tenID == roachpb.SystemTenantID {
 			tlsConfig, err = ctx.GetClientTLSConfig()
 		} else {
-			tlsConfig, err = ctx.GetTenantClientTLSConfig()
+			tlsConfig, err = ctx.GetTenantTLSConfig()
 		}
 		if err != nil {
 			return nil, err

pkg/rpc/tls.go

@@ -73,6 +73,9 @@ type SecurityContext struct {
 func MakeSecurityContext(
 	cfg *base.Config, tlsSettings security.TLSSettings, tenID roachpb.TenantID,
 ) SecurityContext {
+	if tenID.ToUint64() == 0 {
+		panic(errors.AssertionFailedf("programming error: tenant ID not defined"))
+	}
 	return SecurityContext{
 		CertsLocator: security.MakeCertsLocator(cfg.SSLCertsDir),
 		TLSSettings:  tlsSettings,
@@ -151,13 +154,13 @@ func (ctx *SecurityContext) GetClientTLSConfig() (*tls.Config, error) {
 	return tlsCfg, nil
 }
 
-// GetTenantClientTLSConfig returns the client TLS config for the tenant, provided
+// GetTenantTLSConfig returns the client TLS config for the tenant, provided
 // the SecurityContext operates on behalf of a secondary tenant (i.e. not the
 // system tenant).
 //
 // If Insecure is true, return a nil config, otherwise retrieves the client
 // certificate for the configured tenant from the cert manager.
-func (ctx *SecurityContext) GetTenantClientTLSConfig() (*tls.Config, error) {
+func (ctx *SecurityContext) GetTenantTLSConfig() (*tls.Config, error) {
 	// Early out.
 	if ctx.config.Insecure {
 		return nil, nil
@@ -168,7 +171,7 @@ func (ctx *SecurityContext) GetTenantClientTLSConfig() (*tls.Config, error) {
 		return nil, wrapError(err)
 	}
 
-	tlsCfg, err := cm.GetTenantClientTLSConfig()
+	tlsCfg, err := cm.GetTenantTLSConfig()
 	if err != nil {
 		return nil, wrapError(err)
 	}

pkg/security/auth.go

@@ -128,8 +128,7 @@ func UserAuthCertHook(insecureMode bool, tlsState *tls.ConnectionState) (UserAut
 		// The client certificate should not be a tenant client type. For now just
 		// check that it doesn't have OU=Tenants. It would make sense to add
 		// explicit OU=Users to all client certificates and to check for match.
-		ous := tlsState.PeerCertificates[0].Subject.OrganizationalUnit
-		if Contains(ous, TenantsOU) {
+		if IsTenantCertificate(tlsState.PeerCertificates[0]) {
 			return nil,
 				errors.Errorf("using tenant client certificate as user certificate is not allowed")
 		}
@@ -143,6 +142,12 @@ func UserAuthCertHook(insecureMode bool, tlsState *tls.ConnectionState) (UserAut
 	}, nil
 }
 
+// IsTenantCertificate returns true if the passed certificate indicates an
+// inbound Tenant connection.
+func IsTenantCertificate(cert *x509.Certificate) bool {
+	return Contains(cert.Subject.OrganizationalUnit, TenantsOU)
+}
+
 // UserAuthPasswordHook builds an authentication hook based on the security
 // mode, password, and its potentially matching hash.
 func UserAuthPasswordHook(insecureMode bool, password string, hashedPassword []byte) UserAuthHook {

pkg/security/certificate_loader.go

@@ -79,9 +79,9 @@ const (
 	_ PemUsage = iota
 	// CAPem describes the main CA certificate.
 	CAPem
-	// TenantClientCAPem describes the CA certificate used to broker authN/Z for SQL
+	// TenantCAPem describes the CA certificate used to broker authN/Z for SQL
 	// tenants wishing to access the KV layer.
-	TenantClientCAPem
+	TenantCAPem
 	// ClientCAPem describes the CA certificate used to verify client certificates.
 	ClientCAPem
 	// UICAPem describes the CA certificate used to verify the Admin UI server certificate.
@@ -93,8 +93,8 @@ const (
 	UIPem
 	// ClientPem describes a client certificate.
 	ClientPem
-	// TenantClientPem describes a SQL tenant client certificate.
-	TenantClientPem
+	// TenantPem describes a SQL tenant client certificate.
+	TenantPem
 
 	// Maximum allowable permissions.
 	maxKeyPermissions os.FileMode = 0700
@@ -110,7 +110,7 @@ const (
 )
 
 func isCA(usage PemUsage) bool {
-	return usage == CAPem || usage == ClientCAPem || usage == TenantClientCAPem || usage == UICAPem
+	return usage == CAPem || usage == ClientCAPem || usage == TenantCAPem || usage == UICAPem
 }
 
 func (p PemUsage) String() string {
@@ -119,7 +119,7 @@ func (p PemUsage) String() string {
 		return "CA"
 	case ClientCAPem:
 		return "Client CA"
-	case TenantClientCAPem:
+	case TenantCAPem:
 		return "Tenant Client CA"
 	case UICAPem:
 		return "UI CA"
@@ -129,7 +129,7 @@ func (p PemUsage) String() string {
 		return "UI"
 	case ClientPem:
 		return "Client"
-	case TenantClientPem:
+	case TenantPem:
 		return "Tenant Client"
 	default:
 		return "unknown"
@@ -201,7 +201,7 @@ func CertInfoFromFilename(filename string) (*CertInfo, error) {
 			return nil, errors.Errorf("client CA certificate filename should match ca-client%s", certExtension)
 		}
 	case `ca-client-tenant`:
-		fileUsage = TenantClientCAPem
+		fileUsage = TenantCAPem
 		if numParts != 2 {
 			return nil, errors.Errorf("tenant CA certificate filename should match ca%s", certExtension)
 		}
@@ -228,7 +228,7 @@ func CertInfoFromFilename(filename string) (*CertInfo, error) {
 			return nil, errors.Errorf("client certificate filename should match client.<user>%s", certExtension)
 		}
 	case `client-tenant`:
-		fileUsage = TenantClientPem
+		fileUsage = TenantPem
 		// Strip prefix and suffix and re-join middle parts.
 		name = strings.Join(parts[1:numParts-1], `.`)
 		if len(name) == 0 {

pkg/security/certificate_loader_test.go

@@ -108,7 +108,7 @@ func TestLoadEmbeddedCerts(t *testing.T) {
 
 	// Check that all non-CA pairs include a key.
 	for _, c := range certs {
-		if c.FileUsage == security.CAPem || c.FileUsage == security.TenantClientCAPem {
+		if c.FileUsage == security.CAPem || c.FileUsage == security.TenantCAPem {
 			if len(c.KeyFilename) != 0 {
 				t.Errorf("CA key was loaded for CertInfo %+v", c)
 			}