Make the error response to the sys/internal/ui/mounts with no client token consistent #10650
Conversation
vault/logical_system.go
Outdated
| @@ -3363,6 +3363,12 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica | |||
|
|
|||
| me := b.Core.router.MatchingMountEntry(ctx, path) | |||
| if me == nil { | |||
| // To be consistent with the case no client token was supplied, go through the motions of verifying authorization | |||
There was a problem hiding this comment.
Do we also need to handle the equivalent case where the mount exists but replication filtering precludes using it? (see checkReplicatedFiltering below)
There was a problem hiding this comment.
Yep, I think you're right, added.
There was a problem hiding this comment.
Why not call b.verifyAuthorizedMountAccess at the outset? If we're going to return an error when it returns an error, why even bother looking up the mount entry or filtering?
There was a problem hiding this comment.
So that we return the error each code path meant to do if the verifyA..M..A.. succeeds. In other words, only return a consistent error in the absence of a proper authenticated session, otherwise being more specific is okay.
There was a problem hiding this comment.
Hmm, yeah, that may be equivalent and less subject to timing attacks. I'll revisit in the morning but that may be a good point.
There was a problem hiding this comment.
Your right I think, no harm in doing the token/entity check first. Changed.
vault/logical_system.go
Outdated
| return resp, nil | ||
| } | ||
|
|
||
| func (b *SystemBackend) verifyAuthorizedMountAccess(ctx context.Context, req *logical.Request) (*ACL, error) { |
There was a problem hiding this comment.
The name of this method is a bit misleading since I don't think it has anything to do with mounts.
| } | ||
| if entity != nil && entity.Disabled { | ||
| b.logger.Warn("permission denied as the entity on the token is disabled") | ||
| return nil, logical.ErrPermissionDenied |
There was a problem hiding this comment.
Is the change to not return errResp here deliberate?
There was a problem hiding this comment.
I couldn't see why unlike everywhere else that ignored the errResp.
…token consistent (#10650) * Make the error response to the sys/internal/ui/mounts with no client token consistent * changelog * Don't test against an empty mount path * One other spot * Instead, do all token checks first and early out before even looking for the mount
…token consistent (#10650) * Make the error response to the sys/internal/ui/mounts with no client token consistent * changelog * Don't test against an empty mount path * One other spot * Instead, do all token checks first and early out before even looking for the mount
…token consistent (#10650) (#10674) * Make the error response to the sys/internal/ui/mounts with no client token consistent * changelog * Don't test against an empty mount path * One other spot * Instead, do all token checks first and early out before even looking for the mount Co-authored-by: Meggie <[email protected]>
If an authenticated call is made to this endpoint, check the token anyway even if the mount doesn't exist (e.g. don't early out) in order to return the same error whether the mount exists or not.
Addresses VAULT-872.