rpc,security,sql,cli: Add subject_required cluster setting

Previous in sequence: cockroachdb#120786
informs cockroachdb#110616, cockroachdb#118750
fixes CRDB-35884
Epic CRDB-34126

We will be adding a cluster setting `server.client_cert.subject_required.enabled` which
will mandate a requirement for role subject to be set either through subject
role option or root-cert-distinguished-name and node-cert-distinguished-name. It
controls both RPC access and login via authCert.

Release note: None

docs/generated/settings/settings-for-tenants.txt

@@ -77,6 +77,7 @@ server.auth_log.sql_connections.enabled	boolean	false	if set, log SQL client con
 server.auth_log.sql_sessions.enabled	boolean	false	if set, log SQL session login/disconnection events (note: may hinder performance on loaded nodes)	application
 server.authentication_cache.enabled	boolean	true	enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information	application
 server.child_metrics.enabled	boolean	false	enables the exporting of child metrics, additional prometheus time series with extra labels	application
+server.client_cert.subject_required.enabled	boolean	false	mandates a requirement for subject role to be set for db user	application
 server.client_cert_expiration_cache.capacity	integer	1000	the maximum number of client cert expirations stored	application
 server.clock.forward_jump_check.enabled	boolean	false	if enabled, forward clock jumps > max_offset/2 will cause a panic	application
 server.clock.persist_upper_bound_interval	duration	0s	the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.	application

docs/generated/settings/settings.html

@@ -104,6 +104,7 @@
 <tr><td><div id="setting-server-auth-log-sql-sessions-enabled" class="anchored"><code>server.auth_log.sql_sessions.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if set, log SQL session login/disconnection events (note: may hinder performance on loaded nodes)</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-authentication-cache-enabled" class="anchored"><code>server.authentication_cache.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-child-metrics-enabled" class="anchored"><code>server.child_metrics.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables the exporting of child metrics, additional prometheus time series with extra labels</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-server-client-cert-subject-required-enabled" class="anchored"><code>server.client_cert.subject_required.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>mandates a requirement for subject role to be set for db user</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-client-cert-expiration-cache-capacity" class="anchored"><code>server.client_cert_expiration_cache.capacity</code></div></td><td>integer</td><td><code>1000</code></td><td>the maximum number of client cert expirations stored</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-clock-forward-jump-check-enabled" class="anchored"><code>server.clock.forward_jump_check.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if enabled, forward clock jumps &gt; max_offset/2 will cause a panic</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-clock-persist-upper-bound-interval" class="anchored"><code>server.clock.persist_upper_bound_interval</code></div></td><td>duration</td><td><code>0s</code></td><td>the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/cli/interactive_tests/test_distinguished_name_validation.tcl

@@ -105,7 +105,7 @@ proc generate_root_or_node_cert {argv certs_dir name} {
 proc set_role_subject_for_user {argv name role_subject} {
     report "SETTING SUBJECT ROLE OPTION for USER $name with SUBJECT $role_subject"
     send "$argv sql --certs-dir=$::certs_dir -e 'alter role $name with subject \"$role_subject\" login';\r"
-    eexpect $::prompt
+    eexpect "ALTER ROLE"
 }
 
 start_secure_server $argv $certs_dir ""
@@ -158,4 +158,23 @@ send "$argv sql --certs-dir=$certs_dir --user=root -e 'select 1'\r"
 eexpect $::prompt
 end_test
 
+# Check cert still works without setting role subject option when cluster setting subject_required = false
+send "$argv sql --certs-dir=$certs_dir --user=gallant -e 'select 1'\r"
+eexpect $::prompt
+
+# Check cert fails without setting role subject option when cluster setting subject_required = true
+start_test "validating cert auth fails without subject role option when subject_required cluster setting is set"
+send "$argv sql --certs-dir=$certs_dir --user=root -e 'SET CLUSTER SETTING server.client_cert.subject_required.enabled = true'\r"
+eexpect $::prompt
+send "$argv sql --certs-dir=$certs_dir --user=gallant -e 'select 1'\r"
+eexpect "db user gallant does not have a distinguished name set which subject_required cluster setting mandates"
+end_test
+
+# Check cert succeeds after setting role subject option when cluster setting subject_required = true
+start_test "validating cert auth succeeds with valid subject role option when subject_required cluster setting is set"
+set_role_subject_for_user $argv gallant "O=Cockroach,CN=gallant"
+send "$argv sql --certs-dir=$certs_dir --user=gallant -e 'select 1'\r"
+eexpect "(1 row)"
+end_test
+
 stop_server $argv

pkg/rpc/auth.go

@@ -309,13 +309,19 @@ func (a kvAuth) authenticateNetworkRequest(ctx context.Context) (authnResult, er
 	// matches our own). The client could also present a certificate with subject
 	// DN equalling rootSubject or nodeSubject set using
 	// root-cert-distinguished-name and node-cert-distinguished-name cli flags
-	// respectively.
+	// respectively. Additionally if subject_required cluster setting is set, both
+	// root and node users must have a valid DN set.
 	//
 	// 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.CheckRootAndNodeSubjectRequiredSatisfied(a.sv) {
+		return nil, authErrorf(
+			"root and node roles do not have valid DNs set which subject_required cluster setting mandates",
+		)
+	}
 	rootOrNodeDNSet, certDNMatchesRootOrNodeDN := security.CheckCertDNMatchesRootDNorNodeDN(clientCert)
 	if rootOrNodeDNSet && !certDNMatchesRootOrNodeDN {
 		return nil, authErrorf(

pkg/rpc/auth_test.go

@@ -18,6 +18,7 @@ import (
 	"encoding/asn1"
 	"fmt"
 	"io"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -110,6 +111,7 @@ func TestAuthenticateTenant(t *testing.T) {
 		commonName       string
 		rootDNString     string
 		nodeDNString     string
+		subjectRequired  bool
 		expTenID         roachpb.TenantID
 		expErr           string
 		tenantScope      uint64
@@ -192,6 +194,12 @@ func TestAuthenticateTenant(t *testing.T) {
 			expErr: `need root or node client cert to perform RPCs on this server \(this is tenant system; cert is valid for "foo" on all tenants, "bar" on all tenants\)`},
 		{systemID: stid, ous: nil, commonName: "foo", certDNSName: "bar", certPrincipalMap: "bar:root"},
 		{systemID: stid, ous: nil, commonName: "foo", certDNSName: "bar", certPrincipalMap: "bar:node"},
+		{systemID: stid, ous: nil, commonName: "foo", subjectRequired: true,
+			expErr: `root and node roles do not have valid DNs set which subject_required cluster setting mandates`},
+		{systemID: stid, ous: nil, commonName: "foo", subjectRequired: true, rootDNString: "CN=foo",
+			expErr: `root and node roles do not have valid DNs set which subject_required cluster setting mandates`},
+		{systemID: stid, ous: nil, commonName: "foo", subjectRequired: true,
+			rootDNString: "CN=foo", nodeDNString: "CN=bar"},
 	} {
 		t.Run(fmt.Sprintf("from %v to %v (md %q)", tc.commonName, tc.systemID, tc.clientTenantInMD), func(t *testing.T) {
 			err := security.SetCertPrincipalMap(strings.Split(tc.certPrincipalMap, ","))
@@ -248,7 +256,10 @@ func TestAuthenticateTenant(t *testing.T) {
 				ctx = metadata.NewIncomingContext(ctx, md)
 			}
 
-			tenID, err := rpc.TestingAuthenticateTenant(ctx, tc.systemID)
+			clusterSettings := map[settings.InternalKey]settings.EncodedValue{
+				security.ClientCertSubjectRequiredSettingName: {strconv.FormatBool(tc.subjectRequired), "b"},
+			}
+			tenID, err := rpc.TestingAuthenticateTenant(ctx, tc.systemID, clusterSettings)
 
 			if tc.expErr == "" {
 				require.Equal(t, tc.expTenID, tenID)

pkg/rpc/helpers_test.go

@@ -39,9 +39,22 @@ func TestingNewWrappedServerStream(
 // TestingAuthenticateTenant performs authentication of a tenant from a context
 // for testing.
 func TestingAuthenticateTenant(
-	ctx context.Context, serverTenantID roachpb.TenantID,
+	ctx context.Context,
+	serverTenantID roachpb.TenantID,
+	clusterSettings map[settings.InternalKey]settings.EncodedValue,
 ) (roachpb.TenantID, error) {
-	_, authz, err := kvAuth{tenant: tenantAuthorizer{tenantID: serverTenantID}}.authenticateAndSelectAuthzRule(ctx)
+	sv := &settings.Values{}
+	sv.Init(ctx, settings.TestOpaque)
+	u := settings.NewUpdater(sv)
+
+	kvAuthObject := kvAuth{sv: sv, tenant: tenantAuthorizer{tenantID: serverTenantID}}
+	for setting := range clusterSettings {
+		err := u.Set(ctx, setting, clusterSettings[setting])
+		if err != nil {
+			return roachpb.TenantID{}, err
+		}
+	}
+	_, authz, err := kvAuthObject.authenticateAndSelectAuthzRule(ctx)
 	if err != nil {
 		return roachpb.TenantID{}, err
 	}

pkg/security/BUILD.bazel

@@ -5,6 +5,7 @@ go_library(
     srcs = [
         "auth.go",
         "cert_expiry_cache.go",
+        "cert_settings.go",
         "certificate_loader.go",
         "certificate_manager.go",
         "certificate_metrics.go",

pkg/security/auth.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/distinguishedname"
 	"github.com/cockroachdb/cockroach/pkg/security/password"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/errors"
@@ -119,12 +120,12 @@ type UserAuthHook func(
 	clientConnection bool,
 ) error
 
-// applyRootOrNodeDNFlag returns distinguished name set for root or node user
+// ApplyRootOrNodeDNFlag returns distinguished name set for root or node user
 // via root-cert-distinguished-name and node-cert-distinguished flags
 // respectively if systemIdentity conforms to one of these 2 users. It may also
 // return previously set subject option and systemIdentity is not root or node.
 // Root and Node roles cannot have subject role option set for them.
-func applyRootOrNodeDNFlag(
+func ApplyRootOrNodeDNFlag(
 	previouslySetRoleSubject *ldap.DN, systemIdentity username.SQLUsername,
 ) (dn *ldap.DN) {
 	dn = previouslySetRoleSubject
@@ -162,6 +163,17 @@ func CheckCertDNMatchesRootDNorNodeDN(
 	return rootOrNodeDNSet, certDNMatchesRootOrNodeDN
 }
 
+// CheckRootAndNodeSubjectRequiredSatisfied mandates root and node users to have
+// valid DNs set if subject_required cluster setting is set to true.
+func CheckRootAndNodeSubjectRequiredSatisfied(sv *settings.Values) bool {
+	if ClientCertSubjectRequired.Get(sv) {
+		rootDN := rootSubjectMu.getDN()
+		nodeDN := nodeSubjectMu.getDN()
+		return rootDN != nil && nodeDN != nil
+	}
+	return true
+}
+
 // SetCertPrincipalMap sets the global principal map. Each entry in the mapping
 // list must either be empty or have the format <source>:<dest>. The principal
 // map is used to transform principal names found in the Subject.CommonName or
@@ -298,8 +310,6 @@ func UserAuthCertHook(
 			return errors.Errorf("using tenant client certificate as user certificate is not allowed")
 		}
 
-		roleSubject = applyRootOrNodeDNFlag(roleSubject, systemIdentity)
-
 		var certSubject *ldap.DN
 		if roleSubject != nil {
 			var err error

pkg/security/auth_test.go

@@ -458,6 +458,7 @@ func TestAuthenticationHook(t *testing.T) {
 				}
 			}
 
+			roleSubject = security.ApplyRootOrNodeDNFlag(roleSubject, tc.username)
 			hook, err := security.UserAuthCertHook(
 				tc.insecure,
 				makeFakeTLSState(t, tc.tlsSpec),

pkg/security/cert_settings.go

@@ -0,0 +1,32 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package security
+
+import "github.com/cockroachdb/cockroach/pkg/settings"
+
+// All cluster settings necessary for tls client cert authentication.
+const (
+	baseClientCertSettingName            = "server.client_cert."
+	ClientCertSubjectRequiredSettingName = baseClientCertSettingName + "subject_required.enabled"
+)
+
+// ClientCertSubjectRequired mandates a requirement for role subject to be set
+// either through subject role option or root-cert-distinguished-name and
+// node-cert-distinguished-name. It controls both RPC access and login via
+// authCert
+var ClientCertSubjectRequired = settings.RegisterBoolSetting(
+	settings.ApplicationLevel,
+	ClientCertSubjectRequiredSettingName,
+	"mandates a requirement for subject role to be set for db user",
+	false,
+	settings.WithPublic,
+	settings.WithReportable(true),
+)

pkg/sql/pgwire/auth_methods.go

@@ -441,6 +441,14 @@ func authCert(
 			log.Ops.Warningf(ctx, "failed to get cert manager info: %v", err)
 		}
 
+		roleSubject = security.ApplyRootOrNodeDNFlag(roleSubject, systemIdentity)
+		if security.ClientCertSubjectRequired.Get(&execCfg.Settings.SV) && roleSubject == nil {
+			return errors.Newf(
+				"db user %v does not have a distinguished name set which subject_required cluster setting mandates",
+				systemIdentity.Normalized(),
+			)
+		}
+
 		hook, err := security.UserAuthCertHook(
 			false, /*insecure*/
 			&tlsState,