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

Fix paged search rfc2696_cookie for Microsoft Active Directory #369

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

amuta
Copy link

@amuta amuta commented Jul 21, 2020

When building the cookie of a paged search we are currently calling to_ber when we should be calling to_ber_bin to avoid encoding it to UTF-8 and 'breaking' the cookie format. When looking at the method to_ber_bin, it seems to be build with this in mind.

> lib/net/ldap/connection.rb:20
# Converts a string to a BER string but does *not* encode to UTF-8 first.
# This is required for proper representation of binary data for Microsoft
# Active Directory
def to_ber_bin(code = 0x04)
  [code].pack('C') + length.to_ber_length_encoding + self
end

Without this change, when trying to do a paged search the behavior is the same as described in this issue: #36 Wrong paged result from search

#36 (comment)

Even though the page size is set to 126 and the possible result set is generally much larger than 1000 entries (I set the size to a very large number as I want the whole result set) the behaviour I'm seeing with AD is:

a first page containing 126 results
a 2nd page up until 1126 results
then Size Limit exceeded error
as if it was not actually doing any paging (126 results at a time).

The issue is very likely to be related #313 rfc2696 paging not working when server returns additional, empty pagedResultsControl items, because after sending the broken cookie the AD still reponds with what would be the first page after the initial query but wont send the cookie required for continuing the paged search.

The comment below seems to imply that this issue was known and disabling paging was a solution, but the paging is not disabled and it breaks silently.

> lib/net/ldap/connection.rb:241
 # rfc2696_cookie sometimes contains binary data from Microsoft Active Directory
 # this breaks when calling to_ber. (Can't force binary data to UTF-8)
 # we have to disable paging (even though server supports it) to get around this...

I've only tested this against Microsoft Active Directory, and it enabled me to used the paged search.

@amuta amuta force-pushed the fix-paged-search branch from 1e8547a to 92353e9 Compare July 21, 2020 15:29
@amuta amuta marked this pull request as ready for review July 22, 2020 13:10
@HarlemSquirrel
Copy link
Member

@amuta could you please update this from the latest master branch when you have a moment?

…r will try to encode to UTF-8 first and break the cookie's representation of binary data
@amuta
Copy link
Author

amuta commented Dec 7, 2021

@HarlemSquirrel sorry for taking soooo long, but I rebased it and changed it with the code that we are working with. We are using this change at our org for more than 1 year and it is working, but if anyone can try it out with Active Directory and a search with >1.2k results it should be evident.

@amuta
Copy link
Author

amuta commented Dec 8, 2021

The tests are breaking because of lint rules, is it possible to make the linter skip that method? @HarlemSquirrel

@HarlemSquirrel
Copy link
Member

@amuta This should be refactored but for now we could up the limit to 117 in

# Offense count: 48
Metrics/AbcSize:
Max: 116

I don't have access to any Active Directory to test this myself and don't work with LDAP at all right now so we really should get someone to try this out on a real directory before merging in as well.

Are you able to write a test for this use case?

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.

2 participants