ldap getUserDN handle multiple users #151
Conversation
…e to TestClient_Authenticate
| { | ||
| name: "failed-with-anon-bind-upn-domain-multiple-users-returned", | ||
| username: "mallory", | ||
| password: "password", | ||
| clientConfig: &ldap.ClientConfig{ | ||
| URLs: []string{fmt.Sprintf("ldaps://127.0.0.1:%d", td.Port())}, | ||
| Certificates: []string{td.Cert()}, | ||
| DiscoverDN: true, | ||
| UserDN: testdirectory.DefaultUserDN, | ||
| GroupDN: testdirectory.DefaultGroupDN, | ||
| UPNDomain: "example.com", | ||
| }, | ||
| opts: []ldap.Option{ldap.WithGroups()}, | ||
| wantErr: true, | ||
| wantErrContains: "LDAP search for binddn 0 or not unique", | ||
| }, |
There was a problem hiding this comment.
This test covers the failure path for multiple search results during the getUserBindDN call in Authenticate.
For context, I also tried adding another case that somehow could pass with a single search result in getUserBindDN then fail with multiple search results in getUserDN, but I wasn't able to find a test setup that would follow this path.
Since I wasn't able to get a test path to hit the multiple search results in getUserDN from Authenticate, opted to add unit tests directly at the getUserBindDN and getUserDN level in client_test.go, though it may be a bit redundant.
I'm pretty new to LDAP though (first time working with it as of this PR), so appreciate any guidance if there's a simpler way to test the change in getUserDN!
There was a problem hiding this comment.
This looks great to me. I'm also not too familiar with this area of the code (CC @jimlambrt), but I see the problem and I agree that this code fixes it, as described.
| for _, e := range result.Entries { | ||
| userDN = e.DN | ||
| if len(result.Entries) != 1 { | ||
| return "", fmt.Errorf("%s: LDAP search for user 0 or not unique", op) |
There was a problem hiding this comment.
This is a potential breaking change for users. We should get guidance from the cap library owners on how to proceed. In the past we have added config fields to toggle on any new behavior that breaks backwards compatibility.
There was a problem hiding this comment.
Thanks for raising, @fairclothjm! (And out of curiosity, any chance you have an example of this toggle pattern used in practice for previous breaking changes in this library?)
@johanbrandhorst @jimlambrt could we get some guidance on the preferred approach to introduce this potential breaking change?
There was a problem hiding this comment.
I will defer to Jim for the definite recommendation, but this seems to me only to be a breaking change in the case we actually care about - where the security issue exists. That seems enough to me to justify making this change without an option to turn it off.
There was a problem hiding this comment.
I agree with @johanbrandhorst -- I don't think we should offer an option to opt-in or out.
| for _, e := range result.Entries { | ||
| userDN = e.DN | ||
| if len(result.Entries) != 1 { | ||
| return "", fmt.Errorf("%s: LDAP search for user 0 or not unique", op) |
There was a problem hiding this comment.
I agree with @johanbrandhorst -- I don't think we should offer an option to opt-in or out.
This PR adds a check in the
ldappackage'sgetUserDNfunction that ensures the number of entries returned from the user DN LDAP search is one (in the same way we already handle multiple users ingetUserBindDN). This should prevent authentication attacks that can leverage the fact that we currently return the last user DN if there are duplicate users.The bulk of the changes are unit tests additions at the
getUserBindDNandgetUserDNlevel, which include test cases ensuring we error when there are multiple users returned.Ticket: VAULT-27421