From 4b501e93a43b2557f5da47c2a466db04f1c9e740 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 4 May 2023 18:18:47 +0200 Subject: [PATCH 1/3] graph/users: Avoid to leak LDAP error messages to the client --- services/graph/pkg/identity/ldap.go | 119 +++++++++++++++++------ services/graph/pkg/identity/ldap_test.go | 4 +- 2 files changed, 91 insertions(+), 32 deletions(-) diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 980cb21ca2..5059ca4722 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "strconv" "strings" @@ -85,6 +86,10 @@ type userAttributeMap struct { type ldapAttributeValues map[string][]string +type ldapResultToErrMap map[uint16]errorcode.Error + +const ldapGenericErr = math.MaxUint16 + // ParseDisableMechanismType checks that the configuration option for how to disable users is correct. func ParseDisableMechanismType(disableMechanism string) (DisableUserMechanismType, error) { disableMechanism = strings.ToLower(disableMechanism) @@ -188,14 +193,15 @@ func (i *LDAP) CreateUser(ctx context.Context, user libregraph.User) (*libregrap } if err := i.conn.Add(ar); err != nil { - var lerr *ldap.Error - logger.Debug().Err(err).Msg("error adding user") - if errors.As(err, &lerr) { - if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists { - err = errorcode.New(errorcode.NameAlreadyExists, lerr.Error()) - } + msg := "failed to add user" + logger.Error().Err(err).Msg(msg) + errMap := ldapResultToErrMap{ + ldap.LDAPResultEntryAlreadyExists: errorcode.New(errorcode.NameAlreadyExists, "a user with that name already exists"), + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return nil, err + return nil, i.mapLDAPError(err, errMap) } if i.usePwModifyExOp && user.PasswordProfile != nil && user.PasswordProfile.Password != nil { @@ -226,7 +232,15 @@ func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error { } dr := ldap.DelRequest{DN: e.DN} if err = i.conn.Del(&dr); err != nil { - return err + msg := "error deleting user" + logger.Error().Err(err).Msg(msg) + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: errorcode.New(errorcode.ItemNotFound, "user not found"), + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return i.mapLDAPError(err, errMap) } if !i.refintEnabled { @@ -303,7 +317,15 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. if user.PasswordProfile != nil && user.PasswordProfile.GetPassword() != "" { if i.usePwModifyExOp { if err := i.updateUserPassowrd(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil { - return nil, err + msg := "error updating user password" + logger.Error().Err(err).Msg(msg) + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: errorcode.New(errorcode.ItemNotFound, "user not found"), + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) } } else { // password are hashed server side there is no need to check if the new password @@ -331,7 +353,15 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. if updateNeeded { if err := i.conn.Modify(&mr); err != nil { - return nil, err + msg := "error updating user" + logger.Error().Err(err).Msg(msg) + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: errorcode.New(errorcode.ItemNotFound, "user not found"), + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) } } @@ -395,7 +425,7 @@ func (i *LDAP) getEntryByDN(dn string, attrs []string, filter string) (*ldap.Ent res, err := i.conn.Search(searchRequest) if err != nil { i.logger.Error().Err(err).Str("backend", "ldap").Str("dn", dn).Msg("Search ldap by DN failed") - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + return nil, errorcode.New(errorcode.ItemNotFound, "user lookup failed") } if len(res.Entries) == 0 { return nil, ErrNotFound @@ -428,7 +458,7 @@ func (i *LDAP) searchLDAPEntryByFilter(basedn string, attrs []string, filter str res, err := i.conn.Search(searchRequest) if err != nil { i.logger.Error().Err(err).Str("backend", "ldap").Str("dn", basedn).Str("filter", filter).Msg("Search user by filter failed") - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + return nil, errorcode.New(errorcode.ItemNotFound, "user search failed") } if len(res.Entries) == 0 { return nil, ErrNotFound @@ -580,7 +610,13 @@ func (i *LDAP) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([]*lib Msg("GetUsers") res, err := i.conn.Search(searchRequest) if err != nil { - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + msg := "error listing users" + logger.Error().Err(err).Msg(msg) + errMap := ldapResultToErrMap{ + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.AccessDenied, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) } users := make([]*libregraph.User, 0, len(res.Entries)) @@ -626,14 +662,16 @@ func (i *LDAP) changeUserName(ctx context.Context, dn, originalUserName, newUser mrdn := ldap.NewModifyDNRequest(dn, newDNString, true, "") if err := i.conn.ModifyDN(mrdn); err != nil { - var lerr *ldap.Error - logger.Debug().Str("originalDN", dn).Str("newDN", newDNString).Err(err).Msg("Failed to modify DN") - if errors.As(err, &lerr) { - if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists { - err = errorcode.New(errorcode.NameAlreadyExists, lerr.Error()) - } + msg := "error renaming user" + logger.Error().Err(err).Msg(msg) + errMap := ldapResultToErrMap{ + ldap.LDAPResultEntryAlreadyExists: errorcode.New(errorcode.NameAlreadyExists, "a user with that name already exists"), + ldap.LDAPResultNoSuchObject: errorcode.New(errorcode.ItemNotFound, msg), + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return nil, err + return nil, i.mapLDAPError(err, errMap) } parsed, err := ldap.ParseDN(dn) @@ -678,17 +716,17 @@ func (i *LDAP) renameMemberInGroup(ctx context.Context, group *ldap.Entry, oldMe mr.Delete(i.groupAttributeMap.member, []string{oldMember}) mr.Add(i.groupAttributeMap.member, []string{newMember}) if err := i.conn.Modify(mr); err != nil { + logger.Warn().Err(err). + Str("oldMember", oldMember). + Str("newMember", newMember). + Str("groupDN", group.DN). + Msg("Error renaming group members.") var lerr *ldap.Error if errors.As(err, &lerr) { - if lerr.ResultCode == ldap.LDAPResultNoSuchObject { - logger.Warn().Str("group", group.DN).Msg("Group no longer exists") - return nil - } else if lerr.ResultCode == ldap.LDAPResultNoSuchAttribute { - logger.Warn(). - Str("oldMember", oldMember). - Str("newMember", newMember). - Str("groupDN", group.DN). - Msg("member attribute not found, this probably means that the server has refint enabled, please configure the OCIS to respect that.") + // NoSuchObject means that the group no longer exists. Some other client might have deleted it. There is + // not much we can do + // NoSuchAttribute means that the old value is no longer present. We can't do much here either + if lerr.ResultCode == ldap.LDAPResultNoSuchObject || lerr.ResultCode == ldap.LDAPResultNoSuchAttribute { return nil } } @@ -929,9 +967,17 @@ func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string 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") + msg := "failed to update object" + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: errorcode.New(errorcode.ItemNotFound, "object does not exists"), + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return i.mapLDAPError(err, errMap) } - return err + return nil } // expandLDAPAttributeEntries reads an attribute from an ldap entry and expands to users @@ -1133,3 +1179,16 @@ func (i *LDAP) updateAccountEnabledState(logger log.Logger, accountEnabled bool, return updateNeeded, err } + +func (i *LDAP) mapLDAPError(err error, errmap ldapResultToErrMap) errorcode.Error { + var lerr *ldap.Error + if errors.As(err, &lerr) { + if res, ok := errmap[lerr.ResultCode]; ok { + return res + } + } + if res, ok := errmap[ldapGenericErr]; ok { + return res + } + return errorcode.New(errorcode.GeneralException, err.Error()) +} diff --git a/services/graph/pkg/identity/ldap_test.go b/services/graph/pkg/identity/ldap_test.go index 1364c0c3c3..d3bcf91921 100644 --- a/services/graph/pkg/identity/ldap_test.go +++ b/services/graph/pkg/identity/ldap_test.go @@ -282,8 +282,8 @@ func TestGetUsers(t *testing.T) { b, _ := getMockedBackend(lm, lconfig, &logger) _, err = b.GetUsers(context.Background(), odataReqDefault) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) + if err == nil || err.Error() != "generalException" { + t.Errorf("Expected 'generalException' got '%s'", err.Error()) } lm = &mocks.Client{} From 046895a83187bffede669f59eb1bdd278122b55e Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 4 May 2023 18:22:08 +0200 Subject: [PATCH 2/3] graph: Allow disabling users via groupmember ship on "read-only" server When GRAPH_LDAP_SERVER_WRITE_ENABLED is set to false we still allow updates of the accountEnabled property when OCIS_LDAP_DISABLE_USER_MECHANISM is set to "group" Partial-Fix: #6219 --- services/graph/pkg/identity/ldap.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 5059ca4722..6920a43f4c 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -267,7 +267,12 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("UpdateUser") if !i.writeEnabled { - return nil, ErrReadOnly + // still allow eanble/disable User when using DisableMechanismGroup + if i.disableUserMechanism == DisableMechanismGroup && isUserEnabledUpdate(user) { + logger.Error().Str("backend", "ldap").Msg("Allowing accountEnabled Update on read-only backend") + } else { + return nil, ErrReadOnly + } } e, err := i.getLDAPUserByNameOrID(nameOrID) if err != nil { @@ -1192,3 +1197,19 @@ func (i *LDAP) mapLDAPError(err error, errmap ldapResultToErrMap) errorcode.Erro } return errorcode.New(errorcode.GeneralException, err.Error()) } + +func isUserEnabledUpdate(user libregraph.User) bool { + switch { + case user.Id != nil, user.DisplayName != nil, + user.Drive != nil, user.Mail != nil, user.OnPremisesSamAccountName != nil, + user.PasswordProfile != nil, user.Surname != nil, user.GivenName != nil, + user.UserType != nil: + return false + case len(user.AppRoleAssignments) != 0, + len(user.MemberOf) != 0, + len(user.Identities) != 0, + len(user.Drives) != 0: + return false + } + return user.AccountEnabled != nil +} From 906189462c44bc4b4365a829b36d4d8d7f99cc76 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 4 May 2023 18:37:16 +0200 Subject: [PATCH 3/3] graph: Always allow updates to "local" groups when LDAP When GRAPH_LDAP_SERVER_WRITE_ENABLED=false still allow updates of groups if a distinct GRAPH_LDAP_GROUP_CREATE_BASE_DN is configured. Partial-Fix: #6219 --- services/graph/pkg/identity/ldap_group.go | 14 +++++--------- services/graph/pkg/identity/ldap_group_test.go | 4 ++++ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index d19b86e12c..f4c8e85062 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/services/graph/pkg/identity/ldap_group.go @@ -177,7 +177,7 @@ func (i *LDAP) GetGroupMembers(ctx context.Context, groupID string, req *godata. func (i *LDAP) CreateGroup(ctx context.Context, group libregraph.Group) (*libregraph.Group, error) { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("create group") - if !i.writeEnabled { + if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { return nil, errorcode.New(errorcode.NotAllowed, "server is configured read-only") } ar, err := i.groupToAddRequest(group) @@ -201,7 +201,7 @@ func (i *LDAP) CreateGroup(ctx context.Context, group libregraph.Group) (*libreg func (i *LDAP) DeleteGroup(ctx context.Context, id string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("DeleteGroup") - if !i.writeEnabled { + if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { return errorcode.New(errorcode.NotAllowed, "server is configured read-only") } @@ -225,7 +225,7 @@ func (i *LDAP) DeleteGroup(ctx context.Context, id string) error { func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup") - if !i.writeEnabled { + if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { return errorcode.New(errorcode.NotAllowed, "server is configured read-only") } @@ -271,7 +271,7 @@ func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName st func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs []string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup") - if !i.writeEnabled { + if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { return errorcode.New(errorcode.NotAllowed, "server is configured read-only") } ge, err := i.getLDAPGroupByNameOrID(groupID, true) @@ -365,7 +365,7 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error { logger := i.logger.SubloggerWithRequestID(ctx) logger.Debug().Str("backend", "ldap").Msg("RemoveMemberFromGroup") - if !i.writeEnabled { + if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { return errorcode.New(errorcode.NotAllowed, "server is configured read-only") } @@ -562,10 +562,6 @@ func (i *LDAP) createGroupModelFromLDAP(e *ldap.Entry) *libregraph.Group { } func (i *LDAP) isLDAPGroupReadOnly(e *ldap.Entry) bool { - if !i.writeEnabled { - return true - } - groupDN, err := ldap.ParseDN(e.DN) if err != nil { i.logger.Warn().Err(err).Str("dn", e.DN).Msg("Failed to parse DN") diff --git a/services/graph/pkg/identity/ldap_group_test.go b/services/graph/pkg/identity/ldap_group_test.go index f3557cb197..e823444c48 100644 --- a/services/graph/pkg/identity/ldap_group_test.go +++ b/services/graph/pkg/identity/ldap_group_test.go @@ -161,6 +161,10 @@ func TestGetGroup(t *testing.T) { func TestGetGroupReadOnlyBackend(t *testing.T) { readOnlyConfig := lconfig readOnlyConfig.WriteEnabled = false + readOnlyConfig.GroupBaseDN = "ou=groups,dc=test" + readOnlyConfig.GroupCreateBaseDN = "ou=local,ou=group,dc=test" + localGroupEntry := groupEntry + localGroupEntry.DN = "cn=local,ou=local,o=base" lm := &mocks.Client{} lm.On("Search", groupLookupSearchRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)