diff --git a/DEPLOY.md b/DEPLOY.md index aea919b..dc460c6 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -630,6 +630,16 @@ What each one means: should read about 8. A materially higher number is not extra peer rejections: it is claims that never settled, since a lapsed lease or a crashed worker leaves its increment behind with nobody to return it. + + **`last_error_class = 'kek_misconfigured'` with `attempts = 1` is a + configuration incident, not a peer one.** The actor's sealed signing key did + not open under the configured `BRIDGE_KEK`, so the worker poisons on the first + attempt instead of spending eight (`internal/outbound/worker.go`): retrying + cannot change the answer, and the fast poison is what puts the cause in front + of an operator in minutes rather than hours. Expect it in bulk and expect it + to spare newly minted actors — anything sealed *since* the wrong key was + configured opens fine, which is why half the bridge keeps working. Fix + `BRIDGE_KEK` (§6 runbook), restart, then `redrive`. - **echo drop counters rising steadily** — expected and healthy: our own content arriving back from Lemmy and being correctly refused. A counter at **zero** while native content is flowing is the alarming case; it means @@ -977,7 +987,13 @@ validates data presence, not KEK correctness: sealed columns are checked plausibility** (a restore drill must not need to read the KEK). The full proof is drill + boot canary: restore, point a bridge at it with the real `BRIDGE_KEK`, and a wrong pairing fails at startup — the same canary as step 2 -of the KEK runbook above. +of the KEK runbook above. That holds **even when the restore lost the +`plc-rotation` row itself**: with nothing sealed to read back, the canary proves +the KEK against the actor keys that *are* there (`ap_actors.rsa_key_sealed`, +`bridged_actors.signing_key`) and refuses to mint a replacement rotation key +over a populated database it cannot read. Only a database with no sealed +material anywhere — a genuinely fresh install — is allowed to mint on an +unproven key. **Retention and drill cadence interact, and not in your favour.** Retention ages dumps out on `pg-backup.sh`'s own criteria, which is `pg_restore --list` diff --git a/cmd/tidepool/main.go b/cmd/tidepool/main.go index 00d0d4d..7ad8719 100644 --- a/cmd/tidepool/main.go +++ b/cmd/tidepool/main.go @@ -368,7 +368,13 @@ func run(logger *slog.Logger) error { // and opened here, so a wrong or half-rotated key fails startup before any // traffic is served, rather than surfacing later as per-actor decrypt // failures scattered across the commit path. - rotationKey, err := identity.LoadOrCreateRotationKey(ctx, serviceKeys, custodian) + // + // The database goes in beside the store because the canary has a second job + // on the path where there is nothing to open — a restore that lost the + // plc-rotation row. There it proves the KEK against the actor keys already + // at rest instead of minting a fresh rotation key under an unproven one and + // calling that a pass (identity.LoadOrCreateRotationKey says why). + rotationKey, err := identity.LoadOrCreateRotationKey(ctx, database, serviceKeys, custodian) if err != nil { return err } diff --git a/cmd/tidepool/main_test.go b/cmd/tidepool/main_test.go index 16e319a..4f9ad17 100644 --- a/cmd/tidepool/main_test.go +++ b/cmd/tidepool/main_test.go @@ -315,7 +315,7 @@ func TestRunRotateKEKResealsARealDatabase(t *testing.T) { // has. Without it the walk would (rightly) fail the run as a restore that // lost the key, and this test would be measuring that alarm instead of the // happy path. - rotationKey, err := identity.LoadOrCreateRotationKey(ctx, store.NewServiceKeys(database), underPrevious) + rotationKey, err := identity.LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), underPrevious) require.NoError(t, err) // NEGATIVE CONTROL: under BRIDGE_KEK alone the seeded world is unreadable. @@ -347,7 +347,7 @@ func TestRunRotateKEKResealsARealDatabase(t *testing.T) { assert.True(t, bytes.Equal(original.Bytes(), reopened.Bytes()), "the re-sealed key must be the ORIGINAL key material; a different one means the actor's repo can never be signed for again") - rescued, err := identity.LoadOrCreateRotationKey(ctx, store.NewServiceKeys(database), underCurrent) + rescued, err := identity.LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), underCurrent) require.NoError(t, err, "the escrow rotation key must open under BRIDGE_KEK alone after the rotation, or the bridge cannot boot once BRIDGE_KEK_PREVIOUS is unset") assert.True(t, bytes.Equal(rotationKey.Bytes(), rescued.Bytes()), @@ -463,6 +463,15 @@ func TestRunRotateKEKDoesNotDisownACompleteInventory(t *testing.T) { underPrevious, err := identity.NewCustodian(previous) require.NoError(t, err) + // The escrow rotation key first, on the empty database — the order the real + // world has it in: the key predates the actor whose blob later goes bad. + // (LoadOrCreateRotationKey is also the boot canary, and it refuses to mint + // over a populated database whose sealed material cannot be read at all. + // Seeding the damage first would be asking it to bless exactly the state it + // exists to stop, which says nothing about the walk this test is measuring.) + _, err = identity.LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), underPrevious) + require.NoError(t, err) + key, err := atcrypto.GeneratePrivateKeyK256() require.NoError(t, err) sealed, err := underPrevious.EncryptActorKey(rotateIntegrationDID, key) @@ -478,8 +487,6 @@ func TestRunRotateKEKDoesNotDisownACompleteInventory(t *testing.T) { "rotate-integration.lemmy-world.tidepool.example", sealed) require.NoError(t, err) - _, err = identity.LoadOrCreateRotationKey(ctx, store.NewServiceKeys(database), underPrevious) - require.NoError(t, err) t.Setenv("DATABASE_URL", databaseURL) t.Setenv("BRIDGE_KEK", rotateTestKEK) diff --git a/internal/identity/kek_canary.go b/internal/identity/kek_canary.go new file mode 100644 index 0000000..ed58f15 --- /dev/null +++ b/internal/identity/kek_canary.go @@ -0,0 +1,159 @@ +package identity + +import ( + "context" + "database/sql" + "fmt" + + "tidepool/internal/errors" +) + +// The boot canary's proof of the KEK. +// +// Opening the plc-rotation row is what normally proves BRIDGE_KEK at startup: +// the row is sealed, it opens, the key is right. That proof has a hole exactly +// where the row is ABSENT — a restore that missed service_keys, a +// hand-provisioned deploy, a database attached to a service that never booted +// against it. LoadOrCreateRotationKey then takes its create branch, seals a +// fresh key under whatever BRIDGE_KEK it was handed, and reports success. The +// canary satisfied itself with a row it wrote a microsecond earlier. +// +// What follows is the most confusing failure this bridge can produce: newly +// minted actors seal and open under the wrong key perfectly consistently and +// federate fine, while every actor minted before the change fails to unseal, +// one delivery at a time. Half the bridge works. Nothing points at the config. +// +// So when there is nothing at rest to read, the KEK is proven against the OTHER +// sealed material instead — the actor keys — and only a database with none of +// it at all is allowed to mint. + +// kekProbeSampleSize is how many sealed rows per table the probe reads. +// +// More than one, because a single damaged blob would otherwise fail a boot on a +// perfectly good KEK. Few, because this runs on the startup path and one row +// that opens is a complete proof: the question is whether this process holds +// the key the database was sealed with, and any one row answers it. +const kekProbeSampleSize = 5 + +// sealedSample is one blob the probe tried, with the binding it was sealed +// under. +type sealedSample struct { + table string + id string + aad []byte + blob []byte +} + +// VerifyKEKAgainstSealedMaterial reports whether custodian can open the key +// material this database already holds. +// +// It returns nil in the two healthy cases and only in those: at least one +// sampled blob opened (the KEK is proven), or the database holds no sealed +// actor material at all (a fresh install, where there is nothing to be wrong +// about). A sample that is entirely unsealable returns an error satisfying +// IsKeyUnsealable. +// +// It is exported because it is a boot-time assertion an operator may want to +// run on its own — a preflight before a KEK change, and the check a restore +// drill wants before it declares the restore good. +func VerifyKEKAgainstSealedMaterial(ctx context.Context, db *sql.DB, custodian *Custodian) error { + if db == nil { + return errors.NewValidationError("db", "must not be nil: the KEK cannot be proven without the material at rest") + } + samples, err := sampleSealedMaterial(ctx, db) + if err != nil { + return err + } + if len(samples) == 0 { + // Nothing sealed anywhere. A fresh install, and the only state in which + // an unproven KEK is the right answer: there is no material it could be + // wrong about, and the keys minted from here on define what "right" + // means for this database. + return nil + } + + var unsealable, malformed int + var firstUnsealable error + for _, sample := range samples { + switch _, err := custodian.open(sample.blob, sample.aad); { + case err == nil: + // One row that opens settles it. Whatever else is in the table, + // this process holds the key this database was sealed with. + return nil + case IsKeyUnsealable(err): + unsealable++ + if firstUnsealable == nil { + firstUnsealable = fmt.Errorf("identity: %s %s: %w", sample.table, sample.id, err) + } + case errors.IsValidation(err): + // Damaged bytes: rejected on shape before any key was consulted, so + // this row is not evidence either way. It is counted so that a + // sample made ENTIRELY of damage cannot be mistaken for a proof. + malformed++ + default: + return fmt.Errorf("identity: verify KEK against %s %s: %w", sample.table, sample.id, err) + } + } + + if unsealable > 0 { + return fmt.Errorf( + "identity: BRIDGE_KEK does not open the key material already in this database "+ + "(%d of %d sampled actor keys refused, %d unreadable): %w", + unsealable, len(samples), malformed, firstUnsealable) + } + // Every sampled row was damaged, so the KEK is neither proven nor + // disproven. Booting on regardless would mint a rotation key under an + // unverified key over a populated database, which is the failure this whole + // file exists to prevent, so the refusal stands — with its own wording, + // because the operator's move here is a restore, not a config change. + return fmt.Errorf( + "identity: cannot verify BRIDGE_KEK: all %d sampled sealed keys in this database are "+ + "malformed (truncated or an unknown ciphertext version), so none of them can prove "+ + "or disprove the configured key; refusing to mint an escrow rotation key over a "+ + "populated database whose key material cannot be read", malformed) +} + +// sampleSealedMaterial reads a few sealed blobs from each actor table, newest +// binding first, skipping the NULL and zero-length columns that carry no +// ciphertext to test. +func sampleSealedMaterial(ctx context.Context, db *sql.DB) ([]sealedSample, error) { + queries := []struct { + table string + aadPre string + query string + }{ + {"ap_actors", actorRSAKeyAADPrefix, ` + SELECT did, rsa_key_sealed FROM ap_actors + WHERE rsa_key_sealed IS NOT NULL AND octet_length(rsa_key_sealed) > 0 + ORDER BY did LIMIT $1`}, + {"bridged_actors", actorKeyAADPrefix, ` + SELECT did, signing_key FROM bridged_actors + WHERE signing_key IS NOT NULL AND octet_length(signing_key) > 0 + ORDER BY id LIMIT $1`}, + } + + var samples []sealedSample + for _, q := range queries { + rows, err := db.QueryContext(ctx, q.query, kekProbeSampleSize) + if err != nil { + return nil, fmt.Errorf("identity: sample sealed %s: %w", q.table, err) + } + for rows.Next() { + var did string + var blob []byte + if err := rows.Scan(&did, &blob); err != nil { + rows.Close() + return nil, fmt.Errorf("identity: scan sealed %s: %w", q.table, err) + } + samples = append(samples, sealedSample{ + table: q.table, id: did, aad: []byte(q.aadPre + did), blob: blob, + }) + } + if err := rows.Err(); err != nil { + rows.Close() + return nil, fmt.Errorf("identity: read sealed %s: %w", q.table, err) + } + rows.Close() + } + return samples, nil +} diff --git a/internal/identity/kek_reseal_pagination_test.go b/internal/identity/kek_reseal_pagination_test.go index 3a9cbc7..e85f332 100644 --- a/internal/identity/kek_reseal_pagination_test.go +++ b/internal/identity/kek_reseal_pagination_test.go @@ -141,7 +141,7 @@ func seedRotationKeyForPagination(t *testing.T, ctx context.Context, database *s t.Helper() custodian, err := NewCustodian(kek) require.NoError(t, err) - _, err = LoadOrCreateRotationKey(ctx, store.NewServiceKeys(database), custodian) + _, err = LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), custodian) require.NoError(t, err) } diff --git a/internal/identity/kek_reseal_test.go b/internal/identity/kek_reseal_test.go index 6b17b6f..709fb24 100644 --- a/internal/identity/kek_reseal_test.go +++ b/internal/identity/kek_reseal_test.go @@ -135,7 +135,7 @@ func seedRotationDrill(t *testing.T) *resealFixture { require.NoError(t, err) // The escrow rotation key, through its real boot path. - fixture.rotationKey, err = LoadOrCreateRotationKey(ctx, fixture.serviceKeys, custodianA) + fixture.rotationKey, err = LoadOrCreateRotationKey(ctx, fixture.database, fixture.serviceKeys, custodianA) require.NoError(t, err) // And the row that shares that table and must never be re-sealed: the @@ -245,7 +245,7 @@ func TestReseal_RotationDrill(t *testing.T) { _, err = currentOnly(t).DecryptActorRSAKey(resealAPActorDID, storedRSAKeySealed(t, fixture.database, resealAPActorDID)) require.Error(t, err, "before the drill the AP RSA key must NOT open under the new KEK alone") - _, err = LoadOrCreateRotationKey(ctx, fixture.serviceKeys, currentOnly(t)) + _, err = LoadOrCreateRotationKey(ctx, fixture.database, fixture.serviceKeys, currentOnly(t)) require.Error(t, err, "before the drill the escrow rotation key must NOT open under the new KEK alone — this is the boot canary that stops the process") @@ -301,7 +301,7 @@ func TestReseal_RotationDrill(t *testing.T) { "the re-sealed AP RSA key must be the ORIGINAL key; a different one silently breaks every HTTP signature this user sends, and peers reject them without telling us") // The escrow rotation key — the one that controls every bridged DID. - rescued, err := LoadOrCreateRotationKey(ctx, fixture.serviceKeys, currentOnly(t)) + rescued, err := LoadOrCreateRotationKey(ctx, fixture.database, fixture.serviceKeys, currentOnly(t)) require.NoError(t, err, "after the drill the bridge must boot on the current KEK alone; if this fails the operator can never unset BRIDGE_KEK_PREVIOUS") assert.True(t, bytes.Equal(fixture.rotationKey.Bytes(), rescued.Bytes()), @@ -589,7 +589,7 @@ func TestReseal_UnreadableBlobsFailLoudlyAndInPlace(t *testing.T) { afterKeys := NewActorKeys(fixture.actors, currentOnly(t)) requireSigningKeyEquals(t, ctx, afterKeys, resealTombstonedDID, repo.KeyUseDelete, fixture.tombstonedKey, "a row reported as Resealed must actually open under the current KEK alone; a report that counts work it did not do is what an operator trusts when they unset BRIDGE_KEK_PREVIOUS") - rescued, err := LoadOrCreateRotationKey(ctx, fixture.serviceKeys, currentOnly(t)) + rescued, err := LoadOrCreateRotationKey(ctx, fixture.database, fixture.serviceKeys, currentOnly(t)) require.NoError(t, err, "the rotation key was reported Resealed, so it must open under the current KEK alone") assert.True(t, bytes.Equal(fixture.rotationKey.Bytes(), rescued.Bytes()), diff --git a/internal/identity/kek_rotation_test.go b/internal/identity/kek_rotation_test.go index 963ba49..7c4b0b1 100644 --- a/internal/identity/kek_rotation_test.go +++ b/internal/identity/kek_rotation_test.go @@ -101,10 +101,22 @@ func TestCustodianWithPrevious_BothKeysFailReportsOneError(t *testing.T) { 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"), + assert.Equal(t, 1, strings.Count(dualErr.Error(), "does not open under"), "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") + + // ONE INCIDENT, ONE CLASS — but not one sentence. This assertion used to + // require the two messages to be byte-identical, so that alerts written + // before a rotation still matched during it. Matching on the message is now + // the wrong seam: ErrKeyUnsealable is the stable thing to key on, and it is + // identical in both cases, while the TEXT has a job the identical version + // could not do. "BRIDGE_KEK failed", read mid-rotation, tells an operator to + // supply the previous key — which is already set, and already failing. + assert.True(t, IsKeyUnsealable(singleErr), "one KEK, refused: unsealable") + assert.True(t, IsKeyUnsealable(dualErr), "two KEKs, both refused: the same class") + assert.Contains(t, singleErr.Error(), "does not open under BRIDGE_KEK:", + "with a single key configured the message must name that one key and stop there") + assert.Contains(t, dualErr.Error(), "does not open under BRIDGE_KEK or BRIDGE_KEK_PREVIOUS", + "with both configured it must say both were tried, or the operator's next move is the one they already made") } func TestCustodianWithPrevious_MalformedBlobIsNotAKEKProblem(t *testing.T) { diff --git a/internal/identity/keys.go b/internal/identity/keys.go index 1aa610f..9ac0cb2 100644 --- a/internal/identity/keys.go +++ b/internal/identity/keys.go @@ -9,6 +9,7 @@ import ( "crypto/aes" "crypto/cipher" "crypto/rand" + "database/sql" "fmt" "github.com/bluesky-social/indigo/atproto/atcrypto" @@ -182,17 +183,41 @@ func (c *Custodian) open(ciphertext, aad []byte) ([]byte, error) { return plaintext, nil } } - // One unreadable blob is one incident. Reporting only the current key's - // failure keeps the message byte-identical to the single-KEK case, so log - // lines and alerts written before a rotation still match during it. - return nil, fmt.Errorf("identity: open sealed key: %w", err) + // EVERY configured key has now refused well-formed bytes, which is a + // CLASS, not a message: no caller can fix it by trying again, and the + // operator's move is to look at BRIDGE_KEK. Returning it typed is what lets + // the delivery worker poison immediately instead of spending a retry budget + // on an answer that cannot change (worker.go, the SignerFor branch). + // + // The text deliberately differs between the single-KEK and mid-rotation + // cases — it used to be byte-identical so that pre-rotation alerts still + // matched — because "BRIDGE_KEK failed" during a rotation reads as an + // instruction to supply the previous key that is already configured and + // already failing. Anything matching on the old wording should match on + // ErrKeyUnsealable instead, which is exactly why it exists. + return nil, KeyUnsealableError{AAD: string(aad), TriedPrevious: c.previous != nil} } // LoadOrCreateRotationKey returns the bridge's escrow rotation key, // generating and persisting it (sealed with the KEK) on first run. The // service_keys create-once semantics make the bootstrap race safe: a loser // re-reads the winner's key. -func LoadOrCreateRotationKey(ctx context.Context, keys store.ServiceKeys, custodian *Custodian) (*atcrypto.PrivateKeyK256, error) { +// +// It is also the bridge's BOOT CANARY for BRIDGE_KEK, which is why it takes the +// database and not just the service_keys store. On the load path the canary is +// the load itself: a row that will not open fails the boot. On the CREATE path +// there is nothing to open, so the KEK is proven against the actor keys already +// at rest instead (VerifyKEKAgainstSealedMaterial) — without that step the +// create branch seals a fresh rotation key under whatever key it was handed and +// reports success, which is a canary satisfying itself with a row it just wrote. +// +// db must not be nil. The alternative — treating a nil database as "skip the +// check" — puts the hole back one caller at a time. +func LoadOrCreateRotationKey(ctx context.Context, db *sql.DB, keys store.ServiceKeys, custodian *Custodian) (*atcrypto.PrivateKeyK256, error) { + if db == nil { + return nil, errors.NewValidationError("db", + "must not be nil: LoadOrCreateRotationKey is the boot canary for BRIDGE_KEK and needs the material at rest to check it against") + } stored, err := keys.Get(ctx, RotationKeyName) if err == nil { return decryptRotationKey(custodian, stored.KeyMaterial) @@ -201,6 +226,13 @@ func LoadOrCreateRotationKey(ctx context.Context, keys store.ServiceKeys, custod return nil, fmt.Errorf("identity: load rotation key: %w", err) } + // The create branch, and the one place a wrong KEK could otherwise pass + // unnoticed. On a populated database this refuses; on an empty one it is a + // no-op and the first boot mints as it always did. + if err := VerifyKEKAgainstSealedMaterial(ctx, db, custodian); err != nil { + return nil, fmt.Errorf("identity: refusing to mint an escrow rotation key: %w", err) + } + fresh, err := atcrypto.GeneratePrivateKeyK256() if err != nil { return nil, fmt.Errorf("identity: generate rotation key: %w", err) diff --git a/internal/identity/keys_test.go b/internal/identity/keys_test.go index 80c2d8b..1d0a075 100644 --- a/internal/identity/keys_test.go +++ b/internal/identity/keys_test.go @@ -126,14 +126,14 @@ func TestCustodian_CrossContextAADRejected(t *testing.T) { func TestLoadOrCreateRotationKey_PersistsAcrossLoads(t *testing.T) { database := testutil.DB(t) - testutil.Truncate(t, database, "service_keys") + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") keys := store.NewServiceKeys(database) custodian := testCustodian(t) ctx := t.Context() - first, err := LoadOrCreateRotationKey(ctx, keys, custodian) + first, err := LoadOrCreateRotationKey(ctx, database, keys, custodian) require.NoError(t, err) - second, err := LoadOrCreateRotationKey(ctx, keys, custodian) + second, err := LoadOrCreateRotationKey(ctx, database, keys, custodian) require.NoError(t, err) assert.True(t, bytes.Equal(first.Bytes(), second.Bytes()), diff --git a/internal/identity/rotation_key_boot_test.go b/internal/identity/rotation_key_boot_test.go index 5894a58..af15199 100644 --- a/internal/identity/rotation_key_boot_test.go +++ b/internal/identity/rotation_key_boot_test.go @@ -58,7 +58,7 @@ func storedRotationMaterial(t *testing.T, ctx context.Context, keys store.Servic // the legitimate way past the canary for an operator mid-rotation. func TestLoadOrCreateRotationKey_WrongKEKIsTheBootCanary(t *testing.T) { database := testutil.DB(t) - testutil.Truncate(t, database, "service_keys") + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") keys := store.NewServiceKeys(database) ctx := t.Context() @@ -69,7 +69,7 @@ func TestLoadOrCreateRotationKey_WrongKEKIsTheBootCanary(t *testing.T) { // rotation key sealed under it. custodianA, err := NewCustodian(kekA) require.NoError(t, err) - original, err := LoadOrCreateRotationKey(ctx, keys, custodianA) + original, err := LoadOrCreateRotationKey(ctx, database, keys, custodianA) require.NoError(t, err) sealedUnderA := storedRotationMaterial(t, ctx, keys) @@ -80,7 +80,7 @@ func TestLoadOrCreateRotationKey_WrongKEKIsTheBootCanary(t *testing.T) { // else — the fat-fingered rotation, or a config rollback. custodianB, err := NewCustodian(kekB) require.NoError(t, err) - _, err = LoadOrCreateRotationKey(ctx, keys, custodianB) + _, err = LoadOrCreateRotationKey(ctx, database, keys, custodianB) // THEN: boot fails. This error is what keeps the process from reaching // ListenAndServe. @@ -100,7 +100,7 @@ func TestLoadOrCreateRotationKey_WrongKEKIsTheBootCanary(t *testing.T) { // the rotation done properly. rotating, err := NewCustodianWithPrevious(kekB, kekA) require.NoError(t, err) - rescued, err := LoadOrCreateRotationKey(ctx, keys, rotating) + rescued, err := LoadOrCreateRotationKey(ctx, database, keys, rotating) // THEN: boot proceeds, on the ORIGINAL key. This is the whole point of // run 1: the canary is not weakened, it is given a legitimate way past. diff --git a/internal/identity/rotation_key_canary_test.go b/internal/identity/rotation_key_canary_test.go new file mode 100644 index 0000000..43cb3e6 --- /dev/null +++ b/internal/identity/rotation_key_canary_test.go @@ -0,0 +1,228 @@ +package identity + +import ( + "context" + "database/sql" + "testing" + + "github.com/bluesky-social/indigo/atproto/atcrypto" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/store" + "tidepool/internal/testutil" +) + +// The boot canary's OTHER half. +// +// rotation_key_boot_test.go pins the canary on the path where the plc-rotation +// row EXISTS: it will not open under the wrong BRIDGE_KEK, boot fails, nothing +// is written. That test's own comment leans on "the create path is unreachable +// on a provisioned database" — which is true only while the row is there. +// +// These tests are about the state where it is NOT: a restore that missed +// service_keys, a hand-rolled provisioning step, a fresh volume attached to a +// populated database. LoadOrCreateRotationKey takes its create branch, seals a +// brand-new key under whatever BRIDGE_KEK it was handed, and returns success — +// so the wrong-KEK config sails past the one check that was supposed to catch +// it. Every actor key sealed under the real KEK then fails to open one at a +// time, deep in the delivery path, while newly minted actors work perfectly. +// +// A canary that can satisfy itself by writing the thing it was meant to read is +// not a canary. The KEK has to be proven against material the bridge did not +// just create. + +const ( + canaryAPActorDID = "did:plc:7iza6de2dwap2sbkpav7c6c6" + canaryBridgedDID = "did:plc:ewvi7nxzyoun6zhxrhs64oiz" + canaryBridgedActor = "https://lemmy.world/u/canary" +) + +// TestLoadOrCreateRotationKey_WrongKEKWithNoRotationRowMustNotSelfSatisfy is +// the red for the canary hole. +// +// GIVEN a populated database whose actor keys are sealed under KEK A and whose +// plc-rotation row is missing, WHEN the bridge boots under KEK B alone, THEN +// the load must FAIL — and must not mint a replacement rotation key under the +// wrong KEK, because that both hides the misconfiguration and strands every +// bridged DID under a rotation authority nobody holds. +func TestLoadOrCreateRotationKey_WrongKEKWithNoRotationRowMustNotSelfSatisfy(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") + ctx := t.Context() + + custodianA, err := NewCustodian(previousTestKEK()) + require.NoError(t, err) + + // A Coves user's AP signing key, sealed under the REAL KEK. + sealedRSA, err := custodianA.EncryptActorRSAKey(canaryAPActorDID, testRSAKey(t)) + require.NoError(t, err) + _, err = store.NewAPActors(database).Create(ctx, store.APActor{ + DID: canaryAPActorDID, + Kind: store.ActorTypePerson, + ActorID: "https://coves.social/ap/actor/" + canaryAPActorDID, + NormalizedOrigin: "coves.social", + LocalPart: "canary", + RSAKeySealed: sealedRSA, + RSAKeyVersion: 1, + PublicKeyPEM: "-----BEGIN PUBLIC KEY-----\nTEST\n-----END PUBLIC KEY-----\n", + }) + require.NoError(t, err) + + // And the plc-rotation row is simply absent: the restore missed it. + keys := store.NewServiceKeys(database) + require.Equal(t, 0, countServiceKeys(t, database), + "the fixture is a POPULATED database with NO rotation key; that is the whole scenario") + + // WHEN: the bridge boots under a different BRIDGE_KEK. + custodianB, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + _, err = LoadOrCreateRotationKey(ctx, database, keys, custodianB) + + // THEN: boot fails, exactly as it does when the row is present. + require.Error(t, err, + "a wrong BRIDGE_KEK must fail the boot canary whether or not the plc-rotation row happens to exist. With the row absent the create branch seals a fresh key under the wrong KEK and reports success — so the bridge serves traffic, mints new actors that work, and fails to open every pre-existing actor key one delivery at a time") + assert.True(t, IsKeyUnsealable(err), + "the failure must be classified as a KEK that does not open the material at rest, not as a generic load error: it is the difference between an operator checking BRIDGE_KEK and an operator suspecting database corruption") + assert.Contains(t, err.Error(), "BRIDGE_KEK", + "and it must name the variable, because that is what the operator greps") + assert.Equal(t, 0, countServiceKeys(t, database), + "a canary that fails must write NOTHING. Minting a rotation key under the wrong KEK is unrecoverable in the direction that matters: every bridged DID names the old rotation authority, and the row that would have proved the KEK wrong is now a row that proves it right") +} + +// TestLoadOrCreateRotationKey_FreshInstallStillCreates is the guard on the fix. +// +// The check above must not turn a genuinely fresh install into a boot failure: +// with nothing sealed anywhere, there is no material to prove the KEK against +// and no wrong answer to give. The first boot mints the rotation key, and that +// is correct. +func TestLoadOrCreateRotationKey_FreshInstallStillCreates(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") + ctx := t.Context() + + custodian, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + keys := store.NewServiceKeys(database) + + fresh, err := LoadOrCreateRotationKey(ctx, database, keys, custodian) + require.NoError(t, err, + "an empty database has no sealed material to check a KEK against, so the first boot must still mint the rotation key") + require.NotNil(t, fresh) + assert.Equal(t, 1, countServiceKeys(t, database)) + + // And the second boot reads back what the first one wrote. + again, err := LoadOrCreateRotationKey(ctx, database, keys, custodian) + require.NoError(t, err) + assert.Equal(t, fresh.Bytes(), again.Bytes()) +} + +// seedCanaryBridgedActor puts one escrowed atproto signing key at rest, sealed +// under kek, and returns the ciphertext as stored. +func seedCanaryBridgedActor(t *testing.T, ctx context.Context, database *sql.DB, kek []byte) []byte { + t.Helper() + custodian, err := NewCustodian(kek) + require.NoError(t, err) + key, err := atcrypto.GeneratePrivateKeyK256() + require.NoError(t, err) + sealed, err := custodian.EncryptActorKey(canaryBridgedDID, key) + require.NoError(t, err) + _, err = store.NewBridgedActors(database).UpsertActor(ctx, store.BridgedActor{ + APActorID: canaryBridgedActor, + ActorType: store.ActorTypePerson, + DID: canaryBridgedDID, + Handle: "canary.lemmy-world.tidepool.example", + SigningKeyEncrypted: sealed, + ConsentState: store.ConsentStateOK, + }) + require.NoError(t, err) + return sealed +} + +// TestLoadOrCreateRotationKey_CanaryReadsTheEscrowKeysToo pins that the proof +// is not limited to one table. +// +// A bridge that has only ever bridged fediverse actors has escrowed signing +// keys in bridged_actors and nothing in ap_actors. Its KEK is just as provable, +// and a canary that only knew about the Coves-side table would wave that +// database through. +func TestLoadOrCreateRotationKey_CanaryReadsTheEscrowKeysToo(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") + ctx := t.Context() + + seedCanaryBridgedActor(t, ctx, database, previousTestKEK()) + + custodianB, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + _, err = LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), custodianB) + + require.Error(t, err, + "escrowed bridged-actor keys prove the KEK exactly as well as the Coves-side ones; a bridge with only fediverse actors must not be waved through") + assert.True(t, IsKeyUnsealable(err)) + assert.Equal(t, 0, countServiceKeys(t, database)) +} + +// TestLoadOrCreateRotationKey_MidRotationMayStillMint is the rotation window's +// half of the rule, and the reason the check is "opens under ANY configured +// KEK" rather than "opens under BRIDGE_KEK". +// +// An operator mid-rotation runs with BRIDGE_KEK_PREVIOUS set. Their material is +// still sealed under the old key and their configuration is correct — so if the +// rotation row is missing (the restore that lost it), the canary must let the +// boot mint a replacement rather than reading a legitimate rotation as a +// misconfiguration. +func TestLoadOrCreateRotationKey_MidRotationMayStillMint(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") + ctx := t.Context() + + seedCanaryBridgedActor(t, ctx, database, previousTestKEK()) + + minted, err := LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), rotatingCustodian(t)) + require.NoError(t, err, + "a custodian that can open the material at rest — under either of its two keys — has proven itself; refusing here would make a documented rotation step a boot failure") + require.NotNil(t, minted) + require.Equal(t, 1, countServiceKeys(t, database)) + + // And what it minted is sealed under the CURRENT key, which is what makes + // the eventual BRIDGE_KEK_PREVIOUS retirement safe. + underCurrentOnly, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + reopened, err := LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), underCurrentOnly) + require.NoError(t, err, + "a key minted mid-rotation must be sealed under the CURRENT KEK, or unsetting BRIDGE_KEK_PREVIOUS later strands it") + assert.Equal(t, minted.Bytes(), reopened.Bytes()) +} + +// TestLoadOrCreateRotationKey_CanaryRefusesWhenEveryProbeIsDamaged covers the +// third answer, which is neither proof nor disproof. +// +// Damaged bytes are rejected on their shape before any key is consulted, so +// they cannot testify about the KEK. A database whose sealed material is ALL +// damaged therefore leaves the KEK unproven — and minting an escrow rotation +// key over a populated database on an unproven key is the exact move this +// canary exists to stop. It refuses, and says restore rather than reconfigure. +func TestLoadOrCreateRotationKey_CanaryRefusesWhenEveryProbeIsDamaged(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "bridged_actors", "ap_actors", "service_keys") + ctx := t.Context() + + sealed := seedCanaryBridgedActor(t, ctx, database, currentTestKEK()) + // Full length, unknown version byte: read, never opened. + damaged := append([]byte{99}, sealed[1:]...) + _, err := database.ExecContext(ctx, + `UPDATE bridged_actors SET signing_key = $2 WHERE did = $1`, canaryBridgedDID, damaged) + require.NoError(t, err) + + custodian, err := NewCustodian(currentTestKEK()) + require.NoError(t, err) + _, err = LoadOrCreateRotationKey(ctx, database, store.NewServiceKeys(database), custodian) + + require.Error(t, err, + "unreadable material proves nothing about the KEK, and minting on an unproven KEK over a populated database is the failure this check exists for") + assert.False(t, IsKeyUnsealable(err), + "but it must NOT be reported as a wrong KEK: the operator's move here is a restore, and sending them to rotate a key that may be perfectly correct wastes the outage") + assert.Equal(t, 0, countServiceKeys(t, database)) +} diff --git a/internal/identity/unsealable.go b/internal/identity/unsealable.go new file mode 100644 index 0000000..1e4cf63 --- /dev/null +++ b/internal/identity/unsealable.go @@ -0,0 +1,82 @@ +package identity + +import ( + stderrors "errors" + "fmt" +) + +// The classification of "this sealed blob did not open", which exists because +// the four ways it can happen send an operator to four different places and +// only ONE of them is fixable by waiting. +// +// wrong KEK well-formed bytes the AEAD refused → ErrKeyUnsealable, below. +// The key at rest and the key in BRIDGE_KEK are different keys. +// No amount of retrying changes that; a config change does. +// corrupt blob truncated, or an unknown version byte. Rejected before the +// cipher is consulted (Custodian.open), so it says nothing +// about any KEK: it stays errors.ErrInvalidInput. +// wrong key type the plaintext opened but is not the key type the caller +// needs. The KEK is right; the column holds the wrong thing. +// missing no row, no ciphertext: errors.ErrNotFound, and the only one +// of the four that a replication lag can produce. +// +// Before this split every one of them arrived as one opaque fmt.Errorf chain, +// and the only consumer that classifies signer errors — the outbound delivery +// worker — read the whole chain as transient. A permanently wrong BRIDGE_KEK +// spent the full retry budget per delivery and then poisoned, hours later, +// under the AEAD's own words: "message authentication failed". That sentence +// sends an operator to look for corrupted bytes. The bytes are fine. + +// ErrKeyUnsealable marks a sealed blob that is well formed and failed AEAD +// authentication under EVERY configured KEK — the signal that the material at +// rest was sealed under a key this process does not hold. +// +// It is deliberately NOT a validation error: a caller distinguishing "damaged +// bytes" from "wrong key" has to be able to ask the two questions separately, +// and the reseal drill (reseal.go) already leans on malformed being the +// validation case. +// +// A wrong KEK is the headline cause but not the only one that can produce it: +// a single flipped bit in the ciphertext or the GCM tag lands here too, and so +// does a blob copied out of another row or another column, because every +// ciphertext is AAD-bound to its DID and its domain. What the class means +// precisely is "the AEAD said no", and what follows from that — for every +// caller — is that retrying the same bytes with the same configuration will +// get the same answer forever. +var ErrKeyUnsealable = stderrors.New("identity: sealed key does not open under the configured BRIDGE_KEK") + +// KeyUnsealableError names the blob that would not open and the keys that were +// tried, so the message an operator finds in a log line or a dead-letter row +// carries the environment variable they need to grep for. +type KeyUnsealableError struct { + // AAD is the ciphertext's domain+owner binding, which is also the most + // useful identifier: it names both the column the blob came from and the + // DID it belongs to. + AAD string + // TriedPrevious reports that BRIDGE_KEK_PREVIOUS was also configured and + // also failed. Mid-rotation this is the difference between an operator + // supplying the previous key and an operator discovering that the key they + // already supplied is not the right one either. + TriedPrevious bool +} + +func (e KeyUnsealableError) Error() string { + keys := "BRIDGE_KEK" + if e.TriedPrevious { + keys = "BRIDGE_KEK or BRIDGE_KEK_PREVIOUS" + } + return fmt.Sprintf( + "identity: sealed key %q does not open under %s: the material at rest was sealed "+ + "under a different key-encryption key than this process is configured with. "+ + "This is a KEK CONFIGURATION problem, not data corruption — retrying cannot "+ + "change the answer (cipher: message authentication failed)", + e.AAD, keys) +} + +// Unwrap makes errors.Is(err, ErrKeyUnsealable) true. +func (e KeyUnsealableError) Unwrap() error { return ErrKeyUnsealable } + +// IsKeyUnsealable reports whether err is, wraps, or unwraps to +// ErrKeyUnsealable. It is false for a nil error: a predicate that said yes to +// success would poison every delivery on the bridge. +func IsKeyUnsealable(err error) bool { return stderrors.Is(err, ErrKeyUnsealable) } diff --git a/internal/identity/unsealable_test.go b/internal/identity/unsealable_test.go new file mode 100644 index 0000000..57c872b --- /dev/null +++ b/internal/identity/unsealable_test.go @@ -0,0 +1,126 @@ +package identity + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" +) + +// Telling "the KEK is wrong" apart from everything else that can go wrong with +// a sealed blob. +// +// Every failure to unseal used to arrive as one opaque fmt.Errorf chain, and +// the only consumer that classifies it — the outbound delivery worker — read +// the whole chain as transient and retried. A permanently wrong BRIDGE_KEK +// therefore burned the retry budget on every actor, for hours, and then +// poisoned under a message reading "message authentication failed": the exact +// wording that sends an operator looking for data corruption instead of the +// config line they changed. +// +// The four conditions are categorically different and only one of them is +// about the KEK: +// +// wrong KEK the blob is well formed and the AEAD refused it → ErrKeyUnsealable +// corrupt blob truncated, or an unknown version byte → validation +// non-RSA key the plaintext opened but is not a key we can sign with +// missing actor no row, no ciphertext → not found +// +// These tests pin the split at the seam that produces it, because the worker's +// decision (retry vs. poison immediately) is only as good as the classification +// it is handed. + +const unsealableDID = "did:plc:ewvi7nxzyoun6zhxrhs64oiz" + +// custodianFor is a single-KEK custodian, or a fatal test failure. +func custodianFor(t *testing.T, kek []byte) *Custodian { + t.Helper() + c, err := NewCustodian(kek) + require.NoError(t, err) + return c +} + +// TestDecryptActorRSAKey_WrongKEKIsUnsealable is the headline case: the AP +// signing key of a Coves user, sealed before a KEK change and opened after it. +func TestDecryptActorRSAKey_WrongKEKIsUnsealable(t *testing.T) { + sealed, err := custodianFor(t, previousTestKEK()).EncryptActorRSAKey(unsealableDID, testRSAKey(t)) + require.NoError(t, err) + + _, err = custodianFor(t, currentTestKEK()).DecryptActorRSAKey(unsealableDID, sealed) + require.Error(t, err) + + assert.True(t, IsKeyUnsealable(err), + "a well-formed blob the AEAD refused is THE wrong-KEK signal, and it has to be its own class: it is the one unsealing failure that no amount of retrying can fix, and the one that a config line caused") + assert.False(t, errors.IsValidation(err), + "a wrong KEK is not a malformed input. Folding it into validation puts it in the same bucket as a truncated blob, which sends the operator to the storage layer") + assert.False(t, errors.IsNotFound(err), + "the ciphertext is right there; nothing is missing") + assert.Contains(t, err.Error(), "BRIDGE_KEK", + "the message must name the environment variable, because the operator's first move is to grep for it") +} + +// TestDecryptActorKey_WrongKEKIsUnsealable is the same for the atproto escrow +// keys — a different AAD domain and a different key type, one classification. +func TestDecryptActorKey_WrongKEKIsUnsealable(t *testing.T) { + _, sealed := sealedUnder(t, previousTestKEK()) + + _, err := custodianFor(t, currentTestKEK()).DecryptActorKey(rotationTestDID, sealed) + require.Error(t, err) + assert.True(t, IsKeyUnsealable(err), + "both sealed domains must classify a refused AEAD the same way; a caller that has to know which column the blob came from cannot classify anything") + assert.Contains(t, err.Error(), "BRIDGE_KEK") +} + +// TestUnsealable_MalformedBlobIsNotAKEKProblem is the boundary the reseal drill +// already depends on, restated where the classification now lives: bytes +// rejected BEFORE the AEAD is consulted say nothing about any KEK. +func TestUnsealable_MalformedBlobIsNotAKEKProblem(t *testing.T) { + _, sealed := sealedUnder(t, previousTestKEK()) + current := custodianFor(t, currentTestKEK()) + + for _, tc := range []struct { + name string + blob []byte + }{ + {"truncated", sealed[:1+current.aead.NonceSize()]}, + {"unknown version byte", append([]byte{99}, sealed[1:]...)}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := current.DecryptActorKey(rotationTestDID, tc.blob) + require.Error(t, err) + assert.True(t, errors.IsValidation(err), + "a blob rejected on its shape never reached the cipher, so it stays a validation error") + assert.False(t, IsKeyUnsealable(err), + "reporting damaged bytes as a KEK problem would send an operator to rotate a key that is working perfectly — and, at the worker, would poison a delivery under a config alarm that is not the fault") + }) + } +} + +// TestUnsealable_MidRotationCustodianNamesBothKeys pins what an operator sees +// when BRIDGE_KEK_PREVIOUS is set and the blob opens under neither: the message +// has to say both were tried, or the operator's next move is to set the +// variable they already set. +func TestUnsealable_MidRotationCustodianNamesBothKeys(t *testing.T) { + _, sealed := sealedUnder(t, strangerTestKEK()) + + _, err := rotatingCustodian(t).DecryptActorKey(rotationTestDID, sealed) + require.Error(t, err) + assert.True(t, IsKeyUnsealable(err)) + assert.True(t, strings.Contains(err.Error(), "BRIDGE_KEK_PREVIOUS"), + "mid-rotation, a blob that opens under neither key must say so: an operator told only that BRIDGE_KEK failed will 'fix' it by supplying the previous key that is already configured and already failing") +} + +// TestUnsealable_RightKEKStillOpens is the control. A classification that fires +// on the happy path would poison every delivery on the bridge. +func TestUnsealable_RightKEKStillOpens(t *testing.T) { + key, sealed := sealedUnder(t, currentTestKEK()) + + opened, err := custodianFor(t, currentTestKEK()).DecryptActorKey(rotationTestDID, sealed) + require.NoError(t, err) + assert.Equal(t, key.Bytes(), opened.Bytes()) + assert.False(t, IsKeyUnsealable(nil), + "the predicate must be false for a nil error; a helper that says yes to success is a helper that poisons everything") +} diff --git a/internal/outbound/signer_kek_test.go b/internal/outbound/signer_kek_test.go new file mode 100644 index 0000000..d96caa3 --- /dev/null +++ b/internal/outbound/signer_kek_test.go @@ -0,0 +1,116 @@ +package outbound + +import ( + "context" + "crypto/sha256" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/ap" + "tidepool/internal/errors" + "tidepool/internal/identity" + "tidepool/internal/store" +) + +// What the worker does when SignerFor fails, split by WHY it failed. +// +// The worker used to treat every signer error as transient — "a KEK blip, a +// not-yet-replicated actor: retry rather than poison". Two of those are not the +// same thing. A missing or not-yet-replicated actor really does resolve on its +// own. A BRIDGE_KEK that does not open the actor's sealed key never does: the +// key at rest and the key in the environment are simply different keys, and the +// delivery burns its whole retry budget before poisoning hours later under an +// excerpt reading "message authentication failed" — which reads as data +// corruption, not as the config change that caused it. +// +// The split matters most in the shape this failure actually takes in +// production: newly minted actors seal and open fine under the wrong key, while +// every actor that existed before the change fails. Half the bridge works. The +// operator needs the class to say KEK. + +// unsealableSignerError is the error the real signer path produces when the +// configured BRIDGE_KEK does not open an actor's sealed AP key: a genuine +// custodian mismatch, wrapped exactly as personas.actorSigner wraps it, so this +// test cannot pass against a hand-written sentinel the production path never +// emits. +func unsealableSignerError(t *testing.T) error { + t.Helper() + kekA := sha256.Sum256([]byte("outbound-test-kek-at-rest")) + kekB := sha256.Sum256([]byte("outbound-test-kek-configured")) + + atRest, err := identity.NewCustodian(kekA[:]) + require.NoError(t, err) + key, err := ap.GenerateRSAKey() + require.NoError(t, err) + sealed, err := atRest.EncryptActorRSAKey(wActorDID, key) + require.NoError(t, err) + + configured, err := identity.NewCustodian(kekB[:]) + require.NoError(t, err) + _, err = configured.DecryptActorRSAKey(wActorDID, sealed) + require.Error(t, err, "the fixture must really fail to unseal, or this test proves nothing") + return fmt.Errorf("personas: unseal AP key for %s: %w", wActorDID, err) +} + +// TestWorker_UnsealableSignerKeyPoisonsImmediately is the red for the +// misclassification. +// +// GIVEN a delivery whose actor's key will not open under the configured +// BRIDGE_KEK, WHEN the worker runs it with its retry budget untouched, THEN the +// delivery poisons on the FIRST attempt, under a class that names the +// configuration, and nothing is POSTed. +func TestWorker_UnsealableSignerKeyPoisonsImmediately(t *testing.T) { + conn := workerTestDB(t) + seedWorkerActor(t, conn, true, false) + id := seedDelivery(t, conn, "Create", "", createPayload("x")) + + sender := &fakeSender{} + w := newWorker(t, conn, sender, func(o *WorkerOptions) { + o.Signers = fakeSigners{err: unsealableSignerError(t)} + }) + + worked, err := w.DeliverNext(context.Background()) + require.NoError(t, err) + assert.True(t, worked) + + d := getDelivery(t, conn, id) + assert.Equal(t, store.DeliveryStatePoisoned, d.State, + "a key that does not open under the configured BRIDGE_KEK is not a blip: retrying it cannot change the answer, and the retry budget only delays the alarm by hours while every pre-existing actor fails the same way") + assert.Equal(t, store.PoisonClassKEKMisconfigured, d.LastErrorClass, + "the class an operator reads off the dead-letter table must say the KEK is wrong. Under the generic signer class this is indistinguishable from a replication lag, and the excerpt underneath it says 'message authentication failed', which reads as corrupted data") + assert.Contains(t, d.ResponseExcerpt, "BRIDGE_KEK", + "the excerpt is where the operator lands; it has to name the variable") + assert.Zero(t, sender.count(), + "nothing may be POSTed: there is no signature to send") +} + +// TestWorker_MissingSignerStillRetries is the other half, and the reason the +// split is not simply "poison every signer error". +// +// A DID whose actor row has not replicated yet is exactly the transient case +// the old comment described. It must keep its retries: poisoning it would turn +// a few seconds of lag into a permanent non-delivery. +func TestWorker_MissingSignerStillRetries(t *testing.T) { + conn := workerTestDB(t) + seedWorkerActor(t, conn, true, false) + id := seedDelivery(t, conn, "Create", "", createPayload("x")) + + sender := &fakeSender{} + w := newWorker(t, conn, sender, func(o *WorkerOptions) { + o.Signers = fakeSigners{err: errors.NewNotFoundError("ap_actor", wActorDID)} + }) + + worked, err := w.DeliverNext(context.Background()) + require.NoError(t, err) + assert.True(t, worked) + + d := getDelivery(t, conn, id) + assert.Equal(t, store.DeliveryStatePending, d.State, + "an actor row that has not arrived yet is genuinely transient: the delivery stays pending and retries") + assert.Equal(t, store.PoisonClassSigner, d.LastErrorClass, + "and it keeps the generic signer class, so the KEK class stays a signal rather than a synonym for 'signer trouble'") + assert.Zero(t, sender.count()) +} diff --git a/internal/outbound/worker.go b/internal/outbound/worker.go index 3e646f8..5820313 100644 --- a/internal/outbound/worker.go +++ b/internal/outbound/worker.go @@ -15,6 +15,7 @@ import ( "tidepool/internal/ap" "tidepool/internal/errors" + "tidepool/internal/identity" "tidepool/internal/store" ) @@ -323,8 +324,40 @@ func (w *Worker) handle(ctx context.Context, delivery *store.OutboundDelivery) e func (w *Worker) deliver(ctx context.Context, delivery *store.OutboundDelivery, activity *store.OutboundActivity) error { signer, err := w.signers.SignerFor(ctx, activity.ActorDID) if err != nil { - // A signer that cannot be resolved right now is transient (a KEK blip, - // a not-yet-replicated actor): retry rather than poison. + if identity.IsKeyUnsealable(err) { + // The actor's key is well-formed ciphertext that opened under NO + // configured KEK. Retrying re-reads the same bytes with the same + // BRIDGE_KEK and gets the same answer, so the retry budget buys + // nothing but hours of delay before poisoning under an excerpt that + // says "message authentication failed" — which reads as corrupted + // data and sends the operator to the database instead of the config. + // + // WHY THIS CANNOT FIRE DURING A SANCTIONED KEK ROTATION. The + // rotation runbook has the bridge run with BRIDGE_KEK_PREVIOUS set + // (NewCustodianWithPrevious opens under either key, so nothing is + // unsealable), then re-seal every blob (identity.Reseal), and only + // then unset the previous key. Reseal walks the two actor tables + // FIRST and the plc-rotation row LAST, and refuses to exit zero + // while any row is unmoved — so a worker that starts with the new + // KEK alone can only have got past LoadOrCreateRotationKey (the + // boot canary) on a database whose actor keys were already moved. + // A process that reaches this line with an unsealable key is + // therefore not mid-rotation: it is misconfigured, or the blob is + // damaged. Both are terminal for this delivery. + // + // Terminal, not lost: a poisoned delivery is a dead-letter row an + // operator redrives after fixing BRIDGE_KEK, which is the whole + // point of failing loudly on the first attempt instead of quietly + // on the last. + w.logger.Error("actor signing key does not open under the configured BRIDGE_KEK; poisoning without retry", + "activity", delivery.ActivityID, "actor", activity.ActorDID, + "inbox", delivery.TargetInbox, "error", err) + return w.poison(ctx, delivery, store.PoisonClassKEKMisconfigured, err.Error(), 0) + } + // Everything else a signer can fail with IS transient in the way the + // retry budget assumes: an actor row that has not replicated yet, a + // database blip. Those resolve on their own, and poisoning them would + // turn seconds of lag into a permanent non-delivery. return w.releaseOrPoison(ctx, delivery, store.PoisonClassSigner, err.Error(), 0) } diff --git a/internal/store/divergence.go b/internal/store/divergence.go index ce78c6e..fd0639f 100644 --- a/internal/store/divergence.go +++ b/internal/store/divergence.go @@ -729,6 +729,18 @@ const ( // budget is exhausted (releaseOrPoison), which makes it LOOK like a retried // wire failure on every column; it is not one. PoisonClassSigner = "signer" + // PoisonClassKEKMisconfigured: the actor's signing key is well-formed + // ciphertext that opened under NO configured KEK — the material at rest was + // sealed under a different BRIDGE_KEK than this process holds. Split out of + // PoisonClassSigner because the two need OPPOSITE handling and send an + // operator to opposite places: a missing actor row resolves itself and is + // retried, while a wrong KEK gives the same answer on every attempt and + // poisons on the FIRST one (worker.go says why that is safe mid-rotation). + // Under the generic signer class this arrives as a retried wire-ish failure + // whose excerpt reads "message authentication failed", which looks like data + // corruption; the class is the only place the word KEK appears in the + // dead-letter table. + PoisonClassKEKMisconfigured = "kek_misconfigured" ) // neverReachedTheWireClasses are those classes as the divergence sweep @@ -750,7 +762,7 @@ const ( // above AND to this list, or it inflates these counts. var neverReachedTheWireClasses = []string{ PoisonClassParentUnaccepted, PoisonClassParentPoisoned, PoisonClassParentCancelled, - PoisonClassCrossAuthority, PoisonClassSigner, + PoisonClassCrossAuthority, PoisonClassSigner, PoisonClassKEKMisconfigured, } // unknownDeliveryOutcomeRows is the population, shared by the example list and diff --git a/internal/store/divergence_unknown_test.go b/internal/store/divergence_unknown_test.go index d60dbb8..836ea76 100644 --- a/internal/store/divergence_unknown_test.go +++ b/internal/store/divergence_unknown_test.go @@ -264,6 +264,10 @@ func TestUnknownDeliveryOutcomes_APoisonThatNeverReachedTheWireIsNotUnknown(t *t // Every never-wire class, exactly as the worker writes them. neverSent := []string{ "parent_unaccepted", "parent_poisoned", "parent_cancelled", "cross_authority", "signer", + // A key that will not open under the configured BRIDGE_KEK is decided + // before any POST too — earlier than the generic signer class, since it + // poisons on the first attempt rather than after the budget. + "kek_misconfigured", } for _, class := range neverSent { seedPoisonedDelivery(t, database,