diff --git a/cmd/admin-handlers-idp-ldap.go b/cmd/admin-handlers-idp-ldap.go index 4644bc977..90e6ca38e 100644 --- a/cmd/admin-handlers-idp-ldap.go +++ b/cmd/admin-handlers-idp-ldap.go @@ -233,12 +233,12 @@ func (a adminAPIHandlers) AddServiceAccountLDAP(w http.ResponseWriter, r *http.R targetGroups = requestorGroups // Deny if the target user is not LDAP - isLDAP, err := globalIAMSys.LDAPConfig.DoesUsernameExist(targetUser) + foundLDAPDN, err := globalIAMSys.LDAPConfig.GetValidatedDNForUsername(targetUser) if err != nil { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } - if isLDAP == "" { + if foundLDAPDN == "" { err := errors.New("Specified user does not exist on LDAP server") APIErr := errorCodes.ToAPIErrWithErr(ErrAdminNoSuchUser, err) writeErrorResponseJSON(ctx, w, APIErr, r.URL) @@ -374,7 +374,7 @@ func (a adminAPIHandlers) ListAccessKeysLDAP(w http.ResponseWriter, r *http.Requ } } - targetAccount, err := globalIAMSys.LDAPConfig.DoesUsernameExist(userDN) + targetAccount, err := globalIAMSys.LDAPConfig.GetValidatedDNForUsername(userDN) if err != nil { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return diff --git a/cmd/iam.go b/cmd/iam.go index 23343ebde..7b7cec542 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1701,7 +1701,7 @@ func (sys *IAMSys) PolicyDBUpdateLDAP(ctx context.Context, isAttach bool, var dn string var isGroup bool if r.User != "" { - dn, err = sys.LDAPConfig.DoesUsernameExist(r.User) + dn, err = sys.LDAPConfig.GetValidatedDNForUsername(r.User) if err != nil { logger.LogIf(ctx, err) return @@ -1718,22 +1718,26 @@ func (sys *IAMSys) PolicyDBUpdateLDAP(ctx context.Context, isAttach bool, isGroup = false } else { if isAttach { - var exists bool - if exists, err = sys.LDAPConfig.DoesGroupDNExist(r.Group); err != nil { + var foundGroupDN string + if foundGroupDN, err = sys.LDAPConfig.GetValidatedGroupDN(r.Group); err != nil { logger.LogIf(ctx, err) return - } else if !exists { + } else if foundGroupDN == "" { err = errNoSuchGroup return } + // We use the group DN returned by the LDAP server (this may not + // equal the input group name, but we assume it is canonical). + dn = foundGroupDN + } else { + dn = r.Group } - dn = r.Group isGroup = true } userType := stsUser - updatedAt, addedOrRemoved, effectivePolicies, err = sys.store.PolicyDBUpdate(ctx, dn, isGroup, - userType, r.Policies, isAttach) + updatedAt, addedOrRemoved, effectivePolicies, err = sys.store.PolicyDBUpdate( + ctx, dn, isGroup, userType, r.Policies, isAttach) if err != nil { return } diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index b21f780d7..119574b5a 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -29,6 +29,8 @@ import ( minio "github.com/minio/minio-go/v7" cr "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/set" + ldap "github.com/minio/pkg/v2/ldap" + "golang.org/x/exp/slices" ) func runAllIAMSTSTests(suite *TestSuiteIAM, c *check) { @@ -681,6 +683,7 @@ func TestIAMWithLDAPServerSuite(t *testing.T) { suite.SetUpSuite(c) suite.SetUpLDAP(c, ldapServer) suite.TestLDAPSTS(c) + suite.TestLDAPUnicodeVariations(c) suite.TestLDAPSTSServiceAccounts(c) suite.TestLDAPSTSServiceAccountsWithUsername(c) suite.TestLDAPSTSServiceAccountsWithGroups(c) @@ -823,6 +826,170 @@ func (s *TestSuiteIAM) TestLDAPSTS(c *check) { c.Assert(err.Error(), "Access Denied.") } +func (s *TestSuiteIAM) TestLDAPUnicodeVariations(c *check) { + ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) + defer cancel() + + bucket := getRandomBucketName() + err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{}) + if err != nil { + c.Fatalf("bucket create error: %v", err) + } + + // Create policy + policy := "mypolicy" + policyBytes := []byte(fmt.Sprintf(`{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "s3:PutObject", + "s3:GetObject", + "s3:ListBucket" + ], + "Resource": [ + "arn:aws:s3:::%s/*" + ] + } + ] +}`, bucket)) + err = s.adm.AddCannedPolicy(ctx, policy, policyBytes) + if err != nil { + c.Fatalf("policy add error: %v", err) + } + + ldapID := cr.LDAPIdentity{ + Client: s.TestSuiteCommon.client, + STSEndpoint: s.endPoint, + LDAPUsername: "svc.algorithm", + LDAPPassword: "example", + } + + _, err = ldapID.Retrieve() + if err == nil { + c.Fatalf("Expected to fail to create STS cred with no associated policy!") + } + + mustNormalizeDN := func(dn string) string { + normalizedDN, err := ldap.NormalizeDN(dn) + if err != nil { + c.Fatalf("normalize err: %v", err) + } + return normalizedDN + } + + actualUserDN := mustNormalizeDN("uid=svc.algorithm,OU=swengg,DC=min,DC=io") + + // \uFE52 is the unicode dot SMALL FULL STOP used below: + userDNWithUnicodeDot := "uid=svc﹒algorithm,OU=swengg,DC=min,DC=io" + + _, err = s.adm.AttachPolicyLDAP(ctx, madmin.PolicyAssociationReq{ + Policies: []string{policy}, + User: userDNWithUnicodeDot, + }) + if err != nil { + c.Fatalf("Unable to set policy: %v", err) + } + + value, err := ldapID.Retrieve() + if err != nil { + c.Fatalf("Expected to generate STS creds, got err: %#v", err) + } + + usersList, err := s.adm.ListUsers(ctx) + if err != nil { + c.Fatalf("list users should not fail: %v", err) + } + if len(usersList) != 1 { + c.Fatalf("expected user listing output: %#v", usersList) + } + uinfo := usersList[actualUserDN] + if uinfo.PolicyName != policy || uinfo.Status != madmin.AccountEnabled { + c.Fatalf("expected user listing content: %v", uinfo) + } + + minioClient, err := minio.New(s.endpoint, &minio.Options{ + Creds: cr.NewStaticV4(value.AccessKeyID, value.SecretAccessKey, value.SessionToken), + Secure: s.secure, + Transport: s.TestSuiteCommon.client.Transport, + }) + if err != nil { + c.Fatalf("Error initializing client: %v", err) + } + + // Validate that the client from sts creds can access the bucket. + c.mustListObjects(ctx, minioClient, bucket) + + // Validate that the client cannot remove any objects + err = minioClient.RemoveObject(ctx, bucket, "someobject", minio.RemoveObjectOptions{}) + if err.Error() != "Access Denied." { + c.Fatalf("unexpected non-access-denied err: %v", err) + } + + // Remove the policy assignment on the user DN: + _, err = s.adm.DetachPolicyLDAP(ctx, madmin.PolicyAssociationReq{ + Policies: []string{policy}, + User: userDNWithUnicodeDot, + }) + if err != nil { + c.Fatalf("Unable to remove policy setting: %v", err) + } + + _, err = ldapID.Retrieve() + if err == nil { + c.Fatalf("Expected to fail to create a user with no associated policy!") + } + + // Set policy via group and validate policy assignment. + actualGroupDN := mustNormalizeDN("cn=project.c,ou=groups,ou=swengg,dc=min,dc=io") + groupDNWithUnicodeDot := "cn=project﹒c,ou=groups,ou=swengg,dc=min,dc=io" + _, err = s.adm.AttachPolicyLDAP(ctx, madmin.PolicyAssociationReq{ + Policies: []string{policy}, + Group: groupDNWithUnicodeDot, + }) + if err != nil { + c.Fatalf("Unable to attach group policy: %v", err) + } + + value, err = ldapID.Retrieve() + if err != nil { + c.Fatalf("Expected to generate STS creds, got err: %#v", err) + } + + policyResult, err := s.adm.GetLDAPPolicyEntities(ctx, madmin.PolicyEntitiesQuery{ + Policy: []string{policy}, + }) + if err != nil { + c.Fatalf("GetLDAPPolicyEntities should not fail: %v", err) + } + { + // Check that the mapping we created exists. + idx := slices.IndexFunc(policyResult.PolicyMappings, func(e madmin.PolicyEntities) bool { + return e.Policy == policy && slices.Contains(e.Groups, actualGroupDN) + }) + if !(idx >= 0) { + c.Fatalf("expected groupDN (%s) to be present in mapping list: %#v", actualGroupDN, policyResult) + } + } + + minioClient, err = minio.New(s.endpoint, &minio.Options{ + Creds: cr.NewStaticV4(value.AccessKeyID, value.SecretAccessKey, value.SessionToken), + Secure: s.secure, + Transport: s.TestSuiteCommon.client.Transport, + }) + if err != nil { + c.Fatalf("Error initializing client: %v", err) + } + + // Validate that the client from sts creds can access the bucket. + c.mustListObjects(ctx, minioClient, bucket) + + // Validate that the client cannot remove any objects + err = minioClient.RemoveObject(ctx, bucket, "someobject", minio.RemoveObjectOptions{}) + c.Assert(err.Error(), "Access Denied.") +} + func (s *TestSuiteIAM) TestLDAPSTSServiceAccounts(c *check) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() diff --git a/go.mod b/go.mod index 3d117024b..238e896ee 100644 --- a/go.mod +++ b/go.mod @@ -54,7 +54,7 @@ require ( github.com/minio/madmin-go/v3 v3.0.50 github.com/minio/minio-go/v7 v7.0.69 github.com/minio/mux v1.9.0 - github.com/minio/pkg/v2 v2.0.11 + github.com/minio/pkg/v2 v2.0.14 github.com/minio/selfupdate v0.6.0 github.com/minio/sha256-simd v1.0.1 github.com/minio/simdjson-go v0.4.5 @@ -76,6 +76,7 @@ require ( github.com/prometheus/client_model v0.6.0 github.com/prometheus/common v0.50.0 github.com/prometheus/procfs v0.13.0 + github.com/puzpuzpuz/xsync/v3 v3.1.0 github.com/rabbitmq/amqp091-go v1.9.0 github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 github.com/rs/cors v1.10.1 @@ -219,7 +220,6 @@ require ( github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55 // indirect github.com/pquerna/cachecontrol v0.2.0 // indirect github.com/prometheus/prom2json v1.3.3 // indirect - github.com/puzpuzpuz/xsync/v3 v3.1.0 // indirect github.com/rivo/tview v0.0.0-20240307173318-e804876934a1 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/rjeczalik/notify v0.9.3 // indirect diff --git a/go.sum b/go.sum index 1542f1482..d16e9f59a 100644 --- a/go.sum +++ b/go.sum @@ -458,8 +458,8 @@ github.com/minio/minio-go/v7 v7.0.69 h1:l8AnsQFyY1xiwa/DaQskY4NXSLA2yrGsW5iD9nRP github.com/minio/minio-go/v7 v7.0.69/go.mod h1:XAvOPJQ5Xlzk5o3o/ArO2NMbhSGkimC+bpW/ngRKDmQ= 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.11 h1:nlFoFrnwBaNNtVmJJBJ+t1Nzp4jbPzyqdVuc7t9clcQ= -github.com/minio/pkg/v2 v2.0.11/go.mod h1:zbVATXCinLCo+L/4vsPyqgiA4OYPXCJb+/E4KfE396A= +github.com/minio/pkg/v2 v2.0.14 h1:tPhDYxgvv3LNqmfCSe2zsSXrcaIj4ANyL1VRGdEkahI= +github.com/minio/pkg/v2 v2.0.14/go.mod h1:zbVATXCinLCo+L/4vsPyqgiA4OYPXCJb+/E4KfE396A= 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 4c805aee7..0cca442ff 100644 --- a/internal/config/identity/ldap/ldap.go +++ b/internal/config/identity/ldap/ldap.go @@ -27,6 +27,7 @@ import ( ldap "github.com/go-ldap/ldap/v3" "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/minio/internal/auth" + xldap "github.com/minio/pkg/v2/ldap" ) // LookupUserDN searches for the full DN and groups of a given username @@ -57,11 +58,16 @@ func (l *Config) LookupUserDN(username string) (string, []string, error) { return bindDN, groups, nil } -// DoesUsernameExist checks if the given username exists in the LDAP directory. +// GetValidatedDNForUsername checks if the given username exists in the LDAP directory. // The given username could be just the short "login" username or the full DN. -// When the username is found, the full DN is returned, otherwise the returned -// string is empty. If the user is not found, err = nil, otherwise, err != nil. -func (l *Config) DoesUsernameExist(username string) (string, error) { +// +// When the username/DN is found, the full DN returned by the **server** is +// returned, otherwise the returned string is empty. The value returned here is +// the value sent by the LDAP server and is used in minio as the server performs +// LDAP specific normalization (including Unicode normalization). +// +// If the user is not found, err = nil, otherwise, err != nil. +func (l *Config) GetValidatedDNForUsername(username string) (string, error) { conn, err := l.LDAP.Connect() if err != nil { return "", err @@ -107,7 +113,11 @@ func (l *Config) DoesUsernameExist(username string) (string, error) { return "", err } for _, entry := range searchResult.Entries { - foundDistName = append(foundDistName, entry.DN) + normDN, err := xldap.NormalizeDN(entry.DN) + if err != nil { + return "", err + } + foundDistName = append(foundDistName, normDN) } } } @@ -123,26 +133,33 @@ func (l *Config) DoesUsernameExist(username string) (string, error) { return "", nil } -// DoesGroupDNExist checks if the given group DN exists in the LDAP directory. -func (l *Config) DoesGroupDNExist(groupDN string) (bool, error) { +// GetValidatedGroupDN checks if the given group DN exists in the LDAP directory +// and returns the group DN sent by the LDAP server. The value returned by the +// server may not be equal to the input group DN, as LDAP equality is not a +// simple Golang string equality. However, we assume the value returned by the +// LDAP server is canonical. +// +// If the group is not found in the LDAP directory, the returned string is empty +// and err = nil. +func (l *Config) GetValidatedGroupDN(groupDN string) (string, error) { if len(l.LDAP.GroupSearchBaseDistNames) == 0 { - return false, errors.New("no group search Base DNs given") + return "", errors.New("no group search Base DNs given") } gdn, err := ldap.ParseDN(groupDN) if err != nil { - return false, fmt.Errorf("Given group DN could not be parsed: %s", err) + return "", fmt.Errorf("Given group DN could not be parsed: %s", err) } conn, err := l.LDAP.Connect() if err != nil { - return false, err + return "", err } defer conn.Close() // Bind to the lookup user account if err = l.LDAP.LookupBind(conn); err != nil { - return false, err + return "", err } var foundDistName []string @@ -158,22 +175,26 @@ func (l *Config) DoesGroupDNExist(groupDN string) (bool, error) { if ldap.IsErrorWithCode(err, 32) { continue } - return false, err + return "", err } for _, entry := range searchResult.Entries { - foundDistName = append(foundDistName, entry.DN) + normDN, err := xldap.NormalizeDN(entry.DN) + if err != nil { + return "", err + } + foundDistName = append(foundDistName, normDN) } } } if len(foundDistName) == 1 { - return true, nil + return foundDistName[0], nil } else if len(foundDistName) > 1 { // FIXME: This error would happen if the multiple base DNs are given and // some base DNs are subtrees of other base DNs - we should validate // and error out in such cases. - return false, fmt.Errorf("found multiple DNs for the given group DN") + return "", fmt.Errorf("found multiple DNs for the given group DN") } - return false, nil + return "", nil } // Bind - binds to ldap, searches LDAP and returns the distinguished name of the