server,rpc,security: use a separate sql-node.crt for SQL pods

Release note (security update): Multitenant SQL servers now use a
separate certificate with `CN=sql-node` and filename `sql-node.crt`.
Using the `node.crt` generated for the system tenant is not possible
any more.

pkg/cli/cert.go

@@ -293,6 +293,7 @@ var certCmds = []*cobra.Command{
 	mtCreateTenantClientCACertCmd,
 	createNodeCertCmd,
 	createClientCertCmd,
+	mtCreateSQLNodeCertCmd,
 	mtCreateTenantClientCertCmd,
 	listCertsCmd,
 }

pkg/cli/mt.go

@@ -19,6 +19,7 @@ func init() {
 	mtCmd.AddCommand(mtTestDirectorySvr)
 
 	mtCertsCmd.AddCommand(
+		mtCreateSQLNodeCertCmd,
 		mtCreateTenantClientCACertCmd,
 		mtCreateTenantClientCertCmd,
 	)

pkg/cli/mt_cert.go

@@ -84,3 +84,45 @@ Creation fails if the CA expiration time is before the desired certificate expir
 				"failed to write tenant client certificate and key")
 		}),
 }
+
+// A mtCreateSQLNodeCert command generates a SQL server node certificate
+// and stores it in the cert directory.
+var mtCreateSQLNodeCertCmd = &cobra.Command{
+	Use:   "create-tenant-node --certs-dir=<path to cockroach certs dir> --ca-key=<path-to-ca-key> <host 1> <host 2> ... <host N>",
+	Short: "create SQL server certificate and key",
+	Long: `
+Generate a node certificate "<certs-dir>/sql-node.crt" and key "<certs-dir>/sql-node.key".
+
+If --overwrite is true, any existing files are overwritten.
+
+At least one host should be passed in (either IP address or dns name).
+
+Requires a CA cert in "<certs-dir>/ca.crt" and matching key in "--ca-key".
+If "ca.crt" contains more than one certificate, the first is used.
+Creation fails if the CA expiration time is before the desired certificate expiration.
+`,
+	Args: func(cmd *cobra.Command, args []string) error {
+		if len(args) == 0 {
+			return errors.Errorf("create-sql-node requires at least one host name or address, none was specified")
+		}
+		return nil
+	},
+	RunE: clierrorplus.MaybeDecorateError(runCreateSQLNodeCert),
+}
+
+// runCreateSQLNodeCert generates key pair and CA certificate and writes them
+// to their corresponding files.
+// TODO(marc): there is currently no way to specify which CA cert to use if more
+// than one is present. We shoult try to load each certificate along with the key
+// and pick the one that works. That way, the key specifies the certificate.
+func runCreateSQLNodeCert(cmd *cobra.Command, args []string) error {
+	return errors.Wrap(
+		security.CreateSQLNodePair(
+			certCtx.certsDir,
+			certCtx.caKey,
+			certCtx.keySize,
+			certCtx.certificateLifetime,
+			certCtx.overwriteFiles,
+			args),
+		"failed to generate SQL server certificate and key")
+}

pkg/rpc/auth.go

@@ -121,21 +121,33 @@ 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?
+		subj := tlsInfo.State.PeerCertificates[0].Subject
+		if security.Contains(subj.OrganizationalUnit, security.TenantsOU) {
+			// Incoming connection originating from a tenant SQL server,
+			// into a KV node.
+			return tenantFromCommonName(subj.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.
+		if !security.Contains(certUsers, security.SQLNodeUser) {
+			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/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,

pkg/security/certificate_loader.go

@@ -89,6 +89,9 @@ const (
 	// NodePem describes the server certificate for the node, possibly a combined server/client
 	// certificate for user Node if a separate 'client.node.crt' is not present.
 	NodePem
+	// TenantNodePem describes the server certificate for a SQL tenant
+	// server, for connections across SQL tenant servers.
+	TenantNodePem
 	// UIPem describes the server certificate for the admin UI.
 	UIPem
 	// ClientPem describes a client certificate.
@@ -125,6 +128,8 @@ func (p PemUsage) String() string {
 		return "UI CA"
 	case NodePem:
 		return "Node"
+	case TenantNodePem:
+		return "Tenant Node"
 	case UIPem:
 		return "UI"
 	case ClientPem:
@@ -215,6 +220,11 @@ func CertInfoFromFilename(filename string) (*CertInfo, error) {
 		if numParts != 2 {
 			return nil, errors.Errorf("node certificate filename should match node%s", certExtension)
 		}
+	case `sql-node`:
+		fileUsage = TenantNodePem
+		if numParts != 2 {
+			return nil, errors.Errorf("SQL node certificate filename should match node%s", certExtension)
+		}
 	case `ui`:
 		fileUsage = UIPem
 		if numParts != 2 {
@@ -459,7 +469,7 @@ func parseCertificate(ci *CertInfo) error {
 // This should only be called on the NodePem CertInfo when there is no specific
 // client certificate for the 'node' user.
 // Fields required for a valid server certificate are already checked.
-func validateDualPurposeNodeCert(ci *CertInfo) error {
+func validateDualPurposeNodeCert(ci *CertInfo, nodeUser string) error {
 	if ci == nil {
 		return errors.Errorf("no node certificate found")
 	}
@@ -471,9 +481,9 @@ func validateDualPurposeNodeCert(ci *CertInfo) error {
 	// The first certificate is used in client auth.
 	cert := ci.ParsedCertificates[0]
 	principals := getCertificatePrincipals(cert)
-	if !Contains(principals, NodeUser) {
+	if !Contains(principals, nodeUser) {
 		return errors.Errorf("client/server node certificate has principals %q, expected %q",
-			principals, NodeUser)
+			principals, nodeUser)
 	}
 
 	return nil
@@ -487,6 +497,9 @@ func validateCockroachCertificate(ci *CertInfo, cert *x509.Certificate) error {
 	case NodePem:
 		// Common Name is checked only if there is no client certificate for 'node'.
 		// This is done in validateDualPurposeNodeCert.
+	case TenantNodePem:
+		// Common Name is checked only if there is no client certificate for 'sql-node'.
+		// This is done in validateDualPurposeNodeCert.
 	case ClientPem:
 		// Check that CommonName matches the username extracted from the filename.
 		principals := getCertificatePrincipals(cert)

pkg/security/certificate_loader_test.go

@@ -49,6 +49,7 @@ func TestCertNomenclature(t *testing.T) {
 		{"ca-client.crt", "", security.ClientCAPem, ""},
 		{"ca-ui.crt", "", security.UICAPem, ""},
 		{"node.crt", "", security.NodePem, ""},
+		{"sql-node.crt", "", security.TenantNodePem, ""},
 		{"ui.crt", "", security.UIPem, ""},
 		{"client.root.crt", "", security.ClientPem, "root"},
 		{"client.foo-bar.crt", "", security.ClientPem, "foo-bar"},
@@ -191,6 +192,9 @@ func TestNamingScheme(t *testing.T) {
 	parsedGoodNodeCert, goodNodeCert := makeTestCert(t, "node", fullKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth})
 	_, badUserNodeCert := makeTestCert(t, "notnode", fullKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth})
 
+	parsedGoodSQLNodeCert, goodSQLNodeCert := makeTestCert(t, "sql-node", fullKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth})
+	_, badUserSQLNodeCert := makeTestCert(t, "notsqlnode", fullKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth})
+
 	parsedGoodRootCert, goodRootCert := makeTestCert(t, "root", fullKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth})
 	_, notRootCert := makeTestCert(t, "notroot", fullKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth})
 
@@ -250,6 +254,7 @@ func TestNamingScheme(t *testing.T) {
 				{"cr..crt", 0777, []byte{}},
 				{"node.foo.crt", 0777, []byte{}},
 				{"node..crt", 0777, []byte{}},
+				{"sql-node..crt", 0777, []byte{}},
 				{"client.crt", 0777, []byte{}},
 				{"client..crt", 0777, []byte{}},
 			},
@@ -260,6 +265,7 @@ func TestNamingScheme(t *testing.T) {
 			files: []testFile{
 				{"ca.crt", 0777, caCert},
 				{"node.crt", 0777, goodNodeCert},
+				{"sql-node.crt", 0777, goodSQLNodeCert},
 				{"client.root.crt", 0777, goodRootCert},
 			},
 			certs: []security.CertInfo{
@@ -268,6 +274,8 @@ func TestNamingScheme(t *testing.T) {
 					Error: errors.New(".* no such file or directory")},
 				{FileUsage: security.NodePem, Filename: "node.crt",
 					Error: errors.New(".* no such file or directory")},
+				{FileUsage: security.TenantNodePem, Filename: "sql-node.crt",
+					Error: errors.New(".* no such file or directory")},
 			},
 		},
 		{
@@ -278,6 +286,8 @@ func TestNamingScheme(t *testing.T) {
 				{"ca.key", 0777, []byte{}},
 				{"node.crt", 0777, goodNodeCert},
 				{"node.key", 0704, []byte{}},
+				{"sql-node.crt", 0777, goodSQLNodeCert},
+				{"sql-node.key", 0704, []byte{}},
 				{"client.root.crt", 0777, goodRootCert},
 				{"client.root.key", 0704, []byte{}},
 			},
@@ -287,6 +297,8 @@ func TestNamingScheme(t *testing.T) {
 					Error: permissionRequirementErr},
 				{FileUsage: security.NodePem, Filename: "node.crt",
 					Error: permissionRequirementErr},
+				{FileUsage: security.TenantNodePem, Filename: "sql-node.crt",
+					Error: permissionRequirementErr},
 			},
 			skipWindows: true,
 		},
@@ -295,6 +307,8 @@ func TestNamingScheme(t *testing.T) {
 			files: []testFile{
 				{"node.crt", 0777, badUserNodeCert},
 				{"node.key", 0700, []byte("node.key")},
+				{"sql-node.crt", 0777, badUserSQLNodeCert},
+				{"sql-node.key", 0700, []byte("sql-node.key")},
 				{"client.root.crt", 0777, notRootCert},
 				{"client.root.key", 0700, []byte{}},
 			},
@@ -303,6 +317,8 @@ func TestNamingScheme(t *testing.T) {
 					Error: errors.New(`client certificate has principals \["notroot"\], expected "root"`)},
 				{FileUsage: security.NodePem, Filename: "node.crt", KeyFilename: "node.key",
 					FileContents: badUserNodeCert, KeyFileContents: []byte("node.key")},
+				{FileUsage: security.TenantNodePem, Filename: "sql-node.crt", KeyFilename: "sql-node.key",
+					FileContents: badUserSQLNodeCert, KeyFileContents: []byte("sql-node.key")},
 			},
 		},
 		{
@@ -312,6 +328,8 @@ func TestNamingScheme(t *testing.T) {
 				{"ca.key", 0700, []byte("ca.key")},
 				{"node.crt", 0777, goodNodeCert},
 				{"node.key", 0700, []byte("node.key")},
+				{"sql-node.crt", 0777, goodSQLNodeCert},
+				{"sql-node.key", 0700, []byte("sql-node.key")},
 				{"client.root.crt", 0777, goodRootCert},
 				{"client.root.key", 0700, []byte("client.root.key")},
 			},
@@ -321,6 +339,8 @@ func TestNamingScheme(t *testing.T) {
 					Name: "root", FileContents: goodRootCert, KeyFileContents: []byte("client.root.key")},
 				{FileUsage: security.NodePem, Filename: "node.crt", KeyFilename: "node.key",
 					FileContents: goodNodeCert, KeyFileContents: []byte("node.key")},
+				{FileUsage: security.TenantNodePem, Filename: "sql-node.crt", KeyFilename: "sql-node.key",
+					FileContents: goodSQLNodeCert, KeyFileContents: []byte("sql-node.key")},
 			},
 		},
 		{
@@ -330,6 +350,8 @@ func TestNamingScheme(t *testing.T) {
 				{"ca.key", 0700, []byte("ca.key")},
 				{"node.crt", 0777, append(goodNodeCert, caCert...)},
 				{"node.key", 0700, []byte("node.key")},
+				{"sql-node.crt", 0777, append(goodSQLNodeCert, caCert...)},
+				{"sql-node.key", 0700, []byte("sql-node.key")},
 				{"client.root.crt", 0777, append(goodRootCert, caCert...)},
 				{"client.root.key", 0700, []byte("client.root.key")},
 			},
@@ -341,6 +363,9 @@ func TestNamingScheme(t *testing.T) {
 				{FileUsage: security.NodePem, Filename: "node.crt", KeyFilename: "node.key",
 					FileContents: append(goodNodeCert, caCert...), KeyFileContents: []byte("node.key"),
 					ParsedCertificates: []*x509.Certificate{parsedGoodNodeCert, parsedCACert}},
+				{FileUsage: security.TenantNodePem, Filename: "sql-node.crt", KeyFilename: "sql-node.key",
+					FileContents: append(goodSQLNodeCert, caCert...), KeyFileContents: []byte("sql-node.key"),
+					ParsedCertificates: []*x509.Certificate{parsedGoodSQLNodeCert, parsedCACert}},
 			},
 		},
 		{
@@ -350,6 +375,8 @@ func TestNamingScheme(t *testing.T) {
 				{"ca.key", 0777, []byte("ca.key")},
 				{"node.crt", 0777, goodNodeCert},
 				{"node.key", 0777, []byte("node.key")},
+				{"sql-node.crt", 0777, goodSQLNodeCert},
+				{"sql-node.key", 0777, []byte("sql-node.key")},
 				{"client.root.crt", 0777, goodRootCert},
 				{"client.root.key", 0777, []byte("client.root.key")},
 			},
@@ -359,6 +386,8 @@ func TestNamingScheme(t *testing.T) {
 					Name: "root", FileContents: goodRootCert, KeyFileContents: []byte("client.root.key")},
 				{FileUsage: security.NodePem, Filename: "node.crt", KeyFilename: "node.key",
 					FileContents: goodNodeCert, KeyFileContents: []byte("node.key")},
+				{FileUsage: security.TenantNodePem, Filename: "sql-node.crt", KeyFilename: "sql-node.key",
+					FileContents: goodSQLNodeCert, KeyFileContents: []byte("sql-node.key")},
 			},
 			skipChecks: true,
 		},