From bd0fce082dd1c786514886534ca8a5f09bc935fd Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 5 Jan 2021 12:33:21 -0600 Subject: [PATCH 1/5] Make the error response to the sys/internal/ui/mounts with no client token consistent --- vault/logical_system.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index 29076a880f97..679cdf9c5e4b 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -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 + 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 @@ -3386,25 +3392,33 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica fullMountPath = ns.Path + me.Namespace().Path + me.Path } + err = b.verifyAuthorizedMountAccess(ctx, req, fullMountPath) + if err != nil { + return errResp, err + } + + return resp, nil +} + +func (b *SystemBackend) verifyAuthorizedMountAccess(ctx context.Context, req *logical.Request, fullMountPath string) error { // 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 + return err } if entity != nil && entity.Disabled { b.logger.Warn("permission denied as the entity on the token is disabled") - return errResp, logical.ErrPermissionDenied + return 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 + return logical.ErrPermissionDenied } if !hasMountAccess(ctx, acl, fullMountPath) { - return errResp, logical.ErrPermissionDenied + return logical.ErrPermissionDenied } - - return resp, nil + return nil } func (b *SystemBackend) pathInternalCountersRequests(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { From bdc2c9682c2b0221489ddafe0f579f9c8f752326 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 5 Jan 2021 12:34:36 -0600 Subject: [PATCH 2/5] changelog --- changelog/10650.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog/10650.txt diff --git a/changelog/10650.txt b/changelog/10650.txt new file mode 100644 index 000000000000..49c8298839d3 --- /dev/null +++ b/changelog/10650.txt @@ -0,0 +1,4 @@ +```release-note:bug +core: Make the response to an unauthenticated request to sys/internal endpoints consistent regardless of mount existence. +``` + From c4d6c607ef45be63f6dfccd277c86ef93bbd0f9d Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 5 Jan 2021 12:38:10 -0600 Subject: [PATCH 3/5] Don't test against an empty mount path --- vault/logical_system.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index 679cdf9c5e4b..d7f32c27221e 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -3364,7 +3364,7 @@ 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 - err = b.verifyAuthorizedMountAccess(ctx, req, "") + _, err = b.verifyAuthorizedMountAccess(ctx, req) if err != nil { return errResp, err } @@ -3392,33 +3392,34 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica fullMountPath = ns.Path + me.Namespace().Path + me.Path } - err = b.verifyAuthorizedMountAccess(ctx, req, fullMountPath) + 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, fullMountPath string) error { +func (b *SystemBackend) verifyAuthorizedMountAccess(ctx context.Context, req *logical.Request) (*ACL, error) { // 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 err + return nil, err } if entity != nil && entity.Disabled { b.logger.Warn("permission denied as the entity on the token is disabled") - return 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 logical.ErrPermissionDenied + return nil, logical.ErrPermissionDenied } - if !hasMountAccess(ctx, acl, fullMountPath) { - return logical.ErrPermissionDenied - } - return nil + return acl, nil } func (b *SystemBackend) pathInternalCountersRequests(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { From fa9ed2b5a442fe786d99cad1c1bba7af1c5bb4d6 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 6 Jan 2021 13:58:57 -0600 Subject: [PATCH 4/5] One other spot --- vault/logical_system.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vault/logical_system.go b/vault/logical_system.go index d7f32c27221e..734480d512fc 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -3376,6 +3376,12 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica filtered, err := b.Core.checkReplicatedFiltering(ctx, me, "") if err != nil { + // Go through the motions of verifying authorization, as above + _, err = b.verifyAuthorizedMountAccess(ctx, req) + if err != nil { + return errResp, err + } + return nil, err } if filtered { From 7c001f960b218fb833f26571dc0fdbc2f4764950 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Thu, 7 Jan 2021 09:24:37 -0600 Subject: [PATCH 5/5] Instead, do all token checks first and early out before even looking for the mount --- vault/logical_system.go | 49 ++++++++++++----------------------------- 1 file changed, 14 insertions(+), 35 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index 734480d512fc..4ce35fc85d64 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -3354,6 +3354,20 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica } path = sanitizePath(path) + // 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 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 + } + errResp := logical.ErrorResponse(fmt.Sprintf("preflight capability check returned 403, please ensure client's policies grant access to path %q", path)) ns, err := namespace.FromContext(ctx) @@ -3363,12 +3377,6 @@ 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 - _, 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 @@ -3376,12 +3384,6 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica filtered, err := b.Core.checkReplicatedFiltering(ctx, me, "") if err != nil { - // Go through the motions of verifying authorization, as above - _, err = b.verifyAuthorizedMountAccess(ctx, req) - if err != nil { - return errResp, err - } - return nil, err } if filtered { @@ -3398,11 +3400,6 @@ 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 } @@ -3410,24 +3407,6 @@ func (b *SystemBackend) pathInternalUIMountRead(ctx context.Context, req *logica return resp, nil } -func (b *SystemBackend) verifyAuthorizedMountAccess(ctx context.Context, req *logical.Request) (*ACL, error) { - // 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 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 - } - - return acl, nil -} - func (b *SystemBackend) pathInternalCountersRequests(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { counters, err := b.Core.loadAllRequestCounters(ctx, time.Now()) if err != nil {