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

normalize LDAP auth HTTP responses #21282

Merged
merged 3 commits into from
Jun 21, 2023
+10 −14
Merged

normalize LDAP auth HTTP responses #21282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87b805c
normalize LDAP auth responses
raymonstah Jun 15, 2023
e97d27b
add changelog
raymonstah Jun 15, 2023
657ebed
mark release-note as 'change'
raymonstah Jun 21, 2023
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
3 changes: 2 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 @@ -9,6 +9,7 @@ import (
"strings"

"github.com/hashicorp/go-secure-stdlib/strutil"

"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/helper/ldaputil"
"github.com/hashicorp/vault/sdk/logical"
Expand Down Expand Up @@ -96,7 +97,7 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri
if b.Logger().IsDebug() {
b.Logger().Debug("error getting user bind DN", "error", err)
}
return "", nil, logical.ErrorResponse(errUserBindFailed), nil, nil
return "", nil, logical.ErrorResponse(errUserBindFailed), nil, logical.ErrInvalidCredentials
}

if b.Logger().IsDebug() {
Expand Down
13 changes: 2 additions & 11 deletions builtin/credential/ldap/path_login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,17 +83,8 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framew
password := d.Get("password").(string)

effectiveUsername, policies, resp, groupNames, err := b.Login(ctx, req, username, password, cfg.UsernameAsAlias)
// Handle an internal error
if err != nil {
return nil, err
}
if resp != nil {
// Handle a logical error
if resp.IsError() {
return resp, nil
}
} else {
resp = &logical.Response{}
if err != nil || (resp != nil && resp.IsError()) {
return resp, err
}

auth := &logical.Auth{
Expand Down
3 changes: 3 additions & 0 deletions changelog/21282.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:change
auth/ldap: Normalize HTTP response codes when invalid credentials are provided
```
5 changes: 3 additions & 2 deletions vault/request_handling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"github.com/hashicorp/go-secure-stdlib/strutil"
"github.com/hashicorp/go-sockaddr"
"github.com/hashicorp/go-uuid"
uberAtomic "go.uber.org/atomic"

"github.com/hashicorp/vault/command/server"
"github.com/hashicorp/vault/helper/identity"
"github.com/hashicorp/vault/helper/identity/mfa"
Expand All @@ -36,7 +38,6 @@ import (
"github.com/hashicorp/vault/sdk/logical"
"github.com/hashicorp/vault/vault/quotas"
"github.com/hashicorp/vault/vault/tokens"
uberAtomic "go.uber.org/atomic"
)

const (
Expand Down Expand Up @@ -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
Copy link
Contributor

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

Copy link
Contributor Author

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!

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

}

if resp != nil {
Expand Down