From a5ea384a1c8d2742c757996d726b45a0d9819be0 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Mon, 14 Jan 2019 19:27:08 +0100 Subject: [PATCH 1/4] Add helper for checking if an error is a fatal error The double-double negative was really confusing, and this pattern is used a few places in Vault. This negates the double negative, making the devx a bit easier to follow. --- command/server.go | 4 ++-- http/sys_init.go | 3 +-- vault/core.go | 10 ++++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/command/server.go b/command/server.go index f07b6d129e09..ab894901b3e8 100644 --- a/command/server.go +++ b/command/server.go @@ -741,7 +741,7 @@ CLUSTER_SYNTHESIS_COMPLETE: // Initialize the core core, newCoreError := vault.NewCore(coreConfig) if newCoreError != nil { - if !errwrap.ContainsType(newCoreError, new(vault.NonFatalError)) { + if vault.IsFatalError(newCoreError) { c.UI.Error(fmt.Sprintf("Error initializing core: %s", newCoreError)) return 1 } @@ -938,7 +938,7 @@ CLUSTER_SYNTHESIS_COMPLETE: } if err := core.UnsealWithStoredKeys(context.Background()); err != nil { - if !errwrap.ContainsType(err, new(vault.NonFatalError)) { + if vault.IsFatalError(err) { c.UI.Error(fmt.Sprintf("Error initializing core: %s", err)) return 1 } diff --git a/http/sys_init.go b/http/sys_init.go index 39e2c5551d5e..2cf8ea1208b0 100644 --- a/http/sys_init.go +++ b/http/sys_init.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" - "github.com/hashicorp/errwrap" "github.com/hashicorp/vault/vault" ) @@ -104,7 +103,7 @@ func handleSysInitPut(core *vault.Core, w http.ResponseWriter, r *http.Request) result, initErr := core.Initialize(ctx, initParams) if initErr != nil { - if !errwrap.ContainsType(initErr, new(vault.NonFatalError)) { + if vault.IsFatalError(initErr) { respondError(w, http.StatusBadRequest, initErr) return } else { diff --git a/vault/core.go b/vault/core.go index 5c8a4db0e98e..fc7957fbdee1 100644 --- a/vault/core.go +++ b/vault/core.go @@ -107,6 +107,16 @@ func (e *NonFatalError) Error() string { return e.Err.Error() } +// NewNonFatalError returns a new non-fatal error. +func NewNonFatalError(err error) *NonFatalError { + return &NonFatalError{Err: err} +} + +// IsFatalError returns true if the given error is a non-fatal error. +func IsFatalError(err error) bool { + return !errwrap.ContainsType(err, new(NonFatalError)) +} + // ErrInvalidKey is returned if there is a user-based error with a provided // unseal key. This will be shown to the user, so should not contain // information that is sensitive. From a5e6c60f9850df29be4e08b1cebd1785c53a2ff5 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Mon, 14 Jan 2019 19:33:40 +0100 Subject: [PATCH 2/4] Check return value of UnsealWithStoredKeys in sys/init --- http/sys_init.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/http/sys_init.go b/http/sys_init.go index 2cf8ea1208b0..3d5a8b7e5b69 100644 --- a/http/sys_init.go +++ b/http/sys_init.go @@ -135,7 +135,10 @@ func handleSysInitPut(core *vault.Core, w http.ResponseWriter, r *http.Request) } } - core.UnsealWithStoredKeys(ctx) + if err := core.UnsealWithStoredKeys(ctx); err != nil { + respondError(w, http.StatusInternalServerError, err) + return + } respondOk(w, resp) } From 5ad512c25dc6186b1f1d7700b8b6ef1eb9d73cab Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Mon, 14 Jan 2019 19:35:12 +0100 Subject: [PATCH 3/4] Return proper error types when attempting unseal with stored key Prior to this commit, "nil" could have meant unsupported auto-unseal, a transient error, or success. This updates the function to return the correct error type, signaling to the caller whether they should retry or fail. --- vault/core.go | 4 ++++ vault/init.go | 66 +++++++++++++++++++++++++++++---------------------- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/vault/core.go b/vault/core.go index fc7957fbdee1..aac28fbd160b 100644 --- a/vault/core.go +++ b/vault/core.go @@ -396,6 +396,10 @@ type Core struct { // Stores the sealunwrapper for downgrade needs sealUnwrapper physical.Backend + // unsealwithStoredKeysLock is a mutex that prevents multiple processes from + // unsealing with stored keys are the same time. + unsealWithStoredKeysLock sync.Mutex + // Stores any funcs that should be run on successful postUnseal postUnsealFuncs []func() diff --git a/vault/init.go b/vault/init.go index 426cc62a942a..89883c448c18 100644 --- a/vault/init.go +++ b/vault/init.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/hex" + "errors" "fmt" "github.com/hashicorp/errwrap" @@ -270,54 +271,63 @@ func (c *Core) Initialize(ctx context.Context, initParams *InitParams) (*InitRes return results, nil } -// UnsealWithStoredKeys performs auto-unseal using stored keys. +// UnsealWithStoredKeys performs auto-unseal using stored keys. An error +// return value of "nil" implies the Vault instance is unsealed. +// +// Callers should attempt to retry any NOnFatalErrors. Callers should +// not re-attempt fatal errors. func (c *Core) UnsealWithStoredKeys(ctx context.Context) error { + c.unsealWithStoredKeysLock.Lock() + defer c.unsealWithStoredKeysLock.Unlock() + if !c.seal.StoredKeysSupported() { - return nil + return errors.New("stored keys are not supported") } // Disallow auto-unsealing when migrating if c.IsInSealMigration() { - return nil + return NewNonFatalError(errors.New("cannot auto-unseal during seal migration")) } sealed := c.Sealed() if !sealed { + c.Logger().Warn("attempted unseal with stored keys, but vault is already unsealed") return nil } - c.logger.Info("stored unseal keys supported, attempting fetch") + c.Logger().Info("stored unseal keys supported, attempting fetch") keys, err := c.seal.GetStoredKeys(ctx) if err != nil { - c.logger.Error("fetching stored unseal keys failed", "error", err) - return &NonFatalError{Err: errwrap.Wrapf("fetching stored unseal keys failed: {{err}}", err)} + return NewNonFatalError(errwrap.Wrapf("fetching stored unseal keys failed: {{err}}", err)) } + + // This usually happens when auto-unseal is configured, but the servers have + // not been initialized yet. if len(keys) == 0 { - c.logger.Warn("stored unseal key(s) supported but none found") - } else { - unsealed := false - keysUsed := 0 - for _, key := range keys { - unsealed, err = c.Unseal(key) - if err != nil { - c.logger.Error("unseal with stored unseal key failed", "error", err) - return &NonFatalError{Err: errwrap.Wrapf("unseal with stored key failed: {{err}}", err)} - } - keysUsed += 1 - if unsealed { - break - } + return NewNonFatalError(errors.New("stored unseal keys are supported, but none were found")) + } + + unsealed := false + keysUsed := 0 + for _, key := range keys { + unsealed, err = c.Unseal(key) + if err != nil { + return NewNonFatalError(errwrap.Wrapf("unseal with stored key failed: {{err}}", err)) } - if !unsealed { - if c.logger.IsWarn() { - c.logger.Warn("stored unseal key(s) used but Vault not unsealed yet", "stored_keys_used", keysUsed) - } - } else { - if c.logger.IsInfo() { - c.logger.Info("successfully unsealed with stored key(s)", "stored_keys_used", keysUsed) - } + keysUsed++ + if unsealed { + break } } + if !unsealed { + // This most likely means that the user configured Vault to only store a + // subset of the required threshold of keys. We still consider this a + // "success", since trying again would yield the same result. + c.Logger().Warn("vault still sealed after using stored unseal keys", "stored_keys_used", keysUsed) + } else { + c.Logger().Info("unsealed with stored keys", "stored_keys_used", keysUsed) + } + return nil } From 9e646ee613f5bbb5573a5bc69c31eec7d5a426ee Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Mon, 14 Jan 2019 19:38:45 +0100 Subject: [PATCH 4/4] Continuously attempt to unseal if sealed keys are supported This fixes a bug that occurs on bootstrapping an initial cluster. Given a collection of Vault nodes and an initialized storage backend, they will all go into standby waiting for initialization. After one node is initialized, the other nodes had no mechanism by which they "re-check" to see if unseal keys are present. This adds a goroutine to the server command which continually waits for unseal keys to exist. It exits in the following conditions: - the node is unsealed - the node does not support stored keys - a fatal error occurs (as defined by Vault) - the server is shutting down In all other situations, the routine wakes up at the specified interval and attempts to unseal with the stored keys. --- command/server.go | 29 ++++++++++++++++++++++++----- vault/init.go | 2 +- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/command/server.go b/command/server.go index ab894901b3e8..9389570a0d98 100644 --- a/command/server.go +++ b/command/server.go @@ -937,12 +937,31 @@ CLUSTER_SYNTHESIS_COMPLETE: return 1 } - if err := core.UnsealWithStoredKeys(context.Background()); err != nil { - if vault.IsFatalError(err) { - c.UI.Error(fmt.Sprintf("Error initializing core: %s", err)) - return 1 + // Attempt unsealing in a background goroutine. This is needed for when a + // 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. + go func() { + for { + err := core.UnsealWithStoredKeys(context.Background()) + if err == nil { + return + } + + if vault.IsFatalError(err) { + c.logger.Error(fmt.Sprintf("Error unsealing core", "error", err)) + return + } else { + c.logger.Warn(fmt.Sprintf("Failed to unseal core", "error" err)) + } + + select { + case <-c.ShutdownCh: + return + case <-time.After(5 * time.Second): + } } - } + }() // Perform service discovery registrations and initialization of // HTTP server after the verifyOnly check. diff --git a/vault/init.go b/vault/init.go index 89883c448c18..c4ad07ecf028 100644 --- a/vault/init.go +++ b/vault/init.go @@ -281,7 +281,7 @@ func (c *Core) UnsealWithStoredKeys(ctx context.Context) error { defer c.unsealWithStoredKeysLock.Unlock() if !c.seal.StoredKeysSupported() { - return errors.New("stored keys are not supported") + return nil } // Disallow auto-unsealing when migrating