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.

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

Merged
merged 9 commits into from Apr 26, 2022
3 changes: 3 additions & 0 deletions changelog/15067.txt
@@ -0,0 +1,3 @@
```release-note:bug
core: pre-calculate namespace specific paths when tainting a route during postUnseal
```
5 changes: 5 additions & 0 deletions helper/namespace/namespace.go
Expand Up @@ -3,6 +3,7 @@ package namespace
import (
"context"
"errors"
"fmt"
"strings"

"github.com/hashicorp/vault/sdk/helper/consts"
Expand All @@ -15,6 +16,10 @@ type Namespace struct {
Path string `json:"path"`
}

func (n *Namespace) String() string {
return fmt.Sprintf("ID: %s. Path: %s", n.ID, n.Path)
}

const (
RootNamespaceID = "root"
)
Expand Down
9 changes: 6 additions & 3 deletions vault/auth.go
Expand Up @@ -7,7 +7,7 @@ import (
"strings"

"github.com/hashicorp/go-secure-stdlib/strutil"
uuid "github.com/hashicorp/go-uuid"
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/vault/builtin/plugin"
"github.com/hashicorp/vault/helper/namespace"
"github.com/hashicorp/vault/sdk/helper/consts"
Expand Down Expand Up @@ -825,16 +825,19 @@ func (c *Core) setupCredentials(ctx context.Context) error {
path := credentialRoutePrefix + entry.Path
err = c.router.Mount(backend, path, entry, view)
if err != nil {
c.logger.Error("failed to mount auth entry", "path", entry.Path, "error", err)
c.logger.Error("failed to mount auth entry", "path", entry.Path, "namespace", entry.Namespace(), "error", err)
return errLoadAuthFailed
}

if c.logger.IsInfo() {
c.logger.Info("successfully enabled credential backend", "type", entry.Type, "path", entry.Path)
c.logger.Info("successfully enabled credential backend", "type", entry.Type, "path", entry.Path, "namespace", entry.Namespace())
}

// Ensure the path is tainted if set in the mount table
if entry.Tainted {
// Calculate any namespace prefixes here, because when Taint() is called, there won't be
// a namespace to pull from the context. This is similar to what we do above in c.router.Mount().
path = entry.Namespace().Path + path
c.router.Taint(ctx, path)
Copy link
Contributor Author

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.

}

Expand Down
3 changes: 1 addition & 2 deletions vault/rollback_test.go
Expand Up @@ -7,8 +7,7 @@ import (
"time"

log "github.com/hashicorp/go-hclog"
uuid "github.com/hashicorp/go-uuid"

"github.com/hashicorp/go-uuid"
"github.com/hashicorp/vault/helper/namespace"
"github.com/hashicorp/vault/sdk/helper/logging"
)
Expand Down
13 changes: 10 additions & 3 deletions vault/router.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 NewRouter(). That made lots of tests that exercise the router fail with nil pointer exceptions, due to the new logging lines. The actual logger for the router gets assigned here, as part of CreateCore() but it felt like a good idea to have a default assigned to the router from the get-go. Does this seem acceptable as is, or should this default be a more legit logger?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with this.

}
return r
}
Expand Down Expand Up @@ -522,14 +524,17 @@ 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)
r.logger.Trace("trying to route to mount using adjusted path", "namespace", ns, "adjustedPath", adjustedPath, "mount", mount, "raw", raw, "ok", ok)
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
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)
r.logger.Trace("after appending / to adjustedPath, trying again to route to mount using adjusted path", "namespace", ns, "adjustedPath", adjustedPath, "mount", mount, "raw", raw, "ok", ok)
raskchanky marked this conversation as resolved.
Show resolved Hide resolved
}
r.l.RUnlock()
if !ok {
r.logger.Trace("route entry not found", "path", req.Path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could also result in a very high log volume. Can you use a metric instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up removing this in favor of augmented error messages. I could add additional metrics as well, but after thinking this over a bit, I wasn't sure what kind of metric to add. Are you thinking of a counter each time this failure happens? Or something else?

return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath
}
req.Path = adjustedPath
Expand All @@ -551,6 +556,7 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc

// Filtered mounts will have a nil backend
if re.backend == nil {
r.logger.Trace("route entry found, but backend is nil", "path", req.Path)
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath
}

Expand All @@ -560,6 +566,7 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
switch req.Operation {
case logical.RevokeOperation, logical.RollbackOperation:
default:
r.logger.Trace("route entry is tainted, rejecting operations to it that aren't revoke or rollback", "path", req.Path)
return logical.ErrorResponse(fmt.Sprintf("no handler for route '%s'", req.Path)), false, false, logical.ErrUnsupportedPath
}
}
Expand Down
2 changes: 1 addition & 1 deletion vault/router_test.go
Expand Up @@ -5,7 +5,7 @@ import (
"strings"
"testing"

uuid "github.com/hashicorp/go-uuid"
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/vault/helper/namespace"
"github.com/hashicorp/vault/sdk/logical"
)
Expand Down