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

Make the error response to the sys/internal/ui/mounts with no client token consistent #10650

Merged
merged 6 commits into from
Jan 7, 2021
Merged

Make the error response to the sys/internal/ui/mounts with no client token consistent #10650

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bd0fce0
Make the error response to the sys/internal/ui/mounts with no client …
sgmiller Jan 5, 2021
bdc2c96
changelog
sgmiller Jan 5, 2021
c4d6c60
Don't test against an empty mount path
sgmiller Jan 5, 2021
fa9ed2b
One other spot
sgmiller Jan 6, 2021
7c001f9
Instead, do all token checks first and early out before even looking …
sgmiller Jan 7, 2021
c7db022
Merge branch 'master' into sys/internal-consistent-err
sgmiller Jan 7, 2021
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
4 changes: 4 additions & 0 deletions changelog/10650.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:bug
core: Make the response to an unauthenticated request to sys/internal endpoints consistent regardless of mount existence.
```

27 changes: 21 additions & 6 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to handle the equivalent case where the mount exists but replication filtering precludes using it? (see checkReplicatedFiltering below)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think you're right, added.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right I think, no harm in doing the token/entity check first. Changed.

_, err = b.verifyAuthorizedMountAccess(ctx, req)
if err != nil {
return errResp, err
}

// Return a permission denied error here so this path cannot be used to
// brute force a list of mounts.
return errResp, logical.ErrPermissionDenied
Expand All @@ -3386,25 +3392,34 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica
fullMountPath = ns.Path + me.Namespace().Path + me.Path
}

acl, err := b.verifyAuthorizedMountAccess(ctx, req)
if err != nil {
return errResp, err
}

if !hasMountAccess(ctx, acl, fullMountPath) {
return errResp, logical.ErrPermissionDenied
}

return resp, nil
}

func (b *SystemBackend) verifyAuthorizedMountAccess(ctx context.Context, req *logical.Request) (*ACL, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this method is a bit misleading since I don't think it has anything to do with mounts.

// Load the ACL policies so we can walk the prefix for this mount
acl, te, entity, _, err := b.Core.fetchACLTokenEntryAndEntity(ctx, req)
if err != nil {
return nil, err
}
if entity != nil && entity.Disabled {
b.logger.Warn("permission denied as the entity on the token is disabled")
return errResp, logical.ErrPermissionDenied
return nil, logical.ErrPermissionDenied
}
if te != nil && te.EntityID != "" && entity == nil {
b.logger.Warn("permission denied as the entity on the token is invalid")
return nil, logical.ErrPermissionDenied
}

if !hasMountAccess(ctx, acl, fullMountPath) {
return errResp, logical.ErrPermissionDenied
}

return resp, nil
return acl, nil
}

func (b *SystemBackend) pathInternalCountersRequests(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
Expand Down