From 1bd9e76e13379539bc396899729453f6f2c9aa1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 00:07:23 +0200 Subject: [PATCH 1/7] Fix handling of username_as_alias during LDAP authentication There is a bug that was introduced in the LDAP authentication method by https://github.com/hashicorp/vault/pull/11000. It was thought to be backward compatible but has broken a number of users. Later a new parameter `username_as_alias` was introduced in https://github.com/hashicorp/vault/pull/14324 to make it possible for operators to restore the previous behavior. The way it is currently working is not completely backward compatible thought because when username_as_alias is set, a call to GetUserAliasAttributeValue() will first be made, then this value is completely discarded in pathLogin() and replaced by the username as expected. This is an issue because it makes useless calls to the LDAP server and will break backward compatibility if one of the constraints in GetUserAliasAttributeValue() is not respected, even though the resulting value will be discarded anyway. In order to maintain backward compatibility here we have to only call GetUserAliasAttributeValue() if necessary. Since this change of behavior was introduced in 1.9, this fix will need to be backported to the 1.9, 1.10 and 1.11 branches. --- builtin/credential/ldap/backend.go | 17 ++++++++++------- builtin/credential/ldap/path_login.go | 8 ++------ 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/builtin/credential/ldap/backend.go b/builtin/credential/ldap/backend.go index 37ba20ae6c09e..94562723fa9b5 100644 --- a/builtin/credential/ldap/backend.go +++ b/builtin/credential/ldap/backend.go @@ -56,7 +56,7 @@ type backend struct { *framework.Backend } -func (b *backend) Login(ctx context.Context, req *logical.Request, username string, password string) (string, []string, *logical.Response, []string, error) { +func (b *backend) Login(ctx context.Context, req *logical.Request, username string, password string, usernameAsAlias bool) (string, []string, *logical.Response, []string, error) { cfg, err := b.Config(ctx, req) if err != nil { return "", nil, nil, nil, err @@ -195,12 +195,15 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri // Policies from each group may overlap policies = strutil.RemoveDuplicates(policies, true) - entityAliasAttribute, err := ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username) - if err != nil { - return "", nil, logical.ErrorResponse(err.Error()), nil, nil - } - if entityAliasAttribute == "" { - return "", nil, logical.ErrorResponse("missing entity alias attribute value"), nil, nil + entityAliasAttribute := username + if !usernameAsAlias { + entityAliasAttribute, err = ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username) + if err != nil { + return "", nil, logical.ErrorResponse(err.Error()), nil, nil + } + if entityAliasAttribute == "" { + return "", nil, logical.ErrorResponse("missing entity alias attribute value"), nil, nil + } } return entityAliasAttribute, policies, ldapResponse, allGroups, nil diff --git a/builtin/credential/ldap/path_login.go b/builtin/credential/ldap/path_login.go index 49c0cfe9d8fb1..67303911e5a17 100644 --- a/builtin/credential/ldap/path_login.go +++ b/builtin/credential/ldap/path_login.go @@ -73,7 +73,7 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framew username := d.Get("username").(string) password := d.Get("password").(string) - effectiveUsername, policies, resp, groupNames, err := b.Login(ctx, req, username, password) + effectiveUsername, policies, resp, groupNames, err := b.Login(ctx, req, username, password, cfg.UsernameAsAlias) // Handle an internal error if err != nil { return nil, err @@ -103,10 +103,6 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framew }, } - if cfg.UsernameAsAlias { - auth.Alias.Name = username - } - cfg.PopulateTokenAuth(auth) // Add in configured policies from mappings @@ -139,7 +135,7 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f username := req.Auth.Metadata["username"] password := req.Auth.InternalData["password"].(string) - _, loginPolicies, resp, groupNames, err := b.Login(ctx, req, username, password) + _, loginPolicies, resp, groupNames, err := b.Login(ctx, req, username, password, cfg.UsernameAsAlias) if err != nil || (resp != nil && resp.IsError()) { return resp, err } From 92be3035b7c750ea3d16447297f60fc413794ac8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 00:29:59 +0200 Subject: [PATCH 2/7] Add changelog --- changelog/15525.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/15525.txt diff --git a/changelog/15525.txt b/changelog/15525.txt new file mode 100644 index 0000000000000..a4b23820efb55 --- /dev/null +++ b/changelog/15525.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/ldap: A bug during login has been fixed when `username_as_alias` is set. +``` From ac7c85c3de59082a6d3598211629b62010d604f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 00:45:25 +0200 Subject: [PATCH 3/7] Add tests --- builtin/credential/ldap/backend_test.go | 26 +++++++++++++++++++++++++ sdk/helper/ldaputil/client.go | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/builtin/credential/ldap/backend_test.go b/builtin/credential/ldap/backend_test.go index c58fa2f2d10a7..77ab29648d723 100644 --- a/builtin/credential/ldap/backend_test.go +++ b/builtin/credential/ldap/backend_test.go @@ -618,6 +618,31 @@ func TestBackend_basic_authbind_userfilter(t *testing.T) { testAccStepLoginFailure(t, "hermes conrad", "hermes"), }, }) + + // If UserAttr returns multiple attributes that can be used as alias then + // we return an error... + cfg.UserAttr = "employeeType" + cfg.UserFilter = "(cn={{.Username}})" + cfg.UsernameAsAlias = false + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: b, + Steps: []logicaltest.TestStep{ + testAccStepConfigUrl(t, cfg), + testAccStepLoginFailure(t, "hermes conrad", "hermes"), + }, + }) + + // ...unless username_as_alias has been set in which case we don't care + // about the alias returned by the LDAP server and always use the username + cfg.UsernameAsAlias = true + logicaltest.Test(t, logicaltest.TestCase{ + CredentialBackend: b, + Steps: []logicaltest.TestStep{ + testAccStepConfigUrl(t, cfg), + testAccStepLoginNoAttachedPolicies(t, "hermes conrad", "hermes"), + }, + }) + } func TestBackend_basic_authbind_metadata_name(t *testing.T) { @@ -805,6 +830,7 @@ func testAccStepConfigUrl(t *testing.T, cfg *ldaputil.ConfigEntry) logicaltest.T "case_sensitive_names": true, "token_policies": "abc,xyz", "request_timeout": cfg.RequestTimeout, + "username_as_alias": cfg.UsernameAsAlias, }, } } diff --git a/sdk/helper/ldaputil/client.go b/sdk/helper/ldaputil/client.go index 329e69ecc8e54..f3946c8269e2c 100644 --- a/sdk/helper/ldaputil/client.go +++ b/sdk/helper/ldaputil/client.go @@ -244,7 +244,7 @@ func (c *Client) GetUserAliasAttributeValue(cfg *ConfigEntry, conn Connection, u } if len(result.Entries[0].Attributes) != 1 { - return aliasAttributeValue, errwrap.Wrapf("LDAP attribute missing for entity alias mapping{{err}}", err) + return aliasAttributeValue, fmt.Errorf("LDAP attribute missing for entity alias mapping") } if len(result.Entries[0].Attributes[0].Values) != 1 { From ae7e2e91ae19660e879f592c07ff42c1be50c499 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 01:02:54 +0200 Subject: [PATCH 4/7] Format code --- builtin/credential/ldap/backend_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/credential/ldap/backend_test.go b/builtin/credential/ldap/backend_test.go index 77ab29648d723..7a9f0f0eed5e3 100644 --- a/builtin/credential/ldap/backend_test.go +++ b/builtin/credential/ldap/backend_test.go @@ -642,7 +642,6 @@ func TestBackend_basic_authbind_userfilter(t *testing.T) { testAccStepLoginNoAttachedPolicies(t, "hermes conrad", "hermes"), }, }) - } func TestBackend_basic_authbind_metadata_name(t *testing.T) { From 04d8cb9acdae88c484f303aad9751177f93a0c97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 08:40:41 +0200 Subject: [PATCH 5/7] Update builtin/credential/ldap/backend.go Co-authored-by: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> --- builtin/credential/ldap/backend.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/builtin/credential/ldap/backend.go b/builtin/credential/ldap/backend.go index 94562723fa9b5..bf15477a99504 100644 --- a/builtin/credential/ldap/backend.go +++ b/builtin/credential/ldap/backend.go @@ -195,15 +195,16 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri // Policies from each group may overlap policies = strutil.RemoveDuplicates(policies, true) - entityAliasAttribute := username - if !usernameAsAlias { - entityAliasAttribute, err = ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username) - if err != nil { - return "", nil, logical.ErrorResponse(err.Error()), nil, nil - } - if entityAliasAttribute == "" { - return "", nil, logical.ErrorResponse("missing entity alias attribute value"), nil, nil - } + if usernameAsAlias { + return username, policies, ldapResponse, allGroups, nil + } + + entityAliasAttribute, err = ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username) + if err != nil { + return "", nil, logical.ErrorResponse(err.Error()), nil, nil + } + if entityAliasAttribute == "" { + return "", nil, logical.ErrorResponse("missing entity alias attribute value"), nil, nil } return entityAliasAttribute, policies, ldapResponse, allGroups, nil From 3a713c1004d6ec1035c6be35b8ad4eb48dbc3c44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 17:27:42 +0200 Subject: [PATCH 6/7] Format and fix declaration --- builtin/credential/ldap/backend.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/credential/ldap/backend.go b/builtin/credential/ldap/backend.go index bf15477a99504..bbd9f1bfc157d 100644 --- a/builtin/credential/ldap/backend.go +++ b/builtin/credential/ldap/backend.go @@ -198,8 +198,8 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri if usernameAsAlias { return username, policies, ldapResponse, allGroups, nil } - - entityAliasAttribute, err = ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username) + + entityAliasAttribute, err := ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username) if err != nil { return "", nil, logical.ErrorResponse(err.Error()), nil, nil } From a741e0de15c709795bbc12c5df7ca4fc9e48e93c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Fri, 20 May 2022 22:50:14 +0200 Subject: [PATCH 7/7] Reword changelog --- changelog/15525.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/changelog/15525.txt b/changelog/15525.txt index a4b23820efb55..3e44c205b18ff 100644 --- a/changelog/15525.txt +++ b/changelog/15525.txt @@ -1,3 +1,8 @@ ```release-note:bug -auth/ldap: A bug during login has been fixed when `username_as_alias` is set. +auth/ldap: The logic for setting the entity alias when `username_as_alias` is set +has been fixed. The previous behavior would make a request to the LDAP server to +get `user_attr` before discarding it and using the username instead. This would +make it impossible for a user to connect if this attribute was missing or had +multiple values, even though it would not be used anyway. This has been fixed +and the username is now used without making superfluous LDAP searches. ```