-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
normalize LDAP auth HTTP responses #21282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix for information leakage 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Might be worth exploring a bit on response behavior on a couple of different auth methods before merging.
@@ -1406,7 +1407,7 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re | |||
return nil, nil, err | |||
} | |||
} | |||
return nil, nil, resp.Error() | |||
return resp, nil, routeErr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing the userpass auth flow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
changelog/21282.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
@raymonstah @mickael-hc - Sounds good on the backport. Looks like |
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:Demo:
LDAP.Auth.HTTP.Response.Codes.mp4