From bc19a6d305d90a13a24f2bc512a6076269435376 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, 27 Oct 2023 10:47:12 -0400 Subject: [PATCH] api/seal-status: fix deadlock when namespace is set on seal-status calls (#23861) (#23879) * api/seal-status: fix deadlock when namespace is set on seal-status calls * changelog Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> --- changelog/23861.txt | 4 ++ command/server.go | 2 +- http/sys_seal.go | 2 +- vault/core.go | 38 ++++++++++++------- vault/core_test.go | 10 ++--- .../capabilities/node_status/node_status.go | 2 +- vault/hcp_link/internal/wrapped_hcpLink.go | 2 +- vault/init.go | 2 +- vault/logical_system.go | 8 ++-- 9 files changed, 43 insertions(+), 27 deletions(-) create mode 100644 changelog/23861.txt diff --git a/changelog/23861.txt b/changelog/23861.txt new file mode 100644 index 000000000..8c4ac7038 --- /dev/null +++ b/changelog/23861.txt @@ -0,0 +1,4 @@ +```release-note:bug +api/seal-status: Fix deadlock on calls to sys/seal-status with a namespace configured +on the request. +``` diff --git a/command/server.go b/command/server.go index 73975e233..74babc68f 100644 --- a/command/server.go +++ b/command/server.go @@ -1492,7 +1492,7 @@ func (c *ServerCommand) Run(args []string) int { // Vault cluster with multiple servers is configured with auto-unseal but is // uninitialized. Once one server initializes the storage backend, this // goroutine will pick up the unseal keys and unseal this instance. - if !core.IsInSealMigrationMode() { + if !core.IsInSealMigrationMode(true) { go runUnseal(c, core, context.Background()) } diff --git a/http/sys_seal.go b/http/sys_seal.go index 5d32828e7..c4f4c4f2d 100644 --- a/http/sys_seal.go +++ b/http/sys_seal.go @@ -165,7 +165,7 @@ func handleSysSealStatus(core *vault.Core) http.Handler { func handleSysSealStatusRaw(core *vault.Core, w http.ResponseWriter, r *http.Request) { ctx := context.Background() - status, err := core.GetSealStatus(ctx) + status, err := core.GetSealStatus(ctx, true) if err != nil { respondError(w, http.StatusInternalServerError, err) return diff --git a/vault/core.go b/vault/core.go index 7392c1c37..18cb720a8 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1466,10 +1466,14 @@ func (c *Core) Sealed() bool { return atomic.LoadUint32(c.sealed) == 1 } -// SecretProgress returns the number of keys provided so far -func (c *Core) SecretProgress() (int, string) { - c.stateLock.RLock() - defer c.stateLock.RUnlock() +// SecretProgress returns the number of keys provided so far. Lock +// should only be false if the caller is already holding the read +// statelock (such as calls originating from switchedLockHandleRequest). +func (c *Core) SecretProgress(lock bool) (int, string) { + if lock { + c.stateLock.RLock() + defer c.stateLock.RUnlock() + } switch c.unlockInfo { case nil: return 0, "" @@ -3073,22 +3077,30 @@ func (c *Core) unsealKeyToMasterKey(ctx context.Context, seal Seal, combinedKey // configuration in storage is Shamir but the seal in HCL is not. In this // mode we should not auto-unseal (even if the migration is done) and we will // accept unseal requests with and without the `migrate` option, though the migrate -// option is required if we haven't yet performed the seal migration. -func (c *Core) IsInSealMigrationMode() bool { - c.stateLock.RLock() - defer c.stateLock.RUnlock() +// option is required if we haven't yet performed the seal migration. Lock +// should only be false if the caller is already holding the read +// statelock (such as calls originating from switchedLockHandleRequest). +func (c *Core) IsInSealMigrationMode(lock bool) bool { + if lock { + c.stateLock.RLock() + defer c.stateLock.RUnlock() + } return c.migrationInfo != nil } // IsSealMigrated returns true if we're in seal migration mode but migration // has already been performed (possibly by another node, or prior to this node's -// current invocation.) -func (c *Core) IsSealMigrated() bool { - if !c.IsInSealMigrationMode() { +// current invocation). Lock should only be false if the caller is already +// holding the read statelock (such as calls originating from switchedLockHandleRequest). +func (c *Core) IsSealMigrated(lock bool) bool { + if !c.IsInSealMigrationMode(lock) { return false } - c.stateLock.RLock() - defer c.stateLock.RUnlock() + + if lock { + c.stateLock.RLock() + defer c.stateLock.RUnlock() + } done, _ := c.sealMigrated(context.Background()) return done } diff --git a/vault/core_test.go b/vault/core_test.go index e3fad5409..bdac8dff1 100644 --- a/vault/core_test.go +++ b/vault/core_test.go @@ -413,7 +413,7 @@ func TestCore_Unseal_MultiShare(t *testing.T) { t.Fatalf("should be sealed") } - if prog, _ := c.SecretProgress(); prog != 0 { + if prog, _ := c.SecretProgress(true); prog != 0 { t.Fatalf("bad progress: %d", prog) } @@ -432,14 +432,14 @@ func TestCore_Unseal_MultiShare(t *testing.T) { if !unseal { t.Fatalf("should be unsealed") } - if prog, _ := c.SecretProgress(); prog != 0 { + if prog, _ := c.SecretProgress(true); prog != 0 { t.Fatalf("bad progress: %d", prog) } } else { if unseal { t.Fatalf("should not be unsealed") } - if prog, _ := c.SecretProgress(); prog != i+1 { + if prog, _ := c.SecretProgress(true); prog != i+1 { t.Fatalf("bad progress: %d", prog) } } @@ -592,7 +592,7 @@ func TestCore_Unseal_Single(t *testing.T) { t.Fatalf("should be sealed") } - if prog, _ := c.SecretProgress(); prog != 0 { + if prog, _ := c.SecretProgress(true); prog != 0 { t.Fatalf("bad progress: %d", prog) } @@ -604,7 +604,7 @@ func TestCore_Unseal_Single(t *testing.T) { if !unseal { t.Fatalf("should be unsealed") } - if prog, _ := c.SecretProgress(); prog != 0 { + if prog, _ := c.SecretProgress(true); prog != 0 { t.Fatalf("bad progress: %d", prog) } diff --git a/vault/hcp_link/capabilities/node_status/node_status.go b/vault/hcp_link/capabilities/node_status/node_status.go index d86f041d1..2231fe5ee 100644 --- a/vault/hcp_link/capabilities/node_status/node_status.go +++ b/vault/hcp_link/capabilities/node_status/node_status.go @@ -33,7 +33,7 @@ func (c *NodeStatusReporter) GetNodeStatus(ctx context.Context) (retStatus nodes var status nodestatus.NodeStatus - sealStatus, err := c.NodeStatusGetter.GetSealStatus(ctx) + sealStatus, err := c.NodeStatusGetter.GetSealStatus(ctx, true) if err != nil { return status, err } diff --git a/vault/hcp_link/internal/wrapped_hcpLink.go b/vault/hcp_link/internal/wrapped_hcpLink.go index ea9941f01..c348724b1 100644 --- a/vault/hcp_link/internal/wrapped_hcpLink.go +++ b/vault/hcp_link/internal/wrapped_hcpLink.go @@ -16,7 +16,7 @@ import ( type WrappedCoreNodeStatus interface { ActiveTime() time.Time - GetSealStatus(ctx context.Context) (*vault.SealStatusResponse, error) + GetSealStatus(ctx context.Context, lock bool) (*vault.SealStatusResponse, error) IsRaftVoter() bool ListenerAddresses() ([]string, error) LogLevel() string diff --git a/vault/init.go b/vault/init.go index ba537c202..f65db6daa 100644 --- a/vault/init.go +++ b/vault/init.go @@ -451,7 +451,7 @@ func (c *Core) UnsealWithStoredKeys(ctx context.Context) error { } // Disallow auto-unsealing when migrating - if c.IsInSealMigrationMode() && !c.IsSealMigrated() { + if c.IsInSealMigrationMode(true) && !c.IsSealMigrated(true) { return NewNonFatalError(errors.New("cannot auto-unseal during seal migration")) } diff --git a/vault/logical_system.go b/vault/logical_system.go index 7af5741c1..640ce1b30 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -4710,7 +4710,7 @@ type SealStatusResponse struct { Warnings []string `json:"warnings,omitempty"` } -func (core *Core) GetSealStatus(ctx context.Context) (*SealStatusResponse, error) { +func (core *Core) GetSealStatus(ctx context.Context, lock bool) (*SealStatusResponse, error) { sealed := core.Sealed() initialized, err := core.Initialized(ctx) @@ -4763,7 +4763,7 @@ func (core *Core) GetSealStatus(ctx context.Context) (*SealStatusResponse, error clusterID = cluster.ID } - progress, nonce := core.SecretProgress() + progress, nonce := core.SecretProgress(lock) s := &SealStatusResponse{ Type: sealConfig.Type, @@ -4775,7 +4775,7 @@ func (core *Core) GetSealStatus(ctx context.Context) (*SealStatusResponse, error Nonce: nonce, Version: version.GetVersion().VersionNumber(), BuildDate: version.BuildDate, - Migration: core.IsInSealMigrationMode() && !core.IsSealMigrated(), + Migration: core.IsInSealMigrationMode(lock) && !core.IsSealMigrated(lock), ClusterName: clusterName, ClusterID: clusterID, RecoverySeal: core.SealAccess().RecoveryKeySupported(), @@ -4837,7 +4837,7 @@ func (core *Core) GetLeaderStatus() (*LeaderResponse, error) { } func (b *SystemBackend) handleSealStatus(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - status, err := b.Core.GetSealStatus(ctx) + status, err := b.Core.GetSealStatus(ctx, false) if err != nil { return nil, err }