From c839fc78d859ec08eb7bdf785cdd79e68167992c Mon Sep 17 00:00:00 2001
From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com>
Date: Thu, 17 Feb 2022 17:19:44 -0800
Subject: [PATCH] auth/ldap: add resp warning if userfilter doesn't consider
 userattr (#14095)

* auth/ldap: add resp warning if userfilter doesn't consider userattr

* add changelog entry
---
 builtin/credential/ldap/backend_test.go       | 55 +++++++++++++++++++
 builtin/credential/ldap/path_config.go        | 42 +++++++++++++-
 changelog/14095.txt                           |  4 ++
 website/content/api-docs/auth/ldap.mdx        |  2 +
 website/content/docs/auth/ldap.mdx            |  4 +-
 .../partials/ldap-auth-userfilter-warning.mdx |  4 ++
 6 files changed, 108 insertions(+), 3 deletions(-)
 create mode 100644 changelog/14095.txt
 create mode 100644 website/content/partials/ldap-auth-userfilter-warning.mdx

diff --git a/builtin/credential/ldap/backend_test.go b/builtin/credential/ldap/backend_test.go
index 2cf97fdf8..c53fc7819 100644
--- a/builtin/credential/ldap/backend_test.go
+++ b/builtin/credential/ldap/backend_test.go
@@ -11,6 +11,7 @@ import (
 	goldap "github.com/go-ldap/ldap/v3"
 	"github.com/go-test/deep"
 	hclog "github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/go-secure-stdlib/strutil"
 	"github.com/hashicorp/vault/helper/namespace"
 	"github.com/hashicorp/vault/helper/testhelpers/ldap"
 	logicaltest "github.com/hashicorp/vault/helper/testhelpers/logical"
@@ -505,6 +506,30 @@ func TestBackend_basic_authbind_userfilter(t *testing.T) {
 	cleanup, cfg := ldap.PrepareTestContainer(t, "latest")
 	defer cleanup()
 
+	// userattr not used in the userfilter should result in a warning in the response
+	cfg.UserFilter = "((mail={{.Username}}))"
+	logicaltest.Test(t, logicaltest.TestCase{
+		CredentialBackend: b,
+		Steps: []logicaltest.TestStep{
+			testAccStepConfigUrlWarningCheck(t, cfg, logical.UpdateOperation, []string{userFilterWarning}),
+			testAccStepConfigUrlWarningCheck(t, cfg, logical.ReadOperation, []string{userFilterWarning}),
+		},
+	})
+
+	// If both upndomain and userfilter is set, ensure that a warning is still
+	// returned if userattr is not considered
+	cfg.UPNDomain = "planetexpress.com"
+
+	logicaltest.Test(t, logicaltest.TestCase{
+		CredentialBackend: b,
+		Steps: []logicaltest.TestStep{
+			testAccStepConfigUrlWarningCheck(t, cfg, logical.UpdateOperation, []string{userFilterWarning}),
+			testAccStepConfigUrlWarningCheck(t, cfg, logical.ReadOperation, []string{userFilterWarning}),
+		},
+	})
+
+	cfg.UPNDomain = ""
+
 	// Add a liberal user filter, allowing to log in with either cn or email
 	cfg.UserFilter = "(|({{.UserAttr}}={{.Username}})(mail={{.Username}}))"
 
@@ -842,6 +867,36 @@ func testAccStepConfigUrlNoGroupDN(t *testing.T, cfg *ldaputil.ConfigEntry) logi
 	}
 }
 
+func testAccStepConfigUrlWarningCheck(t *testing.T, cfg *ldaputil.ConfigEntry, operation logical.Operation, warnings []string) logicaltest.TestStep {
+	return logicaltest.TestStep{
+		Operation: operation,
+		Path:      "config",
+		Data: map[string]interface{}{
+			"url":                  cfg.Url,
+			"userattr":             cfg.UserAttr,
+			"userdn":               cfg.UserDN,
+			"userfilter":           cfg.UserFilter,
+			"groupdn":              cfg.GroupDN,
+			"groupattr":            cfg.GroupAttr,
+			"binddn":               cfg.BindDN,
+			"bindpass":             cfg.BindPassword,
+			"case_sensitive_names": true,
+			"token_policies":       "abc,xyz",
+			"request_timeout":      cfg.RequestTimeout,
+		},
+		Check: func(response *logical.Response) error {
+			if len(response.Warnings) == 0 {
+				return fmt.Errorf("expected warnings, got none")
+			}
+
+			if !strutil.StrListSubset(response.Warnings, warnings) {
+				return fmt.Errorf("expected response to contain the following warnings:\n%s\ngot:\n%s", warnings, response.Warnings)
+			}
+			return nil
+		},
+	}
+}
+
 func testAccStepGroup(t *testing.T, group string, policies string) logicaltest.TestStep {
 	t.Logf("[testAccStepGroup] - Registering group %s, policy %s", group, policies)
 	return logicaltest.TestStep{
diff --git a/builtin/credential/ldap/path_config.go b/builtin/credential/ldap/path_config.go
index 0b5bec6ad..45e5294c7 100644
--- a/builtin/credential/ldap/path_config.go
+++ b/builtin/credential/ldap/path_config.go
@@ -2,6 +2,7 @@ package ldap
 
 import (
 	"context"
+	"strings"
 
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/consts"
@@ -10,6 +11,8 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
+const userFilterWarning = "userfilter configured does not consider userattr and may result in colliding entity aliases on logins"
+
 func pathConfig(b *backend) *framework.Path {
 	p := &framework.Path{
 		Pattern: `config`,
@@ -110,9 +113,38 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, d *f
 	data := cfg.PasswordlessMap()
 	cfg.PopulateTokenData(data)
 
-	return &logical.Response{
+	resp := &logical.Response{
 		Data: data,
-	}, nil
+	}
+
+	if warnings := b.checkConfigUserFilter(cfg); len(warnings) > 0 {
+		resp.Warnings = warnings
+	}
+
+	return resp, nil
+}
+
+// checkConfigUserFilter performs a best-effort check the config's userfilter.
+// It will checked whether the templated or literal userattr value is present,
+// and if not return a warning.
+func (b *backend) checkConfigUserFilter(cfg *ldapConfigEntry) []string {
+	if cfg == nil || cfg.UserFilter == "" {
+		return nil
+	}
+
+	var warnings []string
+
+	switch {
+	case strings.Contains(cfg.UserFilter, "{{.UserAttr}}"):
+		// Case where the templated userattr value is provided
+	case strings.Contains(cfg.UserFilter, cfg.UserAttr):
+		// Case where the literal userattr value is provided
+	default:
+		b.Logger().Debug(userFilterWarning, "userfilter", cfg.UserFilter, "userattr", cfg.UserAttr)
+		warnings = append(warnings, userFilterWarning)
+	}
+
+	return warnings
 }
 
 func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
@@ -154,6 +186,12 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d *
 		return nil, err
 	}
 
+	if warnings := b.checkConfigUserFilter(cfg); len(warnings) > 0 {
+		return &logical.Response{
+			Warnings: warnings,
+		}, nil
+	}
+
 	return nil, nil
 }
 
diff --git a/changelog/14095.txt b/changelog/14095.txt
new file mode 100644
index 000000000..f534d66c3
--- /dev/null
+++ b/changelog/14095.txt
@@ -0,0 +1,4 @@
+```release-note:improvement
+auth/ldap: Add a response warning and server log whenever the config is accessed
+if `userfilter` doesn't consider `userattr`
+```
diff --git a/website/content/api-docs/auth/ldap.mdx b/website/content/api-docs/auth/ldap.mdx
index da9c40a05..fd2666532 100644
--- a/website/content/api-docs/auth/ldap.mdx
+++ b/website/content/api-docs/auth/ldap.mdx
@@ -90,6 +90,8 @@ This endpoint configures the LDAP auth method.
 
 @include 'tokenfields.mdx'
 
+@include 'ldap-auth-userfilter-warning.mdx'
+
 ### Sample Request
 
 ```shell-session
diff --git a/website/content/docs/auth/ldap.mdx b/website/content/docs/auth/ldap.mdx
index 4a9ff57f4..7703e233b 100644
--- a/website/content/docs/auth/ldap.mdx
+++ b/website/content/docs/auth/ldap.mdx
@@ -118,7 +118,7 @@ There are two alternate methods of resolving the user object used to authenticat
 - `userattr` (string, optional) - Attribute on user attribute object matching the username passed when authenticating. Examples: `sAMAccountName`, `cn`, `uid`
 - `userfilter` (string, optional) - Go template used to construct a ldap user search filter. The template can access the following context variables: \[`UserAttr`, `Username`\]. The default userfilter is `({{.UserAttr}}={{.Username}})` or `(userPrincipalName={{.Username}}@UPNDomain)` if the `upndomain` parameter is set. The user search filter can be used to restrict what user can attempt to log in. For example, to limit login to users that are not contractors, you could write `(&(objectClass=user)({{.UserAttr}}={{.Username}})(!(employeeType=Contractor)))`.
 
-
+@include 'ldap-auth-userfilter-warning.mdx'
 
 #### Binding - Anonymous Search
 
@@ -129,6 +129,8 @@ There are two alternate methods of resolving the user object used to authenticat
 - `deny_null_bind` (bool, optional) - This option prevents users from bypassing authentication when providing an empty password. The default is `true`.
 - `anonymous_group_search` (bool, optional) - Use anonymous binds when performing LDAP group searches. Defaults to `false`.
 
+@include 'ldap-auth-userfilter-warning.mdx'
+
 #### Binding - User Principal Name (AD)
 
 - `upndomain` (string, optional) - userPrincipalDomain used to construct the UPN string for the authenticating user. The constructed UPN will appear as `[username]@UPNDomain`. Example: `example.com`, which will cause vault to bind as `username@example.com`.
diff --git a/website/content/partials/ldap-auth-userfilter-warning.mdx b/website/content/partials/ldap-auth-userfilter-warning.mdx
new file mode 100644
index 000000000..bfb46fca8
--- /dev/null
+++ b/website/content/partials/ldap-auth-userfilter-warning.mdx
@@ -0,0 +1,4 @@
+~> When specifying a `userfilter`, either the templated value `{{.UserAttr}}` or
+the literal value that matches `userattr` should be present in the filter to
+ensure that the search returns a unique result that takes `userattr` into
+consideration for entity alias mapping purposes and avoid possible collisions on login.