From c9e9a8e2b9d98779c71a18fb8c56d1c3e3ea7f21 Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Thu, 4 Apr 2024 11:36:18 -0700 Subject: [PATCH] fix: ldap: use validated base DNs (#19406) This fixes a regression from #19358 which prevents policy mappings created in the latest release from being displayed in policy entity listing APIs. This is due to the possibility that the base DNs in the LDAP config are not in a normalized form and #19358 introduced normalized of mapping keys (user DNs and group DNs). When listing, we check if the policy mappings are on entities that parse as valid DNs that are descendants of the base DNs in the config. Test added that demonstrates a failure without this fix. --- cmd/admin-handlers-idp-ldap.go | 10 +- cmd/sts-handlers.go | 10 +- cmd/sts-handlers_test.go | 61 +++++++++++- go.mod | 2 +- go.sum | 4 +- internal/config/identity/ldap/ldap.go | 134 ++++++++++++-------------- 6 files changed, 137 insertions(+), 84 deletions(-) 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 } }