From 8d2d9fd8bd56033a566e7100a2ae752e84e267b4 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 24 Jul 2018 16:57:25 -0400 Subject: [PATCH] Tackle #4929 a different way (#4932) * Tackle #4929 a different way This turns c.sealed into an atomic, which allows us to call sealInternal without a lock. By doing so we can better control lock grabbing when a condition causing the standby loop to get out of active happens. This encapsulates that logic into two distinct pieces (although they could be combined into one), and makes lock guarding more understandable. * Re-add context canceling to the non-HA version of sealInternal * Return explicitly after stopCh triggered --- command/server.go | 9 +-- http/sys_health.go | 2 +- http/sys_init_test.go | 6 +- http/sys_seal.go | 14 +---- http/sys_seal_test.go | 12 +--- vault/cluster_test.go | 6 +- vault/core.go | 84 +++++++++++-------------- vault/core_test.go | 90 ++++++--------------------- vault/generate_root.go | 10 +-- vault/ha.go | 89 +++++++++++++------------- vault/identity_store_entities_test.go | 18 +----- vault/init.go | 6 +- vault/logical_system_integ_test.go | 30 ++------- vault/rekey.go | 24 +++---- vault/rekey_test.go | 2 +- vault/request_handling.go | 2 +- vault/seal_testing.go | 14 +---- vault/testing.go | 36 ++--------- vault/token_store.go | 8 ++- vault/wrapping.go | 7 ++- 20 files changed, 156 insertions(+), 313 deletions(-) diff --git a/command/server.go b/command/server.go index 0aab187f04..b29b208e84 100644 --- a/command/server.go +++ b/command/server.go @@ -837,14 +837,7 @@ CLUSTER_SYNTHESIS_COMPLETE: return false } - sealedFunc := func() bool { - if sealed, err := core.Sealed(); err == nil { - return sealed - } - return true - } - - if err := sd.RunServiceDiscovery(c.WaitGroup, c.ShutdownCh, coreConfig.RedirectAddr, activeFunc, sealedFunc); err != nil { + if err := sd.RunServiceDiscovery(c.WaitGroup, c.ShutdownCh, coreConfig.RedirectAddr, activeFunc, core.Sealed); err != nil { c.UI.Error(fmt.Sprintf("Error initializing service discovery: %v", err)) return 1 } diff --git a/http/sys_health.go b/http/sys_health.go index be67f2fa29..6bde8b068f 100644 --- a/http/sys_health.go +++ b/http/sys_health.go @@ -114,7 +114,7 @@ func getSysHealth(core *vault.Core, r *http.Request) (int, *HealthResponse, erro ctx := context.Background() // Check system status - sealed, _ := core.Sealed() + sealed := core.Sealed() standby, _ := core.Standby() var replicationState consts.ReplicationState if standby { diff --git a/http/sys_init_test.go b/http/sys_init_test.go index 205f54fd13..f4be3413a9 100644 --- a/http/sys_init_test.go +++ b/http/sys_init_test.go @@ -119,11 +119,7 @@ func TestSysInit_put(t *testing.T) { } } - seal, err := core.Sealed() - if err != nil { - t.Fatalf("err: %s", err) - } - if seal { + if core.Sealed() { t.Fatal("should not be sealed") } } diff --git a/http/sys_seal.go b/http/sys_seal.go index d86d7f2d06..2eebb95cd1 100644 --- a/http/sys_seal.go +++ b/http/sys_seal.go @@ -95,12 +95,7 @@ func handleSysUnseal(core *vault.Core) http.Handler { } if req.Reset { - sealed, err := core.Sealed() - if err != nil { - respondError(w, http.StatusInternalServerError, err) - return - } - if !sealed { + if !core.Sealed() { respondError(w, http.StatusBadRequest, errors.New("vault is unsealed")) return } @@ -164,13 +159,10 @@ func handleSysSealStatus(core *vault.Core) http.Handler { func handleSysSealStatusRaw(core *vault.Core, w http.ResponseWriter, r *http.Request) { ctx := context.Background() - sealed, err := core.Sealed() - if err != nil { - respondError(w, http.StatusInternalServerError, err) - return - } + sealed := core.Sealed() var sealConfig *vault.SealConfig + var err error if core.SealAccess().RecoveryKeySupported() { sealConfig, err = core.SealAccess().RecoveryConfig(ctx) } else { diff --git a/http/sys_seal_test.go b/http/sys_seal_test.go index 902466ee4d..68c9b53075 100644 --- a/http/sys_seal_test.go +++ b/http/sys_seal_test.go @@ -75,11 +75,7 @@ func TestSysSeal(t *testing.T) { resp := testHttpPut(t, token, addr+"/v1/sys/seal", nil) testResponseStatus(t, resp, 204) - check, err := core.Sealed() - if err != nil { - t.Fatalf("err: %s", err) - } - if !check { + if !core.Sealed() { t.Fatal("should be sealed") } } @@ -93,11 +89,7 @@ func TestSysSeal_unsealed(t *testing.T) { resp := testHttpPut(t, token, addr+"/v1/sys/seal", nil) testResponseStatus(t, resp, 204) - check, err := core.Sealed() - if err != nil { - t.Fatalf("err: %s", err) - } - if !check { + if !core.Sealed() { t.Fatal("should be sealed") } } diff --git a/vault/cluster_test.go b/vault/cluster_test.go index 282c3701b3..2b5b661efb 100644 --- a/vault/cluster_test.go +++ b/vault/cluster_test.go @@ -70,11 +70,7 @@ func TestClusterHAFetching(t *testing.T) { } // Verify unsealed - sealed, err := c.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if c.Sealed() { t.Fatal("should not be sealed") } diff --git a/vault/core.go b/vault/core.go index cab0cb373c..33515d5942 100644 --- a/vault/core.go +++ b/vault/core.go @@ -185,7 +185,7 @@ type Core struct { // stateLock protects mutable state stateLock sync.RWMutex - sealed bool + sealed *uint32 standby bool standbyDoneCh chan struct{} @@ -480,7 +480,7 @@ func NewCore(conf *CoreConfig) (*Core, error) { clusterAddr: conf.ClusterAddr, seal: conf.Seal, router: NewRouter(), - sealed: true, + sealed: new(uint32), standby: true, logger: conf.Logger.Named("core"), defaultLeaseTTL: conf.DefaultLeaseTTL, @@ -503,6 +503,8 @@ func NewCore(conf *CoreConfig) (*Core, error) { keepHALockOnStepDown: new(uint32), } + atomic.StoreUint32(c.sealed, 1) + atomic.StoreUint32(c.replicationState, uint32(consts.ReplicationDRDisabled|consts.ReplicationPerformanceDisabled)) c.localClusterCert.Store(([]byte)(nil)) c.localClusterParsedCert.Store((*x509.Certificate)(nil)) @@ -634,19 +636,6 @@ func NewCore(conf *CoreConfig) (*Core, error) { // happens as quickly as possible. func (c *Core) Shutdown() error { c.logger.Debug("shutdown called") - c.stateLock.RLock() - // Tell any requests that know about this to stop - if c.activeContextCancelFunc != nil { - c.activeContextCancelFunc() - } - c.stateLock.RUnlock() - - c.logger.Debug("shutdown initiating internal seal") - // Seal the Vault, causes a leader stepdown - c.stateLock.Lock() - defer c.stateLock.Unlock() - - c.logger.Debug("shutdown running internal seal") return c.sealInternal(false) } @@ -663,10 +652,8 @@ func (c *Core) GetContext() (context.Context, context.CancelFunc) { } // Sealed checks if the Vault is current sealed -func (c *Core) Sealed() (bool, error) { - c.stateLock.RLock() - defer c.stateLock.RUnlock() - return c.sealed, nil +func (c *Core) Sealed() bool { + return atomic.LoadUint32(c.sealed) == 1 } // SecretProgress returns the number of keys provided so far @@ -686,9 +673,6 @@ func (c *Core) SecretProgress() (int, string) { func (c *Core) ResetUnsealProcess() { c.stateLock.Lock() defer c.stateLock.Unlock() - if !c.sealed { - return - } c.unlockInfo = nil } @@ -732,7 +716,7 @@ func (c *Core) Unseal(key []byte) (bool, error) { } // Check if already unsealed - if !c.sealed { + if !c.Sealed() { return true, nil } @@ -774,7 +758,7 @@ func (c *Core) UnsealWithRecoveryKeys(ctx context.Context, key []byte) (bool, er } // Check if already unsealed - if !c.sealed { + if !c.Sealed() { return true, nil } @@ -918,14 +902,14 @@ func (c *Core) unsealInternal(ctx context.Context, masterKey []byte) (bool, erro go c.runStandby(c.standbyDoneCh, c.manualStepDownCh, c.standbyStopCh) } - // Success! - c.sealed = false - // Force a cache bust here, which will also run migration code if c.seal.RecoveryKeySupported() { c.seal.SetRecoveryConfig(ctx, nil) } + // Success! + atomic.StoreUint32(c.sealed, 0) + if c.ha != nil { sd, ok := c.ha.(physical.ServiceDiscovery) if ok { @@ -944,13 +928,12 @@ func (c *Core) unsealInternal(ctx context.Context, masterKey []byte) (bool, erro func (c *Core) SealWithRequest(req *logical.Request) error { defer metrics.MeasureSince([]string{"core", "seal-with-request"}, time.Now()) - c.stateLock.RLock() - - if c.sealed { - c.stateLock.RUnlock() + if c.Sealed() { return nil } + c.stateLock.RLock() + // This will unlock the read lock // We use background context since we may not be active return c.sealInitCommon(context.Background(), req) @@ -961,13 +944,12 @@ func (c *Core) SealWithRequest(req *logical.Request) error { func (c *Core) Seal(token string) error { defer metrics.MeasureSince([]string{"core", "seal"}, time.Now()) - c.stateLock.RLock() - - if c.sealed { - c.stateLock.RUnlock() + if c.Sealed() { return nil } + c.stateLock.RLock() + req := &logical.Request{ Operation: logical.UpdateOperation, Path: "sys/seal", @@ -1094,17 +1076,9 @@ func (c *Core) sealInitCommon(ctx context.Context, req *logical.Request) (retErr } } - // Tell any requests that know about this to stop - if c.activeContextCancelFunc != nil { - c.activeContextCancelFunc() - } - - // Unlock from the request handling + // Unlock; sealing will grab the lock when needed c.stateLock.RUnlock() - //Seal the Vault - c.stateLock.Lock() - defer c.stateLock.Unlock() sealErr := c.sealInternal(false) if sealErr != nil { @@ -1125,15 +1099,13 @@ func (c *Core) UIHeaders() (http.Header, error) { } // sealInternal is an internal method used to seal the vault. It does not do -// any authorization checking. The stateLock must be held prior to calling. +// any authorization checking. func (c *Core) sealInternal(keepLock bool) error { - if c.sealed { + // Mark sealed, and if already marked return + if swapped := atomic.CompareAndSwapUint32(c.sealed, 0, 1); !swapped { return nil } - // Enable that we are sealed to prevent further transactions - c.sealed = true - c.logger.Debug("marked as sealed") // Clear forwarding clients @@ -1143,15 +1115,29 @@ func (c *Core) sealInternal(keepLock bool) error { // Do pre-seal teardown if HA is not enabled if c.ha == nil { + c.stateLock.Lock() + defer c.stateLock.Unlock() // Even in a non-HA context we key off of this for some things c.standby = true + + // Stop requests from processing + if c.activeContextCancelFunc != nil { + c.activeContextCancelFunc() + } + if err := c.preSeal(); err != nil { c.logger.Error("pre-seal teardown failed", "error", err) return fmt.Errorf("internal error") } } else { + // If we are keeping the lock we already have the state write lock + // held. Otherwise grab it here so that when stopCh is triggered we are + // locked. if keepLock { atomic.StoreUint32(c.keepHALockOnStepDown, 1) + } else { + c.stateLock.Lock() + defer c.stateLock.Unlock() } // If we are trying to acquire the lock, force it to return with nil so // runStandby will exit diff --git a/vault/core_test.go b/vault/core_test.go index ff28434745..3a31f02b66 100644 --- a/vault/core_test.go +++ b/vault/core_test.go @@ -72,11 +72,7 @@ func TestCore_Unseal_MultiShare(t *testing.T) { t.Fatalf("err: %v", err) } - sealed, err := c.Sealed() - if err != nil { - t.Fatalf("err: %v", err) - } - if !sealed { + if !c.Sealed() { t.Fatalf("should be sealed") } @@ -112,11 +108,7 @@ func TestCore_Unseal_MultiShare(t *testing.T) { } } - sealed, err = c.Sealed() - if err != nil { - t.Fatalf("err: %v", err) - } - if sealed { + if c.Sealed() { t.Fatalf("should not be sealed") } @@ -131,11 +123,7 @@ func TestCore_Unseal_MultiShare(t *testing.T) { t.Fatalf("err: %v", err) } - sealed, err = c.Sealed() - if err != nil { - t.Fatalf("err: %v", err) - } - if !sealed { + if !c.Sealed() { t.Fatalf("should be sealed") } } @@ -160,11 +148,7 @@ func TestCore_Unseal_Single(t *testing.T) { t.Fatalf("err: %v", err) } - sealed, err := c.Sealed() - if err != nil { - t.Fatalf("err: %v", err) - } - if !sealed { + if !c.Sealed() { t.Fatalf("should be sealed") } @@ -184,11 +168,7 @@ func TestCore_Unseal_Single(t *testing.T) { t.Fatalf("bad progress: %d", prog) } - sealed, err = c.Sealed() - if err != nil { - t.Fatalf("err: %v", err) - } - if sealed { + if c.Sealed() { t.Fatalf("should not be sealed") } } @@ -257,8 +237,8 @@ func TestCore_Shutdown(t *testing.T) { if err := c.Shutdown(); err != nil { t.Fatalf("err: %v", err) } - if sealed, err := c.Sealed(); err != nil || !sealed { - t.Fatalf("err: %v", err) + if !c.Sealed() { + t.Fatal("wasn't sealed") } } @@ -268,8 +248,8 @@ func TestCore_Seal_BadToken(t *testing.T) { if err := c.Seal("foo"); err == nil { t.Fatalf("err: %v", err) } - if sealed, err := c.Sealed(); err != nil || sealed { - t.Fatalf("err: %v", err) + if c.Sealed() { + t.Fatal("was sealed") } } @@ -284,8 +264,8 @@ func TestCore_Seal_SingleUse(t *testing.T) { if err := c.Seal("foo"); err != nil { t.Fatalf("err: %v", err) } - if sealed, err := c.Sealed(); err != nil || !sealed { - t.Fatalf("err: %v, sealed: %t", err, sealed) + if !c.Sealed() { + t.Fatal("not sealed") } for i, key := range keys { unseal, err := TestCoreUnseal(c, key) @@ -1146,11 +1126,7 @@ func TestCore_Standby_Seal(t *testing.T) { } // Verify unsealed - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } @@ -1187,11 +1163,7 @@ func TestCore_Standby_Seal(t *testing.T) { } // Verify unsealed - sealed, err = core2.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core2.Sealed() { t.Fatal("should not be sealed") } @@ -1264,11 +1236,7 @@ func TestCore_StepDown(t *testing.T) { } // Verify unsealed - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } @@ -1305,11 +1273,7 @@ func TestCore_StepDown(t *testing.T) { } // Verify unsealed - sealed, err = core2.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core2.Sealed() { t.Fatal("should not be sealed") } @@ -1462,11 +1426,7 @@ func TestCore_CleanLeaderPrefix(t *testing.T) { } // Verify unsealed - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } @@ -1530,11 +1490,7 @@ func TestCore_CleanLeaderPrefix(t *testing.T) { } // Verify unsealed - sealed, err = core2.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core2.Sealed() { t.Fatal("should not be sealed") } @@ -1647,11 +1603,7 @@ func testCore_Standby_Common(t *testing.T, inm physical.Backend, inmha physical. } // Verify unsealed - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } @@ -1702,11 +1654,7 @@ func testCore_Standby_Common(t *testing.T, inm physical.Backend, inmha physical. } // Verify unsealed - sealed, err = core2.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core2.Sealed() { t.Fatal("should not be sealed") } diff --git a/vault/generate_root.go b/vault/generate_root.go index 37f408e02e..9a8750a0e9 100644 --- a/vault/generate_root.go +++ b/vault/generate_root.go @@ -73,7 +73,7 @@ type GenerateRootResult struct { func (c *Core) GenerateRootProgress() (int, error) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return 0, consts.ErrSealed } if c.standby { @@ -91,7 +91,7 @@ func (c *Core) GenerateRootProgress() (int, error) { func (c *Core) GenerateRootConfiguration() (*GenerateRootConfig, error) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, consts.ErrSealed } if c.standby { @@ -141,7 +141,7 @@ func (c *Core) GenerateRootInit(otp, pgpKey string, strategy GenerateRootStrateg c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return consts.ErrSealed } if c.standby { @@ -211,7 +211,7 @@ func (c *Core) GenerateRootUpdate(ctx context.Context, key []byte, nonce string, // Ensure we are already unsealed c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, consts.ErrSealed } if c.standby { @@ -349,7 +349,7 @@ func (c *Core) GenerateRootUpdate(ctx context.Context, key []byte, nonce string, func (c *Core) GenerateRootCancel() error { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return consts.ErrSealed } if c.standby { diff --git a/vault/ha.go b/vault/ha.go index f65af4b6a9..51fc9f3682 100644 --- a/vault/ha.go +++ b/vault/ha.go @@ -37,14 +37,13 @@ func (c *Core) Leader() (isLeader bool, leaderAddr, clusterAddr string, err erro return false, "", "", ErrHANotEnabled } - c.stateLock.RLock() - // Check if sealed - if c.sealed { - c.stateLock.RUnlock() + if c.Sealed() { return false, "", "", consts.ErrSealed } + c.stateLock.RLock() + // Check if we are the leader if !c.standby { c.stateLock.RUnlock() @@ -153,7 +152,7 @@ func (c *Core) StepDown(req *logical.Request) (retErr error) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil } if c.ha == nil || c.standby { @@ -396,7 +395,7 @@ func (c *Core) waitForLeadership(doneCh, manualStepDownCh, stopCh chan struct{}) // We now have the lock and can use it } - if c.sealed { + if c.Sealed() { c.logger.Warn("grabbed HA lock but already sealed, exiting") lock.Unlock() c.stateLock.Unlock() @@ -475,42 +474,28 @@ func (c *Core) waitForLeadership(doneCh, manualStepDownCh, stopCh chan struct{}) continue } - // Monitor a loss of leadership - releaseHALock := true - grabStateLock := true - select { - case <-leaderLostCh: - c.logger.Warn("leadership lost, stopping active operation") - case <-stopCh: - // This case comes from sealInternal; we will already be having the - // state lock held so we do toggle grabStateLock to false - if atomic.LoadUint32(c.keepHALockOnStepDown) == 1 { - releaseHALock = false + runSealing := func() { + metrics.MeasureSince([]string{"core", "leadership_lost"}, activeTime) + + // Tell any requests that know about this to stop + if c.activeContextCancelFunc != nil { + c.activeContextCancelFunc() + } + + c.standby = true + + if err := c.preSeal(); err != nil { + c.logger.Error("pre-seal teardown failed", "error", err) } - grabStateLock = false - case <-manualStepDownCh: - c.logger.Warn("stepping down from active operation to standby") - manualStepDown = true } - metrics.MeasureSince([]string{"core", "leadership_lost"}, activeTime) - - // Tell any requests that know about this to stop - if c.activeContextCancelFunc != nil { - c.activeContextCancelFunc() - } - - // Attempt the pre-seal process - if grabStateLock { - c.stateLock.Lock() - } - c.standby = true - preSealErr := c.preSeal() - if grabStateLock { - c.stateLock.Unlock() - } - - if releaseHALock { + releaseHALock := func() { + // We may hit this from leaderLostCh or manualStepDownCh if they + // triggered before stopCh, so we check here instead of only in the + // stopCh case so we can try to do the right thing then, too + if atomic.LoadUint32(c.keepHALockOnStepDown) == 1 { + return + } if err := c.clearLeader(uuid); err != nil { c.logger.Error("clearing leader advertisement failed", "error", err) } @@ -518,9 +503,29 @@ func (c *Core) waitForLeadership(doneCh, manualStepDownCh, stopCh chan struct{}) c.heldHALock = nil } - // Check for a failure to prepare to seal - if preSealErr != nil { - c.logger.Error("pre-seal teardown failed", "error", err) + // Monitor a loss of leadership + select { + case <-leaderLostCh: + c.logger.Warn("leadership lost, stopping active operation") + + c.stateLock.Lock() + runSealing() + releaseHALock() + c.stateLock.Unlock() + + case <-stopCh: + runSealing() + releaseHALock() + return + + case <-manualStepDownCh: + manualStepDown = true + c.logger.Warn("stepping down from active operation to standby") + + c.stateLock.Lock() + runSealing() + releaseHALock() + c.stateLock.Unlock() } } } diff --git a/vault/identity_store_entities_test.go b/vault/identity_store_entities_test.go index 29eea08552..5198204ac4 100644 --- a/vault/identity_store_entities_test.go +++ b/vault/identity_store_entities_test.go @@ -306,11 +306,7 @@ func TestIdentityStore_LoadingEntities(t *testing.T) { } } - sealed, err := c.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if c.Sealed() { t.Fatal("should not be sealed") } @@ -402,11 +398,7 @@ func TestIdentityStore_LoadingEntities(t *testing.T) { t.Fatalf("failed to seal core: %v", err) } - sealed, err = c.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if !sealed { + if !c.Sealed() { t.Fatal("should be sealed") } @@ -416,11 +408,7 @@ func TestIdentityStore_LoadingEntities(t *testing.T) { } } - sealed, err = c.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if c.Sealed() { t.Fatal("should not be sealed") } diff --git a/vault/init.go b/vault/init.go index 659858e90a..b463ec2349 100644 --- a/vault/init.go +++ b/vault/init.go @@ -259,11 +259,7 @@ func (c *Core) UnsealWithStoredKeys(ctx context.Context) error { return nil } - sealed, err := c.Sealed() - if err != nil { - c.logger.Error("error checking sealed status in auto-unseal", "error", err) - return errwrap.Wrapf("error checking sealed status in auto-unseal: {{err}}", err) - } + sealed := c.Sealed() if !sealed { return nil } diff --git a/vault/logical_system_integ_test.go b/vault/logical_system_integ_test.go index 3a3c2ae4e3..d633e83314 100644 --- a/vault/logical_system_integ_test.go +++ b/vault/logical_system_integ_test.go @@ -48,11 +48,7 @@ func TestSystemBackend_Plugin_secret(t *testing.T) { t.Fatal(err) } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } // Wait for active so post-unseal takes place @@ -90,11 +86,7 @@ func TestSystemBackend_Plugin_auth(t *testing.T) { t.Fatal(err) } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } // Wait for active so post-unseal takes place @@ -169,11 +161,7 @@ func testPlugin_CatalogRemoved(t *testing.T, btype logical.BackendType, testMoun t.Fatal(err) } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } } @@ -282,11 +270,7 @@ func testPlugin_continueOnError(t *testing.T, btype logical.BackendType, mismatc t.Fatal(err) } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } } @@ -391,11 +375,7 @@ func TestSystemBackend_Plugin_SealUnseal(t *testing.T) { t.Fatal(err) } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } } diff --git a/vault/rekey.go b/vault/rekey.go index a55f184f91..89b3573707 100644 --- a/vault/rekey.go +++ b/vault/rekey.go @@ -61,7 +61,7 @@ type RekeyBackup struct { func (c *Core) RekeyThreshold(ctx context.Context, recovery bool) (int, logical.HTTPCodedError) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return 0, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -95,7 +95,7 @@ func (c *Core) RekeyThreshold(ctx context.Context, recovery bool) (int, logical. func (c *Core) RekeyProgress(recovery, verification bool) (bool, int, logical.HTTPCodedError) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return false, 0, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -126,7 +126,7 @@ func (c *Core) RekeyProgress(recovery, verification bool) (bool, int, logical.HT func (c *Core) RekeyConfig(recovery bool) (*SealConfig, logical.HTTPCodedError) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -200,7 +200,7 @@ func (c *Core) BarrierRekeyInit(config *SealConfig) logical.HTTPCodedError { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -250,7 +250,7 @@ func (c *Core) RecoveryRekeyInit(config *SealConfig) logical.HTTPCodedError { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -299,7 +299,7 @@ func (c *Core) BarrierRekeyUpdate(ctx context.Context, key []byte, nonce string) // Ensure we are already unsealed c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -535,7 +535,7 @@ func (c *Core) RecoveryRekeyUpdate(ctx context.Context, key []byte, nonce string // Ensure we are already unsealed c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -734,7 +734,7 @@ func (c *Core) RekeyVerify(ctx context.Context, key []byte, nonce string, recove // Ensure we are already unsealed c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -848,7 +848,7 @@ func (c *Core) RekeyVerify(ctx context.Context, key []byte, nonce string, recove func (c *Core) RekeyCancel(recovery bool) logical.HTTPCodedError { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -871,7 +871,7 @@ func (c *Core) RekeyCancel(recovery bool) logical.HTTPCodedError { func (c *Core) RekeyVerifyRestart(recovery bool) logical.HTTPCodedError { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -906,7 +906,7 @@ func (c *Core) RekeyVerifyRestart(recovery bool) logical.HTTPCodedError { func (c *Core) RekeyRetrieveBackup(ctx context.Context, recovery bool) (*RekeyBackup, logical.HTTPCodedError) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { @@ -943,7 +943,7 @@ func (c *Core) RekeyRetrieveBackup(ctx context.Context, recovery bool) (*RekeyBa func (c *Core) RekeyDeleteBackup(ctx context.Context, recovery bool) logical.HTTPCodedError { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error()) } if c.standby { diff --git a/vault/rekey_test.go b/vault/rekey_test.go index b0407e46f7..6b65ce5513 100644 --- a/vault/rekey_test.go +++ b/vault/rekey_test.go @@ -226,7 +226,7 @@ func testCore_Rekey_Update_Common(t *testing.T, c *Core, keys [][]byte, root str t.Fatalf("err: %v", err) } } - if sealed, _ := c.Sealed(); sealed { + if c.Sealed() { t.Fatalf("should be unsealed") } } diff --git a/vault/request_handling.go b/vault/request_handling.go index 7115b64dd2..a8cd8af76e 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -268,7 +268,7 @@ func (c *Core) checkToken(ctx context.Context, req *logical.Request, unauth bool func (c *Core) HandleRequest(req *logical.Request) (resp *logical.Response, err error) { c.stateLock.RLock() defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, consts.ErrSealed } if c.standby { diff --git a/vault/seal_testing.go b/vault/seal_testing.go index a3d4abf197..4f6b1d1258 100644 --- a/vault/seal_testing.go +++ b/vault/seal_testing.go @@ -34,18 +34,14 @@ func testCoreUnsealedWithConfigs(t testing.T, barrierConf, recoveryConf *SealCon if err != nil { t.Fatalf("err: %s", err) } - if sealed, _ := core.Sealed(); sealed { + if core.Sealed() { for _, key := range result.SecretShares { if _, err := core.Unseal(TestKeyCopy(key)); err != nil { t.Fatalf("unseal err: %s", err) } } - sealed, err = core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } } @@ -76,11 +72,7 @@ func TestCoreUnsealedWithConfigSealOpts(t testing.T, barrierConf, recoveryConf * } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } diff --git a/vault/testing.go b/vault/testing.go index 920b7b2785..9bb3e6f587 100644 --- a/vault/testing.go +++ b/vault/testing.go @@ -260,11 +260,7 @@ func testCoreUnsealed(t testing.T, core *Core) (*Core, [][]byte, string) { } } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } @@ -293,11 +289,7 @@ func TestCoreUnsealedBackend(t testing.T, backend physical.Backend) (*Core, [][] t.Fatal(err) } - sealed, err := core.Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if core.Sealed() { t.Fatal("should not be sealed") } @@ -740,11 +732,7 @@ func (c *TestCluster) UnsealCoresWithError() error { } // Verify unsealed - sealed, err := c.Cores[0].Sealed() - if err != nil { - return fmt.Errorf("err checking seal status: %s", err) - } - if sealed { + if c.Cores[0].Sealed() { return fmt.Errorf("should not be sealed") } @@ -818,11 +806,7 @@ func (c *TestCluster) ensureCoresSealed() error { if time.Now().After(timeout) { return fmt.Errorf("timeout waiting for core to seal") } - sealed, err := core.Sealed() - if err != nil { - return err - } - if sealed { + if core.Sealed() { break } time.Sleep(250 * time.Millisecond) @@ -842,11 +826,7 @@ func (c *TestCluster) UnsealWithStoredKeys(t testing.T) error { if time.Now().After(timeout) { return fmt.Errorf("timeout waiting for core to unseal") } - sealed, err := core.Sealed() - if err != nil { - return err - } - if !sealed { + if !core.Sealed() { break } time.Sleep(250 * time.Millisecond) @@ -1328,11 +1308,7 @@ func NewTestCluster(t testing.T, base *CoreConfig, opts *TestClusterOptions) *Te } // Verify unsealed - sealed, err := cores[0].Sealed() - if err != nil { - t.Fatalf("err checking seal status: %s", err) - } - if sealed { + if cores[0].Sealed() { t.Fatal("should not be sealed") } diff --git a/vault/token_store.go b/vault/token_store.go index aade87a052..c62ab4fa52 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -85,11 +85,13 @@ func (c *Core) LookupToken(token string) (*logical.TokenEntry, error) { return nil, fmt.Errorf("missing client token") } - c.stateLock.RLock() - defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return nil, consts.ErrSealed } + + c.stateLock.RLock() + defer c.stateLock.RUnlock() + if c.standby { return nil, consts.ErrStandby } diff --git a/vault/wrapping.go b/vault/wrapping.go index 5f2b59d5c5..3dbbfb6cd4 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -319,11 +319,12 @@ func (c *Core) ValidateWrappingToken(req *logical.Request) (bool, error) { return false, fmt.Errorf("token is empty") } - c.stateLock.RLock() - defer c.stateLock.RUnlock() - if c.sealed { + if c.Sealed() { return false, consts.ErrSealed } + + c.stateLock.RLock() + defer c.stateLock.RUnlock() if c.standby { return false, consts.ErrStandby }