From ce90a6fa38dff1aaa6fba5c63d368425d356394a Mon Sep 17 00:00:00 2001 From: nsimons Date: Tue, 29 Nov 2022 02:01:47 +0200 Subject: [PATCH] Make the error and http code clearer when supplying wrong unseal key (#17836) * Fix typos * Return http 400 when wrong unseal key is supplied * Add changelog * Add test cases and change one more return case to http 400 The new case is triggered when key length is within valid range [16, 32], but it has uneven bytes, causing crypto/aes to return invalid key size. * remove expected in unit tests * include error in the new error reason * add multikey and autoseal test cases * return invalid key for few more code paths --- changelog/17836.txt | 3 + http/sys_seal_test.go | 173 ++++++++++++++++++++++++++++++++++++++++-- vault/core.go | 10 +-- vault/seal.go | 4 + 4 files changed, 179 insertions(+), 11 deletions(-) create mode 100644 changelog/17836.txt diff --git a/changelog/17836.txt b/changelog/17836.txt new file mode 100644 index 000000000..66bc2369e --- /dev/null +++ b/changelog/17836.txt @@ -0,0 +1,3 @@ +```release-note:bug +core: trying to unseal with the wrong key now returns HTTP 400 +``` \ No newline at end of file diff --git a/http/sys_seal_test.go b/http/sys_seal_test.go index 698b625b6..c2e39ed16 100644 --- a/http/sys_seal_test.go +++ b/http/sys_seal_test.go @@ -1,11 +1,14 @@ package http import ( + "context" + "encoding/base64" "encoding/hex" "encoding/json" "fmt" "net/http" "strconv" + "strings" "testing" "github.com/go-test/deep" @@ -13,6 +16,7 @@ import ( "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/version" "github.com/hashicorp/vault/vault" + "github.com/hashicorp/vault/vault/seal" ) func TestSysSealStatus(t *testing.T) { @@ -229,16 +233,173 @@ func TestSysUnseal(t *testing.T) { } } -func TestSysUnseal_badKey(t *testing.T) { - core := vault.TestCore(t) - vault.TestCoreInit(t, core) +func subtestBadSingleKey(t *testing.T, seal vault.Seal) { + core := vault.TestCoreWithSeal(t, seal, false) + _, err := core.Initialize(context.Background(), &vault.InitParams{ + BarrierConfig: &vault.SealConfig{ + SecretShares: 1, + SecretThreshold: 1, + }, + RecoveryConfig: &vault.SealConfig{ + SecretShares: 1, + SecretThreshold: 1, + }, + }) + if err != nil { + t.Fatalf("err: %s", err) + } + ln, addr := TestServer(t, core) defer ln.Close() - resp := testHttpPut(t, "", addr+"/v1/sys/unseal", map[string]interface{}{ - "key": "0123", + testCases := []struct { + description string + key string + }{ + // hex key tests + // hexadecimal strings have 2 symbols per byte; size(0xAA) == 1 byte + { + "short hex key", + strings.Repeat("AA", 8), + }, + { + "long hex key", + strings.Repeat("AA", 34), + }, + { + "uneven hex key byte length", + strings.Repeat("AA", 33), + }, + { + "valid hex key but wrong cluster", + "4482691dd3a710723c4f77c4920ee21b96c226bf4829fa6eb8e8262c180ae933", + }, + + // base64 key tests + // base64 strings have min. 1 character per byte; size("m") == 1 byte + { + "short b64 key", + base64.StdEncoding.EncodeToString([]byte(strings.Repeat("m", 8))), + }, + { + "long b64 key", + base64.StdEncoding.EncodeToString([]byte(strings.Repeat("m", 34))), + }, + { + "uneven b64 key byte length", + base64.StdEncoding.EncodeToString([]byte(strings.Repeat("m", 33))), + }, + { + "valid b64 key but wrong cluster", + "RIJpHdOnEHI8T3fEkg7iG5bCJr9IKfpuuOgmLBgK6TM=", + }, + + // other key tests + { + "empty key", + "", + }, + { + "key with bad format", + "ThisKeyIsNeitherB64NorHex", + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + resp := testHttpPut(t, "", addr+"/v1/sys/unseal", map[string]interface{}{ + "key": tc.key, + }) + + testResponseStatus(t, resp, 400) + }) + } +} + +func subtestBadMultiKey(t *testing.T, seal vault.Seal) { + numKeys := 3 + + core := vault.TestCoreWithSeal(t, seal, false) + _, err := core.Initialize(context.Background(), &vault.InitParams{ + BarrierConfig: &vault.SealConfig{ + SecretShares: numKeys, + SecretThreshold: numKeys, + }, + RecoveryConfig: &vault.SealConfig{ + SecretShares: numKeys, + SecretThreshold: numKeys, + }, }) - testResponseStatus(t, resp, 400) + if err != nil { + t.Fatalf("err: %s", err) + } + ln, addr := TestServer(t, core) + defer ln.Close() + + testCases := []struct { + description string + keys []string + }{ + { + "all unseal keys from another cluster", + []string{ + "b189d98fdec3a15bed9b1cce5088f82b92896696b788c07bdf03c73da08279a5e8", + "0fa98232f034177d8d9c2824899a2ac1e55dc6799348533e10510b856aef99f61a", + "5344f5caa852f9ba1967d9623ed286a45ea7c4a529522d25f05d29ff44f17930ac", + }, + }, + { + "mixing unseal keys from different cluster, different share config", + []string{ + "b189d98fdec3a15bed9b1cce5088f82b92896696b788c07bdf03c73da08279a5e8", + "0fa98232f034177d8d9c2824899a2ac1e55dc6799348533e10510b856aef99f61a", + "e04ea3020838c2050c4a169d7ba4d30e034eec8e83e8bed9461bf2646ee412c0", + }, + }, + { + "mixing unseal keys from different clusters, similar share config", + []string{ + "b189d98fdec3a15bed9b1cce5088f82b92896696b788c07bdf03c73da08279a5e8", + "0fa98232f034177d8d9c2824899a2ac1e55dc6799348533e10510b856aef99f61a", + "413f80521b393aa6c4e42e9a3a3ab7f00c2002b2c3bf1e273fc6f363f35f2a378b", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + for i, key := range tc.keys { + resp := testHttpPut(t, "", addr+"/v1/sys/unseal", map[string]interface{}{ + "key": key, + }) + + if i == numKeys-1 { + // last key + testResponseStatus(t, resp, 400) + } else { + // unseal in progress + testResponseStatus(t, resp, 200) + } + + } + }) + } +} + +func TestSysUnseal_BadKeyNewShamir(t *testing.T) { + seal := vault.NewTestSeal(t, + &seal.TestSealOpts{StoredKeys: seal.StoredKeysSupportedShamirRoot}) + + subtestBadSingleKey(t, seal) + subtestBadMultiKey(t, seal) +} + +func TestSysUnseal_BadKeyAutoUnseal(t *testing.T) { + seal := vault.NewTestSeal(t, + &seal.TestSealOpts{StoredKeys: seal.StoredKeysSupportedGeneric}) + + subtestBadSingleKey(t, seal) + subtestBadMultiKey(t, seal) } func TestSysUnseal_Reset(t *testing.T) { diff --git a/vault/core.go b/vault/core.go index 6eb473553..f599d2629 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1305,13 +1305,13 @@ func (c *Core) Unseal(key []byte) (bool, error) { } // unseal takes a key fragment and attempts to use it to unseal Vault. -// Vault may remain unsealed afterwards even when no error is returned, +// Vault may remain sealed afterwards even when no error is returned, // depending on whether enough key fragments were provided to meet the // target threshold. // // The provided key should be a recovery key fragment if the seal // is an autoseal, or a regular seal key fragment for shamir. In -// migration scenarios "seal" in the preceding sentance refers to +// migration scenarios "seal" in the preceding sentence refers to // the migration seal in c.migrationInfo.seal. // // We use getUnsealKey to work out if we have enough fragments, @@ -1555,13 +1555,13 @@ func (c *Core) getUnsealKey(ctx context.Context, seal Seal) ([]byte, error) { } else { unsealKey, err = shamir.Combine(c.unlockInfo.Parts) if err != nil { - return nil, fmt.Errorf("failed to compute combined key: %w", err) + return nil, &ErrInvalidKey{fmt.Sprintf("failed to compute combined key: %v", err)} } } if seal.RecoveryKeySupported() { if err := seal.VerifyRecoveryKey(ctx, unsealKey); err != nil { - return nil, err + return nil, &ErrInvalidKey{fmt.Sprintf("failed to verify recovery key: %v", err)} } } @@ -2794,7 +2794,7 @@ func (c *Core) unsealKeyToMasterKey(ctx context.Context, seal Seal, combinedKey err := seal.GetAccess().Wrapper.(*aeadwrapper.ShamirWrapper).SetAesGcmKeyBytes(combinedKey) if err != nil { - return nil, fmt.Errorf("failed to setup unseal key: %w", err) + return nil, &ErrInvalidKey{fmt.Sprintf("failed to setup unseal key: %v", err)} } storedKeys, err := seal.GetStoredKeys(ctx) if storedKeys == nil && err == nil && allowMissing { diff --git a/vault/seal.go b/vault/seal.go index abe350c5b..d8bafc616 100644 --- a/vault/seal.go +++ b/vault/seal.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "strings" "sync/atomic" "github.com/hashicorp/vault/sdk/helper/jsonutil" @@ -484,6 +485,9 @@ func readStoredKeys(ctx context.Context, storage physical.Backend, encryptor *se pt, err := encryptor.Decrypt(ctx, blobInfo, nil) if err != nil { + if strings.Contains(err.Error(), "message authentication failed") { + return nil, &ErrInvalidKey{Reason: fmt.Sprintf("failed to decrypt keys from storage: %v", err)} + } return nil, &ErrDecrypt{Err: fmt.Errorf("failed to decrypt keys from storage: %w", err)} }