normalize LDAP auth HTTP responses

[raymonstah]

Attempts to fix #20923.

With this change, every combination of an invalid username or invalid password when authenticating against LDAP should return an HTTP 400 with the error message:

{
    "errors": [
        "ldap operation failed: failed to bind as user"
    ]
}

Demo:

LDAP.Auth.HTTP.Response.Codes.mp4

[robmonte]

Nice fix for information leakage 👍

[austingebauer]

LGTM! Might be worth exploring a bit on response behavior on a couple of different auth methods before merging.

[austingebauer]

Just gave this a test with a different auth method (userpass) to see if there are any undesirable side effects. I think this fixes other issues (good thing!). Interesting that userpass auth with an incorrect password returns a 500 before this change. Makes me wonder how long it's behaved like that 🤔 With the changes in this PR, both login attempts below return a 400.

$ vault auth enable userpass

$ vault write auth/userpass/users/test \
    password=password

$ vault login -method=userpass username=testt password=password
Error authenticating: Error making API request.

URL: PUT http://localhost:8200/v1/auth/userpass/login/testt
Code: 400. Errors:

* invalid username or password
$ vault login -method=userpass username=test password=passwordd
Error authenticating: Error making API request.

URL: PUT http://localhost:8200/v1/auth/userpass/login/test
Code: 500. Errors:

* invalid username or password

[raymonstah]

Thanks for testing the userpass auth flow!

[calvn]

If the error is non-nil in this case (and scoped to be logical.ErrInvalidCredentials from the check above), is there a reason to return resp rather than nil?

[austingebauer]

I believe so that a 204 No Content isn't returned (reported in the GH issue) and the response has the errUserBindFailed contents included. @raymonstah can confirm this.

[calvn]

I see what's going -- the resp's Error is what fixes the 204 since the actual content is coming from there, and not the error's string value.

[fairclothjm]

I think we should call this out as a change

change – A change in the product that may require action or review by the operator. Examples would be any kind of API change (as opposed to backwards compatible addition), a notable behavior change, or anything that might require attention before updating.

[mickael-hc]

@raymonstah @austingebauer would it make sense to backport these changes?

[raymonstah]

@raymonstah @austingebauer would it make sense to backport these changes?

Hey @mickael-hc, no objections here regarding a backport. I can't think of any reason not to.

[austingebauer]

@raymonstah @mickael-hc - Sounds good on the backport. Looks like ErrInvalidCredentials was introduced in 1.13, so it probably makes sense to go back to 1.13.x. I believe applying the backport labels to this PR will still result in the backport PRs being created by the automation.