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

Correct Default for MaximumPageSize #20453

Merged
merged 17 commits into from
May 17, 2023
Merged

Correct Default for MaximumPageSize #20453

merged 17 commits into from
May 17, 2023

Conversation

ltcarbonell
Copy link
Contributor

This pull request addresses a problem that was introduced in a previous pull request (#19032). The earlier PR added a new configuration option called max_page_size to the LDAP secret engine. This option allows users to adjust the size of the paged search results. By default, it is set to 2147483647 (math.MaxInt32), but it can also be set to -1 to use non-paged searching.

The issue occurs when upgrading Vault to 1.13.2, 1.12.6, or 1.11.10, that has an existing LDAP Auth configuration. This would result in the max_page_size being set to 0 instead of the intended default of 2147483647.

Resolves #20416

@hghaf099 hghaf099 requested a review from austingebauer May 2, 2023 13:22
@ltcarbonell ltcarbonell force-pushed the ldap-max-page-default branch from dc17a2b to 5edd22c Compare May 2, 2023 15:20
@maxb
Copy link
Contributor

maxb commented May 10, 2023

After the comments I made earlier today, I spent some more time investigating. I uncovered some interesting information.

First, it's kind of weird this option is named max_page_size. IMO it would have been more logical to just call it page_size. It is not actually treated as a maximum, it is simply the page size we ask the LDAP server to provide us with.

Next, sending a page size of zero is always wrong, as this is a special value that instructs the server to terminate an in-progress paging result set, and if sent with an initial query, will prevent the server returning any results (hence the regression that was reported).

Since a page size of zero is invalid, I propose that this PR should not change the Go type used for MaximumPageSize from int to *int, as the simple zero value is available to use as an unspecified value.

Additionally, requesting a page size of 2147483647 never makes any sense, because that's what we send as our size limit - and RFC 2696 that defines the paging control explicitly says a page size equal or greater to the size limit is to be ignored by servers.

Taking all this into account, I agree with the proposal to change the default behaviour to "no paging", because the only useful use of paging, is one set to cooperate with the server defined limits. In a default Active Directory installation, the maximum page size the server will permit is 1000, so Active Directory users will probably want to set max_page_size=1000.

sdk/helper/ldaputil/client.go Outdated Show resolved Hide resolved
sdk/helper/ldaputil/client.go Outdated Show resolved Hide resolved
sdk/helper/ldaputil/config.go Outdated Show resolved Hide resolved
sdk/helper/ldaputil/config.go Outdated Show resolved Hide resolved
website/content/api-docs/auth/ldap.mdx Outdated Show resolved Hide resolved
@CodeGlitcher
Copy link

We run into the same issue.
any value besides 0 works.

Any change this could be added to the release notes as a known issue.
Would have saved me some searching.

@ltcarbonell ltcarbonell requested a review from jasonodonnell May 15, 2023 13:24
@Cajga
Copy link

Cajga commented May 17, 2023

As I wrote it in the related issue opened by others, this bug closed us out from our vault cluster.
We have all policies bind to LDAP groups and we revoked the root token.

It would be nice if you could mention this regression in the CHANGELOG. So, people who use LDAP and read release notes before upgrade (like I did) will not run into this.

@@ -553,7 +553,7 @@ func (c *Client) GetLdapGroups(cfg *ConfigEntry, conn Connection, userDN string,
if cfg.UseTokenGroups {
entries, err = c.performLdapTokenGroupsSearch(cfg, conn, userDN)
} else {
if paging, ok := conn.(PagingConnection); ok && cfg.MaximumPageSize >= 0 {
if paging, ok := conn.(PagingConnection); ok && cfg.MaximumPageSize > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if there's already a test case that covers this, but wondering if we should make sure that setting max_page_size to zero doesn't actually result in an group search that returns no group (which was the main issue with the regression).

Copy link
Contributor

Choose a reason for hiding this comment

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

@calvn When 0 is set it swaps out to the non-paged search which ldaputil has always used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the removal of = makes it so that the zero value doesn't result in a paged search. I was mostly asking whether we should have a test case to cover this to prevent future regression.

@ltcarbonell
Copy link
Contributor Author

ltcarbonell commented May 17, 2023

As I wrote it in the related issue opened by others, this bug closed us out from our vault cluster. We have all policies bind to LDAP groups and we revoked the root token.

It would be nice if you could mention this regression in the CHANGELOG. So, people who use LDAP and read release notes before upgrade (like I did) will not run into this.

Nice suggestion @Cajga. This will be added to this doc here: https://developer.hashicorp.com/vault/docs/upgrading/upgrade-to-1.13.x#known-issues

changelog/20453.txt Outdated Show resolved Hide resolved
Co-authored-by: Austin Gebauer <[email protected]>
@ltcarbonell ltcarbonell enabled auto-merge (squash) May 17, 2023 20:21
@ltcarbonell ltcarbonell merged commit 21b3262 into main May 17, 2023
@ltcarbonell ltcarbonell deleted the ldap-max-page-default branch May 17, 2023 20:56
ltcarbonell added a commit that referenced this pull request May 17, 2023
* default max page size for config

* Add changelog

* update test int to *int

* add testing defaults

* update default to -1, i.e. dont paginate

* update test

* Add error message for invalid search

* Make 0 the default

* cleanup

* Add to known issues doc

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

* Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Add workaround to docs

* Update changelog/20453.txt

Co-authored-by: Austin Gebauer <[email protected]>

---------

Co-authored-by: Jason O'Donnell <[email protected]>
Co-authored-by: Austin Gebauer <[email protected]>
ltcarbonell added a commit that referenced this pull request May 17, 2023
* default max page size for config

* Add changelog

* update test int to *int

* add testing defaults

* update default to -1, i.e. dont paginate

* update test

* Add error message for invalid search

* Make 0 the default

* cleanup

* Add to known issues doc

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

* Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Add workaround to docs

* Update changelog/20453.txt

Co-authored-by: Austin Gebauer <[email protected]>

---------

Co-authored-by: Jason O'Donnell <[email protected]>
Co-authored-by: Austin Gebauer <[email protected]>
ltcarbonell added a commit that referenced this pull request May 17, 2023
* default max page size for config

* Add changelog

* update test int to *int

* add testing defaults

* update default to -1, i.e. dont paginate

* update test

* Add error message for invalid search

* Make 0 the default

* cleanup

* Add to known issues doc

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

* Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx

Co-authored-by: Jason O'Donnell <[email protected]>

* Add workaround to docs

* Update changelog/20453.txt

Co-authored-by: Austin Gebauer <[email protected]>

---------

Co-authored-by: Jason O'Donnell <[email protected]>
Co-authored-by: Austin Gebauer <[email protected]>
ltcarbonell added a commit that referenced this pull request May 17, 2023
* default max page size for config

* Add changelog

* update test int to *int

* add testing defaults

* update default to -1, i.e. dont paginate

* update test

* Add error message for invalid search

* Make 0 the default

* cleanup

* Add to known issues doc

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

* Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx



* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx



* Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx



* Add workaround to docs

* Update changelog/20453.txt



---------

Co-authored-by: Luis (LT) Carbonell <[email protected]>
Co-authored-by: Jason O'Donnell <[email protected]>
Co-authored-by: Austin Gebauer <[email protected]>
ltcarbonell added a commit that referenced this pull request May 17, 2023
* default max page size for config

* Add changelog

* update test int to *int

* add testing defaults

* update default to -1, i.e. dont paginate

* update test

* Add error message for invalid search

* Make 0 the default

* cleanup

* Add to known issues doc

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

* Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx



* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx



* Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx



* Add workaround to docs

* Update changelog/20453.txt



---------

Co-authored-by: Luis (LT) Carbonell <[email protected]>
Co-authored-by: Jason O'Donnell <[email protected]>
Co-authored-by: Austin Gebauer <[email protected]>
ltcarbonell added a commit that referenced this pull request May 17, 2023
* default max page size for config

* Add changelog

* update test int to *int

* add testing defaults

* update default to -1, i.e. dont paginate

* update test

* Add error message for invalid search

* Make 0 the default

* cleanup

* Add to known issues doc

* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx

* Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx



* Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx



* Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx



* Add workaround to docs

* Update changelog/20453.txt



---------

Co-authored-by: Luis (LT) Carbonell <[email protected]>
Co-authored-by: Jason O'Donnell <[email protected]>
Co-authored-by: Austin Gebauer <[email protected]>
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.

Regression 1.13.2: "no LDAP groups found in groupDN"
7 participants