From e4aa11eb11894524b073cdea559f52137525e4e4 Mon Sep 17 00:00:00 2001 From: Sourav Sarangi Date: Thu, 28 Mar 2024 18:31:40 +0530 Subject: [PATCH] rpc,security, cli: Add cert-distinguished-name root/node cli flag Previous in sequence: https://github.com/cockroachdb/cockroach/pull/119958 informs #110616, #118750 fixes CRDB-35884 Epic CRDB-34126 We will be adding 2 new cli flags `root-cert-distinguished-name` and `node-cert-distinguished-name` to provide option to have subject DN for root and node user during server startup. This will enforce the provided certificate by client to exactly match the value set by the above flags both for sql client and RPC authentication. This is needed because subject role option cannot be set for root and node users. Post this the plan is to add a cluster setting `server.client_cert.subject_required` which will mandate that any auth which happens should verify certSubject with rootSubject in case of root user, with nodeSubject in case of node user, with roleSubject otherwise. Release note: None --- pkg/acceptance/generated_cli_test.go | 7 + pkg/cli/cliflags/flags.go | 20 +++ pkg/cli/context.go | 4 + pkg/cli/flags.go | 18 +++ .../test_distinguished_name_validation.tcl | 144 ++++++++++++++++++ pkg/rpc/auth.go | 21 ++- pkg/rpc/auth_test.go | 46 ++++++ pkg/security/auth.go | 102 ++++++++++++- pkg/security/auth_test.go | 100 +++++++----- 9 files changed, 419 insertions(+), 43 deletions(-) create mode 100755 pkg/cli/interactive_tests/test_distinguished_name_validation.tcl diff --git a/pkg/acceptance/generated_cli_test.go b/pkg/acceptance/generated_cli_test.go index 3da13aaa5414..791de85c063d 100644 --- a/pkg/acceptance/generated_cli_test.go +++ b/pkg/acceptance/generated_cli_test.go @@ -172,6 +172,13 @@ func TestDockerCLI_test_disable_replication(t *testing.T) { runTestDockerCLI(t, "test_disable_replication", "../cli/interactive_tests/test_disable_replication.tcl") } +func TestDockerCLI_test_distinguished_name_validation(t *testing.T) { + s := log.Scope(t) + defer s.Close(t) + + runTestDockerCLI(t, "test_distinguished_name_validation", "../cli/interactive_tests/test_distinguished_name_validation.tcl") +} + func TestDockerCLI_test_dump_sig(t *testing.T) { s := log.Scope(t) defer s.Close(t) diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go index d2ca2cd83d5a..0907bec6fcac 100644 --- a/pkg/cli/cliflags/flags.go +++ b/pkg/cli/cliflags/flags.go @@ -873,6 +873,26 @@ fields. It is permissible for the string to contain colons. `, } + RootCertDistinguishedName = FlagInfo{ + Name: "root-cert-distinguished-name", + Description: ` +A string of comma separated list of distinguished-name += mappings in accordance with RFC4514 for the root +user. This strictly needs to match the DN subject in the client certificate +provided for root user if this flag is set. +`, + } + + NodeCertDistinguishedName = FlagInfo{ + Name: "node-cert-distinguished-name", + Description: ` +A string of comma separated list of distinguished-name += mappings in accordance with RFC4514 for the node +user. This strictly needs to match the DN subject in the client certificate +provided for node user if this flag is set. +`, + } + CAKey = FlagInfo{ Name: "ca-key", EnvVar: "COCKROACH_CA_KEY", diff --git a/pkg/cli/context.go b/pkg/cli/context.go index 1d8ffd02f499..311e8f5b6555 100644 --- a/pkg/cli/context.go +++ b/pkg/cli/context.go @@ -479,6 +479,8 @@ var startCtx struct { serverSSLCertsDir string serverCertPrincipalMap []string serverListenAddr string + serverRootCertDN string + serverNodeCertDN string // The TLS auto-handshake parameters. initToken string @@ -532,6 +534,8 @@ func setStartContextDefaults() { startCtx.serverInsecure = baseCfg.Insecure startCtx.serverSSLCertsDir = base.DefaultCertsDirectory startCtx.serverCertPrincipalMap = nil + startCtx.serverRootCertDN = "" + startCtx.serverNodeCertDN = "" startCtx.serverListenAddr = "" startCtx.unencryptedLocalhostHTTP = false startCtx.tempDir = "" diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index 74783ba1e03d..56ee3b52c970 100644 --- a/pkg/cli/flags.go +++ b/pkg/cli/flags.go @@ -493,6 +493,12 @@ func init() { // Certificate principal map. cliflagcfg.StringSliceFlag(f, &startCtx.serverCertPrincipalMap, cliflags.CertPrincipalMap) + // Root cert distinguished name + cliflagcfg.StringFlag(f, &startCtx.serverRootCertDN, cliflags.RootCertDistinguishedName) + + // Node cert distinguished name + cliflagcfg.StringFlag(f, &startCtx.serverNodeCertDN, cliflags.NodeCertDistinguishedName) + // Cluster name verification. cliflagcfg.VarFlag(f, clusterNameSetter{&baseCfg.ClusterName}, cliflags.ClusterName) cliflagcfg.BoolFlag(f, &baseCfg.DisableClusterNameVerification, cliflags.DisableClusterNameVerification) @@ -577,6 +583,12 @@ func init() { // All certs command want to map CNs to SQL principals. cliflagcfg.StringSliceFlag(f, &certCtx.certPrincipalMap, cliflags.CertPrincipalMap) + // Root cert distinguished name + cliflagcfg.StringFlag(f, &startCtx.serverRootCertDN, cliflags.RootCertDistinguishedName) + + // Node cert distinguished name + cliflagcfg.StringFlag(f, &startCtx.serverNodeCertDN, cliflags.NodeCertDistinguishedName) + if cmd == listCertsCmd { // The 'list' subcommand does not write to files and thus does // not need the arguments below. @@ -1083,6 +1095,12 @@ func extraServerFlagInit(cmd *cobra.Command) error { if err := security.SetCertPrincipalMap(startCtx.serverCertPrincipalMap); err != nil { return err } + if err := security.SetRootSubject(startCtx.serverRootCertDN); err != nil { + return err + } + if err := security.SetNodeSubject(startCtx.serverNodeCertDN); err != nil { + return err + } serverCfg.User = username.NodeUserName() serverCfg.Insecure = startCtx.serverInsecure serverCfg.SSLCertsDir = startCtx.serverSSLCertsDir diff --git a/pkg/cli/interactive_tests/test_distinguished_name_validation.tcl b/pkg/cli/interactive_tests/test_distinguished_name_validation.tcl new file mode 100755 index 000000000000..7313aa583a11 --- /dev/null +++ b/pkg/cli/interactive_tests/test_distinguished_name_validation.tcl @@ -0,0 +1,144 @@ +#! /usr/bin/env expect -f + +source [file join [file dirname $argv0] common.tcl] +variable certs_dir "certs" +variable custom_ca_dir "custom-ca-directory" +variable db_dir "logs/db" + +set ::env(COCKROACH_INSECURE) "false" +set ::env(COCKROACH_HOST) "localhost" +spawn /bin/bash +send "PS1=':''/# '\r" + +variable prompt ":/# " +eexpect $prompt + + +send "mkdir -p $certs_dir\r" +eexpect $prompt +send "mkdir -p $custom_ca_dir\r" +eexpect $prompt + +send "$argv cert create-ca --certs-dir=$certs_dir --ca-key=$certs_dir/ca.key\r" +eexpect $prompt + +# Copy openssl CA cnf for generating custom client certs. +set ca_cnf_file [file join [file dirname $argv0] "ocsp_ca.cnf"] +send "cat $ca_cnf_file > $custom_ca_dir/ca.cnf\r" +eexpect $prompt + +report "GENERATING serial.txt index.txt files" +send "touch index.txt; echo '01' > serial.txt\r" +eexpect $prompt + +send "$argv cert create-node localhost --certs-dir=$certs_dir --ca-key=$certs_dir/ca.key\r" +eexpect $prompt +send "$argv cert create-client root --certs-dir=$certs_dir --ca-key=$certs_dir/ca.key\r" +eexpect $prompt + +proc start_secure_server {argv certs_dir extra} { + report "BEGIN START SECURE SERVER" + system "$argv start-single-node --host=localhost --socket-dir=. --certs-dir=$certs_dir --store=$::db_dir --pid-file=server_pid --background $extra >>expect-cmd.log 2>&1; + $argv sql --certs-dir=$certs_dir -e 'select 1'; + $argv sql --certs-dir=$certs_dir -e 'SET CLUSTER SETTING enterprise.license = \"crl-0-EJL04ukFGAEiI0NvY2tyb2FjaCBMYWJzIC0gUHJvZHVjdGlvbiBUZXN0aW5n\"'; + $argv sql --certs-dir=$certs_dir -e 'SET CLUSTER SETTING cluster.organization = \"Cockroach Labs - Production Testing\"';" + report "END START SECURE SERVER" +} + +proc restart_secure_server_distinguished_name_flags {argv certs_dir root_dn node_dn extra} { + stop_server $argv + report "BEGIN START SECURE SERVER WITH DN FLAGS" + system "rm -f server_pid; + $argv start-single-node --host=localhost --root-cert-distinguished-name='$root_dn' --node-cert-distinguished-name='$node_dn' --socket-dir=. --certs-dir=$certs_dir --store=$::db_dir --pid-file=server_pid --background $extra >>expect-cmd.log 2>&1;" + report "END START SECURE SERVER WITH DN FLAGS" +} + +proc expect_exit_status {expected} { + set status [lindex [wait] 3] + if {$status != $expected} { + report "unexpected exit status $status" + exit 1 + } +} + +proc create_user_cert {argv certs_dir name} { + report "GENERATING CERT FOR USER $name" + send "openssl genrsa -out $::certs_dir/client.$name.key\r" + eexpect $::prompt + send "openssl req -new -key $::certs_dir/client.$name.key -out client.$name.csr -batch -subj /O=Cockroach/CN=$name\r" + eexpect $::prompt + send "openssl ca -config $::custom_ca_dir/ca.cnf -keyfile $::certs_dir/ca.key -cert $::certs_dir/ca.crt -policy signing_policy -extensions signing_client_req -out $certs_dir/client.$name.crt -outdir . -in client.$name.csr -batch\r" + eexpect $::prompt + # Uncomment the next line to see more details about the generated cert + #system "openssl x509 -in $::certs_dir/client.$name.crt -text" + send "$argv sql --certs-dir=$::certs_dir -e 'create user $name'\r" + eexpect $::prompt + send "$argv sql --certs-dir=$::certs_dir --user=$name -e 'select 1'\r" + eexpect $::prompt +} + +proc generate_root_or_node_cert {argv certs_dir name} { + report "GENERATING CERT FOR USER $name" + system "openssl genrsa -out $::certs_dir/client.$name.key" + system "openssl req -new -key $::certs_dir/client.$name.key -out client.$name.csr -batch -subj /O=Cockroach/CN=$name" + system "openssl ca -config $::custom_ca_dir/ca.cnf -keyfile $::certs_dir/ca.key -cert $::certs_dir/ca.crt -policy signing_policy -extensions signing_client_req -out $certs_dir/client.$name.crt -outdir . -in client.$name.csr -batch" + # Uncomment the next line to see more details about the generated cert + #system "openssl x509 -in $::certs_dir/client.$name.crt -text" + system "$argv sql --certs-dir=$::certs_dir --user=$name -e 'select 1'" +} + +proc set_role_subject_for_user {argv name role_subject} { + report "SETTING SUBJECT ROLE OPTION for USER $name with SUBJECT $role_subject" + system "$argv sql --certs-dir=$::certs_dir -e 'alter role $name with subject \"$role_subject\" login'" +} + +start_secure_server $argv $certs_dir "" + +# Create users and make sure they can each log in. +create_user_cert $argv $certs_dir goofus +create_user_cert $argv $certs_dir gallant + +# Check cert still works without setting role subject option +system "$argv sql --certs-dir=$certs_dir --user=goofus -e 'select 1'" + +report "Validating subject role option can be set and enforced" + +start_test "invalid role option for cert user goofus" +# Set invalid role subject option for user and check login fails +set_role_subject_for_user $argv goofus "O=foo" +spawn $argv "sql" "--certs-dir=$certs_dir" "--user=goofus" "-e" "select 1" +eexpect "certificate authentication failed for user \"goofus\"" +expect_exit_status 1 +end_test + +start_test "valid role option for cert user goofus" +# Set valid role subject option for user and check login fails +set_role_subject_for_user $argv goofus "O=Cockroach,CN=goofus" +spawn $argv "sql" "--certs-dir=$certs_dir" "--user=goofus" "-e" "select 1" +end_test + +report "Validating node-cert-distinguished-nane and root-cert-distinguished-name can be set and enforced" +# create a new root certificate using custom ca +send "rm -f $certs_dir/client.root.*\r" +eexpect $::prompt +generate_root_or_node_cert $argv $certs_dir root + +start_test "invalid root-cert-distinguished-name" +# Set invalid root-cert-distinguished-nane for cockroach server and check root login fails +set root_dn "O=foo,CN=invalid" +# need to provide correct node dn to start cockroach server as it depends on this to be equal to node.crt dn subject +set node_dn "O=Cockroach,CN=node" +restart_secure_server_distinguished_name_flags $argv $certs_dir "O=foo,CN=invalid" "O=bar,CN=invalid" "" +spawn $argv "sql" "--certs-dir=$certs_dir" "--user=root" "-e" "select 1" +eexpect "certificate authentication failed for user \"root\" (DN: o=foo,cn=invalid)" +expect_exit_status 1 +end_test + +start_test "valid node-cert-distinguished-nane and root-cert-distinguished-name" +# Set valid role subject option for user and check login fails +stop_server $argv +restart_secure_server_distinguished_name_flags $argv $certs_dir "O=Cockroach,CN=root" "O=Cockroach,CN=node" "" +spawn $argv "sql" "--certs-dir=$certs_dir" "--user=root" "-e" "select 1" +end_test + +stop_server $argv diff --git a/pkg/rpc/auth.go b/pkg/rpc/auth.go index 7fd6251ae8e7..d4452ee29d95 100644 --- a/pkg/rpc/auth.go +++ b/pkg/rpc/auth.go @@ -306,19 +306,28 @@ func (a kvAuth) authenticateNetworkRequest(ctx context.Context) (authnResult, er // In that case, we only allow RPCs if the principal is 'node' or // 'root' and the tenant scope in the cert matches this server // (either the cert has scope "global" or its scope tenant ID - // matches our own). + // 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. // // 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. - certUserScope, err := security.GetCertificateUserScope(clientCert) - if err != nil { - return nil, err + rootOrNodeDNSet, certDNMatchesRootOrNodeDN := security.CheckCertDNMatchesRootDNorNodeDN(clientCert) + if rootOrNodeDNSet && !certDNMatchesRootOrNodeDN { + return nil, authErrorf("certificate dn did not match set root dn or node dn") } - if err := checkRootOrNodeInScope(certUserScope, a.tenant.tenantID); err != nil { - return nil, err + if !rootOrNodeDNSet { + certUserScope, err := security.GetCertificateUserScope(clientCert) + if err != nil { + return nil, err + } + if err := checkRootOrNodeInScope(certUserScope, a.tenant.tenantID); err != nil { + return nil, err + } } if tenantIDFromMetadata.IsSet() { diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index e1ee28155240..fdae75db3206 100644 --- a/pkg/rpc/auth_test.go +++ b/pkg/rpc/auth_test.go @@ -15,8 +15,10 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" + "encoding/asn1" "fmt" "io" + "strings" "testing" "github.com/cockroachdb/cockroach/pkg/keys" @@ -106,10 +108,14 @@ func TestAuthenticateTenant(t *testing.T) { systemID roachpb.TenantID ous []string commonName string + rootDNString string + nodeDNString string expTenID roachpb.TenantID expErr string tenantScope uint64 clientTenantInMD string + certPrincipalMap string + certDNSName string }{ {systemID: stid, ous: correctOU, commonName: "10", expTenID: tenTen}, {systemID: stid, ous: correctOU, commonName: roachpb.MinTenantID.String(), expTenID: roachpb.MinTenantID}, @@ -170,14 +176,54 @@ func TestAuthenticateTenant(t *testing.T) { {clientTenantInMD: "123", systemID: tenTen, ous: nil, commonName: "root", expErr: `client tenant identity \(123\) does not match server`}, + {systemID: stid, ous: nil, commonName: "foo", rootDNString: "CN=foo"}, + {systemID: stid, ous: nil, commonName: "foo", nodeDNString: "CN=foo"}, + {systemID: stid, ous: nil, commonName: "foo", rootDNString: "CN=bar", + 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\)`}, + {systemID: stid, ous: nil, commonName: "foo", nodeDNString: "CN=bar", + 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\)`}, + {systemID: stid, ous: nil, commonName: "foo", certPrincipalMap: "foo:root"}, + {systemID: stid, ous: nil, commonName: "foo", certPrincipalMap: "foo:node"}, + {systemID: stid, ous: nil, commonName: "foo", certPrincipalMap: "foo:bar", + expErr: `need root or node client cert to perform RPCs on this server \(this is tenant system; cert is valid for "bar" on all tenants\)`}, + {systemID: stid, ous: nil, commonName: "foo", certDNSName: "root"}, + {systemID: stid, ous: nil, commonName: "foo", certDNSName: "node"}, + {systemID: stid, ous: nil, commonName: "foo", certDNSName: "bar", + 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"}, } { 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, ",")) + if err != nil { + t.Fatal(err) + } + err = security.SetRootSubject(tc.rootDNString) + if err != nil { + t.Fatalf("could not set root subject DN, err: %v", err) + } + err = security.SetNodeSubject(tc.nodeDNString) + if err != nil { + t.Fatalf("could not set node subject DN, err: %v", err) + } + defer func() { + security.UnsetRootSubject() + security.UnsetNodeSubject() + }() + cert := &x509.Certificate{ Subject: pkix.Name{ CommonName: tc.commonName, OrganizationalUnit: tc.ous, }, } + cert.RawSubject, err = asn1.Marshal(cert.Subject.ToRDNSequence()) + if err != nil { + t.Fatalf("unable to marshal rdn sequence to raw subject, err: %v", err) + } + if tc.certDNSName != "" { + cert.DNSNames = append(cert.DNSNames, tc.certDNSName) + } if tc.tenantScope > 0 { tenantSANs, err := security.MakeTenantURISANs( username.MakeSQLUsernameFromPreNormalizedString(tc.commonName), diff --git a/pkg/security/auth.go b/pkg/security/auth.go index 4b52445f4159..dcb915965219 100644 --- a/pkg/security/auth.go +++ b/pkg/security/auth.go @@ -32,6 +32,16 @@ var certPrincipalMap struct { m map[string]string } +var rootSubject struct { + syncutil.RWMutex + dn *ldap.DN +} + +var nodeSubject struct { + syncutil.RWMutex + dn *ldap.DN +} + // 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 @@ -63,6 +73,92 @@ type UserAuthHook func( clientConnection bool, ) error +func SetRootSubject(rootDNString string) error { + if len(rootDNString) == 0 { + return nil + } + rootDN, err := distinguishedname.ParseDN(rootDNString) + if err != nil { + return errors.Errorf("invalid root cert distinguished name: %q", rootDNString) + } + rootSubject.Lock() + rootSubject.dn = rootDN + rootSubject.Unlock() + return nil +} + +func UnsetRootSubject() { + rootSubject.Lock() + rootSubject.dn = nil + rootSubject.Unlock() +} + +// evaluateRootOrNodeDN 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 +// original subject option if it is set for the role. Root and Node roles cannot +// have subject role option set for them. +func evaluateRootOrNodeDN( + setRoleSubject *ldap.DN, systemIdentity username.SQLUsername, +) (dn *ldap.DN) { + dn = setRoleSubject + switch { + case systemIdentity.IsRootUser(): + rootSubject.RLock() + dn = rootSubject.dn + rootSubject.RUnlock() + case systemIdentity.IsNodeUser(): + nodeSubject.RLock() + dn = nodeSubject.dn + nodeSubject.RUnlock() + } + return dn +} + +func CheckCertDNMatchesRootDNorNodeDN( + cert *x509.Certificate, +) (rootOrNodeDNSet bool, certDNMatchesRootOrNodeDN bool) { + rootSubject.RLock() + rootDN := rootSubject.dn + rootSubject.RUnlock() + + nodeSubject.RLock() + nodeDN := nodeSubject.dn + nodeSubject.RUnlock() + + if rootDN != nil || nodeDN != nil { + certDN, err := distinguishedname.ParseDNFromCertificate(cert) + if err != nil { + return true, false + } + if rootDN != nil && certDN.Equal(rootDN) || nodeDN != nil && certDN.Equal(nodeDN) { + return true, true + } + } + + return false, false +} + +func SetNodeSubject(nodeDNString string) error { + if len(nodeDNString) == 0 { + return nil + } + nodeDN, err := distinguishedname.ParseDN(nodeDNString) + if err != nil { + return errors.Errorf("invalid node cert distinguished name: %q", nodeDNString) + } + nodeSubject.Lock() + nodeSubject.dn = nodeDN + nodeSubject.Unlock() + return nil +} + +func UnsetNodeSubject() { + nodeSubject.Lock() + nodeSubject.dn = nil + nodeSubject.Unlock() +} + // SetCertPrincipalMap sets the global principal map. Each entry in the mapping // list must either be empty or have the format :. The principal // map is used to transform principal names found in the Subject.CommonName or @@ -199,6 +295,8 @@ func UserAuthCertHook( return errors.Errorf("using tenant client certificate as user certificate is not allowed") } + roleSubject = evaluateRootOrNodeDN(roleSubject, systemIdentity) + var certSubject *ldap.DN if roleSubject != nil { var err error @@ -331,8 +429,8 @@ func ValidateUserScope( certSubject *ldap.DN, ) bool { // if subject role option is set, it must match the certificate subject - if roleSubject != nil && !roleSubject.Equal(certSubject) { - return false + if roleSubject != nil { + return roleSubject.Equal(certSubject) } for _, scope := range certUserScope { if scope.Username == user { diff --git a/pkg/security/auth_test.go b/pkg/security/auth_test.go index ff6c50b67447..5ba66a4d3203 100644 --- a/pkg/security/auth_test.go +++ b/pkg/security/auth_test.go @@ -338,94 +338,124 @@ func TestAuthenticationHook(t *testing.T) { fieldMismatchSubjectDNString := "O=Cockroach,OU=Marketing Team,UID=b8b40653-7f74-4f14-8a61-59f7f3b18184,CN=foo" subsetSubjectDNString := "O=Cockroach,OU=Order Processing Team,CN=foo" fieldMismatchOnlyOnCommonNameString := "O=Cockroach,OU=Order Processing Team,UID=b8b40653-7f74-4f14-8a61-59f7f3b18184,CN=bar" + rootDNString := "O=Cockroach,OU=Order Processing Team,UID=b8b40653-7f74-4f14-8a61-59f7f3b18184,CN=root" + nodeDNString := "O=Cockroach,OU=Order Processing Team,UID=b8b40653-7f74-4f14-8a61-59f7f3b18184,CN=node" testCases := []struct { - insecure bool - tlsSpec string - username username.SQLUsername - principalMap string - buildHookSuccess bool - publicHookSuccess bool - privateHookSuccess bool - tenantID roachpb.TenantID - isSubjectRoleOptionSet bool - expectedErr string + insecure bool + tlsSpec string + username username.SQLUsername + distinguishedNameString string + principalMap string + buildHookSuccess bool + publicHookSuccess bool + privateHookSuccess bool + tenantID roachpb.TenantID + isSubjectRoleOptionOrDNSet bool + expectedErr string }{ // Insecure mode, empty username. - {true, "", username.SQLUsername{}, "", true, false, false, roachpb.SystemTenantID, false, `user is missing`}, + {true, "", username.SQLUsername{}, subjectDNString, "", true, false, false, roachpb.SystemTenantID, false, `user is missing`}, // Insecure mode, non-empty username. - {true, "", fooUser, "", true, true, false, roachpb.SystemTenantID, false, `user "foo" is not allowed`}, + {true, "", fooUser, subjectDNString, "", true, true, false, roachpb.SystemTenantID, false, `user "foo" is not allowed`}, // Secure mode, no TLS state. - {false, "", username.SQLUsername{}, "", false, false, false, roachpb.SystemTenantID, false, `no client certificates in request`}, + {false, "", username.SQLUsername{}, subjectDNString, "", false, false, false, roachpb.SystemTenantID, false, `no client certificates in request`}, // Secure mode, bad user. - {false, "(CN=foo)", username.NodeUserName(), "", true, false, false, roachpb.SystemTenantID, + {false, "(CN=foo)", username.NodeUserName(), nodeDNString, "", true, false, false, roachpb.SystemTenantID, false, `certificate authentication failed for user "node"`}, // Secure mode, node user. - {false, "(CN=node)", username.NodeUserName(), "", true, true, true, roachpb.SystemTenantID, false, ``}, + {false, "(CN=node)", username.NodeUserName(), nodeDNString, "", true, true, true, roachpb.SystemTenantID, false, ``}, // Secure mode, node cert and unrelated user. - {false, "(CN=node)", fooUser, "", true, false, false, roachpb.SystemTenantID, + {false, "(CN=node)", fooUser, subjectDNString, "", true, false, false, roachpb.SystemTenantID, false, `certificate authentication failed for user "foo"`}, // Secure mode, root user. - {false, "(CN=root)", username.NodeUserName(), "", true, false, false, roachpb.SystemTenantID, + {false, "(CN=root)", username.NodeUserName(), nodeDNString, "", true, false, false, roachpb.SystemTenantID, false, `certificate authentication failed for user "node"`}, // Secure mode, tenant cert, foo user. - {false, "(OU=Tenants,CN=foo)", fooUser, "", true, false, false, roachpb.SystemTenantID, + {false, "(OU=Tenants,CN=foo)", fooUser, subjectDNString, "", true, false, false, roachpb.SystemTenantID, false, `using tenant client certificate as user certificate is not allowed`}, // Secure mode, multiple cert principals. - {false, "(CN=foo)dns:bar", fooUser, "", true, true, false, roachpb.SystemTenantID, false, `user "foo" is not allowed`}, - {false, "(CN=foo)dns:bar", barUser, "", true, true, false, roachpb.SystemTenantID, false, `user "bar" is not allowed`}, + {false, "(CN=foo)dns:bar", fooUser, subjectDNString, "", true, true, false, roachpb.SystemTenantID, false, `user "foo" is not allowed`}, + {false, "(CN=foo)dns:bar", barUser, subjectDNString, "", true, true, false, roachpb.SystemTenantID, false, `user "bar" is not allowed`}, // Secure mode, principal map. - {false, "(CN=foo)dns:bar", blahUser, "foo:blah", true, true, false, roachpb.SystemTenantID, false, `user "blah" is not allowed`}, - {false, "(CN=foo)dns:bar", blahUser, "bar:blah", true, true, false, roachpb.SystemTenantID, false, `user "blah" is not allowed`}, - {false, "(CN=foo)uri:crdb://tenant/123/user/foo", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123), + {false, "(CN=foo)dns:bar", blahUser, subjectDNString, "foo:blah", true, true, false, roachpb.SystemTenantID, false, `user "blah" is not allowed`}, + {false, "(CN=foo)dns:bar", blahUser, subjectDNString, "bar:blah", true, true, false, roachpb.SystemTenantID, false, `user "blah" is not allowed`}, + {false, "(CN=foo)uri:crdb://tenant/123/user/foo", fooUser, subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123), false, `user "foo" is not allowed`}, - {false, "(CN=foo)uri:crdb://tenant/123/user/foo", fooUser, "", true, false, false, roachpb.SystemTenantID, + {false, "(CN=foo)uri:crdb://tenant/123/user/foo", fooUser, subjectDNString, "", true, false, false, roachpb.SystemTenantID, false, `certificate authentication failed for user "foo"`}, - {false, "(CN=foo)", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123), + {false, "(CN=foo)", fooUser, subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123), false, `user "foo" is not allowed`}, - {false, "(CN=foo)uri:crdb://tenant/1/user/foo", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123), + {false, "(CN=foo)uri:crdb://tenant/1/user/foo", fooUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123), false, `certificate authentication failed for user "foo"`}, - {false, "(CN=foo)uri:crdb://tenant/123/user/foo", blahUser, "", true, false, false, roachpb.MustMakeTenantID(123), + {false, "(CN=foo)uri:crdb://tenant/123/user/foo", blahUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123), false, `certificate authentication failed for user "blah"`}, // Secure mode, client cert having full DN, foo user with subject role // option not set. - {false, "(" + subjectDNString + ")", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123), + {false, "(" + subjectDNString + ")", fooUser, subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123), false, `user "foo" is not allowed`}, // Secure mode, client cert having full DN, foo user with subject role // option set matching TLS cert subject. - {false, "(" + subjectDNString + ")", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123), + {false, "(" + subjectDNString + ")", fooUser, subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123), true, `user "foo" is not allowed`}, // Secure mode, client cert having full DN, foo user with subject role // option set, TLS cert DN empty. - {false, "(CN=foo)", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123), + {false, "(CN=foo)", fooUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123), true, `certificate authentication failed for user "foo"`}, // Secure mode, client cert having full DN, foo user with subject role // option set, TLS cert DN mismatches on OU field. - {false, "(" + fieldMismatchSubjectDNString + ")", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123), + {false, "(" + fieldMismatchSubjectDNString + ")", fooUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123), true, `certificate authentication failed for user "foo"`}, // Secure mode, client cert having full DN, foo user with subject role // option set, TLS cert DN subset of role subject DN. - {false, "(" + subsetSubjectDNString + ")", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123), + {false, "(" + subsetSubjectDNString + ")", fooUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123), true, `certificate authentication failed for user "foo"`}, // Secure mode, client cert having full DN, foo user with subject role // option set mismatching TLS cert subject only on CN(required for // matching) having DNS as foo. - {false, "(" + fieldMismatchOnlyOnCommonNameString + ")dns:foo", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123), + {false, "(" + fieldMismatchOnlyOnCommonNameString + ")dns:foo", fooUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123), true, `certificate authentication failed for user "foo"`}, + {false, "(" + rootDNString + ")", username.RootUserName(), rootDNString, "", true, true, false, roachpb.MustMakeTenantID(123), + true, `user "root" is not allowed`}, + {false, "(" + nodeDNString + ")", username.NodeUserName(), nodeDNString, "", true, true, true, roachpb.MustMakeTenantID(123), + true, ""}, + // tls cert dn matching root dn set, where CN != root + {false, "(" + subjectDNString + ")", username.RootUserName(), subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123), + true, `user "root" is not allowed`}, } ctx := context.Background() for i, tc := range testCases { t.Run(fmt.Sprintf("%d: tls:%s user:%v tenant:%v", i, tc.tlsSpec, tc.username, tc.tenantID), func(t *testing.T) { + defer func() { + security.UnsetRootSubject() + security.UnsetNodeSubject() + }() err := security.SetCertPrincipalMap(strings.Split(tc.principalMap, ",")) if err != nil { t.Fatal(err) } var roleSubject *ldap.DN - if tc.isSubjectRoleOptionSet { - roleSubject, _ = distinguishedname.ParseDN(subjectDNString) + if tc.isSubjectRoleOptionOrDNSet { + switch tc.username { + case username.RootUserName(): + err = security.SetRootSubject(tc.distinguishedNameString) + if err != nil { + t.Fatalf("could not set root subject DN, err: %v", err) + } + case username.NodeUserName(): + err = security.SetNodeSubject(tc.distinguishedNameString) + if err != nil { + t.Fatalf("could not set node subject DN, err: %v", err) + } + default: + roleSubject, err = distinguishedname.ParseDN(tc.distinguishedNameString) + if err != nil { + t.Fatalf("could not set role subject, err: %v", err) + } + } } hook, err := security.UserAuthCertHook(