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.
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
When tainting a route during setup, pre-calculate the namespace specific path #15067
Changes from 6 commits
b7b4937
1323b52
83dce64
2a39144
825ce15
e536eb8
ed52180
d33c3fc
7b9a7e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:bug | ||
core: pre-calculate namespace specific paths when tainting a route during postUnseal | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,9 @@ import ( | |
"sync/atomic" | ||
"time" | ||
|
||
metrics "github.com/armon/go-metrics" | ||
radix "github.com/armon/go-radix" | ||
hclog "github.com/hashicorp/go-hclog" | ||
"github.com/armon/go-metrics" | ||
"github.com/armon/go-radix" | ||
"github.com/hashicorp/go-hclog" | ||
"github.com/hashicorp/go-secure-stdlib/strutil" | ||
"github.com/hashicorp/vault/helper/namespace" | ||
"github.com/hashicorp/vault/sdk/helper/consts" | ||
|
@@ -47,6 +47,8 @@ func NewRouter() *Router { | |
storagePrefix: radix.New(), | ||
mountUUIDCache: radix.New(), | ||
mountAccessorCache: radix.New(), | ||
// this will get replaced in production with a real logger but it's useful to have a default in place for tests | ||
logger: hclog.NewNullLogger(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The router has a logger, but before this PR, it didn't get assigned as part of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with this. |
||
} | ||
return r | ||
} | ||
|
@@ -522,15 +524,27 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc | |
r.l.RLock() | ||
adjustedPath := req.Path | ||
mount, raw, ok := r.root.LongestPrefix(ns.Path + adjustedPath) | ||
if r.logger.IsTrace() { | ||
raskchanky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
r.logger.Trace("trying to route to mount using adjusted path", "namespace", ns, "adjustedPath", adjustedPath) | ||
if !ok { | ||
r.logger.Trace("prefix search returned", "mount", mount, "raw", raw) | ||
} | ||
} | ||
if !ok && !strings.HasSuffix(adjustedPath, "/") { | ||
// Re-check for a backend by appending a slash. This lets "foo" mean | ||
// "foo/" at the root level which is almost always what we want. | ||
adjustedPath += "/" | ||
mount, raw, ok = r.root.LongestPrefix(ns.Path + adjustedPath) | ||
if r.logger.IsTrace() { | ||
r.logger.Trace("after appending / to adjustedPath, trying again to route to mount using adjusted path", "namespace", ns, "adjustedPath", adjustedPath) | ||
if !ok { | ||
r.logger.Trace("after appending / to adjustedPath, prefix search returned", "mount", mount, "raw", raw) | ||
} | ||
} | ||
} | ||
r.l.RUnlock() | ||
if !ok { | ||
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath | ||
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s', route entry not found", req.Path)), false, false, logical.ErrUnsupportedPath | ||
} | ||
req.Path = adjustedPath | ||
defer metrics.MeasureSince([]string{ | ||
|
@@ -551,7 +565,10 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc | |
|
||
// Filtered mounts will have a nil backend | ||
if re.backend == nil { | ||
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath | ||
if r.logger.IsTrace() { | ||
r.logger.Trace("route entry found, but backend is nil") | ||
} | ||
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s', nil backend", req.Path)), false, false, logical.ErrUnsupportedPath | ||
} | ||
|
||
// If the path is tainted, we reject any operation except for | ||
|
@@ -560,7 +577,10 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc | |
switch req.Operation { | ||
case logical.RevokeOperation, logical.RollbackOperation: | ||
default: | ||
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath | ||
if r.logger.IsTrace() { | ||
r.logger.Trace("route entry is tainted, rejecting operations to it that aren't revoke or rollback") | ||
} | ||
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s', entry tainted", req.Path)), false, false, logical.ErrUnsupportedPath | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is called as part of postUnseal(), as best as I can tell, we're passing an empty context all the way through these methods, originating here: https://github.com/hashicorp/vault/blob/main/vault/core.go#L1693 That means the namespace specific path that we need to add to the path won't exist in the context, as Taint() expects it to. Instead, we use the namespace specific path from the mount entry.