From 65f8e67ce89f8cab2ff39f59c28d12ac130882e6 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: Fri, 16 Jun 2023 12:32:51 -0400 Subject: [PATCH] backport of commit c5549cdac681676ae52ea173d737ee1c5d1949a2 (#21271) 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 0000000000..b291ec7b4b --- /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 7a2f220ed7..3b2881a7fc 100644 --- a/helper/testhelpers/teststorage/teststorage.go +++ b/helper/testhelpers/teststorage/teststorage.go @@ -8,9 +8,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" @@ -238,5 +247,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 6200e03a6d..f3692a5c9d 100644 --- a/vault/auth.go +++ b/vault/auth.go @@ -61,16 +61,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 } @@ -86,8 +76,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 { @@ -221,6 +216,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 @@ -256,7 +264,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) } @@ -523,11 +531,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 78f3a3da2e..79888e3ea1 100644 --- a/vault/core.go +++ b/vault/core.go @@ -3854,8 +3854,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 3985f618ed..b4f817f86f 100644 --- a/vault/mount.go +++ b/vault/mount.go @@ -545,23 +545,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 { @@ -644,6 +638,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 @@ -712,6 +707,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 @@ -791,7 +799,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) } @@ -1040,11 +1048,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 0a060ee78f..efb5caf641 100644 --- a/vault/mount_util.go +++ b/vault/mount_util.go @@ -25,7 +25,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 {