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

Jump to bottom

Fix sudo paths missing from OpenAPI and docs #21772

Merged
merged 9 commits into from
Jul 19, 2023
Merged

Fix sudo paths missing from OpenAPI and docs #21772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
01f87c9
Fix sudo paths missing from OpenAPI and docs
maxb Jul 12, 2023
f7ca560
Merge branch 'main' into fix-stealth-sudo-paths
maxb Jul 13, 2023
64530c2
changelog
maxb Jul 14, 2023
ed8edc0
Merge branch 'main' into fix-stealth-sudo-paths
maxb Jul 14, 2023
b4244c4
Merge branch 'main' into fix-stealth-sudo-paths
maxb Jul 18, 2023
22b66de
Merge branch 'main' into fix-stealth-sudo-paths
maxb Jul 19, 2023
bbe23bd
As discussed in PR, delete test which is just testing equality of a c…
maxb Jul 19, 2023
c1b1fb6
Restore sudo check as requested, and add comment
maxb Jul 19, 2023
651bbae
Update vault/token_store.go
maxb Jul 19, 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
8 changes: 4 additions & 4 deletions api/sudo_paths.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import (
// path matches that path or not (useful specifically for the paths that
// contain templated fields.)
var sudoPaths = map[string]*regexp.Regexp{
"/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`),
// TODO /auth/token/revoke-orphan requires sudo but isn't represented as such in the OpenAPI spec
"/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`),
"/auth/token/revoke-orphan": regexp.MustCompile(`^/auth/token/revoke-orphan$`),
"/pki/root": regexp.MustCompile(`^/pki/root$`),
"/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`),
"/sys/audit": regexp.MustCompile(`^/sys/audit$`),
Expand Down Expand Up @@ -45,8 +45,8 @@ var sudoPaths = map[string]*regexp.Regexp{
"/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`),
"/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`),
"/sys/rotate": regexp.MustCompile(`^/sys/rotate$`),
// TODO /sys/seal requires sudo but isn't represented as such in the OpenAPI spec
// TODO /sys/step-down requires sudo but isn't represented as such in the OpenAPI spec
"/sys/seal": regexp.MustCompile(`^/sys/seal$`),
"/sys/step-down": regexp.MustCompile(`^/sys/step-down$`),

// enterprise-only paths
"/sys/replication/dr/primary/secondary-token": regexp.MustCompile(`^/sys/replication/dr/primary/secondary-token$`),
Expand Down
3 changes: 3 additions & 0 deletions changelog/21772.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
core: Fix OpenAPI representation and `-output-policy` recognition of some non-standard sudo paths
```
5 changes: 5 additions & 0 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ func NewSystemBackend(core *Core, logger log.Logger) *SystemBackend {
"storage/raft/snapshot-auto/config/*",
"leases",
"internal/inspect/*",
// sys/seal and sys/step-down actually have their sudo requirement enforced through hardcoding
// PolicyCheckOpts.RootPrivsRequired in dedicated calls to Core.performPolicyChecks, but we still need
// to declare them here so that the generated OpenAPI spec gets their sudo status correct.
"seal",
"step-down",
},

Unauthenticated: []string{
Expand Down
36 changes: 0 additions & 36 deletions vault/logical_system_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,42 +41,6 @@ import (
"github.com/mitchellh/mapstructure"
)

func TestSystemBackend_RootPaths(t *testing.T) {
expected := []string{
"auth/*",
"remount",
"audit",
"audit/*",
"raw",
"raw/*",
"replication/primary/secondary-token",
"replication/performance/primary/secondary-token",
"replication/dr/primary/secondary-token",
"replication/reindex",
"replication/dr/reindex",
"replication/performance/reindex",
"rotate",
"config/cors",
"config/auditing/*",
"config/ui/headers/*",
"plugins/catalog/*",
"revoke-prefix/*",
"revoke-force/*",
"leases/revoke-prefix/*",
"leases/revoke-force/*",
"leases/lookup/*",
"storage/raft/snapshot-auto/config/*",
"leases",
"internal/inspect/*",
}

b := testSystemBackend(t)
actual := b.SpecialPaths().Root
if !reflect.DeepEqual(actual, expected) {
t.Fatalf("bad: mismatch\nexpected:\n%#v\ngot:\n%#v", expected, actual)
}
}

func TestSystemConfigCORS(t *testing.T) {
b := testSystemBackend(t)
paths := b.(*SystemBackend).configPaths()
Expand Down
10 changes: 6 additions & 4 deletions vault/token_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,8 +783,8 @@ func NewTokenStore(ctx context.Context, logger log.Logger, core *Core, config *l

PathsSpecial: &logical.Paths{
Root: []string{
"revoke-orphan/*",
"accessors*",
"revoke-orphan",
"accessors/",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer:

The prior definition of revoke-orphan/* is just plain wrong, so it is fixed.

The prior definition of accessors* technically works, because it uses a needless wildcard - that change is just for precision.


// Most token store items are local since tokens are local, but a
Expand Down Expand Up @@ -3285,8 +3285,8 @@ func (ts *TokenStore) revokeCommon(ctx context.Context, req *logical.Request, da
return nil, nil
}

// handleRevokeOrphan handles the auth/token/revoke-orphan/id path for revocation of tokens
// in a way that leaves child tokens orphaned. Normally, using sys/revoke/leaseID will revoke
// handleRevokeOrphan handles the auth/token/revoke-orphan path for revocation of tokens
// in a way that leaves child tokens orphaned. Normally, using sys/leases/revoke/{lease_id} will revoke
// the token and all children.
func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
// Parse the id
Expand All @@ -3295,6 +3295,8 @@ func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Reque
return logical.ErrorResponse("missing token ID"), logical.ErrInvalidRequest
}

// TODO #21772 makes the sudo check below redundant, by correcting the TokenStore's PathsSpecial.Root to match this endpoint

// Check if the client token has sudo/root privileges for the requested path
isSudo := ts.System().(extendedSystemView).SudoPrivilege(ctx, req.MountPoint+req.Path, req.ClientToken)

Expand Down
13 changes: 7 additions & 6 deletions vault/token_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package vault
import (
"context"
"encoding/json"
"errors"
"fmt"
"path"
"reflect"
Expand Down Expand Up @@ -2325,12 +2326,12 @@ func TestTokenStore_HandleRequest_RevokeOrphan(t *testing.T) {
testMakeServiceTokenViaBackend(t, ts, root, "child", "60s", []string{"root", "foo"})
testMakeServiceTokenViaBackend(t, ts, "child", "sub-child", "50s", []string{"foo"})

req := logical.TestRequest(t, logical.UpdateOperation, "revoke-orphan")
req := logical.TestRequest(t, logical.UpdateOperation, "auth/token/revoke-orphan")
req.Data = map[string]interface{}{
"token": "child",
}
req.ClientToken = root
resp, err := ts.HandleRequest(namespace.RootContext(nil), req)
resp, err := c.HandleRequest(namespace.RootContext(nil), req)
if err != nil || (resp != nil && resp.IsError()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: Since the common sudo-enforcing logic is part of the Core, we need to fix tests which care about the sudo behaviour, and were previously slipping requests directly to the backend, bypassing the Core, to make the request via the proper layers.

This test is the case where the requesting token does have sudo rights, testing the "allowed" case, and the next test below is testing the "denied" case.

t.Fatalf("err: %v\nresp: %#v", err, resp)
}
Expand Down Expand Up @@ -2384,14 +2385,14 @@ func TestTokenStore_HandleRequest_RevokeOrphan_NonRoot(t *testing.T) {
t.Fatalf("bad: %v", out)
}

req := logical.TestRequest(t, logical.UpdateOperation, "revoke-orphan")
req := logical.TestRequest(t, logical.UpdateOperation, "auth/token/revoke-orphan")
req.Data = map[string]interface{}{
"token": "child",
}
req.ClientToken = "child"
resp, err := ts.HandleRequest(namespace.RootContext(nil), req)
if err != logical.ErrInvalidRequest {
t.Fatalf("did not get error when non-root revoking itself with orphan flag; resp is %#v", resp)
resp, err := c.HandleRequest(namespace.RootContext(nil), req)
if !errors.Is(err, logical.ErrPermissionDenied) {
t.Fatalf("did not get expected error when non-root revoking itself with orphan flag; resp is %#v; err is %#v", resp, err)
}

time.Sleep(200 * time.Millisecond)
Expand Down
Loading