From 3ddd7a14f03c0f7aa702ce308aff813789062bbb Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Thu, 15 Jun 2023 16:41:45 -0400 Subject: [PATCH] backport of commit c5549cdac681676ae52ea173d737ee1c5d1949a2 (#21272) Co-authored-by: Nick Cabatoff --- changelog/21260.txt | 4 ++ helper/testhelpers/teststorage/teststorage.go | 32 +++++++++++++++ vault/auth.go | 37 ++++++++++-------- vault/core.go | 4 +- vault/mount.go | 39 ++++++++++--------- vault/mount_util.go | 2 +- 6 files changed, 80 insertions(+), 38 deletions(-) create mode 100644 changelog/21260.txt diff --git a/changelog/21260.txt b/changelog/21260.txt new file mode 100644 index 000000000..b291ec7b4 --- /dev/null +++ b/changelog/21260.txt @@ -0,0 +1,4 @@ +```release-note:bug +core: Change where we evaluate filtered paths as part of mount operations; this is part of an enterprise bugfix that will +have its own changelog entry. Fix wrong lock used in ListAuths link meta interface implementation. +``` diff --git a/helper/testhelpers/teststorage/teststorage.go b/helper/testhelpers/teststorage/teststorage.go index f065e187d..edbc26e67 100644 --- a/helper/testhelpers/teststorage/teststorage.go +++ b/helper/testhelpers/teststorage/teststorage.go @@ -11,9 +11,18 @@ import ( "time" "github.com/hashicorp/go-hclog" + logicalKv "github.com/hashicorp/vault-plugin-secrets-kv" + "github.com/hashicorp/vault/audit" + auditFile "github.com/hashicorp/vault/builtin/audit/file" + auditSocket "github.com/hashicorp/vault/builtin/audit/socket" + auditSyslog "github.com/hashicorp/vault/builtin/audit/syslog" + logicalDb "github.com/hashicorp/vault/builtin/logical/database" + "github.com/hashicorp/vault/builtin/plugin" "github.com/hashicorp/vault/helper/testhelpers" + "github.com/hashicorp/vault/helper/testhelpers/corehelpers" vaulthttp "github.com/hashicorp/vault/http" "github.com/hashicorp/vault/physical/raft" + "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/physical" physFile "github.com/hashicorp/vault/sdk/physical/file" "github.com/hashicorp/vault/sdk/physical/inmem" @@ -241,5 +250,28 @@ func ClusterSetup(conf *vault.CoreConfig, opts *vault.TestClusterOptions, setup setup = InmemBackendSetup } setup(&localConf, &localOpts) + if localConf.CredentialBackends == nil { + localConf.CredentialBackends = map[string]logical.Factory{ + "plugin": plugin.Factory, + } + } + if localConf.LogicalBackends == nil { + localConf.LogicalBackends = map[string]logical.Factory{ + "plugin": plugin.Factory, + "database": logicalDb.Factory, + // This is also available in the plugin catalog, but is here due to the need to + // automatically mount it. + "kv": logicalKv.Factory, + } + } + if localConf.AuditBackends == nil { + localConf.AuditBackends = map[string]audit.Factory{ + "file": auditFile.Factory, + "socket": auditSocket.Factory, + "syslog": auditSyslog.Factory, + "noop": corehelpers.NoopAuditFactory(nil), + } + } + return &localConf, &localOpts } diff --git a/vault/auth.go b/vault/auth.go index 6047c0410..59ea4f401 100644 --- a/vault/auth.go +++ b/vault/auth.go @@ -64,16 +64,6 @@ func (c *Core) enableCredential(ctx context.Context, entry *MountEntry) error { return err } - // Re-evaluate filtered paths - if err := runFilteredPathsEvaluation(ctx, c); err != nil { - c.logger.Error("failed to evaluate filtered paths", "error", err) - - // We failed to evaluate filtered paths so we are undoing the mount operation - if disableCredentialErr := c.disableCredentialInternal(ctx, entry.Path, MountTableUpdateStorage); disableCredentialErr != nil { - c.logger.Error("failed to disable credential", "error", disableCredentialErr) - } - return err - } return nil } @@ -89,8 +79,13 @@ func (c *Core) enableCredentialInternal(ctx context.Context, entry *MountEntry, return fmt.Errorf("backend path must be specified") } + c.mountsLock.Lock() c.authLock.Lock() - defer c.authLock.Unlock() + unlock := func() { + c.authLock.Unlock() + c.mountsLock.Unlock() + } + defer unlock() ns, err := namespace.FromContext(ctx) if err != nil { @@ -224,6 +219,19 @@ func (c *Core) enableCredentialInternal(ctx context.Context, entry *MountEntry, return err } + // Re-evaluate filtered paths + if err := runFilteredPathsEvaluation(ctx, c, false); err != nil { + c.logger.Error("failed to evaluate filtered paths", "error", err) + + unlock() + unlock = func() {} + // We failed to evaluate filtered paths so we are undoing the mount operation + if disableCredentialErr := c.disableCredentialInternal(ctx, entry.Path, MountTableUpdateStorage); disableCredentialErr != nil { + c.logger.Error("failed to disable credential", "error", disableCredentialErr) + } + return err + } + if !nilMount { // restore the original readOnlyErr, so we can write to the view in // Initialize() if necessary @@ -259,7 +267,7 @@ func (c *Core) disableCredential(ctx context.Context, path string) error { } // Re-evaluate filtered paths - if err := runFilteredPathsEvaluation(ctx, c); err != nil { + if err := runFilteredPathsEvaluation(ctx, c, true); err != nil { // Even we failed to evaluate filtered paths, the unmount operation was still successful c.logger.Error("failed to evaluate filtered paths", "error", err) } @@ -526,11 +534,6 @@ func (c *Core) remountCredEntryForceInternal(ctx context.Context, path string, u return err } - // Re-evaluate filtered paths - if err := runFilteredPathsEvaluation(ctx, c); err != nil { - c.logger.Error("failed to evaluate filtered paths", "error", err) - return err - } return nil } diff --git a/vault/core.go b/vault/core.go index 511e5e3f9..11427e837 100644 --- a/vault/core.go +++ b/vault/core.go @@ -3866,8 +3866,8 @@ func (c *Core) ListAuths() ([]*MountEntry, error) { return nil, fmt.Errorf("vault is sealed") } - c.mountsLock.RLock() - defer c.mountsLock.RUnlock() + c.authLock.RLock() + defer c.authLock.RUnlock() var entries []*MountEntry diff --git a/vault/mount.go b/vault/mount.go index 4df50c070..ff7bca06d 100644 --- a/vault/mount.go +++ b/vault/mount.go @@ -567,23 +567,17 @@ func (c *Core) mount(ctx context.Context, entry *MountEntry) error { return err } - // Re-evaluate filtered paths - if err := runFilteredPathsEvaluation(ctx, c); err != nil { - c.logger.Error("failed to evaluate filtered paths", "error", err) - - // We failed to evaluate filtered paths so we are undoing the mount operation - if unmountInternalErr := c.unmountInternal(ctx, entry.Path, MountTableUpdateStorage); unmountInternalErr != nil { - c.logger.Error("failed to unmount", "error", unmountInternalErr) - } - return err - } - return nil } func (c *Core) mountInternal(ctx context.Context, entry *MountEntry, updateStorage bool) error { c.mountsLock.Lock() - defer c.mountsLock.Unlock() + c.authLock.Lock() + unlock := func() { + c.authLock.Unlock() + c.mountsLock.Unlock() + } + defer unlock() ns, err := namespace.FromContext(ctx) if err != nil { @@ -666,6 +660,7 @@ func (c *Core) mountInternal(ctx context.Context, entry *MountEntry, updateStora if err != nil { return err } + origReadOnlyErr := view.getReadOnlyErr() // Mark the view as read-only until the mounting is complete and @@ -734,6 +729,19 @@ func (c *Core) mountInternal(ctx context.Context, entry *MountEntry, updateStora return err } + // Re-evaluate filtered paths + if err := runFilteredPathsEvaluation(ctx, c, false); err != nil { + c.logger.Error("failed to evaluate filtered paths", "error", err) + + unlock() + unlock = func() {} + // We failed to evaluate filtered paths so we are undoing the mount operation + if unmountInternalErr := c.unmountInternal(ctx, entry.Path, MountTableUpdateStorage); unmountInternalErr != nil { + c.logger.Error("failed to unmount", "error", unmountInternalErr) + } + return err + } + if !nilMount { // restore the original readOnlyErr, so we can write to the view in // Initialize() if necessary @@ -813,7 +821,7 @@ func (c *Core) unmount(ctx context.Context, path string) error { } // Re-evaluate filtered paths - if err := runFilteredPathsEvaluation(ctx, c); err != nil { + if err := runFilteredPathsEvaluation(ctx, c, true); err != nil { // Even we failed to evaluate filtered paths, the unmount operation was still successful c.logger.Error("failed to evaluate filtered paths", "error", err) } @@ -1062,11 +1070,6 @@ func (c *Core) remountForceInternal(ctx context.Context, path string, updateStor return err } - // Re-evaluate filtered paths - if err := runFilteredPathsEvaluation(ctx, c); err != nil { - c.logger.Error("failed to evaluate filtered paths", "error", err) - return err - } return nil } diff --git a/vault/mount_util.go b/vault/mount_util.go index d3261da16..ffd937b78 100644 --- a/vault/mount_util.go +++ b/vault/mount_util.go @@ -28,7 +28,7 @@ func addKnownPath(*Core, string) {} func preprocessMount(*Core, *MountEntry, *BarrierView) (bool, error) { return false, nil } func clearIgnoredPaths(context.Context, *Core, logical.Backend, string) error { return nil } func addLicenseCallback(*Core, logical.Backend) {} -func runFilteredPathsEvaluation(context.Context, *Core) error { return nil } +func runFilteredPathsEvaluation(context.Context, *Core, bool) error { return nil } // ViewPath returns storage prefix for the view func (e *MountEntry) ViewPath() string {