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.

Sign up for GitHub

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

Jump to bottom

Correct Default for MaximumPageSize #20453

Merged
merged 17 commits into from
May 17, 2023
Merged

Correct Default for MaximumPageSize #20453

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3de4b64
default max page size for config
ltcarbonell May 1, 2023
26532e7
Add changelog
ltcarbonell May 1, 2023
c95acc8
update test int to *int
ltcarbonell May 2, 2023
5edd22c
add testing defaults
ltcarbonell May 2, 2023
54e6c0d
update default to -1, i.e. dont paginate
ltcarbonell May 4, 2023
4b292a2
update test
ltcarbonell May 5, 2023
8749a5b
Add error message for invalid search
ltcarbonell May 10, 2023
77750b8
Make 0 the default
ltcarbonell May 16, 2023
066f35e
cleanup
ltcarbonell May 16, 2023
a4ce574
Add to known issues doc
ltcarbonell May 17, 2023
bbeee7f
Merge branch 'main' into ldap-max-page-default
ltcarbonell May 17, 2023
bb13939
Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx
ltcarbonell May 17, 2023
b4d085f
Update website/content/docs/upgrading/upgrade-to-1.11.x.mdx
ltcarbonell May 17, 2023
cd566a2
Update website/content/docs/upgrading/upgrade-to-1.13.x.mdx
ltcarbonell May 17, 2023
5032278
Update website/content/docs/upgrading/upgrade-to-1.12.x.mdx
ltcarbonell May 17, 2023
0920b12
Add workaround to docs
ltcarbonell May 17, 2023
90e020a
Update changelog/20453.txt
ltcarbonell May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/20453.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ldap: Set default value for `max_page_size` properly
ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
```
2 changes: 1 addition & 1 deletion sdk/helper/ldaputil/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

entries, err = c.performLdapFilterGroupsSearchPaging(cfg, paging, userDN, username)
} else {
entries, err = c.performLdapFilterGroupsSearch(cfg, conn, userDN, username)
Expand Down
5 changes: 2 additions & 3 deletions sdk/helper/ldaputil/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"encoding/pem"
"errors"
"fmt"
"math"
"strings"
"text/template"

Expand Down Expand Up @@ -255,8 +254,8 @@ Default: ({{.UserAttr}}={{.Username}})`,

"max_page_size": {
Type: framework.TypeInt,
Description: "The maximum number of results to return for a single paged query. If not set, the server default will be used for paged searches. A requested max_page_size of 0 is interpreted as no limit by LDAP servers. If set to a negative value, search requests will not be paged.",
Default: math.MaxInt32,
Description: "If set to a value greater than 0, the LDAP backend will use the LDAP server's paged search control to request pages of up to the given size. This can be used to avoid hitting the LDAP server's maximum result size limit. Otherwise, the LDAP backend will not use the paged search control.",
Default: 0,
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/helper/ldaputil/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ var jsonConfigDefault = []byte(`
"request_timeout": 90,
"connection_timeout": 30,
"dereference_aliases": "never",
"max_page_size": 2147483647,
"max_page_size": 0,
"CaseSensitiveNames": false,
"ClientTLSCert": "",
"ClientTLSKey": ""
Expand Down
9 changes: 4 additions & 5 deletions website/content/api-docs/auth/ldap.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ This endpoint configures the LDAP auth method.
names will be normalized to lower case. Case will still be preserved when
sending the username to the LDAP server at login time; this is only for
matching local user/group definitions.
- `connection_timeout` `(integer: 30 or string: "30s")` - Timeout, in seconds,
when attempting to connect to the LDAP server before trying the next URL in
- `connection_timeout` `(integer: 30 or string: "30s")` - Timeout, in seconds,
when attempting to connect to the LDAP server before trying the next URL in
the configuration.
- `request_timeout` `(integer: 90 or string: "90s")` - Timeout, in seconds, for
the connection when making requests against the server before returning back
Expand Down Expand Up @@ -97,11 +97,10 @@ This endpoint configures the LDAP auth method.
- `dereference_aliases` `(string: never)` - When aliases should be dereferenced
on search operations. Accepted values are 'never', 'finding', 'searching',
'always'. Defaults to 'never'.
- `max_page_size` `(int: math.MaxInt32)` - If set to a value greater than 0, the LDAP
- `max_page_size` `(int: 0)` - If set to a value greater than 0, the LDAP
backend will use the LDAP server's paged search control to request pages of
up to the given size. This can be used to avoid hitting the LDAP server's
maximum result size limit. A value of 0 will be interpreted by the LDAP
server as unlimited. If set to -1, the LDAP backend will not use the
maximum result size limit. Otherwise, the LDAP backend will not use the
paged search control.

@include 'tokenfields.mdx'
Expand Down
2 changes: 1 addition & 1 deletion website/content/docs/auth/ldap.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ Use `vault path-help` for more details.
### Other

- `username_as_alias` (bool, optional) - If set to true, forces the auth method to use the username passed by the user as the alias name.
- `max_page_size` (int, optional) - The maximum number of results to return for a single LDAP query. This is useful for preventing large queries from being run against the LDAP server. The default is the maximum value for an int32.
- `max_page_size` (int, optional) - If set to a value greater than 0, the LDAP backend will use the LDAP server's paged search control to request pages of up to the given size. This can be used to avoid hitting the LDAP server's maximum result size limit. Otherwise, the LDAP backend will not use the paged search control.

## Examples:

Expand Down
17 changes: 16 additions & 1 deletion website/content/docs/upgrading/upgrade-to-1.11.x.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,19 @@ API path by setting the [bool config option](/vault/api-docs/secret/databases/el

@include 'raft-retry-join-failure.mdx'

@include 'tokenization-rotation-persistence.mdx'
@include 'tokenization-rotation-persistence.mdx'

### LDAP Pagination Issue

There was a regression introduced in 1.11.10 relating to LDAP maximum page sizes, resulting in
an error `no LDAP groups found in groupDN [...] only policies from locally-defined groups available`. The issue
occurs when upgrading Vault with an instance that has an existing LDAP Auth configuration.

As a workaround, disable paged searching using the following:
```shell-session
vault write auth/ldap/config max_page_size=-1
```

#### Impacted Versions

Affects Vault 1.11.10.
17 changes: 16 additions & 1 deletion website/content/docs/upgrading/upgrade-to-1.12.x.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ It will produce errors in Vault Server's logs such as:
error=
| 1 error occurred:
| * panic generating audit log
|
|
```

As a workaround, [listing plugins by type](/vault/api-docs/system/plugins-catalog#list-plugins-1)
Expand Down Expand Up @@ -184,3 +184,18 @@ Affects version 1.12.3. A fix will be released in 1.12.4.
@include 'tokenization-rotation-persistence.mdx'

@include 'ocsp-redirect.mdx'

### LDAP Pagination Issue

There was a regression introduced in 1.12.6 relating to LDAP maximum page sizes, resulting in
an error `no LDAP groups found in groupDN [...] only policies from locally-defined groups available`. The issue
occurs when upgrading Vault with an instance that has an existing LDAP Auth configuration.

As a workaround, disable paged searching using the following:
```shell-session
vault write auth/ldap/config max_page_size=-1
```

#### Impacted Versions

Affects Vault 1.12.6.
14 changes: 14 additions & 0 deletions website/content/docs/upgrading/upgrade-to-1.13.x.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,20 @@ accommodates the default minimum duration of an STS token and overrides the defa

Affects Vault 1.13.0 only.

### LDAP Pagination Issue

There was a regression introduced in 1.13.2 relating to LDAP maximum page sizes, resulting in
an error `no LDAP groups found in groupDN [...] only policies from locally-defined groups available`. The issue
occurs when upgrading Vault with an instance that has an existing LDAP Auth configuration.

As a workaround, disable paged searching using the following:
```shell-session
vault write auth/ldap/config max_page_size=-1
```

#### Impacted Versions

Affects Vault 1.13.2.

### PKI Cross-Cluster Revocation Requests and Unified CRL/OCSP

Expand Down