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.

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

Conversation

mitchelldavis
Copy link
Contributor

Addressing #2387!

@jefferai jefferai changed the title Updating the ldap backend to address issue #2387 Use service bind for searching LDAP groups Mar 28, 2017
@jefferai
Copy link
Member

@shomron Any chance I can get your opinion on this?

@jefferai jefferai added this to the 0.7.1 milestone Mar 28, 2017
Copy link
Contributor

@shomron shomron left a comment

Choose a reason for hiding this comment

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

Overall 👍 - this is the fix follows what we originally discussed way back in August.

Aside from improving the error messages I think we can proceed with this logic.

Separately, the code could do with a little refactoring and cleanup to make it easier to follow and manage the relation between DiscoverDN and BindDN/BindPassword in a more concise way.

Regarding the discussion in #2387, we should definitely not perform the group search before user authentication - that would just expose the LDAP server to unauthenticated DoS.

@@ -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.

// the BindDN should be the one to search, not the user logging in.
if cfg.DiscoverDN || (cfg.BindDN != "" && cfg.BindPassword != "") {
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

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

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...

@jackdwyer
Copy link

@mitchelldavis thanks for addressing this, I just started working on JumpCloud LDAP integration with our Vault cluster 👍

@mitchelldavis
Copy link
Contributor Author

@shomron, I believe I've addressed all that. Does the code reflect what you were thinking?

@lattwood
Copy link

@shomron @mitchelldavis anything I can do to help get this into the next release?

@mitchelldavis
Copy link
Contributor Author

I'm not sure @lattwood. I think we're just waiting for a final review.

Copy link
Contributor

@shomron shomron left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this! Two minor nits, but looks good!

In the future I'd like to see the tests overhauled with a mock LDAP provider, allowing us to capture this behavior in test.

if err != nil {
return nil, logical.ErrorResponse(err.Error()), nil
}

if b.Logger().IsDebug() {
b.Logger().Debug("auth/ldap: BindDN fetched", "username", username, "binddn", bindDN)
b.Logger().Debug("auth/ldap: BindDN fetched", "username", username, "binddn", userBindDN)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Debug message does not reflect binddn->userbinddn.

// 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 {
return nil, logical.ErrorResponse("Encountered an error while attempting to re-bind with the BindDN User: " + 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.

nit: use fmt.

return nil, logical.ErrorResponse(fmt.Sprintf("Encountered an error while attempting to re-bind with the BindDN User: %v", err)), nil

@jefferai
Copy link
Member

@shomron thanks for reviewing this!

@shomron @mitchelldavis any backwards incompatible or notably changed behavior as a result of this?

In addition, any necessary documentation changes?

@mitchelldavis
Copy link
Contributor Author

mitchelldavis commented Apr 11, 2017

@jefferai, @shomron, that build issue looks like the vault unit tests failed. Have we seen that happening in other places because I didn't touch that code.

@jefferai, to answer your question about the documentation - The documentation didn't really specify much to begin with. It makes sense to me to mention that the binddn user needs permission to search groups because it's what is being used to do that search.

@jefferai
Copy link
Member

@mitchelldavis We get some transient unit test failures that we know of (often issues with the MySQL docker container) so don't worry about it if it's not a part of your changes.

Please add the mention in the docs about the binddn user needing permission to search groups.

Still waiting to hear from you and @shomron about any backwards incompatibilities.

@mitchelldavis
Copy link
Contributor Author

@jefferai, I've added those updates to the docs. I would love @shomron to look at the few lines I edited/added though. I'm not an ldap expert so want to make sure that I'm not adding confusion or am outright wrong.

@lattwood
Copy link

@mitchelldavis might be worth mentioning the user and group searches are done under separate (but identical) binds.

@mitchelldavis
Copy link
Contributor Author

Because we have to bind to the binddn user twice @lattwood?

@shomron
Copy link
Contributor

shomron commented Apr 17, 2017

The clarification in the documentation looks good to me.

@jefferai I don't see this as a backwards-incompatible change, more of a bugfix. It might be worthwhile adding something along these lines to the release notes:

LDAP Auth Backend - Group membership queries will now run as the binddn user when binddn/bindpass are configured, rather than as the authenticating user as was the case previously.

@jefferai
Copy link
Member

Sounds good to me!

Merging -- thanks everyone!

@jefferai jefferai merged commit a208159 into hashicorp:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants