From b9b33f720f66a3db1c3900fd8598e2b29cc87384 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 13 Apr 2023 10:31:47 -0400 Subject: [PATCH 1/5] sdk/ldaputil: add connection_timeout configurable --- sdk/helper/ldaputil/client.go | 6 ++++++ sdk/helper/ldaputil/config.go | 12 ++++++++++++ sdk/helper/ldaputil/config_test.go | 21 ++++++++++++--------- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/sdk/helper/ldaputil/client.go b/sdk/helper/ldaputil/client.go index b0e1187d56d8..54beac200977 100644 --- a/sdk/helper/ldaputil/client.go +++ b/sdk/helper/ldaputil/client.go @@ -31,6 +31,12 @@ func (c *Client) DialLDAP(cfg *ConfigEntry) (Connection, error) { var retErr *multierror.Error var conn Connection urls := strings.Split(cfg.Url, ",") + + // Default timeout in the pacakge is 60 seconds, which we default to on our + // end. This is useful if you want to take advantage of the URL list to increase + // availability of LDAP. + ldap.DefaultTimeout = time.Duration(cfg.ConnectionTimeout) * time.Second + for _, uut := range urls { u, err := url.Parse(uut) if err != nil { diff --git a/sdk/helper/ldaputil/config.go b/sdk/helper/ldaputil/config.go index df4996af7511..37330f7a59a7 100644 --- a/sdk/helper/ldaputil/config.go +++ b/sdk/helper/ldaputil/config.go @@ -239,6 +239,12 @@ Default: ({{.UserAttr}}={{.Username}})`, Default: "90s", }, + "connection_timeout": { + Type: framework.TypeDurationSecond, + Description: "Timeout, in seconds, when attempting to connect to the LDAP server before trying the next URL in the configuration.", + Default: "60s", + }, + "dereference_aliases": { Type: framework.TypeString, Description: "When aliases should be dereferenced on search operations. Accepted values are 'never', 'finding', 'searching', 'always'. Defaults to 'never'.", @@ -411,6 +417,10 @@ func NewConfigEntry(existing *ConfigEntry, d *framework.FieldData) (*ConfigEntry cfg.RequestTimeout = d.Get("request_timeout").(int) } + if _, ok := d.Raw["connection_timeout"]; ok || !hadExisting { + cfg.ConnectionTimeout = d.Get("connection_timeout").(int) + } + if _, ok := d.Raw["dereference_aliases"]; ok || !hadExisting { cfg.DerefAliases = d.Get("dereference_aliases").(string) } @@ -441,6 +451,7 @@ type ConfigEntry struct { UseTokenGroups bool `json:"use_token_groups"` UsePre111GroupCNBehavior *bool `json:"use_pre111_group_cn_behavior"` RequestTimeout int `json:"request_timeout"` + ConnectionTimeout int `json:"connection_timeout"` DerefAliases string `json:"dereference_aliases"` // These json tags deviate from snake case because there was a past issue @@ -479,6 +490,7 @@ func (c *ConfigEntry) PasswordlessMap() map[string]interface{} { "use_token_groups": c.UseTokenGroups, "anonymous_group_search": c.AnonymousGroupSearch, "request_timeout": c.RequestTimeout, + "connection_timeout": c.ConnectionTimeout, "username_as_alias": c.UsernameAsAlias, "dereference_aliases": c.DerefAliases, } diff --git a/sdk/helper/ldaputil/config_test.go b/sdk/helper/ldaputil/config_test.go index 62be2a182fd6..4ae86b9fca28 100644 --- a/sdk/helper/ldaputil/config_test.go +++ b/sdk/helper/ldaputil/config_test.go @@ -74,15 +74,16 @@ func testConfig(t *testing.T) *ConfigEntry { t.Helper() return &ConfigEntry{ - Url: "ldap://138.91.247.105", - UserDN: "example,com", - BindDN: "kitty", - BindPassword: "cats", - TLSMaxVersion: "tls12", - TLSMinVersion: "tls12", - RequestTimeout: 30, - ClientTLSCert: "", - ClientTLSKey: "", + Url: "ldap://138.91.247.105", + UserDN: "example,com", + BindDN: "kitty", + BindPassword: "cats", + TLSMaxVersion: "tls12", + TLSMinVersion: "tls12", + RequestTimeout: 30, + ConnectionTimeout: 30, + ClientTLSCert: "", + ClientTLSKey: "", } } @@ -141,6 +142,7 @@ var jsonConfig = []byte(`{ "tls_max_version": "tls12", "tls_min_version": "tls12", "request_timeout": 30, + "connection_timeout": 30, "ClientTLSCert": "", "ClientTLSKey": "" }`) @@ -171,6 +173,7 @@ var jsonConfigDefault = []byte(` "use_pre111_group_cn_behavior": null, "username_as_alias": false, "request_timeout": 90, + "connection_timeout": 60, "dereference_aliases": "never", "CaseSensitiveNames": false, "ClientTLSCert": "", From 1c647650cdef065ed44c8e28b2b1039ab0c03f78 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 13 Apr 2023 10:37:36 -0400 Subject: [PATCH 2/5] changelog --- changelog/20144.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog/20144.txt diff --git a/changelog/20144.txt b/changelog/20144.txt new file mode 100644 index 000000000000..ef8b9a01810c --- /dev/null +++ b/changelog/20144.txt @@ -0,0 +1,4 @@ +```release-note:improvement +sdk/ldaputil: added `connection_timeout` to tune connection timeout duration +for all LDAP plugins. +``` From 96e1e7b2785f6a407865a61a233078de60a1451a Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 13 Apr 2023 10:45:40 -0400 Subject: [PATCH 3/5] Update doc --- website/content/api-docs/auth/ldap.mdx | 3 +++ website/content/api-docs/secret/ad.mdx | 1 + website/content/api-docs/secret/ldap.mdx | 3 +++ 3 files changed, 7 insertions(+) diff --git a/website/content/api-docs/auth/ldap.mdx b/website/content/api-docs/auth/ldap.mdx index 9b00079001b6..19acf671a782 100644 --- a/website/content/api-docs/auth/ldap.mdx +++ b/website/content/api-docs/auth/ldap.mdx @@ -35,6 +35,9 @@ 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: 60 or string: "60s")` - 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 an error. diff --git a/website/content/api-docs/secret/ad.mdx b/website/content/api-docs/secret/ad.mdx index 61c7b99bf282..823101446c45 100644 --- a/website/content/api-docs/secret/ad.mdx +++ b/website/content/api-docs/secret/ad.mdx @@ -47,6 +47,7 @@ text that fulfills those requirements. `{{PASSWORD}}` must appear exactly once a ### Connection parameters - `url` (string, optional) - The LDAP server to connect to. Examples: `ldaps://ldap.myorg.com`, `ldaps://ldap.myorg.com:636`. This can also be a comma-delineated list of URLs, e.g. `ldaps://ldap.myorg.com,ldaps://ldap.myorg.com:636`, in which case the servers will be tried in-order if there are errors during the connection process. Default is `ldap://127.0.0.1`. +- `connection_timeout` `(integer: 60 or string: "60s")` - 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 an error. - `starttls` (bool, optional) - If true, issues a `StartTLS` command after establishing an unencrypted connection. - `insecure_tls` - (bool, optional) - If true, skips LDAP server SSL certificate verification - insecure, use with caution! diff --git a/website/content/api-docs/secret/ldap.mdx b/website/content/api-docs/secret/ldap.mdx index 1bc96a1feacb..99b4c32b4a6f 100644 --- a/website/content/api-docs/secret/ldap.mdx +++ b/website/content/api-docs/secret/ldap.mdx @@ -53,6 +53,9 @@ to search and change entry passwords in LDAP. string for authentication. The constructed UPN will appear as `[binddn]@[upndomain]`. For example, if `upndomain=example.com` and `binddn=admin`, the UPN string `admin@example.com` will be used to log in to Active Directory. +- `connection_timeout` `(integer: 60 or string: "60s")` - Timeout, in seconds, + when attempting to connect to the LDAP server before trying the next URL in + the configuration. - `request_timeout` `(integer: 90, string: "90s" )` - Timeout, in seconds, for the connection when making requests against the server before returning back an error. - `starttls` `(bool: )` - If true, issues a `StartTLS` command after establishing an unencrypted connection. From f18f345091835f715b0658924f9814e9a1764155 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 13 Apr 2023 11:22:32 -0400 Subject: [PATCH 4/5] Fix test --- builtin/credential/ldap/backend_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/builtin/credential/ldap/backend_test.go b/builtin/credential/ldap/backend_test.go index cd4775582704..7f81e0bfa764 100644 --- a/builtin/credential/ldap/backend_test.go +++ b/builtin/credential/ldap/backend_test.go @@ -832,6 +832,7 @@ func testAccStepConfigUrl(t *testing.T, cfg *ldaputil.ConfigEntry) logicaltest.T "case_sensitive_names": true, "token_policies": "abc,xyz", "request_timeout": cfg.RequestTimeout, + "connection_timeout": cfg.ConnectionTimeout, "username_as_alias": cfg.UsernameAsAlias, }, } @@ -854,6 +855,7 @@ func testAccStepConfigUrlWithAuthBind(t *testing.T, cfg *ldaputil.ConfigEntry) l "case_sensitive_names": true, "token_policies": "abc,xyz", "request_timeout": cfg.RequestTimeout, + "connection_timeout": cfg.ConnectionTimeout, }, } } @@ -874,6 +876,7 @@ func testAccStepConfigUrlWithDiscover(t *testing.T, cfg *ldaputil.ConfigEntry) l "case_sensitive_names": true, "token_policies": "abc,xyz", "request_timeout": cfg.RequestTimeout, + "connection_timeout": cfg.ConnectionTimeout, }, } } @@ -891,6 +894,7 @@ func testAccStepConfigUrlNoGroupDN(t *testing.T, cfg *ldaputil.ConfigEntry) logi "discoverdn": true, "case_sensitive_names": true, "request_timeout": cfg.RequestTimeout, + "connection_timeout": cfg.ConnectionTimeout, }, } } @@ -911,6 +915,7 @@ func testAccStepConfigUrlWarningCheck(t *testing.T, cfg *ldaputil.ConfigEntry, o "case_sensitive_names": true, "token_policies": "abc,xyz", "request_timeout": cfg.RequestTimeout, + "connection_timeout": cfg.ConnectionTimeout, }, Check: func(response *logical.Response) error { if len(response.Warnings) == 0 { @@ -1192,6 +1197,7 @@ func TestLdapAuthBackend_ConfigUpgrade(t *testing.T) { "token_period": "5m", "token_explicit_max_ttl": "24h", "request_timeout": cfg.RequestTimeout, + "connection_timeout": cfg.ConnectionTimeout, }, Storage: storage, Connection: &logical.Connection{}, @@ -1233,6 +1239,7 @@ func TestLdapAuthBackend_ConfigUpgrade(t *testing.T) { CaseSensitiveNames: falseBool, UsePre111GroupCNBehavior: new(bool), RequestTimeout: cfg.RequestTimeout, + ConnectionTimeout: cfg.ConnectionTimeout, UsernameAsAlias: false, DerefAliases: "never", }, From 0e405c39fb4ad650203c0c7b3b91cf01982b8cd4 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 13 Apr 2023 11:53:36 -0400 Subject: [PATCH 5/5] Change default to 30s --- sdk/helper/ldaputil/config.go | 2 +- sdk/helper/ldaputil/config_test.go | 6 +++--- website/content/api-docs/auth/ldap.mdx | 2 +- website/content/api-docs/secret/ad.mdx | 2 +- website/content/api-docs/secret/ldap.mdx | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/sdk/helper/ldaputil/config.go b/sdk/helper/ldaputil/config.go index 37330f7a59a7..12919ad504dc 100644 --- a/sdk/helper/ldaputil/config.go +++ b/sdk/helper/ldaputil/config.go @@ -242,7 +242,7 @@ Default: ({{.UserAttr}}={{.Username}})`, "connection_timeout": { Type: framework.TypeDurationSecond, Description: "Timeout, in seconds, when attempting to connect to the LDAP server before trying the next URL in the configuration.", - Default: "60s", + Default: "30s", }, "dereference_aliases": { diff --git a/sdk/helper/ldaputil/config_test.go b/sdk/helper/ldaputil/config_test.go index 4ae86b9fca28..e6f9997f0f7a 100644 --- a/sdk/helper/ldaputil/config_test.go +++ b/sdk/helper/ldaputil/config_test.go @@ -81,7 +81,7 @@ func testConfig(t *testing.T) *ConfigEntry { TLSMaxVersion: "tls12", TLSMinVersion: "tls12", RequestTimeout: 30, - ConnectionTimeout: 30, + ConnectionTimeout: 15, ClientTLSCert: "", ClientTLSKey: "", } @@ -142,7 +142,7 @@ var jsonConfig = []byte(`{ "tls_max_version": "tls12", "tls_min_version": "tls12", "request_timeout": 30, - "connection_timeout": 30, + "connection_timeout": 15, "ClientTLSCert": "", "ClientTLSKey": "" }`) @@ -173,7 +173,7 @@ var jsonConfigDefault = []byte(` "use_pre111_group_cn_behavior": null, "username_as_alias": false, "request_timeout": 90, - "connection_timeout": 60, + "connection_timeout": 30, "dereference_aliases": "never", "CaseSensitiveNames": false, "ClientTLSCert": "", diff --git a/website/content/api-docs/auth/ldap.mdx b/website/content/api-docs/auth/ldap.mdx index 19acf671a782..2084937435ef 100644 --- a/website/content/api-docs/auth/ldap.mdx +++ b/website/content/api-docs/auth/ldap.mdx @@ -35,7 +35,7 @@ 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: 60 or string: "60s")` - Timeout, in seconds, +- `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 diff --git a/website/content/api-docs/secret/ad.mdx b/website/content/api-docs/secret/ad.mdx index 823101446c45..7c76bdf5833a 100644 --- a/website/content/api-docs/secret/ad.mdx +++ b/website/content/api-docs/secret/ad.mdx @@ -47,7 +47,7 @@ text that fulfills those requirements. `{{PASSWORD}}` must appear exactly once a ### Connection parameters - `url` (string, optional) - The LDAP server to connect to. Examples: `ldaps://ldap.myorg.com`, `ldaps://ldap.myorg.com:636`. This can also be a comma-delineated list of URLs, e.g. `ldaps://ldap.myorg.com,ldaps://ldap.myorg.com:636`, in which case the servers will be tried in-order if there are errors during the connection process. Default is `ldap://127.0.0.1`. -- `connection_timeout` `(integer: 60 or string: "60s")` - Timeout, in seconds, when attempting to connect to the LDAP server before trying the next URL in the configuration. +- `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 an error. - `starttls` (bool, optional) - If true, issues a `StartTLS` command after establishing an unencrypted connection. - `insecure_tls` - (bool, optional) - If true, skips LDAP server SSL certificate verification - insecure, use with caution! diff --git a/website/content/api-docs/secret/ldap.mdx b/website/content/api-docs/secret/ldap.mdx index 99b4c32b4a6f..a5ee22ccfbab 100644 --- a/website/content/api-docs/secret/ldap.mdx +++ b/website/content/api-docs/secret/ldap.mdx @@ -53,7 +53,7 @@ to search and change entry passwords in LDAP. string for authentication. The constructed UPN will appear as `[binddn]@[upndomain]`. For example, if `upndomain=example.com` and `binddn=admin`, the UPN string `admin@example.com` will be used to log in to Active Directory. -- `connection_timeout` `(integer: 60 or string: "60s")` - Timeout, in seconds, +- `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, string: "90s" )` - Timeout, in seconds, for the connection when