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
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
5 changes: 4 additions & 1 deletion builtin/credential/ldap/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,9 @@ func TestLdapAuthBackend_ConfigUpgrade(t *testing.T) {
falseBool := new(bool)
*falseBool = false

maxPageInt := new(int)
*maxPageInt = 1000

exp := &ldapConfigEntry{
TokenParams: tokenutil.TokenParams{
TokenPeriod: 5 * time.Minute,
Expand All @@ -1243,7 +1246,7 @@ func TestLdapAuthBackend_ConfigUpgrade(t *testing.T) {
ConnectionTimeout: cfg.ConnectionTimeout,
UsernameAsAlias: false,
DerefAliases: "never",
MaximumPageSize: 1000,
MaximumPageSize: maxPageInt,
},
}

Expand Down
6 changes: 6 additions & 0 deletions builtin/credential/ldap/path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ func (b *backend) Config(ctx context.Context, req *logical.Request) (*ldapConfig
persistNeeded = true
}

if result.MaximumPageSize == nil {
result.MaximumPageSize = new(int)
*result.MaximumPageSize = -1
persistNeeded = true
}

if persistNeeded && (b.System().LocalMount() || !b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary|consts.ReplicationPerformanceStandby)) {
entry, err := logical.StorageEntryJSON("config", result)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions changelog/20453.txt
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
```
4 changes: 3 additions & 1 deletion helper/testhelpers/ldap/ldaphelper.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ func PrepareTestContainer(t *testing.T, version string) (cleanup func(), cfg *ld
cfg.GroupDN = "ou=people,dc=planetexpress,dc=com"
cfg.GroupAttr = "cn"
cfg.RequestTimeout = 60
cfg.MaximumPageSize = 1000

maxPageSize := 1000
cfg.MaximumPageSize = &maxPageSize

svc, err := runner.StartService(context.Background(), func(ctx context.Context, host string, port int) (docker.ServiceConfig, error) {
connURL := fmt.Sprintf("ldap://%s:%d", host, port)
Expand Down
9 changes: 7 additions & 2 deletions sdk/helper/ldaputil/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,11 @@ func (c *Client) performLdapFilterGroupsSearchPaging(cfg *ConfigEntry, conn Pagi
return make([]*ldap.Entry, 0), nil
}

if *cfg.MaximumPageSize < 0 {
c.Logger.Warn("maximumpagesize is negative, will not query server")
return make([]*ldap.Entry, 0), nil
}

ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
// If groupfilter was defined, resolve it as a Go template and use the query for
// returning the user's groups
if c.Logger.IsDebug() {
Expand Down Expand Up @@ -432,7 +437,7 @@ func (c *Client) performLdapFilterGroupsSearchPaging(cfg *ConfigEntry, conn Pagi
cfg.GroupAttr,
},
SizeLimit: math.MaxInt32,
}, uint32(cfg.MaximumPageSize))
}, uint32(*cfg.MaximumPageSize))
if err != nil {
return nil, fmt.Errorf("LDAP search failed: %w", err)
}
Expand Down Expand Up @@ -553,7 +558,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 {
ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
entries, err = c.performLdapFilterGroupsSearchPaging(cfg, paging, userDN, username)
} else {
entries, err = c.performLdapFilterGroupsSearch(cfg, conn, userDN, username)
Expand Down
8 changes: 4 additions & 4 deletions sdk/helper/ldaputil/config.go
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 @@ -256,7 +255,7 @@ 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.",
ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
Default: math.MaxInt32,
Default: -1,
},
}
}
Expand Down Expand Up @@ -433,7 +432,8 @@ func NewConfigEntry(existing *ConfigEntry, d *framework.FieldData) (*ConfigEntry
}

if _, ok := d.Raw["max_page_size"]; ok || !hadExisting {
cfg.MaximumPageSize = d.Get("max_page_size").(int)
cfg.MaximumPageSize = new(int)
*cfg.MaximumPageSize = d.Get("max_page_size").(int)
}

return cfg, nil
Expand Down Expand Up @@ -464,7 +464,7 @@ type ConfigEntry struct {
RequestTimeout int `json:"request_timeout"`
ConnectionTimeout int `json:"connection_timeout"`
DerefAliases string `json:"dereference_aliases"`
MaximumPageSize int `json:"max_page_size"`
MaximumPageSize *int `json:"max_page_size"`
ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved

// These json tags deviate from snake case because there was a past issue
// where the tag was being ignored, causing it to be jsonified as "CaseSensitiveNames", etc.
Expand Down
2 changes: 1 addition & 1 deletion sdk/helper/ldaputil/config_test.go
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": -1,
"CaseSensitiveNames": false,
"ClientTLSCert": "",
"ClientTLSKey": ""
Expand Down
6 changes: 3 additions & 3 deletions website/content/api-docs/auth/ldap.mdx
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,7 +97,7 @@ 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: -1)` - 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
ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
Expand Down