ldap search with paging

[jackielii]

this fixes #7702

[hashicorp-cla]

All committers have signed the CLA.

[jackielii]

already done the cla, somehow it still shows not signed

[tyrannosaurus-becks]

Hi @jackielii , thank you for providing this pull request!

To err on the side of caution, would it be possible to add an LDAP configuration parameter of ldap_search_page_size and default it to 0? Then the logic would be, if it's set to > 0, use pagination, otherwise don't?

The reason is, I'd like to maintain the exact current behavior for folks unless they elect to set the page size. Then we can fix the problem, while also being 100% sure we won't break anything for anyone.

[tyrannosaurus-becks]

Also, I'm not sure why the CLA signing isn't working, could there be a mismatch between the email that Github thinks was used for this pull request, and the email that Github thinks you're using to sign it? Or maybe it was a transient failure? Would you be willing to try again?

Sorry for the trouble with it! :-)

[jackielii]

@tyrannosaurus-becks no probs. Just amended the commit message for the author email.

when it comes to ldap_search_page_size, as the comment says:

// LDAPSearchPageSize is the page size to search LDAP directory.
// Default size is usually 1000, but this number doesn't affect the result.
// It only affect the number of round trips the client has to do.
var LDAPSearchPageSize uint32 = 500

The paging is done within the call conn.SearchWithPaging, it only affects how many round trips it has to use, it always returns the full set as a slice. I know, this feels like a wired API.

[ncabatoff]

Hi @jackielii,

I agree with @tyrannosaurus-becks, I think for backwards-compatibility reasons we should make the new behaviour opt-in with a config option. There are so many different LDAP/AD implementations out there that I'm afraid without the opt-in behaviour that we're likely to have a regression somewhere.

[jackielii]

Hi @jackielii,

I agree with @tyrannosaurus-becks, I think for backwards-compatibility reasons we should make the new behaviour opt-in with a config option. There are so many different LDAP/AD implementations out there that I'm afraid without the opt-in behaviour that we're likely to have a regression somewhere.

Sorry for the late reply, as I mentioned in the earlier comments: this doesn't change the behavior of the search, it only allows searching against a bigger directory.

I think the confusion comes from the SearchWithPaging vs Search: if you look at the API for SearchWithPaging. It states that:

SearchWithPaging accepts a search request and desired page size in order to execute LDAP queries to fulfill the search request. All paged LDAP query responses will be buffered and the final result will be returned atomically.

A requested pagingSize of 0 is interpreted as no limit by LDAP servers.

Search is basically SearchWithPaging with pagingSize of 0. This might be fine for some LDAP implementations, but on the MS Active Directory it triggers an error LDAP Result Code 4 "Size Limit Exceeded"

So in summary, SearchWithPaging is really just a safer Search

[useful-devops-tools]

@ncabatoff and @jackielii

Is there any update on this Pull request?
We're running into the same issue now:
LDAP Result Code 4 "Size Limit Exceeded

It looks like this PR would solve the issue with pagination.

[ncabatoff]

So in summary, SearchWithPaging is really just a safer Search

I am anything but an expert in LDAP, so take this with a grain of salt, but: I don't believe the above is true. At a high level, perhaps, but in practice from looking at rfc2696 and the openldap client code linked, SearchWithPaging uses an LDAP extension and does not issue the same request as Search does just with a 0 instead of a non-zero page size. The RFC is over 20y old now, probably everyone supports this extension at this point, but my experience with LDAP implementations is that they have subtle differences that can come back to bite you.

I maintain my belief that we should make this opt-in behaviour using a new config option. In any event, isn't it desirable to be able to configure the paging size?

[ltcarbonell]

Closing out this PR, work duplicated by the change here