From 597a7852530b4224370a71242f7ef6d54a6b21f1 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 25 May 2024 06:43:06 -0700 Subject: [PATCH] fix: authenticate LDAP via actual DN instead of normalized DN (#19805) fix: authenticate LDAP via actual DN instead of normalized DN Normalized DN is only for internal representation, not for external communication, any communication to LDAP must be based on actual user DN. LDAP servers do not understand normalized DN. fixes #19757 --- cmd/admin-handlers-idp-ldap.go | 1 + cmd/admin-handlers-users.go | 1 + cmd/ftp-server-driver.go | 1 + cmd/iam-store.go | 6 +++ cmd/iam.go | 9 +++- cmd/sftp-server.go | 5 +- cmd/sts-handlers.go | 9 ++-- cmd/sts-handlers_test.go | 72 ++++++++++++++++++++++++++- go.mod | 4 +- go.sum | 8 +-- internal/config/identity/ldap/ldap.go | 22 +++++--- 11 files changed, 118 insertions(+), 20 deletions(-) diff --git a/cmd/admin-handlers-idp-ldap.go b/cmd/admin-handlers-idp-ldap.go index d34208f48..6dec56cbc 100644 --- a/cmd/admin-handlers-idp-ldap.go +++ b/cmd/admin-handlers-idp-ldap.go @@ -282,6 +282,7 @@ func (a adminAPIHandlers) AddServiceAccountLDAP(w http.ResponseWriter, r *http.R } targetUser = lookupResult.NormDN opts.claims[ldapUser] = targetUser // DN + opts.claims[ldapActualUser] = lookupResult.ActualDN // Add LDAP attributes that were looked up into the claims. for attribKey, attribValue := range lookupResult.Attributes { diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index abfc1d4cf..d460f07d4 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -709,6 +709,7 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque } targetUser = lookupResult.NormDN opts.claims[ldapUser] = targetUser // username DN + opts.claims[ldapActualUser] = lookupResult.ActualDN // Add LDAP attributes that were looked up into the claims. for attribKey, attribValue := range lookupResult.Attributes { diff --git a/cmd/ftp-server-driver.go b/cmd/ftp-server-driver.go index beb812e86..4c64b1e19 100644 --- a/cmd/ftp-server-driver.go +++ b/cmd/ftp-server-driver.go @@ -306,6 +306,7 @@ func (driver *ftpDriver) getMinIOClient(ctx *ftp.Context) (*minio.Client, error) claims[expClaim] = UTCNow().Add(expiryDur).Unix() claims[ldapUser] = lookupResult.NormDN + claims[ldapActualUser] = lookupResult.ActualDN claims[ldapUserN] = ctx.Sess.LoginUser() // Add LDAP attributes that were looked up into the claims. diff --git a/cmd/iam-store.go b/cmd/iam-store.go index d6b2c3640..6d025e8b0 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -1951,6 +1951,12 @@ func (store *IAMStoreSys) GetAllParentUsers() map[string]ParentUserInfo { subClaimValue = subFromToken } } + if v, ok := claims[ldapActualUser]; ok { + subFromToken, ok := v.(string) + if ok { + subClaimValue = subFromToken + } + } roleArn := openid.DummyRoleARN.String() s, ok := claims[roleArnClaim] diff --git a/cmd/iam.go b/cmd/iam.go index 0e5b95ef8..5e6173bca 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1351,12 +1351,17 @@ func (sys *IAMSys) purgeExpiredCredentialsForExternalSSO(ctx context.Context) { func (sys *IAMSys) purgeExpiredCredentialsForLDAP(ctx context.Context) { parentUsers := sys.store.GetAllParentUsers() var allDistNames []string - for parentUser := range parentUsers { + for parentUser, info := range parentUsers { if !sys.LDAPConfig.IsLDAPUserDN(parentUser) { continue } - allDistNames = append(allDistNames, parentUser) + if info.subClaimValue != "" { + // we need to ask LDAP about the actual user DN not normalized DN. + allDistNames = append(allDistNames, info.subClaimValue) + } else { + allDistNames = append(allDistNames, parentUser) + } } expiredUsers, err := sys.LDAPConfig.GetNonEligibleUserDistNames(allDistNames) diff --git a/cmd/sftp-server.go b/cmd/sftp-server.go index 576767e25..df9bb2fca 100644 --- a/cmd/sftp-server.go +++ b/cmd/sftp-server.go @@ -248,8 +248,9 @@ func startSFTPServer(args []string) { return nil, errAuthentication } criticalOptions := map[string]string{ - ldapUser: targetUser, - ldapUserN: c.User(), + ldapUser: targetUser, + ldapActualUser: lookupResult.ActualDN, + ldapUserN: c.User(), } for attribKey, attribValue := range lookupResult.Attributes { // we skip multi-value attributes here, as they cannot diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go index 9494fe30b..2bcf9e434 100644 --- a/cmd/sts-handlers.go +++ b/cmd/sts-handlers.go @@ -74,8 +74,9 @@ const ( parentClaim = "parent" // LDAP claim keys - ldapUser = "ldapUser" // this is a key name for a DN value - ldapUserN = "ldapUsername" // this is a key name for the short/login username + ldapUser = "ldapUser" // this is a key name for a normalized DN value + ldapActualUser = "ldapActualUser" // this is a key name for the actual DN value + ldapUserN = "ldapUsername" // this is a key name for the short/login username // Claim key-prefix for LDAP attributes ldapAttribPrefix = "ldapAttrib_" @@ -677,6 +678,7 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r * return } ldapUserDN := lookupResult.NormDN + ldapActualUserDN := lookupResult.ActualDN // Check if this user or their groups have a policy applied. ldapPolicies, err := globalIAMSys.PolicyDBGet(ldapUserDN, groupDistNames...) @@ -687,7 +689,7 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r * if len(ldapPolicies) == 0 && newGlobalAuthZPluginFn() == nil { writeSTSErrorResponse(ctx, w, ErrSTSInvalidParameterValue, fmt.Errorf("expecting a policy to be set for user `%s` or one of their groups: `%s` - rejecting this request", - ldapUserDN, strings.Join(groupDistNames, "`,`"))) + ldapActualUserDN, strings.Join(groupDistNames, "`,`"))) return } @@ -699,6 +701,7 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r * claims[expClaim] = UTCNow().Add(expiryDur).Unix() claims[ldapUser] = ldapUserDN + claims[ldapActualUser] = ldapActualUserDN claims[ldapUserN] = ldapUsername // Add lookup up LDAP attributes as claims. for attrib, value := range lookupResult.Attributes { diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index 19edbfe85..971f6712a 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -723,6 +723,7 @@ func TestIAMWithLDAPServerSuite(t *testing.T) { suite.TestLDAPSTSServiceAccountsWithUsername(c) suite.TestLDAPSTSServiceAccountsWithGroups(c) suite.TestLDAPAttributesLookup(c) + suite.TestLDAPCyrillicUser(c) suite.TearDownSuite(c) }, ) @@ -1872,6 +1873,75 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccountsWithGroups(c *check) { c.mustNotCreateSvcAccount(ctx, globalActiveCred.AccessKey, userAdmClient) } +func (s *TestSuiteIAM) TestLDAPCyrillicUser(c *check) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + _, err := s.adm.AttachPolicyLDAP(ctx, madmin.PolicyAssociationReq{ + Policies: []string{"readwrite"}, + User: "uid=Пользователь,ou=people,ou=swengg,dc=min,dc=io", + }) + if err != nil { + c.Fatalf("Unable to set policy: %v", err) + } + + cases := []struct { + username string + dn string + }{ + { + username: "Пользователь", + dn: "uid=Пользователь,ou=people,ou=swengg,dc=min,dc=io", + }, + } + + conn, err := globalIAMSys.LDAPConfig.LDAP.Connect() + if err != nil { + c.Fatalf("LDAP connect failed: %v", err) + } + defer conn.Close() + + for i, testCase := range cases { + ldapID := cr.LDAPIdentity{ + Client: s.TestSuiteCommon.client, + STSEndpoint: s.endPoint, + LDAPUsername: testCase.username, + LDAPPassword: "example", + } + + value, err := ldapID.Retrieve() + if err != nil { + c.Fatalf("Expected to generate STS creds, got err: %#v", err) + } + + // Retrieve the STS account's credential object. + u, ok := globalIAMSys.GetUser(ctx, value.AccessKeyID) + if !ok { + c.Fatalf("Expected to find user %s", value.AccessKeyID) + } + + if u.Credentials.AccessKey != value.AccessKeyID { + c.Fatalf("Expected access key %s, got %s", value.AccessKeyID, u.Credentials.AccessKey) + } + + // Retrieve the credential's claims. + secret, err := getTokenSigningKey() + if err != nil { + c.Fatalf("Error getting token signing key: %v", err) + } + claims, err := getClaimsFromTokenWithSecret(value.SessionToken, secret) + if err != nil { + c.Fatalf("Error getting claims from token: %v", err) + } + + // Validate claims. + dnClaim := claims[ldapActualUser].(string) + if dnClaim != testCase.dn { + c.Fatalf("Test %d: unexpected dn claim: %s", i+1, dnClaim) + } + } +} + func (s *TestSuiteIAM) TestLDAPAttributesLookup(c *check) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -1942,7 +2012,7 @@ func (s *TestSuiteIAM) TestLDAPAttributesLookup(c *check) { } // Validate claims. Check if the sshPublicKey claim is present. - dnClaim := claims[ldapUser].(string) + dnClaim := claims[ldapActualUser].(string) if dnClaim != testCase.dn { c.Fatalf("Test %d: unexpected dn claim: %s", i+1, dnClaim) } diff --git a/go.mod b/go.mod index 3cae47538..9ccdf09d8 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,8 @@ module github.com/minio/minio go 1.21 +replace github.com/minio/console => github.com/donatello/console v0.12.6-0.20240522161239-e2303ef9d681 + require ( cloud.google.com/go/storage v1.40.0 github.com/Azure/azure-storage-blob-go v0.15.0 @@ -55,7 +57,7 @@ require ( github.com/minio/madmin-go/v3 v3.0.52 github.com/minio/minio-go/v7 v7.0.70 github.com/minio/mux v1.9.0 - github.com/minio/pkg/v3 v3.0.0 + github.com/minio/pkg/v3 v3.0.1 github.com/minio/selfupdate v0.6.0 github.com/minio/simdjson-go v0.4.5 github.com/minio/sio v0.3.1 diff --git a/go.sum b/go.sum index 5bfb91f09..068dfd438 100644 --- a/go.sum +++ b/go.sum @@ -128,6 +128,8 @@ github.com/decred/dcrd/dcrec/secp256k1/v4 v4.3.0 h1:rpfIENRNNilwHwZeG5+P150SMrnN github.com/decred/dcrd/dcrec/secp256k1/v4 v4.3.0/go.mod h1:v57UDF4pDQJcEfFUCRop3lJL149eHGSe9Jvczhzjo/0= github.com/docker/go-units v0.5.0 h1:69rxXcBk27SvSaaxTtLh/8llcHD8vYHT7WSdRZ/jvr4= github.com/docker/go-units v0.5.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= +github.com/donatello/console v0.12.6-0.20240522161239-e2303ef9d681 h1:Cik6pFMXH35U5hje8pXhgtNVkcNwxQ1f1AzIXx1M878= +github.com/donatello/console v0.12.6-0.20240522161239-e2303ef9d681/go.mod h1:JyqeznIlKwgSx2Usz4CNq0i9WlDMJF75m8lbPV38p4I= github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= @@ -425,8 +427,6 @@ github.com/minio/cli v1.24.2 h1:J+fCUh9mhPLjN3Lj/YhklXvxj8mnyE/D6FpFduXJ2jg= github.com/minio/cli v1.24.2/go.mod h1:bYxnK0uS629N3Bq+AOZZ+6lwF77Sodk4+UL9vNuXhOY= github.com/minio/colorjson v1.0.7 h1:n69M42mIuQHdzbsxlmwji1zxDypaw4o39rHjAmX4Dh4= github.com/minio/colorjson v1.0.7/go.mod h1:9LGM5yybI+GuhSbuzAerbSgvFb4j8ux9NzyONR+NrAY= -github.com/minio/console v1.4.1 h1:P7hgyQi+36aYH90WPME3d/eLJ+a1jxnfhwxLjUOe9kY= -github.com/minio/console v1.4.1/go.mod h1:JyqeznIlKwgSx2Usz4CNq0i9WlDMJF75m8lbPV38p4I= github.com/minio/csvparser v1.0.0 h1:xJEHcYK8ZAjeW4hNV9Zu30u+/2o4UyPnYgyjWp8b7ZU= github.com/minio/csvparser v1.0.0/go.mod h1:lKXskSLzPgC5WQyzP7maKH7Sl1cqvANXo9YCto8zbtM= github.com/minio/dnscache v0.1.1 h1:AMYLqomzskpORiUA1ciN9k7bZT1oB3YZN4cEIi88W5o= @@ -454,8 +454,8 @@ github.com/minio/mux v1.9.0 h1:dWafQFyEfGhJvK6AwLOt83bIG5bxKxKJnKMCi0XAaoA= github.com/minio/mux v1.9.0/go.mod h1:1pAare17ZRL5GpmNL+9YmqHoWnLmMZF9C/ioUCfy0BQ= github.com/minio/pkg/v2 v2.0.17 h1:ndmGlitUj/eCVRPmfsAw3KlbtVNxqk0lQIvDXlcTHiQ= github.com/minio/pkg/v2 v2.0.17/go.mod h1:V+OP/fKRD/qhJMQpdXXrCXcLYjGMpHKEE26zslthm5k= -github.com/minio/pkg/v3 v3.0.0 h1:0vOKHgwpya//mb7RH0i1lyPMH2IBBF5hJMNY5Bk2WlY= -github.com/minio/pkg/v3 v3.0.0/go.mod h1:53gkSUVHcfYoskOs5YAJ3D99nsd2SKru90rdE9whlXU= +github.com/minio/pkg/v3 v3.0.1 h1:qts6g9rYjAdeomRdwjnMc1IaQ6KbaJs3dwqBntXziaw= +github.com/minio/pkg/v3 v3.0.1/go.mod h1:53gkSUVHcfYoskOs5YAJ3D99nsd2SKru90rdE9whlXU= github.com/minio/selfupdate v0.6.0 h1:i76PgT0K5xO9+hjzKcacQtO7+MjJ4JKA8Ak8XQ9DDwU= github.com/minio/selfupdate v0.6.0/go.mod h1:bO02GTIPCMQFTEvE5h4DjYB58bCoZ35XLeBf0buTDdM= github.com/minio/sha256-simd v0.1.1/go.mod h1:B5e1o+1/KgNmWrSQK08Y6Z1Vb5pwIktudl0J58iy0KM= diff --git a/internal/config/identity/ldap/ldap.go b/internal/config/identity/ldap/ldap.go index e48537b8e..30a69c6ea 100644 --- a/internal/config/identity/ldap/ldap.go +++ b/internal/config/identity/ldap/ldap.go @@ -51,7 +51,7 @@ func (l *Config) LookupUserDN(username string) (*xldap.DNSearchResult, []string, return nil, nil, errRet } - groups, err := l.LDAP.SearchForUserGroups(conn, username, lookupRes.NormDN) + groups, err := l.LDAP.SearchForUserGroups(conn, username, lookupRes.ActualDN) if err != nil { return nil, nil, err } @@ -200,9 +200,9 @@ func (l *Config) Bind(username, password string) (*xldap.DNSearchResult, []strin } // Authenticate the user credentials. - err = conn.Bind(lookupResult.NormDN, password) + err = conn.Bind(lookupResult.ActualDN, password) if err != nil { - errRet := fmt.Errorf("LDAP auth failed for DN %s: %w", lookupResult.NormDN, err) + errRet := fmt.Errorf("LDAP auth failed for DN %s: %w", lookupResult.ActualDN, err) return nil, nil, errRet } @@ -212,7 +212,7 @@ func (l *Config) Bind(username, password string) (*xldap.DNSearchResult, []strin } // User groups lookup. - groups, err := l.LDAP.SearchForUserGroups(conn, username, lookupResult.NormDN) + groups, err := l.LDAP.SearchForUserGroups(conn, username, lookupResult.ActualDN) if err != nil { return nil, nil, err } @@ -288,7 +288,7 @@ func (l *Config) GetNonEligibleUserDistNames(userDistNames []string) ([]string, return nil, err } - // Evaluate the filter again with generic wildcard instead of specific values + // Evaluate the filter again with generic wildcard instead of specific values filter := strings.ReplaceAll(l.LDAP.UserDNSearchFilter, "%s", "*") nonExistentUsers := []string{} @@ -305,7 +305,11 @@ func (l *Config) GetNonEligibleUserDistNames(userDistNames []string) ([]string, if err != nil { // Object does not exist error? if ldap.IsErrorWithCode(err, 32) { - nonExistentUsers = append(nonExistentUsers, dn) + ndn, err := ldap.ParseDN(dn) + if err != nil { + return nil, err + } + nonExistentUsers = append(nonExistentUsers, ndn.String()) continue } return nil, err @@ -313,7 +317,11 @@ func (l *Config) GetNonEligibleUserDistNames(userDistNames []string) ([]string, if len(searchResult.Entries) == 0 { // DN was not found - this means this user account is // expired. - nonExistentUsers = append(nonExistentUsers, dn) + ndn, err := ldap.ParseDN(dn) + if err != nil { + return nil, err + } + nonExistentUsers = append(nonExistentUsers, ndn.String()) } } return nonExistentUsers, nil