diff --git a/backend/internal/ldapsync/service.go b/backend/internal/ldapsync/service.go index 32067805..0c0cea0a 100644 --- a/backend/internal/ldapsync/service.go +++ b/backend/internal/ldapsync/service.go @@ -779,29 +779,24 @@ func (s *Service) saveProfilePicture(parentCtx context.Context, userId string, p return nil } -// normalizeLDAPDN returns a canonical lowercase form of a DN for use as a map key. -// Different LDAP servers may format the same DN with varying attribute type casing (e.g. "CN=" vs "cn=") or extra whitespace (e.g. "dc=example, dc=com"). -// Without normalization, cache lookups in usernamesByDN would miss when a member attribute value uses a different format than the DN returned in the search entry -// -// ldap.ParseDN is used instead of simple lowercasing because it correctly handles multi-valued RDNs (joined with "+") and strips inter-component whitespace. -// If parsing fails for any reason, we fall back to a simple lowercase+trim. +// normalizeLDAPDN returns a canonical lowercase form of a DN for use as a map key +// Escaping preserves the distinction between separators and literal characters inside attribute values func normalizeLDAPDN(dn string) string { + // Parse components so equivalent formatting shares a cache key parsed, err := ldap.ParseDN(dn) if err != nil { return strings.ToLower(strings.TrimSpace(dn)) } - // Reconstruct the DN in a canonical form: lowercase type=lowercase value, with RDN components separated by "," and multi-value attributes by "+" - parts := make([]string, 0, len(parsed.RDNs)) + // Preserve case-insensitive lookups while letting the LDAP serializer escape values and sort multi-valued RDNs for _, rdn := range parsed.RDNs { - attrs := make([]string, 0, len(rdn.Attributes)) for _, attr := range rdn.Attributes { - attrs = append(attrs, strings.ToLower(attr.Type)+"="+strings.ToLower(attr.Value)) + attr.Type = strings.ToLower(attr.Type) + attr.Value = strings.ToLower(attr.Value) } - parts = append(parts, strings.Join(attrs, "+")) } - return strings.Join(parts, ",") + return parsed.String() } // getDNProperty returns the value of a property from a LDAP identifier diff --git a/backend/internal/ldapsync/service_test.go b/backend/internal/ldapsync/service_test.go index 4b9a74fe..66edfc23 100644 --- a/backend/internal/ldapsync/service_test.go +++ b/backend/internal/ldapsync/service_test.go @@ -196,6 +196,84 @@ func TestLdapServiceSyncAllMapsPosixGroupMemberUid(t *testing.T) { assert.ElementsMatch(t, []string{"alice", "bob"}, usernames(group.Users)) } +func TestLdapServiceSyncAllKeepsDistinctDNMemberships(t *testing.T) { + tests := []struct { + name string + memberDN string + attackerDN string + }{ + { + name: "escaped comma", + memberDN: "cn=admin,ou=staff,dc=example,dc=com", + attackerDN: `cn=admin\,ou=staff,dc=example,dc=com`, + }, + { + name: "escaped plus", + memberDN: "cn=admin+ou=staff,dc=example,dc=com", + attackerDN: `cn=admin\+ou=staff,dc=example,dc=com`, + }, + } + + for _, tt := range tests { + for _, attackerFirst := range []bool{false, true} { + order := "/attacker last" + if attackerFirst { + order = "/attacker first" + } + t.Run(tt.name+order, func(t *testing.T) { + // Keep the usernames and immutable IDs distinct so only DN resolution can confuse the users + entries := []*ldap.Entry{ + ldapEntry(tt.memberDN, map[string][]string{ + "entryUUID": {"u-admin"}, + "uid": {"admin"}, + "givenName": {"Admin"}, + "sn": {"User"}, + }), + ldapEntry(tt.attackerDN, map[string][]string{ + "entryUUID": {"u-attacker"}, + "uid": {"attacker"}, + "givenName": {"Attacker"}, + "sn": {"User"}, + }), + } + if attackerFirst { + entries[0], entries[1] = entries[1], entries[0] + } + + // Exercise the persisted admin flag and ordinary group memberships through the same sync + service, db := newTestLdapService(t, newFakeLDAPClient( + ldapSearchResult(entries...), + ldapSearchResult( + ldapEntry("cn=admins,ou=groups,dc=example,dc=com", map[string][]string{ + "entryUUID": {"g-admins"}, + "cn": {"admins"}, + "member": {tt.memberDN}, + }), + ldapEntry("cn=restricted,ou=groups,dc=example,dc=com", map[string][]string{ + "entryUUID": {"g-restricted"}, + "cn": {"restricted"}, + "member": {tt.memberDN}, + }), + ), + )) + require.NoError(t, service.SyncAll(t.Context(), defaultTestLDAPAppConfig())) + + var admin, attacker model.User + require.NoError(t, db.First(&admin, "ldap_id = ?", "u-admin").Error) + require.NoError(t, db.First(&attacker, "ldap_id = ?", "u-attacker").Error) + assert.True(t, admin.IsAdmin) + assert.False(t, attacker.IsAdmin) + + for _, groupID := range []string{"g-admins", "g-restricted"} { + var group model.UserGroup + require.NoError(t, db.Preload("Users").First(&group, "ldap_id = ?", groupID).Error) + assert.Equal(t, []string{"admin"}, usernames(group.Users)) + } + }) + } + } +} + func TestLdapServiceSyncAllHandlesDuplicateLDAPIDsInSingleRun(t *testing.T) { service, db := newTestLdapService(t, newFakeLDAPClient( ldapSearchResult( @@ -547,6 +625,26 @@ func TestNormalizeLDAPDN(t *testing.T) { input: "cn=alice+uid=a123,dc=example,dc=com", expected: "cn=alice+uid=a123,dc=example,dc=com", }, + { + name: "reordered multi-valued RDN", + input: "UID=A123+CN=Alice,dc=example,dc=com", + expected: "cn=alice+uid=a123,dc=example,dc=com", + }, + { + name: "escaped comma remains part of the value", + input: `CN=Admin\,OU=Staff,DC=example,DC=com`, + expected: `cn=admin\,ou=staff,dc=example,dc=com`, + }, + { + name: "hex-escaped comma shares the same key", + input: `cn=admin\2cou=staff,dc=example,dc=com`, + expected: `cn=admin\,ou=staff,dc=example,dc=com`, + }, + { + name: "escaped plus remains part of the value", + input: `cn=admin\+ou=staff,dc=example,dc=com`, + expected: `cn=admin\+ou=staff,dc=example,dc=com`, + }, { name: "invalid DN falls back to lowercase+trim", input: " NOT A VALID DN ",