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

@@ -0,0 +1,3 @@
+```release-note:bug
+core: trying to unseal with the wrong key now returns HTTP 400
+```

http/sys_seal_test.go

@@ -1,18 +1,22 @@
 package http
 
 import (
+	"context"
+	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/go-test/deep"
 	"github.com/hashicorp/vault/helper/namespace"
 	"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) {

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 {

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)}
 	}