Skip to content

Commit

Permalink
rpc,security, cli: Add cert-distinguished-name root/node cli flag
Browse files Browse the repository at this point in the history
Previous in sequence: cockroachdb#119958
informs cockroachdb#110616, cockroachdb#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
  • Loading branch information
souravcrl committed Mar 28, 2024
1 parent 1671c4c commit e4aa11e
Show file tree
Hide file tree
Showing 9 changed files with 419 additions and 43 deletions.
7 changes: 7 additions & 0 deletions pkg/acceptance/generated_cli_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions pkg/cli/cliflags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,26 @@ fields. It is permissible for the <cert-principal> string to contain colons.
`,
}

RootCertDistinguishedName = FlagInfo{
Name: "root-cert-distinguished-name",
Description: `
A string of comma separated list of distinguished-name
<attribute-type>=<attribute-value> 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
<attribute-type>=<attribute-value> 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",
Expand Down
4 changes: 4 additions & 0 deletions pkg/cli/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,8 @@ var startCtx struct {
serverSSLCertsDir string
serverCertPrincipalMap []string
serverListenAddr string
serverRootCertDN string
serverNodeCertDN string

// The TLS auto-handshake parameters.
initToken string
Expand Down Expand Up @@ -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 = ""
Expand Down
18 changes: 18 additions & 0 deletions pkg/cli/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
144 changes: 144 additions & 0 deletions pkg/cli/interactive_tests/test_distinguished_name_validation.tcl
Original file line number Diff line number Diff line change
@@ -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
21 changes: 15 additions & 6 deletions pkg/rpc/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
46 changes: 46 additions & 0 deletions pkg/rpc/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ import (
"crypto/tls"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"fmt"
"io"
"strings"
"testing"

"github.com/cockroachdb/cockroach/pkg/keys"
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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),
Expand Down
Loading

0 comments on commit e4aa11e

Please sign in to comment.