amadan
server: VAPID pair out of the DB — a sidecar keyfile hibernation never ships to S3 (#724 step 5) (#750)
* server: move the VAPID pair out of the DB into a sidecar keyfile — #724 step 5 The Web Push VAPID private key lived in the config table, which hibernation now replicates to S3 — a bucket leak would hand out a private key. The pair now lives in a 0600 keyfile next to the DB (<db>.vapid, like <db>-blobs/); legacy config rows migrate into the file on first boot (same keys, so existing subscriptions keep working) and are scrubbed if a restored snapshot resurrects them. A corrupt keyfile fails loud rather than silently re-minting (that would break every subscription). deleteInstance archives the keyfile to trash with the DB; docs/secrets.md + docs/instance-migration.md updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * server: fail loud on unreadable keyfile + transient DB errors; log fresh mints over live subscriptions Review findings: a non-ENOENT read error (root-owned copy after a migration, disk trouble) fell through to the mint path and renamed a fresh pair over the real file — the exact silent rotation the malformed-file check exists to stop. Same family: a transient SQLite error during the one migration boot minted fresh and then destroyed the intact legacy rows. Both now error and retry next boot. A genuine fresh mint with existing subscriptions logs loudly (it means a DB restored without its keyfile — those subscriptions are dead until members revisit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> ---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
6 files changed,
+164
−13
control.go+1 −1db.go+6 −1db_test.go+78 −0docs/instance-migration.md+5 −0docs/secrets.md+9 −4push.go+65 −7
diff --git a/control.go b/control.go| index 40eceb6..39375ac 100644 |
| --- a/control.go |
| +++ b/control.go |
| @@ -477,7 +477,7 @@ func deleteInstance(reg *Registry, host string) error { |
| if err := os.MkdirAll(trash, 0o700); err != nil { |
| return fmt.Errorf("trash dir: %v", err) |
| } |
| - for _, name := range []string{host + ".db", host + ".db-wal", host + ".db-shm", host + ".db" + litestream.MetaDirSuffix, host + ".env"} { |
| + for _, name := range []string{host + ".db", host + ".db-wal", host + ".db-shm", host + ".db" + litestream.MetaDirSuffix, host + ".env", host + ".vapid"} { |
| src := filepath.Join(instanceDataDir, name) |
| if _, err := os.Stat(src); err == nil { |
| if err := os.Rename(src, filepath.Join(trash, name)); err != nil { |
diff --git a/db.go b/db.go| index ad00839..5eef5ac 100644 |
| --- a/db.go |
| +++ b/db.go |
| @@ -218,6 +218,10 @@ type DB struct { |
| // (<db>-blobs/), but nothing outside blobstore.go may assume that — the |
| // blobs *table* keeps the metadata (size, thread tag) either way. |
| blobs blobStore |
| + // vapidPath is the sidecar keyfile (<db>.vapid) holding the Web Push VAPID |
| + // pair — outside the DB so hibernation's S3 replica never carries a private |
| + // key (see ensureVAPID in push.go). |
| + vapidPath string |
| } |
| func openDB(path string) (*DB, error) { |
| @@ -241,7 +245,8 @@ func openDB(path string) (*DB, error) { |
| if err != nil { |
| return nil, err |
| } |
| - db := &DB{sql: d, blobs: blobs} |
| + db := &DB{sql: d, blobs: blobs, |
| + vapidPath: strings.TrimSuffix(path, filepath.Ext(path)) + ".vapid"} |
| if err := db.migrate(); err != nil { |
| return nil, err |
| } |
diff --git a/db_test.go b/db_test.go| index 0c0f8e4..c254072 100644 |
| --- a/db_test.go |
| +++ b/db_test.go |
| @@ -2,6 +2,7 @@ package main |
| import ( |
| "fmt" |
| + "os" |
| "testing" |
| "time" |
| ) |
| @@ -229,6 +230,83 @@ func TestVAPIDPersists(t *testing.T) { |
| if pub != pub2 || priv != priv2 { |
| t.Fatal("VAPID keys should persist across calls") |
| } |
| + // The pair lives in the sidecar keyfile, never the DB (the DB replicates |
| + // to S3 under hibernation; the private key must not ride along). |
| + if fi, err := os.Stat(db.vapidPath); err != nil { |
| + t.Fatalf("vapid keyfile: %v", err) |
| + } else if fi.Mode().Perm() != 0o600 { |
| + t.Fatalf("vapid keyfile mode = %v, want 0600", fi.Mode().Perm()) |
| + } |
| + if _, ok, _ := db.getConfig("vapid_private"); ok { |
| + t.Fatal("vapid_private must not be stored in the config table") |
| + } |
| + if _, ok, _ := db.getConfig("vapid_public"); ok { |
| + t.Fatal("vapid_public must not be stored in the config table") |
| + } |
| +} |
| + |
| +// Pre-keyfile instances hold the pair in the config table; first boot must move |
| +// it into the file (same keys — regenerating would break every subscription) |
| +// and scrub the DB copy. |
| +func TestVAPIDMigratesLegacyConfigRows(t *testing.T) { |
| + db := newTestDB(t) |
| + if err := db.setConfig("vapid_public", "legacy-pub"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := db.setConfig("vapid_private", "legacy-priv"); err != nil { |
| + t.Fatal(err) |
| + } |
| + pub, priv, err := db.ensureVAPID() |
| + if err != nil || pub != "legacy-pub" || priv != "legacy-priv" { |
| + t.Fatalf("ensureVAPID: %v pub=%q priv=%q, want the legacy pair", err, pub, priv) |
| + } |
| + if _, ok, _ := db.getConfig("vapid_private"); ok { |
| + t.Fatal("legacy vapid_private row should be deleted after migration") |
| + } |
| + // A restored older DB snapshot may resurrect the rows; the file stays |
| + // authoritative and the rows are scrubbed again. |
| + db.setConfig("vapid_private", "resurrected") |
| + pub2, priv2, err := db.ensureVAPID() |
| + if err != nil || pub2 != "legacy-pub" || priv2 != "legacy-priv" { |
| + t.Fatalf("keyfile should win over resurrected config rows: %v %q %q", err, pub2, priv2) |
| + } |
| + if _, ok, _ := db.getConfig("vapid_private"); ok { |
| + t.Fatal("resurrected vapid_private row should be scrubbed") |
| + } |
| +} |
| + |
| +func TestVAPIDMalformedKeyfileFailsLoud(t *testing.T) { |
| + db := newTestDB(t) |
| + if err := os.WriteFile(db.vapidPath, []byte("not json"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := db.ensureVAPID(); err == nil { |
| + t.Fatal("a corrupt keyfile must error, not silently re-mint (that breaks push subscriptions)") |
| + } |
| +} |
| + |
| +// A keyfile that exists but can't be READ (root-owned after an operator copy, |
| +// disk trouble) must error too — falling through to the mint path would rename |
| +// a fresh pair over the real one and silently break every subscription. |
| +func TestVAPIDUnreadableKeyfileFailsLoud(t *testing.T) { |
| + if os.Getuid() == 0 { |
| + t.Skip("root reads through file modes") |
| + } |
| + db := newTestDB(t) |
| + if err := os.WriteFile(db.vapidPath, []byte(`{"Public":"p","Private":"k"}`), 0o000); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := db.ensureVAPID(); err == nil { |
| + t.Fatal("an unreadable keyfile must error, not mint over the real pair") |
| + } |
| + // The file survives untouched for the operator to fix. |
| + if err := os.Chmod(db.vapidPath, 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + pub, _, err := db.ensureVAPID() |
| + if err != nil || pub != "p" { |
| + t.Fatalf("keyfile should be intact after the failed boot: %v pub=%q", err, pub) |
| + } |
| } |
| func findUser(t *testing.T, users []User, id string) User { |
diff --git a/docs/instance-migration.md b/docs/instance-migration.md| index f922f1e..1e4df79 100644 |
| --- a/docs/instance-migration.md |
| +++ b/docs/instance-migration.md |
| @@ -61,6 +61,11 @@ These are the disk-backed attachment bytes the DB references by id; copy them at |
| or after the DB snapshot (blobs uploaded to the source afterward are subject to |
| the same gap check as messages — see step 5). |
| +**Also copy the VAPID keyfile** (`/data/<host>.vapid`, another DB sibling — |
| +kept out of the DB so S3 replicas never carry a private key). Without it the |
| +destination re-mints the pair on first boot and every member's web-push |
| +subscription silently breaks until their next visit re-subscribes them. |
| + |
| ### 4. Import on the destination (preserve token + config) |
| With the migrated DB already at `/data/<host>.db`, run `add-instance` with |
diff --git a/docs/secrets.md b/docs/secrets.md| index 8864244..ea759c6 100644 |
| --- a/docs/secrets.md |
| +++ b/docs/secrets.md |
| @@ -29,10 +29,15 @@ live in `CLAUDE.local.md` and 1Password, not here. |
| mint a replacement via the relay's admin API, swap the env, restart, revoke |
| the old — no Apple interaction needed. |
| - **VAPID keypair** — Web Push self-identification, generated on first use |
| - and stored in the instance DB (`config` table). *Blast radius*: junk web |
| - pushes to that instance's subscribers. *Rotate*: delete the config rows and |
| - restart to regenerate; existing push subscriptions break and members |
| - re-subscribe on next visit. |
| + and stored in a 0600 sidecar keyfile next to the DB (`<db>.vapid`) — |
| + deliberately *not* in the DB, so hibernation's S3 replica of `lchat.db` |
| + never carries a private key (docs/no-plaintext.md § 2; pre-keyfile |
| + instances migrate the legacy `config` rows into the file on first boot). |
| + Corollary: a hibernated instance restored onto a *different* box loses the |
| + file and re-mints on wake — see *Rotate* for what that costs. *Blast |
| + radius*: junk web pushes to that instance's subscribers. *Rotate*: delete |
| + the keyfile and restart to regenerate; existing push subscriptions break |
| + and members re-subscribe on next visit. |
| ## Home |
diff --git a/push.go b/push.go| index 10bbb0d..3febe86 100644 |
| --- a/push.go |
| +++ b/push.go |
| @@ -3,11 +3,13 @@ package main |
| import ( |
| "encoding/json" |
| "errors" |
| + "fmt" |
| "io" |
| "log" |
| "net" |
| "net/http" |
| "net/url" |
| + "os" |
| "sync" |
| "sync/atomic" |
| "time" |
| @@ -132,25 +134,81 @@ func (db *DB) deleteSubscriptionsForToken(token string) error { |
| // ensureVAPID returns the server's VAPID keypair, generating and persisting one on |
| // first use. VAPID lets the server authenticate itself to the browser push services |
| // with no third-party account — it is unrelated to message encryption. |
| +// |
| +// The pair lives in a 0600 sidecar file next to the DB (<db>.vapid, like the |
| +// <db>-blobs dir), deliberately NOT in the config table: hibernation replicates |
| +// lchat.db to S3, and a bucket leak must not hand out private keys of any kind |
| +// (docs/no-plaintext.md § 2). The pair used to live in config — those legacy |
| +// rows are moved into the file and deleted on first boot, so existing push |
| +// subscriptions keep working across the change. |
| func (db *DB) ensureVAPID() (pub, priv string, err error) { |
| - pub, okPub, _ := db.getConfig("vapid_public") |
| - priv, okPriv, _ := db.getConfig("vapid_private") |
| - if okPub && okPriv { |
| - return pub, priv, nil |
| + // The file is authoritative once it exists. A restored/older DB snapshot may |
| + // still carry the legacy config rows — clear them so the DB (and its S3 |
| + // replica) never re-grows a private key. |
| + b, readErr := os.ReadFile(db.vapidPath) |
| + if readErr == nil { |
| + var k struct{ Public, Private string } |
| + if err := json.Unmarshal(b, &k); err != nil || k.Public == "" || k.Private == "" { |
| + // Fail loud rather than silently minting a new pair: regenerating |
| + // breaks every existing push subscription, so a corrupt file is an |
| + // operator problem (restore or delete it), not something to paper over. |
| + return "", "", fmt.Errorf("vapid keyfile %s is malformed; restore it or delete it to re-mint (breaks existing push subscriptions)", db.vapidPath) |
| + } |
| + db.deleteVAPIDConfig() |
| + return k.Public, k.Private, nil |
| + } |
| + if !os.IsNotExist(readErr) { |
| + // EACCES/EIO etc. must NOT fall through to the mint path — that would |
| + // rename a fresh pair over a file that exists but couldn't be read |
| + // (e.g. an operator's root-owned copy after a migration) and silently |
| + // break every subscription. Only a genuinely absent file mints. |
| + return "", "", fmt.Errorf("vapid keyfile %s: %w", db.vapidPath, readErr) |
| + } |
| + // Legacy: the pair is still in the DB — move it into the file. Fresh |
| + // instance: mint one. Either way the file is written before the config |
| + // rows are deleted, so a crash in between leaves both copies (file wins |
| + // next boot), never neither. |
| + pub, okPub, errPub := db.getConfig("vapid_public") |
| + priv, okPriv, errPriv := db.getConfig("vapid_private") |
| + if errPub != nil || errPriv != nil { |
| + // A transient DB error here must not mint a fresh pair (the delete |
| + // below would then destroy the intact legacy rows) — retry next boot. |
| + return "", "", fmt.Errorf("vapid config read: %v / %v", errPub, errPriv) |
| + } |
| + if !okPub || !okPriv { |
| + priv, pub, err = webpush.GenerateVAPIDKeys() |
| + if err != nil { |
| + return "", "", err |
| + } |
| + // A fresh mint with existing subscriptions means the old pair is gone |
| + // (a DB restored onto a new box) — say so, since those subscriptions |
| + // are now dead until members revisit (docs/secrets.md). |
| + if subs, _ := db.listSubscriptions(); len(subs) > 0 { |
| + log.Printf("vapid: minted a NEW keypair with %d existing push subscriptions — they break until members re-subscribe on next visit", len(subs)) |
| + } |
| } |
| - priv, pub, err = webpush.GenerateVAPIDKeys() |
| + blob, err := json.Marshal(struct{ Public, Private string }{pub, priv}) |
| if err != nil { |
| return "", "", err |
| } |
| - if err = db.setConfig("vapid_public", pub); err != nil { |
| + // Write-temp-then-rename so a crash mid-write can't leave a torn keyfile. |
| + tmp := db.vapidPath + ".tmp" |
| + if err = os.WriteFile(tmp, blob, 0o600); err != nil { |
| return "", "", err |
| } |
| - if err = db.setConfig("vapid_private", priv); err != nil { |
| + if err = os.Rename(tmp, db.vapidPath); err != nil { |
| return "", "", err |
| } |
| + db.deleteVAPIDConfig() |
| return pub, priv, nil |
| } |
| +// deleteVAPIDConfig scrubs the legacy config-table copy of the VAPID pair. |
| +// Best-effort: a failed delete just means the next boot retries. |
| +func (db *DB) deleteVAPIDConfig() { |
| + db.sql.Exec("DELETE FROM config WHERE key IN ('vapid_public', 'vapid_private')") |
| +} |
| + |
| // pushNewMessage delivers the ciphertext blob to every subscription. The server |
| // never sees plaintext: the payload IS the same ciphertext stored in the DB, and |
| // the service worker decrypts it locally with the group key. Expired subscriptions |