Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql,security, ccl: Match certificate DN against SUBJECT role option #119958

Merged
merged 1 commit into from
Mar 27, 2024
Merged

sql,security, ccl: Match certificate DN against SUBJECT role option #119958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/ccl/gssapiccl/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ go_library(
"//pkg/sql/pgwire/identmap",
"//pkg/sql/sem/tree",
"@com_github_cockroachdb_errors//:errors",
"@com_github_go_ldap_ldap_v3//:ldap",
],
"//conditions:default": [],
}),
Expand Down
3 changes: 2 additions & 1 deletion pkg/ccl/gssapiccl/gssapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/identmap"
"github.com/cockroachdb/errors"
"github.com/go-ldap/ldap/v3"
)

const (
Expand Down Expand Up @@ -82,7 +83,7 @@ func authGSS(
}

behaviors.SetAuthenticator(func(
_ context.Context, _ username.SQLUsername, _ bool, _ pgwire.PasswordRetrievalFn,
_ context.Context, _ username.SQLUsername, _ bool, _ pgwire.PasswordRetrievalFn, _ *ldap.DN,
) error {
// Enforce krb_realm option, if any.
if realms := entry.GetOptions("krb_realm"); len(realms) > 0 {
Expand Down
11 changes: 7 additions & 4 deletions pkg/ccl/logictestccl/testdata/logic_test/subject
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ statement error failed to parse distinguished name foo: DN ended with incomplete
CREATE ROLE role_with_subject SUBJECT 'foo'

statement ok
CREATE ROLE role_with_subject SUBJECT 'foo=bar'
CREATE ROLE role_with_subject SUBJECT 'CN=bar'

query T
SELECT value FROM system.role_options
WHERE username = 'role_with_subject'
AND option = 'SUBJECT'
----
foo=bar
CN=bar

statement ok
ALTER ROLE role_with_subject SUBJECT 'O=US, CN=role_with_subject'
Expand Down Expand Up @@ -60,8 +60,11 @@ AND option = 'SUBJECT'
----
O=US\,A, CN=role_with_subject

statement error SUBJECT CN must match "role_with_subject" but got "different_role"
statement ok
ALTER ROLE role_with_subject SUBJECT 'O=US\,A, CN=different_role'

statement error SUBJECT must have only one CN attribute
statement ok
ALTER ROLE role_with_subject SUBJECT 'CN=role_with_subject, O=US\,A, CN=different_role'

statement error SUBJECT contains illegal field type "OrgUnit", should be one of \["CN" "L" "ST" "O" "OU" "C" "STREET" "DC" "UID"\]
ALTER ROLE role_with_subject SUBJECT 'CN=role_with_subject, O=US\,A, CN=different_role, OrgUnit=Marketing';
4 changes: 4 additions & 0 deletions pkg/security/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ go_library(
deps = [
"//pkg/roachpb",
"//pkg/security/certnames",
"//pkg/security/distinguishedname",
"//pkg/security/password",
"//pkg/security/securityassets",
"//pkg/security/username",
Expand All @@ -48,6 +49,7 @@ go_library(
"//pkg/util/uuid",
"@com_github_cockroachdb_errors//:errors",
"@com_github_cockroachdb_errors//oserror",
"@com_github_go_ldap_ldap_v3//:ldap",
"@org_golang_x_crypto//bcrypt",
"@org_golang_x_crypto//ocsp",
"@org_golang_x_sync//errgroup",
Expand Down Expand Up @@ -82,6 +84,7 @@ go_test(
"//pkg/roachpb",
"//pkg/rpc",
"//pkg/security/certnames",
"//pkg/security/distinguishedname",
"//pkg/security/securityassets",
"//pkg/security/securitytest",
"//pkg/security/username",
Expand All @@ -100,6 +103,7 @@ go_test(
"//pkg/util/timeutil",
"//pkg/util/uuid",
"@com_github_cockroachdb_errors//:errors",
"@com_github_go_ldap_ldap_v3//:ldap",
"@com_github_stretchr_testify//require",
"@org_golang_x_exp//rand",
] + select({
Expand Down
63 changes: 48 additions & 15 deletions pkg/security/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,28 @@ import (
"strings"

"github.com/cockroachdb/cockroach/pkg/roachpb"
"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/util/metric"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
"github.com/cockroachdb/errors"
"github.com/go-ldap/ldap/v3"
)

var certPrincipalMap struct {
syncutil.RWMutex
m map[string]string
}

// CertificateUserScope indicates the scope of a user certificate i.e.
// which tenant the user is allowed to authenticate on. Older client certificates
// 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
// authenticate on any tenant strictly for backward compatibility with the
// older certificates.
// authenticate on any tenant strictly for backward compatibility with the older
// certificates. A certificate must specify the SQL user name in either the CN
// or a DNS SAN entry, so one certificate has multiple candidate usernames. The
// GetCertificateUserScope function expands a cert into a set of "scopes" with
// each possible username (and tenant ID).
type CertificateUserScope struct {
Username string
TenantID roachpb.TenantID
Expand Down Expand Up @@ -100,7 +105,10 @@ func getCertificatePrincipals(cert *x509.Certificate) []string {
return results
}

// GetCertificateUserScope extracts the certificate scopes from a client certificate.
// GetCertificateUserScope extracts the certificate scopes from a client
// certificate. It tries to get CRDB prefixed SAN URIs and extracts tenantID and
// user information. If there is no such URI, then it gets principal transformed
// CN and SAN DNSNames with global scope.
func GetCertificateUserScope(
peerCert *x509.Certificate,
) (userScopes []CertificateUserScope, _ error) {
Expand Down Expand Up @@ -148,6 +156,7 @@ func UserAuthCertHook(
tlsState *tls.ConnectionState,
tenantID roachpb.TenantID,
certManager *CertificateManager,
roleSubject *ldap.DN,
) (UserAuthHook, error) {
var certUserScope []CertificateUserScope
if !insecureMode {
Expand Down Expand Up @@ -190,7 +199,15 @@ func UserAuthCertHook(
return errors.Errorf("using tenant client certificate as user certificate is not allowed")
}

if ValidateUserScope(certUserScope, systemIdentity.Normalized(), tenantID) {
var certSubject *ldap.DN
if roleSubject != nil {
var err error
if certSubject, err = distinguishedname.ParseDNFromCertificate(peerCert); err != nil {
return errors.Errorf("could not parse certificate subject DN")
}
}

if ValidateUserScope(certUserScope, systemIdentity.Normalized(), tenantID, roleSubject, certSubject) {
if certManager != nil {
certManager.MaybeUpsertClientExpiration(
ctx,
Expand All @@ -200,8 +217,13 @@ func UserAuthCertHook(
}
return nil
}
return errors.WithDetailf(errors.Errorf("certificate authentication failed for user %q", systemIdentity),
"The client certificate is valid for %s.", FormatUserScopes(certUserScope))
return errors.WithDetailf(
errors.Errorf(
"certificate authentication failed for user %q (DN: %s)",
systemIdentity,
roleSubject,
),
"The client certificate (DN: %s) is valid for %s.", certSubject, FormatUserScopes(certUserScope))
}, nil
}

Expand Down Expand Up @@ -294,17 +316,28 @@ func (i *PasswordUserAuthError) FormatError(p errors.Printer) error {
return i.err
}

// ValidateUserScope returns true if the user is a valid user for the tenant based on the certificate
// user scope. It also returns true if the certificate is a global certificate. A client certificate
// is considered global only when it doesn't contain a tenant SAN which is only possible for older
// client certificates created prior to introducing tenant based scoping for the client.
// ValidateUserScope returns true if the user is a valid user for the tenant
// based on the certificate user scope. It also returns true if the certificate
// is a global certificate. A client certificate is considered global only when
// it doesn't contain a tenant SAN which is only possible for older client
// certificates created prior to introducing tenant based scoping for the
// client. Additionally, if subject role option is set for a user, we check if
// certificate parsed subject DN matches the set subject.
func ValidateUserScope(
certUserScope []CertificateUserScope, user string, tenantID roachpb.TenantID,
certUserScope []CertificateUserScope,
user string,
tenantID roachpb.TenantID,
roleSubject *ldap.DN,
certSubject *ldap.DN,
) bool {
// if subject role option is set, it must match the certificate subject
if roleSubject != nil && !roleSubject.Equal(certSubject) {
return false
}
for _, scope := range certUserScope {
if scope.Username == user {
// If username matches, allow authentication to succeed if the tenantID is a match
// or if the certificate scope is global.
// If username matches, allow authentication to succeed if
// the tenantID is a match or if the certificate scope is global.
if scope.TenantID == tenantID || scope.Global {
return true
}
Expand Down
Loading
Loading