From 29d892982402e43348203ea1974c057524ab2da0 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Fri, 27 Oct 2023 09:59:50 -0400 Subject: [PATCH] api/seal-status: fix deadlock when namespace is set on seal-status calls (#23861) * api/seal-status: fix deadlock when namespace is set on seal-status calls * changelog --- 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 0000000000..8c4ac70380 --- /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 76cf3ac7a3..684c5ab0d6 100644 --- a/command/server.go +++ b/command/server.go @@ -1532,7 +1532,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, ctx) } diff --git a/http/sys_seal.go b/http/sys_seal.go index 4448d9b2c0..14bfa41b00 100644 --- a/http/sys_seal.go +++ b/http/sys_seal.go @@ -176,7 +176,7 @@ func handleSysSealBackendStatus(core *vault.Core) http.Handler { func handleSysSealStatusRaw(core *vault.Core, w http.ResponseWriter, opt ...ListenerConfigOption) { 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 dd9e4c223d..68661f963d 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1454,10 +1454,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, "" @@ -3128,22 +3132,30 @@ func (c *Core) unsealKeyToRootKey(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 d52ae907f7..14e51ad3fa 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) } } @@ -600,7 +600,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) } @@ -612,7 +612,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 3c9166c906..d2f52c07b2 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 2464ff0b95..3760622302 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 01c7ba8728..8a76bb3400 100644 --- a/vault/init.go +++ b/vault/init.go @@ -445,7 +445,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 b5fa6c692d..ca514c647d 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -4969,7 +4969,7 @@ type SealBackendStatusResponse struct { Backends []SealBackendStatus `json:"backends"` } -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) @@ -5029,7 +5029,7 @@ func (core *Core) GetSealStatus(ctx context.Context) (*SealStatusResponse, error sealType = core.seal.BarrierSealConfigType().String() } - progress, nonce := core.SecretProgress() + progress, nonce := core.SecretProgress(lock) s := &SealStatusResponse{ Type: sealType, @@ -5041,7 +5041,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(), @@ -5140,7 +5140,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 }