From 39fb9cbf6c32f65ee1c17e6a04161229442dd2fe Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Thu, 22 Oct 2026 23:00:05 +0200 Subject: [PATCH] D30 rotation hardened after review: the key file is replaced by an fsynced rename over it (never missing or empty), the first old key stays private.key.prev, later ones dated (3 kept), the once-an-hour bound survives a restart; red-proved; CONTEXT; unreleased --- CHANGELOG.md | 11 ++++- CONTEXT.md | 8 ++++ internal/wgtunnel/rekey.go | 71 ++++++++++++++++++++++++++++++--- internal/wgtunnel/rekey_test.go | 62 ++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d3db7a..fa3b3f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,17 @@ in flight. A failed registration is retried by the existing re-key-in-place path. Logs carry public keys only. Tests `TestD30_*` (4), red-proved. A host-loss DR that installs the escrowed (older) key registers it again, and the hub re-keys in place back to it — the same path. +- **Review of the night's own code (felhom.eu `REPORT-night-2026-10-23.md`, N6, N7).** The rotation never leaves the box + without a readable key: the new key is written and fsynced to a temp file, the current key gets its kept name by a + hard link, then the temp file is renamed OVER `private.key` and the directory fsynced (a power cut or a kill between + two renames used to leave no key, or an empty one). `private.key.prev` keeps the FIRST old key (the escrow may hold it); + later ones are `private.key.prev.`, the newest three kept. The once-an-hour bound is saved in `wg/last-rekey`, so + an agent restart does not reset it. Tests `TestD30_TwoRotationsKeepTheFirstOldKey`, + `TestD30_TheHourBoundSurvivesARestart`, red-proved. **Live (felhom.eu `audits/night-2026-10-23/P2-D30/`):** the D30 + re-key ran on a test box (before these two changes): clock corrected → 15 min → new key registered → the box switched + at its next desired-state fetch → a handshake with the new key. - **Known gap, unchanged (filed):** after a reboot the kernel forgets the future stamp (latest handshake 0), so neither - this re-key nor the hub's alarm sees a box whose clock was corrected and which then restarted. + this re-key nor the hub's alarm sees a box whose clock was corrected and which then restarted (R-1223). ## Unreleased (2026-10-22 night) — the tunnel's future-stamped handshake is reported (R-1204) — COMMITTED, NOT RELEASED (no agent release tonight); root SSH by key only (R-982, D4) is a READY PATCH, not on main diff --git a/CONTEXT.md b/CONTEXT.md index 91546e7..df3dd5a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -1,5 +1,13 @@ # CONTEXT — felhom-agent working state +> **2026-10-22/23 night — D30 on `main` (`ae69ad4`), unreleased; the D4 patches v2 ready.** Operator ruling D30 +> (2026-10-22): a handshake stamped in the future for 15 min → a new WG key pair (`wgtunnel/rekey.go`; old key kept as +> `wg/private.key.prev`), registered on `POST /hosts//wg` (the hub re-keys in place; its full-list push swaps the +> key on ep0). Live on a test box (felhom.eu `audits/night-2026-10-23/P2-D30/`): with ep0 free to start a handshake the +> tunnel heals by itself (no re-key, correct); with ep0's starts dropped it stays dead and the agent re-keyed at 15 min. +> The interface switches at the next desired-state fetch. Gap: a reboot forgets the future stamp (R-1223). The D4 bundle +> patch v2 (`P3-review/felhom-agent-bundle-D4-v2.patch`): `try-reload-or-restart`, an `sshd -T` read-back reported as +> `sshd_dropin_not_in_force`, a reload in undo. Next agent release = D30 + D4 v2 + R-1204's field, the operator vouches. > **2026-10-20 — v0.161.0 RELEASED (operator yes), NOT VOUCHED; on demo-hp, Tester 1, demo-felhom** (signed jobs, all > committed, 76/76). R-921 `host_busy` (proven live on demo-hp during a press), R-1070 the tunnel `absent` state (9202's > real Docker 29 answer is the fixture row), R-1010 the janitor's one `force=1`. Vouch: version 0.161.0, sha256 diff --git a/internal/wgtunnel/rekey.go b/internal/wgtunnel/rekey.go index 4251fcb..157eefb 100644 --- a/internal/wgtunnel/rekey.go +++ b/internal/wgtunnel/rekey.go @@ -7,6 +7,8 @@ import ( "fmt" "os" "path/filepath" + "sort" + "strings" "time" "gitea.dooplex.hu/admin/felhom-agent/internal/hub" @@ -66,6 +68,9 @@ func (m *Manager) clockAheadRekeyLocked(ctx context.Context, block *hub.WireWire if now.Sub(m.futureSince) < m.rekeyAfter { return false } + if m.lastRekey.IsZero() { + m.lastRekey = m.loadLastRekey() + } if !m.lastRekey.IsZero() && now.Sub(m.lastRekey) < rekeyMinGap { return false } @@ -77,9 +82,11 @@ func (m *Manager) clockAheadRekeyLocked(ctx context.Context, block *hub.WireWire if err != nil { m.logger.Error("wgtunnel: re-key after a future handshake FAILED — the old key stays", "err", err) m.lastRekey = now // not again before rekeyMinGap + m.saveLastRekey(now) return false } m.lastRekey, m.futureSince = now, time.Time{} + m.saveLastRekey(now) m.logger.Warn("wgtunnel: RE-KEYED after a handshake stamped in the future (D30) — registering the new key; the hub re-keys the peer in place and the endpoint takes the new key", "old_pubkey", pub, "new_pubkey", newPub, "future_s", fut) m.nextRegAt = time.Time{} // register now, not after an older backoff @@ -87,8 +94,11 @@ func (m *Manager) clockAheadRekeyLocked(ctx context.Context, block *hub.WireWire return true } -// rotateKey makes a new key pair: the new private key is written next to the old one, the old one is kept as -// private.key.prev, then the new one takes its place (renames in one directory). Returns the new PUBLIC key only. +// rotateKey makes a new key pair, never leaving the box without a readable key (review 2026-10-23 night, N6): the new +// key is written and fsynced to a temp file; the current key gets a second name by a HARD LINK (the escrow may hold +// it — `private.key.prev` keeps the first such key, later ones `private.key.prev.`, the newest three kept, N7); +// then the temp file is renamed OVER the current name (atomic) and the directory fsynced. At every instant +// `private.key` is a complete key. Returns the new PUBLIC key only. func rotateKey(stateDir string) (string, error) { dir := filepath.Join(stateDir, "wg") cur := filepath.Join(dir, keyFileName) @@ -105,18 +115,67 @@ func rotateKey(stateDir string) (string, error) { return "", err } tmp := filepath.Join(dir, keyFileName+".new") - if err := os.WriteFile(tmp, []byte(base64.StdEncoding.EncodeToString(priv)+"\n"), 0o600); err != nil { + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { return "", fmt.Errorf("writing the new key: %w", err) } - if err := os.Rename(cur, filepath.Join(dir, prevKeyName)); err != nil { + _, werr := f.Write([]byte(base64.StdEncoding.EncodeToString(priv) + "\n")) + serr := f.Sync() + cerr := f.Close() + if werr != nil || serr != nil || cerr != nil { + os.Remove(tmp) + return "", fmt.Errorf("writing the new key: %v %v %v", werr, serr, cerr) + } + prev := filepath.Join(dir, prevKeyName) + if _, err := os.Lstat(prev); err == nil { + prev = filepath.Join(dir, fmt.Sprintf("%s.%d", prevKeyName, rekeyNow().Unix())) + } + if err := os.Link(cur, prev); err != nil { os.Remove(tmp) return "", fmt.Errorf("keeping the old key: %w", err) } if err := os.Rename(tmp, cur); err != nil { - // put the old key back — never leave the box without a key - os.Rename(filepath.Join(dir, prevKeyName), cur) os.Remove(tmp) return "", fmt.Errorf("installing the new key: %w", err) } + if d, err := os.Open(dir); err == nil { + d.Sync() + d.Close() + } + prunePrevKeys(dir, 3) return pub, nil } + +// rekeyNow is the clock for the kept-key names (a seam). +var rekeyNow = time.Now + +// prunePrevKeys keeps the newest `keep` of the `private.key.prev.` files (`private.key.prev` itself always stays). +func prunePrevKeys(dir string, keep int) { + m, _ := filepath.Glob(filepath.Join(dir, prevKeyName+".*")) + if len(m) <= keep { + return + } + sort.Strings(m) // the same digit count until 2286: lexical = time order + for _, p := range m[:len(m)-keep] { + os.Remove(p) + } +} + +// lastRekeyFile records the last re-key's time across agent restarts (N7: the once-an-hour bound was in memory only). +const lastRekeyFile = "last-rekey" + +func (m *Manager) loadLastRekey() time.Time { + b, err := os.ReadFile(filepath.Join(m.wgDir(), lastRekeyFile)) + if err != nil { + return time.Time{} + } + var u int64 + if _, err := fmt.Sscanf(strings.TrimSpace(string(b)), "%d", &u); err != nil { + return time.Time{} + } + return time.Unix(u, 0) +} + +func (m *Manager) saveLastRekey(t time.Time) { + _ = os.WriteFile(filepath.Join(m.wgDir(), lastRekeyFile), []byte(fmt.Sprintf("%d\n", t.Unix())), 0o600) +} diff --git a/internal/wgtunnel/rekey_test.go b/internal/wgtunnel/rekey_test.go index e7bd30a..239a8c3 100644 --- a/internal/wgtunnel/rekey_test.go +++ b/internal/wgtunnel/rekey_test.go @@ -3,6 +3,7 @@ package wgtunnel import ( "context" "errors" + "fmt" "os" "path/filepath" "strings" @@ -144,3 +145,64 @@ func TestD30_AFailedRegistrationIsRetriedByTheExistingPath(t *testing.T) { t.Fatalf("the existing re-key-in-place path must register the new key: %+v", mk) } } + +// Review 2026-10-23 night. N7: a second rotation keeps the FIRST old key as `private.key.prev` (the escrow may hold it) +// and the later one beside it; N6: the current key is replaced by a rename OVER it, so `private.key` is never missing. +// RED-PROOF: with the „prev exists → a dated name" branch removed, TwoRotations fails (the first key overwritten) — +// seen failing 2026-10-23 night. +func TestD30_TwoRotationsKeepTheFirstOldKey(t *testing.T) { + fh := &fakeHub{} + m, _, active, _ := testManager(t, fh) + establishWG(t, m, active) + first := readKey(t, m, keyFileName) + old := rekeyNow + defer func() { rekeyNow = old }() + rekeyNow = func() time.Time { return time.Unix(hs, 0) } + if _, err := rotateKey(m.stateDir); err != nil { + t.Fatal(err) + } + second := readKey(t, m, keyFileName) + rekeyNow = func() time.Time { return time.Unix(hs+7200, 0) } + if _, err := rotateKey(m.stateDir); err != nil { + t.Fatal(err) + } + if readKey(t, m, prevKeyName) != first { + t.Fatal("private.key.prev must keep the FIRST old key") + } + if readKey(t, m, fmt.Sprintf("%s.%d", prevKeyName, hs+7200)) != second { + t.Fatal("the second old key must be kept beside it") + } + if cur := readKey(t, m, keyFileName); cur == first || cur == second { + t.Fatal("the current key must be new") + } +} + +// N7: the once-an-hour bound survives an agent restart (it was in memory only). RED-PROOF: with loadLastRekey removed +// from clockAheadRekeyLocked, this fails — seen failing 2026-10-23 night. +func TestD30_TheHourBoundSurvivesARestart(t *testing.T) { + fh := &fakeHub{} + m, rr, active, _ := testManager(t, fh) + pub := establishWG(t, m, active) + ctx := context.Background() + t0 := int64(hs - 3*3600) + at(m, t0) + m.Watchdog(ctx, true, testBlock(pub)) + at(m, t0+16*60) + m.Watchdog(ctx, true, testBlock(pub)) + newPub := localPub(t, m) + if newPub == pub { + t.Fatal("fixture: the first re-key must happen") + } + // The agent restarts: a NEW manager on the same state; the served block now names the new key; the sign is back. + m2 := NewManager(rr, fh, m.stateDir, m.logger) + m2.isActive, m2.wgPresent, m2.resolver = m.isActive, m.wgPresent, m.resolver + m2.Apply(ctx, true, testBlock(newPub)) + at(m2, t0+20*60) + m2.Watchdog(ctx, true, testBlock(newPub)) + at(m2, t0+40*60) + m2.Watchdog(ctx, true, testBlock(newPub)) + if localPub(t, m2) != newPub { + t.Fatal("a restart must not reset the once-an-hour bound") + } + _ = active +}