diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index 7b3825b04f6a..66b53079a501 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/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 diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index 433333c85b56..416fadc4db71 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -104,6 +104,7 @@
server.auth_log.sql_sessions.enabled
booleanfalseif set, log SQL session login/disconnection events (note: may hinder performance on loaded nodes)Serverless/Dedicated/Self-Hosted
server.authentication_cache.enabled
booleantrueenables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related informationServerless/Dedicated/Self-Hosted
server.child_metrics.enabled
booleanfalseenables the exporting of child metrics, additional prometheus time series with extra labelsServerless/Dedicated/Self-Hosted +
server.client_cert.subject_required.enabled
booleanfalsemandates a requirement for subject role to be set for db userServerless/Dedicated/Self-Hosted
server.client_cert_expiration_cache.capacity
integer1000the maximum number of client cert expirations storedServerless/Dedicated/Self-Hosted
server.clock.forward_jump_check.enabled
booleanfalseif enabled, forward clock jumps > max_offset/2 will cause a panicServerless/Dedicated/Self-Hosted
server.clock.persist_upper_bound_interval
duration0sthe 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.Serverless/Dedicated/Self-Hosted diff --git a/pkg/cli/interactive_tests/test_distinguished_name_validation.tcl b/pkg/cli/interactive_tests/test_distinguished_name_validation.tcl index 28038dbf7dce..ac5e81431562 100755 --- a/pkg/cli/interactive_tests/test_distinguished_name_validation.tcl +++ b/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 diff --git a/pkg/rpc/auth.go b/pkg/rpc/auth.go index e50cf8fc91bf..31f962fd8a5e 100644 --- a/pkg/rpc/auth.go +++ b/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( diff --git a/pkg/rpc/auth_test.go b/pkg/rpc/auth_test.go index 61dbcfd039ff..19b8f448bb5b 100644 --- a/pkg/rpc/auth_test.go +++ b/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) diff --git a/pkg/rpc/helpers_test.go b/pkg/rpc/helpers_test.go index ac7c480f85d4..e2631d6752a7 100644 --- a/pkg/rpc/helpers_test.go +++ b/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 } diff --git a/pkg/security/BUILD.bazel b/pkg/security/BUILD.bazel index 1922fe447665..57c21f5ee74b 100644 --- a/pkg/security/BUILD.bazel +++ b/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", diff --git a/pkg/security/auth.go b/pkg/security/auth.go index ac05f87fb0ac..fe950100c97a 100644 --- a/pkg/security/auth.go +++ b/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 :. 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 diff --git a/pkg/security/auth_test.go b/pkg/security/auth_test.go index 5ba66a4d3203..ee6b43863dcd 100644 --- a/pkg/security/auth_test.go +++ b/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), diff --git a/pkg/security/cert_settings.go b/pkg/security/cert_settings.go new file mode 100644 index 000000000000..606eed2006cc --- /dev/null +++ b/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), +) diff --git a/pkg/sql/pgwire/auth_methods.go b/pkg/sql/pgwire/auth_methods.go index 939a266e3cb8..e97ef8c7d3e7 100644 --- a/pkg/sql/pgwire/auth_methods.go +++ b/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,