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.

Sign up for GitHub

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

Jump to bottom

VAULT 18227/introduce cap ldap library #22185

Merged
merged 12 commits into from
Sep 14, 2023
Merged

VAULT 18227/introduce cap ldap library #22185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
be83f12
use cap/ldap library instead of ldaputil
raymonstah Aug 2, 2023
75b1b9c
normalize ldap authentication error messages; deprecate connection_ti…
raymonstah Aug 2, 2023
c6cd37f
add changelog
raymonstah Aug 2, 2023
b391c03
use user defined attribute instead of assuming 'cn'
raymonstah Aug 4, 2023
c7203fd
use DenyNullBind for AllowEmptyPasswordBinds
raymonstah Aug 7, 2023
011a850
update cap/ldap for pagination support
raymonstah Aug 24, 2023
11af2cd
update cap/ldap for parallel group search support
raymonstah Sep 12, 2023
c4f3530
Merge remote-tracking branch 'origin/main' into VAULT-18227/introduce…
raymonstah Sep 12, 2023
e85b139
Merge remote-tracking branch 'origin/main' into VAULT-18227/introduce…
raymonstah Sep 12, 2023
cc3b46d
tidy
raymonstah Sep 12, 2023
2bbc7b2
undeprecate existing ldap client
raymonstah Sep 12, 2023
1449774
Update website/content/api-docs/secret/ldap.mdx
raymonstah Sep 12, 2023
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
84 changes: 15 additions & 69 deletions builtin/credential/ldap/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"strings"

"github.com/hashicorp/cap/ldap"
"github.com/hashicorp/go-secure-stdlib/strutil"

"github.com/hashicorp/vault/sdk/framework"
Expand Down Expand Up @@ -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{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏼

Logger: b.Logger(),
LDAP: ldaputil.NewLDAP(),
}

c, err := ldapClient.DialLDAP(cfg.ConfigEntry)
ldapClient, err := ldap.NewClient(ctx, ldaputil.ConvertConfig(cfg.ConfigEntry))
if err != nil {
return "", nil, logical.ErrorResponse(err.Error()), nil, nil
}
if c == nil {
return "", nil, logical.ErrorResponse("invalid connection returned from LDAP dial"), nil, nil
}

// Clean connection
defer c.Close()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


userBindDN, err := ldapClient.GetUserBindDN(cfg.ConfigEntry, c, username)
if err != nil {
if b.Logger().IsDebug() {
b.Logger().Debug("error getting user bind DN", "error", err)
}
return "", nil, logical.ErrorResponse(errUserBindFailed), nil, logical.ErrInvalidCredentials
}

if b.Logger().IsDebug() {
b.Logger().Debug("user binddn fetched", "username", username, "binddn", userBindDN)
}

// Try to bind as the login user. This is where the actual authentication takes place.
if len(password) > 0 {
err = c.Bind(userBindDN, password)
} else {
err = c.UnauthenticatedBind(userBindDN)
}
if err != nil {
if b.Logger().IsDebug() {
b.Logger().Debug("ldap bind failed", "error", err)
}
return "", nil, logical.ErrorResponse(errUserBindFailed), nil, logical.ErrInvalidCredentials
}
defer ldapClient.Close(ctx)

// We re-bind to the BindDN if it's defined because we assume
// the BindDN should be the one to search, not the user logging in.
if cfg.BindDN != "" && cfg.BindPassword != "" {
if err := c.Bind(cfg.BindDN, cfg.BindPassword); err != nil {
if b.Logger().IsDebug() {
b.Logger().Debug("error while attempting to re-bind with the BindDN User", "error", err)
}
return "", nil, logical.ErrorResponse("ldap operation failed: failed to re-bind with the BindDN user"), nil, logical.ErrInvalidCredentials
}
if b.Logger().IsDebug() {
b.Logger().Debug("re-bound to original binddn")
}
}

userDN, err := ldapClient.GetUserDN(cfg.ConfigEntry, c, userBindDN, username)
c, err := ldapClient.Authenticate(ctx, username, password, ldap.WithGroups(), ldap.WithUserAttributes())
if err != nil {
return "", nil, logical.ErrorResponse(err.Error()), nil, nil
}

if cfg.AnonymousGroupSearch {
c, err = ldapClient.DialLDAP(cfg.ConfigEntry)
if err != nil {
return "", nil, logical.ErrorResponse("ldap operation failed: failed to connect to LDAP server"), nil, nil
if strings.Contains(err.Error(), "discovery of user bind DN failed") ||
strings.Contains(err.Error(), "unable to bind user") {
Comment on lines +90 to +91
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return "", nil, logical.ErrorResponse(errUserBindFailed), nil, logical.ErrInvalidCredentials
}
defer c.Close() // Defer closing of this connection as the deferal above closes the other defined connection
}

ldapGroups, err := ldapClient.GetLdapGroups(cfg.ConfigEntry, c, userDN, username)
if err != nil {
return "", nil, logical.ErrorResponse(err.Error()), nil, nil
}
if b.Logger().IsDebug() {
b.Logger().Debug("groups fetched from server", "num_server_groups", len(ldapGroups), "server_groups", ldapGroups)
}

ldapGroups := c.Groups
ldapResponse := &logical.Response{
Data: map[string]interface{}{},
}
Expand All @@ -162,6 +106,10 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri
ldapResponse.AddWarning(errString)
}

for _, warning := range c.Warnings {
ldapResponse.AddWarning(string(warning))
}

var allGroups []string
canonicalUsername := username
cs := *cfg.CaseSensitiveNames
Expand Down Expand Up @@ -206,13 +154,11 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri
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 == "" {
Comment on lines -209 to -213
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of this can be found here.

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

userAttrValues := c.UserAttributes[cfg.UserAttr]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if len(userAttrValues) == 0 {
return "", nil, logical.ErrorResponse("missing entity alias attribute value"), nil, nil
}
entityAliasAttribute := userAttrValues[0]

return entityAliasAttribute, policies, ldapResponse, allGroups, nil
}
Expand Down
5 changes: 5 additions & 0 deletions changelog/22185.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
```release-note:improvement
auth/ldap: introduce cap/ldap.Client for LDAP authentication
auth/ldap: deprecates `connection_timeout` in favor of `request_timeout` for timeouts
sdk/ldaputil: deprecates Client in favor of cap/ldap.Client
```
3 changes: 2 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ require (
github.com/go-errors/errors v1.5.0
github.com/go-git/go-git/v5 v5.7.0
github.com/go-jose/go-jose/v3 v3.0.0
github.com/go-ldap/ldap/v3 v3.4.4
github.com/go-ldap/ldap/v3 v3.4.5
github.com/go-sql-driver/mysql v1.7.1
github.com/go-test/deep v1.1.0
github.com/go-zookeeper/zk v1.0.3
Expand All @@ -75,6 +75,7 @@ require (
github.com/google/go-metrics-stackdriver v0.2.0
github.com/google/tink/go v1.7.0
github.com/hashicorp/cap v0.3.4
github.com/hashicorp/cap/ldap v0.0.0-20230907231022-8e71bfc048ed
github.com/hashicorp/consul-template v0.33.0
github.com/hashicorp/consul/api v1.23.0
github.com/hashicorp/errwrap v1.1.0
Expand Down
Loading
Loading