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

Use service bind for searching LDAP groups #2534

Merged
merged 6 commits into from
Apr 18, 2017
Merged

Use service bind for searching LDAP groups #2534

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
71fd6e8
Updating the ldap backend to address issue #2387
Mar 28, 2017
7583c60
Addressing lines 127 and 129 from @shomron's comments
Apr 5, 2017
ab7f99c
Changing 'bind' to 're-bind'.
Apr 5, 2017
ee62b8f
Addressing line 136 of @shomron's comments
Apr 5, 2017
286c3b8
Addressing @somron's review requests on formatting and clarity.
Apr 11, 2017
9ec9434
Updating the docs to add a little more clarity on which user is perfo…
Apr 14, 2017
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
13 changes: 12 additions & 1 deletion builtin/credential/ldap/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,17 @@ func (b *backend) Login(req *logical.Request, username string, password string)
return nil, logical.ErrorResponse(fmt.Sprintf("LDAP bind failed: %v", err)), nil
}

// 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.DiscoverDN || (cfg.BindDN != "" && cfg.BindPassword != "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is symmetric with the logic from getBindDN - my only hesitation is that in cases where (cfg.DiscoverDN == true && (cfg.BindDN == "" || cfg.BindPassword == "") - we will do an anonymous rebind after an authenticated user bind - potentially resulting in more restricted privileges.

I am not sure whether that is a realistic configuration to know whether this is a valid concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shomron, should I just get rid of cfg.DiscoverDN clause? And simply re-bind if the second clause is true? That makes the most sense to me.

if err := c.Bind(cfg.BindDN, cfg.BindPassword); err != nil {
return nil, logical.ErrorResponse(err.Error()), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we are performing up to three binds per login - I think we should improve the error messages to more clearly indicate where in the process we failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So maybe:

return nil, logical.ErrorResponse("Encountered an error when re-binding as the BindDN User: " + err.Error()), nil

}
if b.Logger().IsDebug() {
b.Logger().Debug("auth/ldap: Re-Bound to original BindDN")
}
}

userDN, err := b.getUserDN(cfg, c, bindDN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Between bindDN and cfg.BindDN, following the flow is a bit challenging. Perhaps we should rename the bindDN local variable to something like userBindDN, to differentiate it from the service bind DN - cfg.BindDN ?

Also note userBindDN would not necessarily equal userDN - userDN is the canonical representation of the user, whereas userBindDN could in fact be using a different mechanism such as user principal name (user@domain).

Copy link
Contributor Author

@mitchelldavis mitchelldavis Apr 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That would make sense to me, but if we're going to go down that path, shouldn't we rename the getBindDN method then? Line 107 is where bindDN get's set so...

if err != nil {
return nil, logical.ErrorResponse(err.Error()), nil
Expand Down Expand Up @@ -165,7 +176,7 @@ func (b *backend) Login(req *logical.Request, username string, password string)
policies = append(policies, group.Policies...)
}
}
if user !=nil && user.Policies != nil {
if user != nil && user.Policies != nil {
policies = append(policies, user.Policies...)
}
// Policies from each group may overlap
Expand Down