Authenticate to "login" endpoint for non-existent mount path bug #13162
Conversation
akshya96
requested review from
briankassouf,
ncabatoff,
pmmukh,
ccapurso and
HridoyRoy
akshya96
changed the title
| @@ -126,7 +126,7 @@ func (c *Core) fetchACLTokenEntryAndEntity(ctx context.Context, req *logical.Req | |||
|
|
|||
| // Ensure there is a client token | |||
| if req.ClientToken == "" { | |||
| return nil, nil, nil, nil, &logical.StatusBadRequest{Err: "missing client token"} | |||
| return nil, nil, nil, nil, logical.ErrPermissionDenied | |||
There was a problem hiding this comment.
I'm still of the opinion that the generic PermissionDenied error is actually less descriptive than a missing client token message, which more accurately captures what is going on here. I'm happy to let this go in if others think differently, but I'm of the opinion that we should more towards more descriptive error messages in each clause as opposed to consistently throwing a message like logical.ErrPermissionDenied no matter which error clause of a function the error occurs in.
There was a problem hiding this comment.
Hmm I think I'm also in favor of keeping the more descriptive missing client token error here. Also, I don't think this really addresses the issue reported, imo we would either want to change https://github.com/hashicorp/vault-enterprise/blob/main/vault/router.go#L810 to return true for a login attempt on a non-existent mount, or close the issue out as a won't do.
There was a problem hiding this comment.
( retracting my comment around the PR not addressing the issue, after going through slack conversation )
vault/router_test.go
Outdated
| @@ -373,6 +373,7 @@ func TestRouter_LoginPath(t *testing.T) { | |||
| {"auth/foo/bar", false}, | |||
| {"auth/foo/login", true}, | |||
| {"auth/foo/login/", false}, | |||
| {"auth/invalid/login/", false}, | |||
There was a problem hiding this comment.
should this be auth/invalid/login , since auth/foo/login/ is not a valid path, and so I'm guessing the trailing slash makes the login path invalid even if invalid were to be actually valid like foo ?
There was a problem hiding this comment.
I think if we're changing an API response code (in this case from 400 to 403) this should have a CHANGES entry in the changelog. Also, what is the target milestone? I'd suggest 1.10. WDYT @ncabatoff
http/help_test.go
Outdated
| @@ -14,8 +15,9 @@ func TestHelp(t *testing.T) { | |||
| TestServerAuth(t, addr, token) | |||
|
|
|||
| resp := testHttpGet(t, "", addr+"/v1/sys/mounts?help=1") | |||
| if resp.StatusCode != http.StatusBadRequest { | |||
| t.Fatal("expected bad request with no token") | |||
| fmt.Println(resp.StatusCode) | |||
There was a problem hiding this comment.
Will remove the print statement. Was using it to debug
@kalafut That works for me. |
Added entry to changelog |
Fixes issue #11152
Testing:
{"errors":["permission denied"]}
403