From 345057bea06304fc10c6d48e97931fa4df360c1b Mon Sep 17 00:00:00 2001 From: Bretton Date: Sat, 15 Aug 2026 21:39:13 -0700 Subject: [PATCH] =?UTF-8?q?test(config,identity):=20RED=20=E2=80=94=20the?= =?UTF-8?q?=20previous-KEK=20edge=20set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config: malformed values blamed on BRIDGE_KEK_PREVIOUS (not BRIDGE_KEK), byte-level equality refusal, both encodings accepted, optional in production. Custodian: dual-open, one clean both-fail error identical to the single-KEK shape, malformed blobs classified as data incidents not KEK incidents (byte-identical with and without previous), cross-AAD refused through the retry path, nil-previous equivalence. Co-Authored-By: Claude Fable 5 --- internal/config/kek_previous_test.go | 138 ++++++++++++++++ internal/identity/kek_rotation_test.go | 220 +++++++++++++++++++++++++ 2 files changed, 358 insertions(+) create mode 100644 internal/config/kek_previous_test.go create mode 100644 internal/identity/kek_rotation_test.go diff --git a/internal/config/kek_previous_test.go b/internal/config/kek_previous_test.go new file mode 100644 index 0000000..6c017d2 --- /dev/null +++ b/internal/config/kek_previous_test.go @@ -0,0 +1,138 @@ +package config + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The base64 spellings of the same two KEKs the acceptance test writes in +// hex. BRIDGE_KEK has always accepted either form; BRIDGE_KEK_PREVIOUS must +// too, or an operator who generated both keys the same way gets told only one +// of them is malformed. +const ( + previousBridgeKEKBase64 = "iwrNHxCOGS83v5MKOAfjn3zqMk7oppnZ1VS3HUAxNE0=" + currentBridgeKEKBase64 = "qGm/20EwBv+prc53trzMftReHLCWUDPWRShQbvN3bW8=" +) + +// loadWithPreviousKEK runs Load over a production environment carrying both +// KEKs and returns whatever Load decided — failures included, unlike +// loadRotatingConfig in the acceptance test. +func loadWithPreviousKEK(t *testing.T, current, previous string) (*Config, error) { + t.Helper() + setProductionEnv(t) + t.Setenv("BRIDGE_KEK", current) + t.Setenv("BRIDGE_KEK_PREVIOUS", previous) + return Load(discardLogger()) +} + +// namesOnlyPreviousKEK asserts that an error blames BRIDGE_KEK_PREVIOUS and +// never the current key. The subtlety: "BRIDGE_KEK_PREVIOUS" contains +// "BRIDGE_KEK" as a substring, so a naive NotContains would reject a correct +// message. Cut the full variable name out first, then look for what is left. +func namesOnlyPreviousKEK(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + msg := err.Error() + assert.Contains(t, msg, "BRIDGE_KEK_PREVIOUS", + "a malformed previous KEK must name BRIDGE_KEK_PREVIOUS, so the operator edits the variable they actually broke") + assert.NotContains(t, strings.ReplaceAll(msg, "BRIDGE_KEK_PREVIOUS", ""), "BRIDGE_KEK", + "the message must not also blame BRIDGE_KEK: an operator mid-rotation sent to fix a key that is already correct will rotate the wrong one and orphan every sealed key") +} + +func TestLoad_RejectsMalformedPreviousKEK(t *testing.T) { + tests := []struct { + name string + previous string + }{ + { + // 63 characters: not decodable as hex (odd length), not + // decodable as base64 (length not a multiple of 4). The classic + // truncated-paste. + name: "odd length hex", + previous: "8b0acd1f108e192f37bf930a3807e39f7cea324ee8a699d9d554b71d4031344", + }, + { + // Valid base64, but only 16 bytes — AES-256 needs 32. + name: "base64 of the wrong number of bytes", + previous: "MDEyMzQ1Njc4OWFiY2RlZg==", + }, + { + name: "not an encoding at all", + previous: "the-old-key-is-in-the-password-manager", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := loadWithPreviousKEK(t, currentBridgeKEKHex, tt.previous) + require.Error(t, err, + "a BRIDGE_KEK_PREVIOUS that cannot be decoded must stop startup; silently ignoring it would leave the bridge unable to open pre-rotation keys with no sign of why") + namesOnlyPreviousKEK(t, err) + }) + } +} + +func TestLoad_RejectsPreviousKEKEqualToCurrent(t *testing.T) { + // Same bytes, same spelling: the operator copied the old value into the + // new variable and changed nothing. + _, err := loadWithPreviousKEK(t, currentBridgeKEKHex, currentBridgeKEKHex) + require.Error(t, err, + "BRIDGE_KEK_PREVIOUS equal to BRIDGE_KEK is not a rotation; accepting it lets an operator believe they have rotated when every key is still sealed under the original") + assert.Contains(t, err.Error(), "BRIDGE_KEK_PREVIOUS", + "the refusal must name the variable the operator has to change") + + // Same bytes, different spelling. The check is on the decoded key, not + // the string: hex here, base64 there, still one key and still not a + // rotation. + _, err = loadWithPreviousKEK(t, currentBridgeKEKHex, currentBridgeKEKBase64) + require.Error(t, err, + "the same key written in hex and base64 is still the same key; comparing the raw strings instead of the decoded bytes would wave this through") +} + +func TestLoad_AcceptsPreviousKEKInEitherEncoding(t *testing.T) { + expected := mustDecodeKEKHex(t, previousBridgeKEKHex) + + for _, tt := range []struct { + name string + previous string + }{ + {name: "hex", previous: previousBridgeKEKHex}, + {name: "base64", previous: previousBridgeKEKBase64}, + } { + t.Run(tt.name, func(t *testing.T) { + cfg, err := loadWithPreviousKEK(t, currentBridgeKEKHex, tt.previous) + require.NoError(t, err) + assert.Equal(t, expected, cfg.BridgeKEKPrevious, + "BRIDGE_KEK_PREVIOUS must decode to the same 32 bytes BRIDGE_KEK would, in whichever encoding the operator pasted it") + }) + } +} + +func TestLoad_PreviousKEKIsOptionalInProduction(t *testing.T) { + // The steady state, and by far the common one: no rotation in progress. + // BRIDGE_KEK is required in production; BRIDGE_KEK_PREVIOUS must not be, + // or adding rotation support breaks every existing deployment on restart. + setProductionEnv(t) + t.Setenv("BRIDGE_KEK", currentBridgeKEKHex) + + cfg, err := Load(discardLogger()) + require.NoError(t, err, + "production must start with BRIDGE_KEK alone; requiring BRIDGE_KEK_PREVIOUS would refuse to boot every bridge that has never rotated") + assert.Nil(t, cfg.BridgeKEKPrevious, + "an unset BRIDGE_KEK_PREVIOUS must yield no previous key at all, not empty bytes a custodian might try to build a cipher from") +} + +func TestLoad_PreviousKEKIsOptionalInDevelopment(t *testing.T) { + // Development defaults BRIDGE_KEK to a fixed public key. There is no + // corresponding default for the previous key: a dev bridge has nothing + // sealed under an older one. + clearConfigEnv(t) + + cfg, err := Load(discardLogger()) + require.NoError(t, err) + assert.Nil(t, cfg.BridgeKEKPrevious, + "development must not invent a previous KEK; a defaulted second key would make every dev custodian silently accept material no production bridge would") +} diff --git a/internal/identity/kek_rotation_test.go b/internal/identity/kek_rotation_test.go new file mode 100644 index 0000000..963ba49 --- /dev/null +++ b/internal/identity/kek_rotation_test.go @@ -0,0 +1,220 @@ +package identity + +import ( + "bytes" + "crypto/rand" + "crypto/rsa" + "crypto/sha256" + "strings" + "testing" + + "github.com/bluesky-social/indigo/atproto/atcrypto" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" +) + +// The KEKs of a rotation, plus a third that was never the bridge's key — the +// stand-in for a ciphertext that arrived from somewhere it should not have. +func previousTestKEK() []byte { + sum := sha256.Sum256([]byte("tidepool-test-kek-previous")) + return sum[:] +} + +func currentTestKEK() []byte { + sum := sha256.Sum256([]byte("tidepool-test-kek-current")) + return sum[:] +} + +func strangerTestKEK() []byte { + sum := sha256.Sum256([]byte("tidepool-test-kek-stranger")) + return sum[:] +} + +const rotationTestDID = "did:plc:ewvi7nxzyoun6zhxrhs64oiz" + +// rotatingCustodian is the custodian a bridge runs mid-rotation. +func rotatingCustodian(t *testing.T) *Custodian { + t.Helper() + c, err := NewCustodianWithPrevious(currentTestKEK(), previousTestKEK()) + require.NoError(t, err) + return c +} + +// sealedUnder returns an actor signing key and its ciphertext under kek. +func sealedUnder(t *testing.T, kek []byte) (*atcrypto.PrivateKeyK256, []byte) { + t.Helper() + custodian, err := NewCustodian(kek) + require.NoError(t, err) + key, err := atcrypto.GeneratePrivateKeyK256() + require.NoError(t, err) + sealed, err := custodian.EncryptActorKey(rotationTestDID, key) + require.NoError(t, err) + return key, sealed +} + +func TestCustodianWithPrevious_OpensKeySealedUnderPrevious(t *testing.T) { + key, sealed := sealedUnder(t, previousTestKEK()) + + opened, err := rotatingCustodian(t).DecryptActorKey(rotationTestDID, sealed) + require.NoError(t, err, + "the whole point of a previous KEK: material sealed before the rotation must still open") + assert.True(t, bytes.Equal(key.Bytes(), opened.Bytes()), + "the key recovered under the previous KEK must be the original, not merely something that decrypted") +} + +func TestCustodianWithPrevious_SealsUnderCurrentOnly(t *testing.T) { + custodian := rotatingCustodian(t) + key, err := atcrypto.GeneratePrivateKeyK256() + require.NoError(t, err) + sealed, err := custodian.EncryptActorKey(rotationTestDID, key) + require.NoError(t, err) + + currentOnly, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + opened, err := currentOnly.DecryptActorKey(rotationTestDID, sealed) + require.NoError(t, err, + "seal must always use the current KEK; if it ever used the previous one the rotation could never be finished and the old key could never be retired") + assert.True(t, bytes.Equal(key.Bytes(), opened.Bytes())) + + previousOnly, err := NewCustodian(previousTestKEK()) + require.NoError(t, err) + _, err = previousOnly.DecryptActorKey(rotationTestDID, sealed) + require.Error(t, err, + "a key sealed during the rotation window must NOT open under the retired KEK; if it does, the old key is still live key material and retiring it is a data-loss event") +} + +func TestCustodianWithPrevious_BothKeysFailReportsOneError(t *testing.T) { + // A blob sealed under a KEK the bridge has never held: pasted from + // another deployment, or restored from a backup taken two rotations ago. + // Neither key opens it, and the operator must see ONE failure describing + // that — not a pair of stacked attempts they have to read past. + _, stranger := sealedUnder(t, strangerTestKEK()) + + currentOnly, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + _, singleErr := currentOnly.DecryptActorKey(rotationTestDID, stranger) + require.Error(t, singleErr) + + _, dualErr := rotatingCustodian(t).DecryptActorKey(rotationTestDID, stranger) + require.Error(t, dualErr, + "a blob under an unknown KEK must not open just because two keys were tried") + + assert.Equal(t, 1, strings.Count(dualErr.Error(), "open sealed key"), + "a failed retry must not stack a second open error onto the first; one unreadable blob is one incident to the operator reading the log") + assert.Equal(t, singleErr.Error(), dualErr.Error(), + "an unreadable blob must look identical whether or not a previous KEK is configured, so log lines and alerts written before rotation still match after it") +} + +func TestCustodianWithPrevious_MalformedBlobIsNotAKEKProblem(t *testing.T) { + // Retry belongs on authentication failure alone. A blob that is truncated + // or carries an unknown version byte is corrupt storage, not a wrong key: + // it must be reported as malformed under one KEK or two, so an operator + // does not go hunting through key history for a database problem. + _, sealed := sealedUnder(t, currentTestKEK()) + + badVersion := append([]byte{}, sealed...) + badVersion[0] = 99 + + for _, tt := range []struct { + name string + blob []byte + }{ + {name: "truncated", blob: sealed[:8]}, + {name: "unknown version byte", blob: badVersion}, + } { + t.Run(tt.name, func(t *testing.T) { + currentOnly, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + _, singleErr := currentOnly.DecryptActorKey(rotationTestDID, tt.blob) + require.Error(t, singleErr) + + _, dualErr := rotatingCustodian(t).DecryptActorKey(rotationTestDID, tt.blob) + require.Error(t, dualErr) + + assert.True(t, errors.IsValidation(dualErr), + "a malformed sealed blob must stay a validation error with a previous KEK configured; classifying it otherwise turns a storage-corruption incident into a key-management goose chase") + assert.NotContains(t, dualErr.Error(), "message authentication failed", + "a malformed blob must not be reported as an authentication failure: it was never decrypted under either key, so nothing about the KEKs is in question") + assert.Equal(t, singleErr.Error(), dualErr.Error(), + "a malformed blob must read exactly the same with and without a previous KEK") + }) + } +} + +func TestCustodianWithPrevious_CrossAADRejectedUnderBothKeys(t *testing.T) { + // The AAD prefixes keep the K256 escrow domain and the AP RSA domain + // apart. A second KEK widens which keys can decrypt; it must not widen + // which domains a ciphertext is accepted in. + custodian := rotatingCustodian(t) + + // An actor signing key sealed under the PREVIOUS KEK — the blob the + // retry path is there to open — offered to the RSA domain. + _, sealedK256 := sealedUnder(t, previousTestKEK()) + _, err := custodian.DecryptActorRSAKey(rotationTestDID, sealedK256) + require.Error(t, err, + "an escrow signing key must not open as an AP RSA key even when the previous KEK is the one that can decrypt it; the retry must re-try the KEK, never the AAD") + + // And the reverse, also sealed under the previous KEK. + previousOnly, err := NewCustodian(previousTestKEK()) + require.NoError(t, err) + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + sealedRSA, err := previousOnly.EncryptActorRSAKey(rotationTestDID, rsaKey) + require.NoError(t, err) + _, err = custodian.DecryptActorKey(rotationTestDID, sealedRSA) + require.Error(t, err, + "an AP RSA key must not open as an escrow signing key under either KEK") + + // The DID binding survives the second key too. + _, sealedForOwner := sealedUnder(t, previousTestKEK()) + _, err = custodian.DecryptActorKey("did:plc:44ybard66vv44zksje25o7dz", sealedForOwner) + require.Error(t, err, + "a key sealed for one DID must not open under another, whichever KEK decrypts it") +} + +func TestCustodianWithPrevious_NilPreviousMatchesNewCustodian(t *testing.T) { + // Run 1 ships alone: a bridge that never sets BRIDGE_KEK_PREVIOUS must + // behave exactly as it did before rotation support existed. + dual, err := NewCustodianWithPrevious(currentTestKEK(), nil) + require.NoError(t, err, + "a nil previous KEK is the normal case, not an error") + + _, sealedUnderOld := sealedUnder(t, previousTestKEK()) + _, err = dual.DecryptActorKey(rotationTestDID, sealedUnderOld) + require.Error(t, err, + "without a previous KEK the custodian must refuse old material exactly as NewCustodian does; anything else would mean a second key was being used that the operator never configured") + + key, err := atcrypto.GeneratePrivateKeyK256() + require.NoError(t, err) + sealed, err := dual.EncryptActorKey(rotationTestDID, key) + require.NoError(t, err) + opened, err := dual.DecryptActorKey(rotationTestDID, sealed) + require.NoError(t, err) + assert.True(t, bytes.Equal(key.Bytes(), opened.Bytes()), + "the ordinary seal/open round trip must be untouched when no previous KEK is set") + + // And what it seals is readable by a plain single-KEK custodian, so the + // two constructors are interchangeable at rest. + plain, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + _, err = plain.DecryptActorKey(rotationTestDID, sealed) + require.NoError(t, err, + "ciphertext written with a nil previous KEK must be indistinguishable from ciphertext written by NewCustodian") +} + +func TestNewCustodianWithPrevious_RejectsBadPreviousLength(t *testing.T) { + _, err := NewCustodianWithPrevious(currentTestKEK(), []byte("0123456789abcdef")) + require.Error(t, err, + "a 16-byte previous KEK cannot build an AES-256 cipher; accepting it would mean discovering the problem only when a pre-rotation key failed to open") + assert.True(t, errors.IsValidation(err)) + assert.Contains(t, strings.ToLower(err.Error()), "previous", + "the error must say WHICH key is the wrong length; an operator told only that 'the KEK' is bad will check the one that is fine") +} + +func TestNewCustodianWithPrevious_RejectsBadCurrentLength(t *testing.T) { + _, err := NewCustodianWithPrevious([]byte("short"), previousTestKEK()) + require.Error(t, err, "the current KEK is validated exactly as NewCustodian validates it") + assert.True(t, errors.IsValidation(err)) +} -- 2.51.2