Add Configurable LDAP Max Page Size #19032
Conversation
There was a problem hiding this comment.
Thanks for taking this on, @ltcarbonell! Just had a question to start.
| @@ -400,7 +400,7 @@ func (c *Client) performLdapFilterGroupsSearchPaging(cfg *ConfigEntry, conn Pagi | |||
| cfg.GroupAttr, | |||
| }, | |||
| SizeLimit: math.MaxInt32, | |||
| }, math.MaxInt32) | |||
| }, uint32(cfg.MaximumPageSize)) | |||
There was a problem hiding this comment.
Wondering if you were able to test that setting this to 0 works with an LDAP server that does not permit paging? Just want to make sure that the reporter's issue is solved by setting this to 0.
Slightly related to that, do you know under which pre-condition we'd enter the branch at client.go#L525? I tried to find where a connection would satisfy PagingConnection or Connection, but it's not super clear. I wonder if we're always landing on a PagingConnection and can delete the other code path.
There was a problem hiding this comment.
Wondering if you were able to test that setting this to 0 works with an LDAP server that does not permit paging? Just want to make sure that the reporter's issue is solved by setting this to 0.
I am having some trouble setting up a dev LDAP instance that doesn't support paging to actually confirm this, but I will keep chugging along at it.
Slightly related to that, do you know under which pre-condition we'd enter the branch at client.go#L525? I tried to find where a connection would satisfy PagingConnection or Connection, but it's not super clear. I wonder if we're always landing on a PagingConnection and can delete the other code path.
My thinking with this was since this is a function that is part of the SDK, some other process outside of the core Vault code could be passing in a Connection into GetLdapGroups and wouldn't necessarily have updated to using a PagingConnection. But you are correct, in terms of the Vault codebase, we would never actually reach that path.
There was a problem hiding this comment.
@austingebauer was able to confirm (with the help of @nordicmachine here) that this resolves the issue they were experiencing when paging was disabled by the LDAP server.
Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
* Add config flag for LDAP max page size * Add changelog * move changelog to correct file * cleanup * Default to non-paged searching for with -1 * Update website/content/api-docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update website/content/docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update tests --------- Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
* Add config flag for LDAP max page size * Add changelog * move changelog to correct file * cleanup * Default to non-paged searching for with -1 * Update website/content/api-docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update website/content/docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update tests --------- Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
* Add config flag for LDAP max page size * Add changelog * move changelog to correct file * cleanup * Default to non-paged searching for with -1 * Update website/content/api-docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update website/content/docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update tests --------- Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
…20281) * Add Configurable LDAP Max Page Size (#19032) * Add config flag for LDAP max page size * Add changelog * move changelog to correct file * cleanup * Default to non-paged searching for with -1 * Update website/content/api-docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update website/content/docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update tests --------- Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * remove --------- Co-authored-by: Luis (LT) Carbonell <lt.carbonell@hashicorp.com> Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
…20283) * Add Configurable LDAP Max Page Size (#19032) * Add config flag for LDAP max page size * Add changelog * move changelog to correct file * cleanup * Default to non-paged searching for with -1 * Update website/content/api-docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update website/content/docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update tests --------- Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * remove --------- Co-authored-by: Luis (LT) Carbonell <lt.carbonell@hashicorp.com> Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
…20282) * Add Configurable LDAP Max Page Size (#19032) * Add config flag for LDAP max page size * Add changelog * move changelog to correct file * cleanup * Default to non-paged searching for with -1 * Update website/content/api-docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update website/content/docs/auth/ldap.mdx Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * Update tests --------- Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> * remove --------- Co-authored-by: Luis (LT) Carbonell <lt.carbonell@hashicorp.com> Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
Resolves #18875
#17640 introduced paging into LDAP authentication. This is resulting in an issue where directories that do not permit paging are erroring out (see #18875). This change allows for a configurable option to see this manually (or disable it completely by setting the value to 0)