VAULT 18227/introduce cap ldap library #22185

raymonstah · 2023-08-02T21:18:07Z

Summary

Introduces the cap/ldap package as an alternative to the existing LDAP client. This allows for better security updates, patches, etc in one library to improve DRY-ness across our repos that leverage LDAP. Internally, this addresses VAULT-18227.

Note:
Since the cap/ldap package doesn't differentiate between connection_timeout and request_timeout like Vault does, this also introduces a change where it takes the minimum timeout between the two values, and uses that as the timeout for both. As a result, this PR also deprecates the connection_timeout field introduced in #20144. As a result, ldaputil.Client has also been deprecated, since cap/ldap.Client can be used to replace it.

Testing

Configuration

➜  vault git:(VAULT-18227/introduce-cap-ldap-library) ✗ vault auth enable ldap
Success! Enabled ldap auth method at: ldap/
➜  vault git:(VAULT-18227/introduce-cap-ldap-library) vault write auth/ldap/config \
binddn="cn=admin,dc=example,dc=org" \
bindpass="adminpassword" \
url="ldap://localhost:1389" \
userdn="ou=users,dc=example,dc=org" \
groupdn="ou=users,dc=example,dc=org" \
userattr="uid" \
request_timeout=3s
Success! Data written to: auth/ldap/config

Valid credentials

➜  vault git:(VAULT-18227/introduce-cap-ldap-library) vault login -method=ldap username=alice
Password (will be hidden):
Success! You are now authenticated...

Wrong username

➜  vault git:(VAULT-18227/introduce-cap-ldap-library) vault login -method=ldap username=aliced
Password (will be hidden):
Error authenticating: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/auth/ldap/login/aliced
Code: 400. Errors:

* ldap operation failed: failed to bind as user

Wrong Password

➜  vault git:(VAULT-18227/introduce-cap-ldap-library) vault login -method=ldap username=alice
Password (will be hidden):
Error authenticating: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/auth/ldap/login/alice
Code: 400. Errors:

* ldap operation failed: failed to bind as user

…meout

github-actions · 2023-08-02T21:38:53Z

CI Results:
All Go tests succeeded! ✅

raymonstah · 2023-08-02T21:26:42Z

sdk/helper/ldaputil/connection.go

@@ -14,7 +14,7 @@ import (
 // but through an interface.
 type Connection interface {
 	Bind(username, password string) error
-	Close()
+	Close() error


Changed as a result of updating the go-ldap/ldap library to v3.4.5 when cap/ldap was installed.

raymonstah · 2023-08-02T21:26:45Z

website/content/api-docs/secret/ldap.mdx

@@ -53,9 +53,6 @@ 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: 30 or string: "30s")` - Timeout, in seconds, 


Moved to Deprecated Parameters and added a note.

raymonstah · 2023-08-02T21:28:09Z

helper/testhelpers/ldap/ldaphelper.go

 	"github.com/hashicorp/vault/sdk/helper/docker"
 	"github.com/hashicorp/vault/sdk/helper/ldaputil"
 )

 func PrepareTestContainer(t *testing.T, version string) (cleanup func(), cfg *ldaputil.ConfigEntry) {
-	// Skipping on ARM, as this image can't run on ARM architecture
-	if strings.Contains(runtime.GOARCH, "arm") {


I was able to run these tests locally on my Mac M1, but if this causes test failures I'll revert these changes.

raymonstah · 2023-08-02T21:30:48Z

builtin/credential/ldap/backend.go

-	entityAliasAttribute, err := ldapClient.GetUserAliasAttributeValue(cfg.ConfigEntry, c, username)
-	if err != nil {
-		return "", nil, logical.ErrorResponse(err.Error()), nil, nil
-	}
-	if entityAliasAttribute == "" {


The implementation of this can be found here.

I believe it boils down to finding the 'CN' (common name) attribute.

raymonstah · 2023-08-02T21:33:16Z

builtin/credential/ldap/backend.go


 	// Clean connection
-	defer c.Close()


All of the code deleted below is implemented via the Authenticate method found in cap/ldap.

We do need to specify options to also get groups and attributes.

raymonstah · 2023-08-02T21:33:38Z

builtin/credential/ldap/backend.go

@@ -76,82 +77,25 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri
 		return "", nil, logical.ErrorResponse("password cannot be of zero length when passwordless binds are being denied"), nil, nil
 	}

-	ldapClient := ldaputil.Client{


raymonstah · 2023-08-02T21:36:16Z

builtin/credential/ldap/backend.go

+		if strings.Contains(err.Error(), "discovery of user bind DN failed") ||
+			strings.Contains(err.Error(), "unable to bind user") {


A little manual, as the cap/ldap library doesn't have any exported error types we can leverage.

github-actions · 2023-08-03T19:05:36Z

Build Results:
All builds succeeded! ✅

jimlambrt · 2023-08-04T13:15:59Z

I'm happy to see cap/ldap get adopted by Vault! I just have one question/suggestion, but otherwise it looks good.

jimlambrt · 2023-08-04T13:13:50Z

builtin/credential/ldap/backend.go

-		return "", nil, logical.ErrorResponse(err.Error()), nil, nil
-	}
-	if entityAliasAttribute == "" {
+	cn := c.UserAttributes["cn"]


I'm not sure this is quite correct. It appears that
Client.GetUserAliasAttributeValue(...) retrieves cfg.UserAttr which could be
really any attribute name (of course it's typically cn or uid). So perhaps
this should be something like userAttrValues := c.UserAttributes[cfg.UserAttr]?

You're right, thanks for catching this!

austingebauer

Thanks for taking this on, @raymonstah! Looking good. Left a few questions/suggestions.

austingebauer · 2023-08-07T15:43:16Z

builtin/credential/ldap/backend.go

-		return "", nil, logical.ErrorResponse(err.Error()), nil, nil
-	}
-	if entityAliasAttribute == "" {
+	userAttrValues := c.UserAttributes[cfg.UserAttr]


UUIC c.UserAttributes[cfg.UserAttr] is set from cap/ldap getUserBindDN()? Would be worth double-checking that the cap/ldap implementation would result in the same entity alias as the prior code in GetUserAliasAttributeValue(). We've have entity alias changes result in privilege escalation in the past so worth a look!

Yeah it seems like the cap/ldap implementation doesn't apply the userfilter but I'm not sure if that would matter since the userfilter would've already been applied from the search to get the user bind dn.

But based on the existing client,

vault/sdk/helper/ldaputil/client.go

Line 149 in be83f12

cfg.UserAttr, // Return only needed attributes

it does like it uses cfg.UserAttr as well.

austingebauer · 2023-08-07T15:55:38Z

sdk/helper/ldaputil/config.go

+		StartTLS:                             cfg.StartTLS,
+		BindDN:                               cfg.BindDN,
+		BindPassword:                         cfg.BindPassword,
+		AllowEmptyPasswordBinds:              true,


Wondering if this should be cfg.DenyNullBind instead of true?

yeah I think that's correct. or more accurately, !cfg.DenyNullBind. Updated!

sdk/helper/ldaputil/config.go

raymonstah · 2023-08-08T20:51:16Z

Closing, until pagination gets added to cap/ldap.

…-cap-ldap-library

austingebauer

LGTM given that our tests are passing. Thanks for following up on this @raymonstah!

austingebauer · 2023-09-12T00:57:09Z

sdk/helper/ldaputil/client.go

@@ -24,6 +24,7 @@ import (
 	"github.com/hashicorp/go-secure-stdlib/tlsutil"
 )

+// Deprecated: Use ldap.Client instead


I'm thinking we'll want to leave this as not deprecated. It's still used in a couple of different plugins (openldap example) for secrets engine use cases. I don't think cap/ldap will ever replace the usage given that it's targeted at authentication.

jimlambrt

Just a minor bit about go.sum

sdk/go.mod

website/content/api-docs/secret/ldap.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

jimlambrt

Ty!

raymonstah added 2 commits August 1, 2023 17:46

use cap/ldap library instead of ldaputil

be83f12

normalize ldap authentication error messages; deprecate connection_ti…

75b1b9c

…meout

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 2, 2023

raymonstah added this to the 1.15 milestone Aug 2, 2023

add changelog

c6cd37f

raymonstah commented Aug 2, 2023

View reviewed changes

raymonstah marked this pull request as ready for review August 3, 2023 18:52

raymonstah requested a review from a team as a code owner August 3, 2023 18:52

raymonstah requested review from a team and jasonodonnell August 3, 2023 18:52

raymonstah requested a review from jimlambrt August 3, 2023 19:10

jimlambrt reviewed Aug 4, 2023

View reviewed changes

use user defined attribute instead of assuming 'cn'

b391c03

austingebauer reviewed Aug 7, 2023

View reviewed changes

use DenyNullBind for AllowEmptyPasswordBinds

c7203fd

raymonstah mentioned this pull request Aug 8, 2023

Address LDAP issues #22249

Merged

This was referenced Aug 8, 2023

Backport of Address LDAP issues into release/1.14.x #22251

Merged

Backport of Address LDAP issues into release/1.13.x #22252

Merged

raymonstah closed this Aug 8, 2023

hc-github-team-secure-vault-core mentioned this pull request Aug 9, 2023

Backport of Address LDAP issues into release/1.12.x #22261

Merged

raymonstah added 2 commits August 24, 2023 16:05

update cap/ldap for pagination support

011a850

update cap/ldap for parallel group search support

11af2cd

raymonstah reopened this Sep 12, 2023

raymonstah requested a review from a team as a code owner September 12, 2023 00:09

Merge remote-tracking branch 'origin/main' into VAULT-18227/introduce…

c4f3530

…-cap-ldap-library

raymonstah requested review from jimlambrt and austingebauer September 12, 2023 00:12

Merge remote-tracking branch 'origin/main' into VAULT-18227/introduce…

e85b139

…-cap-ldap-library

vercel bot deployed to Preview September 12, 2023 00:24 View deployment

austingebauer approved these changes Sep 12, 2023

View reviewed changes

raymonstah modified the milestones: 1.15.0-rc1, 1.16 Sep 12, 2023

jimlambrt requested changes Sep 12, 2023

View reviewed changes

sdk/go.mod Show resolved Hide resolved

tidy

cc3b46d

schavis requested changes Sep 12, 2023

View reviewed changes

website/content/api-docs/secret/ldap.mdx Outdated Show resolved Hide resolved

raymonstah and others added 2 commits September 12, 2023 13:38

undeprecate existing ldap client

2bbc7b2

Update website/content/api-docs/secret/ldap.mdx

1449774

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

vercel bot deployed to Preview September 12, 2023 20:48 View deployment

jimlambrt approved these changes Sep 13, 2023

View reviewed changes

raymonstah requested a review from schavis September 13, 2023 21:07

schavis approved these changes Sep 13, 2023

View reviewed changes

raymonstah merged commit 018e567 into main Sep 14, 2023
109 checks passed

raymonstah deleted the VAULT-18227/introduce-cap-ldap-library branch September 14, 2023 17:26

raymonstah mentioned this pull request Sep 14, 2023

downgrade go-ldap client to v3.4.4 due to race conditions in tests #23103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VAULT 18227/introduce cap ldap library #22185

VAULT 18227/introduce cap ldap library #22185

raymonstah commented Aug 2, 2023 •

edited by jira bot

Loading

github-actions bot commented Aug 2, 2023 •

edited

Loading

raymonstah Aug 2, 2023 •

edited

Loading

raymonstah Aug 2, 2023

raymonstah Aug 2, 2023

raymonstah Aug 2, 2023

raymonstah Aug 2, 2023

raymonstah Aug 2, 2023

raymonstah Aug 2, 2023

github-actions bot commented Aug 3, 2023

jimlambrt commented Aug 4, 2023

jimlambrt Aug 4, 2023

raymonstah Aug 4, 2023

austingebauer left a comment

austingebauer Aug 7, 2023

raymonstah Aug 7, 2023

austingebauer Aug 7, 2023

raymonstah Aug 7, 2023

raymonstah commented Aug 8, 2023

austingebauer left a comment

austingebauer Sep 12, 2023

jimlambrt left a comment

jimlambrt left a comment

		if strings.Contains(err.Error(), "discovery of user bind DN failed") \|\|
		strings.Contains(err.Error(), "unable to bind user") {

VAULT 18227/introduce cap ldap library #22185

VAULT 18227/introduce cap ldap library #22185

Conversation

raymonstah commented Aug 2, 2023 • edited by jira bot Loading

Summary

Testing

github-actions bot commented Aug 2, 2023 • edited Loading

raymonstah Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 3, 2023

jimlambrt commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austingebauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymonstah commented Aug 8, 2023

austingebauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimlambrt left a comment

Choose a reason for hiding this comment

jimlambrt left a comment

Choose a reason for hiding this comment

raymonstah commented Aug 2, 2023 •

edited by jira bot

Loading

github-actions bot commented Aug 2, 2023 •

edited

Loading

raymonstah Aug 2, 2023 •

edited

Loading