Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1879980: Fix swallowed error message in GroupDetector #719

Closed
wants to merge 1 commit into from
Closed

Bug 1879980: Fix swallowed error message in GroupDetector #719

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/helpers/groupsync/groupdetector/groupdetector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type GroupBasedDetector struct {
func (l *GroupBasedDetector) Exists(ldapGroupUID string) (bool, error) {
group, err := l.groupGetter.GroupEntryFor(ldapGroupUID)
if ldapquery.IsQueryOutOfBoundsError(err) || ldapquery.IsEntryNotFoundError(err) || ldapquery.IsNoSuchObjectError(err) {
return false, nil
return false, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does your fix from go-ldap/ldap#307 applies, I'm worried this is breaking expected scripts where we wanted to ignore, I'd be more inclined to log with V(4), for example, rather than hard fail.

Copy link
Contributor Author

@zerodayz zerodayz Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @soltysh this is related to https://bugzilla.redhat.com/show_bug.cgi?id=1879980

The groupSync deleted the groups because it can't find the correct OU

OU=Groups,OU=Unix != OU=groups,OU=unix

I believe this will return also if you query wrong DN. I will take some time to check it again. (its been a while ago)

Now this means you hit the error ldapquery.IsQueryOutOfBoundsError(err), but that err is never returned.

Not sure which case is used for ignoring, but if you by mistake set the wrong DN, which will hit OutOfBounds Error. GroupSync will delete all your OpenShift groups because this error is swallowed and GroupSync doesn't know you hit the wrong DN, instead it just can't see any groups, so it will sync with nothing.

The other patch for upper/lowercase in that case you are in correct DN but just wrong case.

For example:

usersQuery:
        baseDN: "ou=users,dc=redhat,dc=com"

usersQuery:
        baseDN: "ou=Users,dc=redhat,dc=com"

With go-ldap/ldap#307, we can then easily update oc client, especially https://github.com/openshift/library-go/blob/master/pkg/security/ldapquery/query.go#L116

From:
		if !baseDN.AncestorOf(dn) && !baseDN.Equal(dn) {

To:
		if !baseDN.AncestorOfFold(dn) && !baseDN.EqualFold(dn) {

But we would also need to bump the version of go-ldap to include that fix, I am not sure how to do that part.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we would also need to bump the version of go-ldap to include that fix, I am not sure how to do that part.

Hmm... it looks like we're using quite old ldap library

oc/go.mod

Line 55 in 64b59fe

gopkg.in/ldap.v2 v2.5.1
whereas the current version is 3.3 which would also require updating what we're using in oc. Lemme know if you're interested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will talk to you, I would like to take this.

}
if err != nil {
return false, err
Expand Down