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

@@ -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
 	}

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",

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()