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.

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

Switch to go-ldap/ldap for filter (de-)compilation #68

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

rhafer
Copy link
Collaborator

@rhafer rhafer commented Jul 12, 2022

The filter parsing code that was originally imported from
https://github.com/nmcclain/ldap/ had some issues with decompiling Filters
from BER to their string representation. This removes the filter
parsing from idm and uses the code from go-ldap/ldap now, which is
basically a drop-in replacement.

Fixes: #67

Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

@rhafer i dont remember exactly why I added a local variant of the filter functionality in the first place. Is the go-ldap stuff newer?

There are some changes in the filter implementation https://github.com/libregraph/idm/commits/ba794e219d769b14407879d54008d6e13cf8f593/internal/ldapserver/filter.go?browsing_rename_history=true&new_path=pkg/ldapserver/filter.go&original_branch=master - please verify that those are indeed no longer needed / useful.

Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

@rhafer can we keep this change API compatible / not remove or change any publich API in the ldapserver package. Should be doable by importing them from go-ldap in the filter.go file and exporting public identifiers or wrapper functions for the stuff. This would avoid the need for a new major release and makes the fix available for anyone using the ldapserver package from here.

@rhafer
Copy link
Collaborator Author

rhafer commented Jul 12, 2022

i dont remember exactly why I added a local variant of the filter functionality in the first place. Is the go-ldap stuff newer?

Yes. E.q. it has the required Escape fixes https://github.com/go-ldap/ldap/blob/master/v3/filter.go#L148

@rhafer
Copy link
Collaborator Author

rhafer commented Jul 12, 2022

There are some changes in the filter implementation https://github.com/libregraph/idm/commits/ba794e219d769b14407879d54008d6e13cf8f593/internal/ldapserver/filter.go?browsing_rename_history=true&new_path=pkg/ldapserver/filter.go&original_branch=master - please verify that those are indeed no longer needed / useful.

As far as I can see the only relevant change in idm that we would loose in idm is this hunk from c2b9f72

 func CompileFilter(filter string) (*ber.Packet, error) {
-	if len(filter) == 0 || filter[0] != '(' {
+	if filter == "" || filter[0] != '(' {
		return nil, ldap.NewError(ldap.ErrorFilterCompile, errors.New("ldap: filter does not start with an '('"))
	}
	packet, pos, err := compileFilter(filter, 1)

I seriously doubt that that change has any relevant impact. 😄

@longsleep
Copy link
Collaborator

I seriously doubt that that change has any relevant impact.

This one probably not. But others were made after benchmarking / profiling. Especially the stuff with the case insensitive comparison.

@rhafer
Copy link
Collaborator Author

rhafer commented Jul 12, 2022

This one probably not. But others were made after benchmarking / profiling. Especially the stuff with the case insensitive comparison.

Sure. But that was in ServerApplyFilter. Which I haven't touched.

The filter parsing code that was originally imported from
https://github.com/nmcclain/ldap/ had some issues with decompiling Filters
from BER to their string representation. This replaces the filter
parsing in a backwards compatible way and uses the more uptodate code from
go-ldap/ldap now, which is basically a drop-in replacement.

Fixes: libregraph#67
Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rhafer rhafer merged commit 45c5c6b into libregraph:master Jul 13, 2022
@rhafer rhafer deleted the filter branch July 13, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors when processing filters that require escaping
2 participants