diff --git a/cmd/admin-handlers-idp-ldap.go b/cmd/admin-handlers-idp-ldap.go index 850d78524..8302df13a 100644 --- a/cmd/admin-handlers-idp-ldap.go +++ b/cmd/admin-handlers-idp-ldap.go @@ -253,13 +253,16 @@ func (a adminAPIHandlers) AddServiceAccountLDAP(w http.ResponseWriter, r *http.R opts.claims[k] = v } } else { - isDN := globalIAMSys.LDAPConfig.IsLDAPUserDN(targetUser) + // We still need to ensure that the target user is a valid LDAP user. + // + // The target user may be supplied as a (short) username or a DN. + // However, for now, we only support using the short username. opts.claims[ldapUserN] = targetUser // simple username targetUser, targetGroups, err = globalIAMSys.LDAPConfig.LookupUserDN(targetUser) if err != nil { // if not found, check if DN - if strings.Contains(err.Error(), "not found") && isDN { + if strings.Contains(err.Error(), "not found") && globalIAMSys.LDAPConfig.ParsesAsDN(targetUser) { // warn user that DNs are not allowed err = fmt.Errorf("Must use short username to add service account. %w", err) } @@ -377,7 +380,8 @@ func (a adminAPIHandlers) ListAccessKeysLDAP(w http.ResponseWriter, r *http.Requ if err != nil { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return - } else if userDN == "" { + } + if targetAccount == "" { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errNoSuchUser), r.URL) return } diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go index 35f34dcc7..701159346 100644 --- a/cmd/sts-handlers.go +++ b/cmd/sts-handlers.go @@ -74,8 +74,8 @@ const ( parentClaim = "parent" // LDAP claim keys - ldapUser = "ldapUser" - ldapUserN = "ldapUsername" + ldapUser = "ldapUser" // this is a key name for a DN value + ldapUserN = "ldapUsername" // this is a key name for the short/login username // Role Claim key roleArnClaim = "roleArn" @@ -676,7 +676,11 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r * } // Check if this user or their groups have a policy applied. - ldapPolicies, _ := globalIAMSys.PolicyDBGet(ldapUserDN, groupDistNames...) + ldapPolicies, err := globalIAMSys.PolicyDBGet(ldapUserDN, groupDistNames...) + if err != nil { + writeSTSErrorResponse(ctx, w, ErrSTSInternalError, err) + return + } 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", diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index 119574b5a..415f8c080 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -663,6 +663,36 @@ func (s *TestSuiteIAM) SetUpLDAP(c *check, serverAddr string) { s.RestartIAMSuite(c) } +// SetUpLDAPWithNonNormalizedBaseDN - expects to setup an LDAP test server using +// the test LDAP container and canned data from +// https://github.com/minio/minio-ldap-testing +// +// Sets up non-normalized base DN configuration for testing. +func (s *TestSuiteIAM) SetUpLDAPWithNonNormalizedBaseDN(c *check, serverAddr string) { + ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) + defer cancel() + + configCmds := []string{ + "identity_ldap", + fmt.Sprintf("server_addr=%s", serverAddr), + "server_insecure=on", + "lookup_bind_dn=cn=admin,dc=min,dc=io", + "lookup_bind_password=admin", + // `DC` is intentionally capitalized here. + "user_dn_search_base_dn=DC=min,DC=io", + "user_dn_search_filter=(uid=%s)", + // `DC` is intentionally capitalized here. + "group_search_base_dn=ou=swengg,DC=min,dc=io", + "group_search_filter=(&(objectclass=groupofnames)(member=%d))", + } + _, err := s.adm.SetConfigKV(ctx, strings.Join(configCmds, " ")) + if err != nil { + c.Fatalf("unable to setup LDAP for tests: %v", err) + } + + s.RestartIAMSuite(c) +} + const ( EnvTestLDAPServer = "_MINIO_LDAP_TEST_SERVER" ) @@ -677,7 +707,7 @@ func TestIAMWithLDAPServerSuite(t *testing.T) { ldapServer := os.Getenv(EnvTestLDAPServer) if ldapServer == "" { - c.Skip("Skipping LDAP test as no LDAP server is provided.") + c.Skipf("Skipping LDAP test as no LDAP server is provided via %s", EnvTestLDAPServer) } suite.SetUpSuite(c) @@ -693,6 +723,35 @@ func TestIAMWithLDAPServerSuite(t *testing.T) { } } +// This test is for a fix added to handle non-normalized base DN values in the +// LDAP configuration. It runs the existing LDAP sub-tests with a non-normalized +// LDAP configuration. +func TestIAMWithLDAPNonNormalizedBaseDNConfigServerSuite(t *testing.T) { + for i, testCase := range iamTestSuites { + t.Run( + fmt.Sprintf("Test: %d, ServerType: %s", i+1, testCase.ServerTypeDescription), + func(t *testing.T) { + c := &check{t, testCase.serverType} + suite := testCase + + ldapServer := os.Getenv(EnvTestLDAPServer) + if ldapServer == "" { + c.Skipf("Skipping LDAP test as no LDAP server is provided via %s", EnvTestLDAPServer) + } + + suite.SetUpSuite(c) + suite.SetUpLDAPWithNonNormalizedBaseDN(c, ldapServer) + suite.TestLDAPSTS(c) + suite.TestLDAPUnicodeVariations(c) + suite.TestLDAPSTSServiceAccounts(c) + suite.TestLDAPSTSServiceAccountsWithUsername(c) + suite.TestLDAPSTSServiceAccountsWithGroups(c) + suite.TearDownSuite(c) + }, + ) + } +} + func (s *TestSuiteIAM) TestLDAPSTS(c *check) { ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) defer cancel() diff --git a/go.mod b/go.mod index 2a367b32b..f53009b54 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.14 + github.com/minio/pkg/v2 v2.0.16 github.com/minio/selfupdate v0.6.0 github.com/minio/sha256-simd v1.0.1 github.com/minio/simdjson-go v0.4.5 diff --git a/go.sum b/go.sum index f6a144614..b359fe24c 100644 --- a/go.sum +++ b/go.sum @@ -453,8 +453,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.14 h1:tPhDYxgvv3LNqmfCSe2zsSXrcaIj4ANyL1VRGdEkahI= -github.com/minio/pkg/v2 v2.0.14/go.mod h1:zbVATXCinLCo+L/4vsPyqgiA4OYPXCJb+/E4KfE396A= +github.com/minio/pkg/v2 v2.0.16 h1:qBw2D08JE7fu4UORIxx0O4L09NM0wtMrw9sJRU5R1u0= +github.com/minio/pkg/v2 v2.0.16/go.mod h1:V+OP/fKRD/qhJMQpdXXrCXcLYjGMpHKEE26zslthm5k= 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 0cca442ff..99e6c87b1 100644 --- a/internal/config/identity/ldap/ldap.go +++ b/internal/config/identity/ldap/ldap.go @@ -30,7 +30,8 @@ import ( xldap "github.com/minio/pkg/v2/ldap" ) -// LookupUserDN searches for the full DN and groups of a given username +// LookupUserDN searches for the full DN and groups of a given short/login +// username. func (l *Config) LookupUserDN(username string) (string, []string, error) { conn, err := l.LDAP.Connect() if err != nil { @@ -80,10 +81,9 @@ func (l *Config) GetValidatedDNForUsername(username string) (string, error) { } // Check if the passed in username is a valid DN. - parsedUsernameDN, err := ldap.ParseDN(username) - if err != nil { - // Since the passed in username was not a DN, we consider it as a login - // username and attempt to check it exists in the directory. + if !l.ParsesAsDN(username) { + // We consider it as a login username and attempt to check it exists in + // the directory. bindDN, err := l.LDAP.LookupUserDN(conn, username) if err != nil { if strings.Contains(err.Error(), "not found") { @@ -96,41 +96,28 @@ func (l *Config) GetValidatedDNForUsername(username string) (string, error) { // Since the username is a valid DN, check that it is under a configured // base DN in the LDAP directory. - var foundDistName []string + + // Check that userDN exists in the LDAP directory. + validatedUserDN, err := xldap.LookupDN(conn, username) + if err != nil { + return "", fmt.Errorf("Error looking up user DN %s: %w", username, err) + } + if validatedUserDN == "" { + return "", nil + } + + // This will return an error as the argument is validated to be a DN. + udn, _ := ldap.ParseDN(validatedUserDN) + + // Check that the user DN is under a configured user base DN in the LDAP + // directory. for _, baseDN := range l.LDAP.UserDNSearchBaseDistNames { - // BaseDN should not fail to parse. - baseDNParsed, _ := ldap.ParseDN(baseDN) - if baseDNParsed.AncestorOf(parsedUsernameDN) { - searchRequest := ldap.NewSearchRequest(username, ldap.ScopeBaseObject, ldap.NeverDerefAliases, - 0, 0, false, "(objectClass=*)", nil, nil) - searchResult, err := conn.Search(searchRequest) - if err != nil { - // Check if there is no matching result. - // Ref: https://ldap.com/ldap-result-code-reference/ - if ldap.IsErrorWithCode(err, 32) { - continue - } - return "", err - } - for _, entry := range searchResult.Entries { - normDN, err := xldap.NormalizeDN(entry.DN) - if err != nil { - return "", err - } - foundDistName = append(foundDistName, normDN) - } + if baseDN.Parsed.AncestorOf(udn) { + return validatedUserDN, nil } } - if len(foundDistName) == 1 { - 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 "", fmt.Errorf("found multiple DNs for the given username") - } - return "", nil + return "", fmt.Errorf("User DN %s is not under any configured user base DN", validatedUserDN) } // GetValidatedGroupDN checks if the given group DN exists in the LDAP directory @@ -146,11 +133,6 @@ func (l *Config) GetValidatedGroupDN(groupDN string) (string, error) { return "", errors.New("no group search Base DNs given") } - gdn, err := ldap.ParseDN(groupDN) - if err != nil { - return "", fmt.Errorf("Given group DN could not be parsed: %s", err) - } - conn, err := l.LDAP.Connect() if err != nil { return "", err @@ -162,39 +144,28 @@ func (l *Config) GetValidatedGroupDN(groupDN string) (string, error) { return "", err } - var foundDistName []string + // Check that groupDN exists in the LDAP directory. + validatedGroupDN, err := xldap.LookupDN(conn, groupDN) + if err != nil { + return "", fmt.Errorf("Error looking up group DN %s: %w", groupDN, err) + } + if validatedGroupDN == "" { + return "", nil + } + + gdn, err := ldap.ParseDN(validatedGroupDN) + if err != nil { + return "", fmt.Errorf("Given group DN %s could not be parsed: %w", validatedGroupDN, err) + } + + // Check that the group DN is under a configured group base DN in the LDAP + // directory. for _, baseDN := range l.LDAP.GroupSearchBaseDistNames { - // BaseDN should not fail to parse. - baseDNParsed, _ := ldap.ParseDN(baseDN) - if baseDNParsed.AncestorOf(gdn) { - searchRequest := ldap.NewSearchRequest(groupDN, ldap.ScopeBaseObject, ldap.NeverDerefAliases, 0, 0, false, "(objectClass=*)", nil, nil) - searchResult, err := conn.Search(searchRequest) - if err != nil { - // Check if there is no matching result. - // Ref: https://ldap.com/ldap-result-code-reference/ - if ldap.IsErrorWithCode(err, 32) { - continue - } - return "", err - } - for _, entry := range searchResult.Entries { - normDN, err := xldap.NormalizeDN(entry.DN) - if err != nil { - return "", err - } - foundDistName = append(foundDistName, normDN) - } + if baseDN.Parsed.AncestorOf(gdn) { + return validatedGroupDN, nil } } - if len(foundDistName) == 1 { - 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 "", fmt.Errorf("found multiple DNs for the given group DN") - } - return "", nil + return "", fmt.Errorf("Group DN %s is not under any configured group base DN", validatedGroupDN) } // Bind - binds to ldap, searches LDAP and returns the distinguished name of the @@ -259,10 +230,21 @@ func (l Config) GetExpiryDuration(dsecs string) (time.Duration, error) { return dur, nil } +// ParsesAsDN determines if the given string could be a valid DN based on +// parsing alone. +func (l Config) ParsesAsDN(dn string) bool { + _, err := ldap.ParseDN(dn) + return err == nil +} + // IsLDAPUserDN determines if the given string could be a user DN from LDAP. func (l Config) IsLDAPUserDN(user string) bool { + udn, err := ldap.ParseDN(user) + if err != nil { + return false + } for _, baseDN := range l.LDAP.UserDNSearchBaseDistNames { - if strings.HasSuffix(user, ","+baseDN) { + if baseDN.Parsed.AncestorOf(udn) { return true } } @@ -270,9 +252,13 @@ func (l Config) IsLDAPUserDN(user string) bool { } // IsLDAPGroupDN determines if the given string could be a group DN from LDAP. -func (l Config) IsLDAPGroupDN(user string) bool { +func (l Config) IsLDAPGroupDN(group string) bool { + gdn, err := ldap.ParseDN(group) + if err != nil { + return false + } for _, baseDN := range l.LDAP.GroupSearchBaseDistNames { - if strings.HasSuffix(user, ","+baseDN) { + if baseDN.Parsed.AncestorOf(gdn) { return true } }