Skip to content

Commit

Permalink
Merge #130979
Browse files Browse the repository at this point in the history 
130979: sql/pgwire: treat system identity as string, not SQLUsername r=rafiss a=rafiss

The external system identity may not be a SQLUsername -- that mapping is performed at a different phase of authentication. To make this more clear, we stop using the SQLUsername type for system identity.

Epic CRDB-33829
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
  • Loading branch information
@rafiss
craig[bot] and rafiss committed Sep 19, 2024
2 parents 9d91855 + eb67431 commit 06c1f94
Show file tree
Hide file tree
Showing 21 changed files with 177 additions and 148 deletions.
25 changes: 9 additions & 16 deletions pkg/ccl/gssapiccl/gssapi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,7 @@ func authGSS(

// Update the incoming connection with the GSS username. We'll expect
// to see this value come back to the mapper function below.
if u, err := username.MakeSQLUsernameFromUserInput(gssUser, username.PurposeValidation); err != nil {
return nil, err
} else {
behaviors.SetReplacementIdentity(u)
}
behaviors.SetReplacementIdentity(gssUser)

// We enforce that the "map" and/or "include_realm=0" options are set
// in the HBA validation function below.
Expand All @@ -84,7 +80,7 @@ func authGSS(
}

behaviors.SetAuthenticator(func(
_ context.Context, _ username.SQLUsername, _ bool, _ pgwire.PasswordRetrievalFn, _ *ldap.DN,
_ context.Context, _ string, _ bool, _ pgwire.PasswordRetrievalFn, _ *ldap.DN,
) error {
// Enforce krb_realm option, if any.
if realms := entry.GetOptions("krb_realm"); len(realms) > 0 {
Expand Down Expand Up @@ -142,19 +138,16 @@ func checkEntry(_ *settings.Values, entry hba.Entry) error {
}

// stripRealm removes the realm data, if any, from the provided username.
func stripRealm(u username.SQLUsername) (username.SQLUsername, error) {
norm := u.Normalized()
if idx := strings.Index(norm, "@"); idx != -1 {
norm = norm[:idx]
func stripRealm(u string) (username.SQLUsername, error) {
if idx := strings.Index(u, "@"); idx != -1 {
u = u[:idx]
}
return username.MakeSQLUsernameFromUserInput(norm, username.PurposeValidation)
return username.MakeSQLUsernameFromUserInput(u, username.PurposeValidation)
}

// stripRealmMapper is a pgwire.RoleMapper that just strips the trailing
// realm information, if any, from the gssapi username.
func stripRealmMapper(
_ context.Context, systemIdentity username.SQLUsername,
) ([]username.SQLUsername, error) {
func stripRealmMapper(_ context.Context, systemIdentity string) ([]username.SQLUsername, error) {
ret, err := stripRealm(systemIdentity)
return []username.SQLUsername{ret}, err
}
Expand All @@ -163,12 +156,12 @@ func stripRealmMapper(
// the incoming identity has its realm information stripped before the
// next mapping is applied.
func stripAndDelegateMapper(delegate pgwire.RoleMapper) pgwire.RoleMapper {
return func(ctx context.Context, systemIdentity username.SQLUsername) ([]username.SQLUsername, error) {
return func(ctx context.Context, systemIdentity string) ([]username.SQLUsername, error) {
next, err := stripRealm(systemIdentity)
if err != nil {
return nil, err
}
return delegate(ctx, next)
return delegate(ctx, next.Normalized())
}
}

Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/ldapccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ go_library(
"//pkg/server/telemetry",
"//pkg/settings",
"//pkg/settings/cluster",
"//pkg/sql/lexbase",
"//pkg/sql/pgwire",
"//pkg/sql/pgwire/hba",
"//pkg/sql/pgwire/identmap",
Expand Down
3 changes: 2 additions & 1 deletion pkg/ccl/ldapccl/authentication_ldap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/server/telemetry"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/identmap"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
Expand Down Expand Up @@ -109,7 +110,7 @@ func (authManager *ldapAuthManager) FetchLDAPUserDN(
"cannot find provided user %s on LDAP server", user.Normalized())
}

retrievedUserDN, err = distinguishedname.ParseDN(userDN)
retrievedUserDN, err = distinguishedname.ParseDN(lexbase.NormalizeName(userDN))
if err != nil {
return nil, redact.Sprintf("error parsing user DN %s obtained from LDAP server: %v", userDN, err),
errors.WithDetailf(
Expand Down
33 changes: 17 additions & 16 deletions pkg/security/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ type CertificateUserScope struct {
// that may have been applied to the given connection.
type UserAuthHook func(
ctx context.Context,
systemIdentity username.SQLUsername,
systemIdentity string,
clientConnection bool,
) error

Expand All @@ -125,14 +125,12 @@ type UserAuthHook func(
// 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(
previouslySetRoleSubject *ldap.DN, systemIdentity username.SQLUsername,
) (dn *ldap.DN) {
func applyRootOrNodeDNFlag(previouslySetRoleSubject *ldap.DN, systemIdentity string) (dn *ldap.DN) {
dn = previouslySetRoleSubject
switch {
case systemIdentity.IsRootUser():
case systemIdentity == username.RootUser:
dn = rootSubjectMu.getDN()
case systemIdentity.IsNodeUser():
case systemIdentity == username.NodeUser:
dn = nodeSubjectMu.getDN()
}
return dn
Expand Down Expand Up @@ -287,13 +285,12 @@ func UserAuthCertHook(
}
}

return func(ctx context.Context, systemIdentity username.SQLUsername, clientConnection bool) error {
// TODO(marc): we may eventually need stricter user syntax rules.
if systemIdentity.Undefined() {
return func(ctx context.Context, systemIdentity string, clientConnection bool) error {
if systemIdentity == "" {
return errors.New("user is missing")
}

if !clientConnection && !systemIdentity.IsNodeUser() {
if !clientConnection && systemIdentity != username.NodeUser {
return errors.Errorf("user %q is not allowed", systemIdentity)
}

Expand All @@ -315,7 +312,7 @@ func UserAuthCertHook(
if subjectRequired && roleSubject == nil {
return errors.Newf(
"user %q does not have a distinguished name set which subject_required cluster setting mandates",
systemIdentity.Normalized(),
systemIdentity,
)
}

Expand All @@ -327,7 +324,7 @@ func UserAuthCertHook(
}
}

if ValidateUserScope(certUserScope, systemIdentity.Normalized(), tenantID, roleSubject, certSubject) {
if ValidateUserScope(certUserScope, systemIdentity, tenantID, roleSubject, certSubject) {
if certManager != nil {
certManager.MaybeUpsertClientExpiration(
ctx,
Expand Down Expand Up @@ -375,8 +372,12 @@ func IsTenantCertificate(cert *x509.Certificate) bool {
func UserAuthPasswordHook(
insecureMode bool, passwordStr string, hashedPassword password.PasswordHash, gauge *metric.Gauge,
) UserAuthHook {
return func(ctx context.Context, systemIdentity username.SQLUsername, clientConnection bool) error {
if systemIdentity.Undefined() {
return func(ctx context.Context, systemIdentity string, clientConnection bool) error {
u, err := username.MakeSQLUsernameFromUserInput(systemIdentity, username.PurposeValidation)
if err != nil {
return err
}
if u.Undefined() {
return errors.New("user is missing")
}

Expand All @@ -390,15 +391,15 @@ func UserAuthPasswordHook(

// If the requested user has an empty password, disallow authentication.
if len(passwordStr) == 0 {
return NewErrPasswordUserAuthFailed(systemIdentity)
return NewErrPasswordUserAuthFailed(u)
}
ok, err := password.CompareHashAndCleartextPassword(ctx,
hashedPassword, passwordStr, GetExpensiveHashComputeSemWithGauge(ctx, gauge))
if err != nil {
return err
}
if !ok {
return NewErrPasswordUserAuthFailed(systemIdentity)
return NewErrPasswordUserAuthFailed(u)
}

return nil
Expand Down
28 changes: 14 additions & 14 deletions pkg/security/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,9 @@ func TestAuthenticationHook(t *testing.T) {
defer leaktest.AfterTest(t)()
defer func() { _ = security.SetCertPrincipalMap(nil) }()

fooUser := username.MakeSQLUsernameFromPreNormalizedString("foo")
barUser := username.MakeSQLUsernameFromPreNormalizedString("bar")
blahUser := username.MakeSQLUsernameFromPreNormalizedString("blah")
fooUser := "foo"
barUser := "bar"
blahUser := "blah"
subjectDNString := "O=Cockroach,OU=Order Processing Team,UID=b8b40653-7f74-4f14-8a61-59f7f3b18184,CN=foo"
fieldMismatchSubjectDNString := "O=Cockroach,OU=Marketing Team,UID=b8b40653-7f74-4f14-8a61-59f7f3b18184,CN=foo"
subsetSubjectDNString := "O=Cockroach,OU=Order Processing Team,CN=foo"
Expand All @@ -388,7 +388,7 @@ func TestAuthenticationHook(t *testing.T) {
testCases := []struct {
insecure bool
tlsSpec string
username username.SQLUsername
username string
distinguishedNameString string
principalMap string
buildHookSuccess bool
Expand All @@ -400,21 +400,21 @@ func TestAuthenticationHook(t *testing.T) {
expectedErr string
}{
// Insecure mode, empty username.
{true, "", username.SQLUsername{}, "", "", true, false, false, roachpb.SystemTenantID, false, false, `user is missing`},
{true, "", username.EmptyRole, "", "", true, false, false, roachpb.SystemTenantID, false, false, `user is missing`},
// Insecure mode, non-empty username.
{true, "", fooUser, "", "", true, true, false, roachpb.SystemTenantID, false, false, `user "foo" is not allowed`},
// Secure mode, no TLS state.
{false, "", username.SQLUsername{}, "", "", false, false, false, roachpb.SystemTenantID, false, false, `no client certificates in request`},
{false, "", username.EmptyRole, "", "", false, false, false, roachpb.SystemTenantID, false, false, `no client certificates in request`},
// Secure mode, bad user.
{false, "(CN=foo)", username.NodeUserName(), "", "", true, false, false, roachpb.SystemTenantID,
{false, "(CN=foo)", username.NodeUser, "", "", true, false, false, roachpb.SystemTenantID,
false, false, `certificate authentication failed for user "node"`},
// Secure mode, node user.
{false, "(CN=node)", username.NodeUserName(), "", "", true, true, true, roachpb.SystemTenantID, false, false, ``},
{false, "(CN=node)", username.NodeUser, "", "", true, true, true, roachpb.SystemTenantID, false, false, ``},
// Secure mode, node cert and unrelated user.
{false, "(CN=node)", fooUser, "", "", true, false, false, roachpb.SystemTenantID,
false, 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.NodeUser, "", "", true, false, false, roachpb.SystemTenantID,
false, false, `certificate authentication failed for user "node"`},
// Secure mode, tenant cert, foo user.
{false, "(OU=Tenants,CN=foo)", fooUser, "", "", true, false, false, roachpb.SystemTenantID,
Expand Down Expand Up @@ -460,12 +460,12 @@ func TestAuthenticationHook(t *testing.T) {
// matching) having DNS as foo.
{false, "(" + fieldMismatchOnlyOnCommonNameString + ")dns:foo", fooUser, subjectDNString, "", true, false, false, roachpb.MustMakeTenantID(123),
true, false, `certificate authentication failed for user "foo"`},
{false, "(" + rootDNString + ")", username.RootUserName(), rootDNString, "", true, true, false, roachpb.MustMakeTenantID(123),
{false, "(" + rootDNString + ")", username.RootUser, rootDNString, "", true, true, false, roachpb.MustMakeTenantID(123),
true, false, `user "root" is not allowed`},
{false, "(" + nodeDNString + ")", username.NodeUserName(), nodeDNString, "", true, true, true, roachpb.MustMakeTenantID(123),
{false, "(" + nodeDNString + ")", username.NodeUser, nodeDNString, "", true, true, true, roachpb.MustMakeTenantID(123),
true, false, ""},
// tls cert dn matching root dn set, where CN != root
{false, "(" + subjectDNString + ")", username.RootUserName(), subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123),
{false, "(" + subjectDNString + ")", username.RootUser, subjectDNString, "", true, true, false, roachpb.MustMakeTenantID(123),
true, false, `user "root" is not allowed`},
{false, "(" + subjectDNString + ")", fooUser, "", "", true, false, false, roachpb.MustMakeTenantID(123),
false, true, `user "foo" does not have a distinguished name set which subject_required cluster setting mandates`},
Expand All @@ -489,12 +489,12 @@ func TestAuthenticationHook(t *testing.T) {
var roleSubject *ldap.DN
if tc.isSubjectRoleOptionOrDNSet {
switch tc.username {
case username.RootUserName():
case username.RootUser:
err = security.SetRootSubject(tc.distinguishedNameString)
if err != nil {
t.Fatalf("could not set root subject DN, err: %v", err)
}
case username.NodeUserName():
case username.NodeUser:
err = security.SetNodeSubject(tc.distinguishedNameString)
if err != nil {
t.Fatalf("could not set node subject DN, err: %v", err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/security/certificate_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,11 @@ func (cm *CertificateManager) RegisterExpirationCache(cache *ClientCertExpiratio
// given client certificate. An update is contingent on whether the old
// expiration is after the new expiration.
func (cm *CertificateManager) MaybeUpsertClientExpiration(
ctx context.Context, identity username.SQLUsername, expiration int64,
ctx context.Context, identity string, expiration int64,
) {
if cache := cm.clientCertExpirationCache; cache != nil {
cache.MaybeUpsert(ctx,
identity.Normalized(),
identity,
expiration,
cm.certMetrics.ClientExpiration,
cm.certMetrics.ClientTTL,
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/conn_executor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ func newSessionData(args SessionArgs) *sessiondata.SessionData {
LocalOnlySessionData: sessiondatapb.LocalOnlySessionData{
ResultsBufferSize: args.ConnResultsBufferSize,
IsSuperuser: args.IsSuperuser,
SystemIdentityProto: args.SystemIdentity.EncodeProto(),
SystemIdentityProto: args.SystemIdentity,
},
}
if len(args.CustomOptionSessionDefaults) > 0 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/exec_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2266,7 +2266,7 @@ type SessionArgs struct {
IsSuperuser bool
IsSSL bool
ReplicationMode sessiondatapb.ReplicationMode
SystemIdentity username.SQLUsername
SystemIdentity string
SessionDefaults SessionDefaults
CustomOptionSessionDefaults SessionDefaults
// RemoteAddr is the client's address. This is nil iff this is an internal
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/pgwire/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ go_library(
"//pkg/sql/catalog/colinfo",
"//pkg/sql/clusterunique",
"//pkg/sql/lex",
"//pkg/sql/lexbase",
"//pkg/sql/parser",
"//pkg/sql/parser/statements",
"//pkg/sql/pgrepl/pgreplparser",
Expand Down
Loading

0 comments on commit 06c1f94

Please sign in to comment.