From 3e071c9a47df7c18336ffd60d903e017ff2dd6fa Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 3 May 2023 13:24:49 +0200 Subject: [PATCH] graph-ldap: Fix possible races when editing group membership in parallel As the standard LDAP groups (groupOfNames) require at least one "member" value to be present in a group, we have workarounds in place that add an empty member ("") when creating a new group or when removing the last member from the group. This can cause a race condition when e.g. multiple request to remove members from a group an running in parallel, as we need to read the group before we can construct the modification request. If some other request modified the group (e.g. deleted the 2nd last member) after we read it, we create non-working modification request. These changes try to catch those errors and retry the modification request once. Fixes: #6170 --- services/graph/pkg/identity/ldap.go | 60 ++++++++++++------- .../pkg/identity/ldap_education_class.go | 6 +- services/graph/pkg/identity/ldap_group.go | 46 ++++++++++---- 3 files changed, 74 insertions(+), 38 deletions(-) diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 259b32b587f..980cb21ca20 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -238,12 +238,10 @@ func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error { for _, group := range groupEntries { logger.Debug().Str("group", group.DN).Str("user", e.DN).Msg("Cleaning up group membership") - if mr, err := i.removeEntryByDNAndAttributeFromEntry(group, e.DN, i.groupAttributeMap.member); err == nil { - if err = i.conn.Modify(mr); err != nil { - // Errors when deleting the memberships are only logged as warnings but not returned - // to the user as we already successfully deleted the users itself - logger.Warn().Str("group", group.DN).Str("user", e.DN).Err(err).Msg("failed to remove member") - } + if err := i.removeEntryByDNAndAttributeFromEntry(group, e.DN, i.groupAttributeMap.member); err != nil { + // Errors when deleting the memberships are only logged as warnings but not returned + // to the user as we already successfully deleted the users itself + logger.Warn().Str("group", group.DN).Str("user", e.DN).Err(err).Msg("failed to remove member") } } } @@ -878,40 +876,62 @@ func stringToScope(scope string) (int, error) { } // removeEntryByDNAndAttributeFromEntry creates a request to remove a single member entry by attribute and DN from an ldap entry -func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string, attribute string) (*ldap.ModifyRequest, error) { +func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string, attribute string) error { nOldDN, err := ldapdn.ParseNormalize(dn) if err != nil { - return nil, err + return err } - entries := entry.GetEqualFoldAttributeValues(attribute) + + currentValues := entry.GetEqualFoldAttributeValues(attribute) + i.logger.Error().Interface("members", currentValues).Msg("current values") found := false - for _, entry := range entries { - if entry == "" { + for _, currentValue := range currentValues { + if currentValue == "" { continue } - if nEntry, err := ldapdn.ParseNormalize(entry); err != nil { + if normalizedCurrentValue, err := ldapdn.ParseNormalize(currentValue); err != nil { // We couldn't parse the entry value as a DN. Let's keep it // as it is but log a warning - i.logger.Warn().Str("entryDN", entry).Err(err).Msg("Couldn't parse DN") + i.logger.Warn().Str("member", currentValue).Err(err).Msg("Couldn't parse DN") continue } else { - if nEntry == nOldDN { + if normalizedCurrentValue == nOldDN { found = true } } } if !found { - i.logger.Debug().Str("backend", "ldap").Str("entry", entry.DN).Str("target", dn). - Msg("The target is not an entry in the attribute list") - return nil, ErrNotFound + i.logger.Error().Str("backend", "ldap").Str("entry", entry.DN).Str("target", dn). + Msg("The target value is not present in the attribute list") + return ErrNotFound } - mr := ldap.ModifyRequest{DN: entry.DN} - if len(entries) == 1 { + mr := &ldap.ModifyRequest{DN: entry.DN} + if len(currentValues) == 1 { mr.Add(attribute, []string{""}) } mr.Delete(attribute, []string{dn}) - return &mr, nil + + err = i.conn.Modify(mr) + var lerr *ldap.Error + if err != nil && errors.As(err, &lerr) { + if lerr.ResultCode == ldap.LDAPResultObjectClassViolation { + // objectclass "groupOfName" requires at least one member to be present, some other go-routine + // must have removed the 2nd last member from the group after we read the group. We adapt the + // modification request to replace the last member with an empty member and re-try. + i.logger.Debug().Err(err). + Msg("Failed to remove last group member. Retrying once. Replacing last group member with an empty member value.") + mr.Add(attribute, []string{""}) + err = i.conn.Modify(mr) + } + } + + if err != nil { + i.logger.Error().Err(err).Str("entry", entry.DN).Str("attribute", attribute).Str("target value", dn). + Msg("Failed to remove dn attribute from entry") + } + + return err } // expandLDAPAttributeEntries reads an attribute from an ldap entry and expands to users diff --git a/services/graph/pkg/identity/ldap_education_class.go b/services/graph/pkg/identity/ldap_education_class.go index c7f657355d6..283506e39ab 100644 --- a/services/graph/pkg/identity/ldap_education_class.go +++ b/services/graph/pkg/identity/ldap_education_class.go @@ -460,9 +460,5 @@ func (i *LDAP) RemoveTeacherFromEducationClass(ctx context.Context, classID stri return err } - if mr, err := i.removeEntryByDNAndAttributeFromEntry(class, teacher.DN, i.educationConfig.classAttributeMap.teachers); err == nil { - return i.conn.Modify(mr) - } - - return nil + return i.removeEntryByDNAndAttributeFromEntry(class, teacher.DN, i.educationConfig.classAttributeMap.teachers) } diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index 67a4abe2688..d19b86e12cd 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/services/graph/pkg/identity/ldap_group.go @@ -325,18 +325,37 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs } if len(newMemberDN) > 0 { - mr.Add(i.groupAttributeMap.member, newMemberDN) - - if err := i.conn.Modify(&mr); err != nil { - if lerr, ok := err.(*ldap.Error); ok { - if lerr.ResultCode == ldap.LDAPResultAttributeOrValueExists { - err = fmt.Errorf("Duplicate member entries in request") - } else { - logger.Info().Err(err).Msg("Failed to modify group member entries on PATCH group") - err = fmt.Errorf("Unknown error when trying to modify group member entries") + // Small retry loop. It might be that, when reading the group we found the empty group member ("", + // line 289 above). Our modify operation tries to delete that value. However another go-routine + // might have done that in parallel. In that case + // (LDAPResultNoSuchAttribute) we need to retry the modification + // without the delete. + for j := 0; j < 2; j++ { + mr.Add(i.groupAttributeMap.member, newMemberDN) + if err := i.conn.Modify(&mr); err != nil { + if lerr, ok := err.(*ldap.Error); ok { + switch lerr.ResultCode { + case ldap.LDAPResultAttributeOrValueExists: + err = fmt.Errorf("Duplicate member entries in request") + case ldap.LDAPResultNoSuchAttribute: + if len(mr.Changes) == 2 { + // We tried the special case for adding the first group member, but some + // other request running in parallel did that already. Retry with a "normal" + // modification + logger.Debug().Err(err). + Msg("Failed to add first group member. Retrying once, without deleting the empty member value.") + mr.Changes = make([]ldap.Change, 0, 1) + continue + } + default: + logger.Info().Err(err).Msg("Failed to modify group member entries on PATCH group") + err = fmt.Errorf("Unknown error when trying to modify group member entries") + } } + return err } - return err + // succeeded + break } } return nil @@ -365,12 +384,13 @@ func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, member logger.Debug().Str("backend", "ldap").Str("memberID", memberID).Msg("Error looking up group member") return err } + logger.Debug().Str("backend", "ldap").Str("groupdn", ge.DN).Str("member", me.DN).Msg("remove member") - if mr, err := i.removeEntryByDNAndAttributeFromEntry(ge, me.DN, i.groupAttributeMap.member); err == nil { - return i.conn.Modify(mr) + if err = i.removeEntryByDNAndAttributeFromEntry(ge, me.DN, i.groupAttributeMap.member); err != nil { + logger.Error().Err(err).Str("backend", "ldap").Str("group", groupID).Str("member", memberID).Msg("Failed to remove member from group.") } - return nil + return err } func (i *LDAP) groupToAddRequest(group libregraph.Group) (*ldap.AddRequest, error) {