v0.289.0: off-site key cannot delete — append-only rclone transport, box sends only its public key (hub registrar), retention only inside a hub window behind the fake-snapshot guard (decisions 68-69, R-820, R-822)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS

controller/internal/offsiteapply/offsiteapply_test.go

@@ -3,7 +3,10 @@ package offsiteapply
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
+	"fmt"
 	"log"
 	"os"
 	"path/filepath"
@@ -15,19 +18,36 @@ import (
 
 // --- fakes ---
 
-type fakeConsumer struct {
-	pw     string
-	err    error
-	calls  int
-	panics bool
+// fakeRegistrar is the hub's key registrar. `calls` counts Register — the one call that writes to the
+// provider; it is the "did the bridge go to the hub" signal the settle-gate and retry tests count.
+type fakeRegistrar struct {
+	calls     int
+	confirms  []string
+	gotPub    string
+	wrongFP   bool
+	err       error
+	confirmEr error
+	panics    bool
 }
 
-func (f *fakeConsumer) Consume(_ context.Context) (string, error) {
+func (f *fakeRegistrar) Register(_ context.Context, pub string) (string, error) {
 	if f.panics {
-		panic("consume must NOT be called (idempotent no-op)")
+		panic("register must NOT be called")
 	}
 	f.calls++
-	return f.pw, f.err
+	f.gotPub = pub
+	if f.err != nil {
+		return "", f.err
+	}
+	if f.wrongFP {
+		return "SHA256:somebody-else", nil
+	}
+	return FingerprintOf(pub)
+}
+
+func (f *fakeRegistrar) Confirm(_ context.Context, fp string) error {
+	f.confirms = append(f.confirms, fp)
+	return f.confirmEr
 }
 
 type fakeScanner struct {
@@ -39,40 +59,25 @@ func (f *fakeScanner) Scan(_ context.Context, _ string, _ int) (string, string,
 	return f.fp, f.line, f.err
 }
 
-type fakeKeyGen struct{ priv, pub string }
+// realKeyGen returns ONE real pair (the bridge parses keys now — fingerprints are real).
+type realKeyGen struct{ priv, pub string }
 
-func (f *fakeKeyGen) Generate() (string, string, error) { return f.priv, f.pub, nil }
-
-type fakeInstaller struct {
-	err          error
-	calls        int
-	gotPub       string
-	gotPriv      string
-	gotPw        string
-	gotKnownHost string
-}
-
-func (f *fakeInstaller) Install(_ context.Context, _, _ string, _ int, password, privPEM, pub, knownHosts string) error {
-	f.calls++
-	f.gotPub, f.gotPriv, f.gotPw, f.gotKnownHost = pub, privPEM, password, knownHosts
-	return f.err
-}
+func (f *realKeyGen) Generate() (string, string, error) { return f.priv, f.pub, nil }
 
+// fakeProber models the provider: a key reaches the pinned server once the registrar installed it
+// (pinnedAfterRegister) or from the start (pinnedInitially).
 type fakeProber struct {
-	pem    string
-	ok     bool
-	panics bool
-	calls  int
-	gotKH  string
+	reg                 *fakeRegistrar
+	pinnedInitially     bool
+	pinnedAfterRegister bool
+	calls               int
+	gotKH, gotPriv      string
 }
 
-func (f *fakeProber) Probe(_ context.Context, _, _ string, _ int, kh string) (string, bool) {
-	if f.panics {
-		panic("prober must NOT be called (verify must precede the probe)")
-	}
+func (f *fakeProber) Probe(_ context.Context, _, _ string, _ int, kh, priv string) bool {
 	f.calls++
-	f.gotKH = kh
-	return f.pem, f.ok
+	f.gotKH, f.gotPriv = kh, priv
+	return f.pinnedInitially || (f.pinnedAfterRegister && f.reg.calls > 0)
 }
 
 type fakeEnabler struct {
@@ -90,187 +95,200 @@ func (f *fakeEnabler) ConfigureOffbox(_ context.Context, host, _ string, _ int,
 	return f.err
 }
 
-func newBridge(t *testing.T, o config.OffsiteConfig) (*Bridge, *fakeConsumer, *fakeInstaller, *fakeEnabler, *bytes.Buffer) {
+var testPriv, testPub = func() (string, string) {
+	p, q, err := ED25519KeyGen{}.Generate()
+	if err != nil {
+		panic(err)
+	}
+	return p, q
+}()
+
+var otherPriv, otherPub = func() (string, string) {
+	p, q, _ := ED25519KeyGen{}.Generate()
+	return p, q
+}()
+
+func newBridge(t *testing.T, o config.OffsiteConfig) (*Bridge, *fakeRegistrar, *fakeProber, *fakeEnabler, *bytes.Buffer) {
 	t.Helper()
 	cfg := &config.Config{}
 	cfg.Offsite = o
-	cons := &fakeConsumer{pw: "the-transient-pw"}
-	inst := &fakeInstaller{}
+	reg := &fakeRegistrar{}
+	pr := &fakeProber{reg: reg, pinnedAfterRegister: true}
 	en := &fakeEnabler{}
 	var logbuf bytes.Buffer
 	b := &Bridge{
 		Cfg:        cfg,
-		Consumer:   cons,
+		Registrar:  reg,
 		Scanner:    &fakeScanner{fp: "SHA256:goodfp", line: "[h]:23 ssh-ed25519 AAAAKEY"},
-		KeyGen:     &fakeKeyGen{priv: "PRIVPEM", pub: "ssh-ed25519 AAAAPUB felhom"},
-		Installer:  inst,
+		KeyGen:     &realKeyGen{priv: testPriv, pub: testPub},
+		Prober:     pr,
 		Enabler:    en,
 		MarkerPath: filepath.Join(t.TempDir(), "offbox", "applied_marker"),
 		Logger:     log.New(&logbuf, "", 0),
 	}
-	return b, cons, inst, en, &logbuf
+	return b, reg, pr, en, &logbuf
 }
 
 func goodOffsite() config.OffsiteConfig {
 	return config.OffsiteConfig{Enabled: true, Type: "shared", Host: "h", User: "u", Port: 23, RepoPath: "/home/felhom-repo", QuotaGB: 50, HostFingerprint: "SHA256:goodfp"}
 }
 
-// Scenario A — full apply: consume → verify-pin → install → configure offbox → marker persisted; pw not logged.
-func TestBridge_AppliesEndToEnd(t *testing.T) {
-	b, cons, inst, en, logbuf := newBridge(t, goodOffsite())
+// A fresh box: verify host key → register the PUBLIC key → prove the pin → configure → confirm → marker.
+// What crosses to the hub is a public key and nothing else.
+func TestBridge_FreshBoxRegistersPublicKeyOnly(t *testing.T) {
+	b, reg, pr, en, logbuf := newBridge(t, goodOffsite())
 	if err := b.Reconcile(context.Background()); err != nil {
 		t.Fatalf("reconcile: %v", err)
 	}
-	if cons.calls != 1 {
-		t.Fatalf("consume calls = %d, want 1", cons.calls)
+	if reg.calls != 1 || reg.gotPub != testPub {
+		t.Fatalf("register calls=%d pub=%q", reg.calls, reg.gotPub)
 	}
-	if inst.calls != 1 || inst.gotPw != "the-transient-pw" || inst.gotPub == "" {
-		t.Fatalf("installer not called with pw+pub: %+v", inst)
+	if strings.Contains(reg.gotPub, "PRIVATE") {
+		t.Fatal("the private key was sent to the hub")
 	}
-	if inst.gotKnownHost != "[h]:23 ssh-ed25519 AAAAKEY" {
-		t.Fatalf("installer must receive the scanner-verified known_hosts to pin (no TOFU), got %q", inst.gotKnownHost)
+	if pr.calls != 2 || pr.gotKH != "[h]:23 ssh-ed25519 AAAAKEY" {
+		t.Fatalf("probe calls=%d kh=%q (want before+after register, pinned to the scanned host key)", pr.calls, pr.gotKH)
 	}
-	if en.calls != 1 || en.gotHost != "h" || en.gotKnownHost != "[h]:23 ssh-ed25519 AAAAKEY" || en.gotPriv != "PRIVPEM" {
-		t.Fatalf("enabler not called with the pinned known_hosts + key: %+v", en)
+	fp, _ := FingerprintOf(testPub)
+	if en.calls != 1 || en.gotPriv != testPriv || en.gotQuotaGB != 50 {
+		t.Fatalf("enabler: %+v", en)
 	}
-	if en.gotQuotaGB != 50 {
-		t.Fatalf("the bridge must map the descriptor's quota_gb into the target (SLICE 4), got %d", en.gotQuotaGB)
+	if len(reg.confirms) != 1 || reg.confirms[0] != fp {
+		t.Fatalf("confirm = %v, want [%s]", reg.confirms, fp)
 	}
-	if b.readMarker() != descriptorHash(b.Cfg.Offsite) {
-		t.Fatal("marker not persisted after a successful apply")
+	if _, err := os.Stat(b.MarkerPath); err != nil {
+		t.Fatalf("marker not persisted: %v", err)
 	}
-	if strings.Contains(logbuf.String(), "the-transient-pw") {
-		t.Fatal("the one-time password LEAKED into a log line")
+	if !strings.Contains(logbuf.String(), "append-only") {
+		t.Fatalf("log does not say append-only:\n%s", logbuf.String())
 	}
 }
 
-// SLICE 4 — a quota-only descriptor change re-applies (the hash includes QuotaGB), and with a working
-// key it costs no password: key-auth-first re-pins + remaps the quota.
-func TestBridge_QuotaChangeReappliesWithoutConsume(t *testing.T) {
-	b, cons, _, en, _ := newBridge(t, goodOffsite())
-	cons.panics = true
-	b.Prober = &fakeProber{pem: "EXISTINGPEM", ok: true}
-	// marker for the OLD quota (25) already applied; the descriptor now says 50
-	old := b.Cfg.Offsite
-	old.QuotaGB = 25
-	_ = os.MkdirAll(filepath.Dir(b.MarkerPath), 0o700)
-	_ = os.WriteFile(b.MarkerPath, []byte(descriptorHash(old)), 0o600)
-	if err := b.Reconcile(context.Background()); err != nil {
-		t.Fatalf("quota-change reconcile: %v", err)
+// THE MIGRATION: a box upgraded from the password era has an installed (unpinned) key and an applied
+// marker written under the OLD descriptor hash. The new hash ("|pinned-v1") re-applies once; the SAME
+// key is registered (no new keypair) and comes back pinned.
+func TestBridge_UpgradedBoxReRegistersSameKey(t *testing.T) {
+	b, reg, _, en, _ := newBridge(t, goodOffsite())
+	b.Existing = func() string { return otherPriv }
+	b.KeyGen = nil // must not be needed
+	o := goodOffsite()
+	if err := os.MkdirAll(filepath.Dir(b.MarkerPath), 0o700); err != nil {
+		t.Fatal(err)
 	}
-	if en.calls != 1 || en.gotQuotaGB != 50 {
-		t.Fatalf("a quota raise must re-apply and map the NEW quota (no consume): %+v", en)
-	}
-}
-
-// Key-auth-first (Scenario B) — the existing key still works: NO consume, NO install; re-verify + re-pin +
-// reconfigure with the EXISTING key, marker updated.
-func TestBridge_KeyAuthFirstSkipsConsume(t *testing.T) {
-	b, cons, inst, en, _ := newBridge(t, goodOffsite())
-	cons.panics = true // the whole point: a working key must NEVER consume the one-time password
-	prober := &fakeProber{pem: "EXISTINGPEM", ok: true}
-	b.Prober = prober
-	if err := b.Reconcile(context.Background()); err != nil {
-		t.Fatalf("key-auth-first reconcile: %v", err)
-	}
-	if prober.calls != 1 || prober.gotKH != "[h]:23 ssh-ed25519 AAAAKEY" {
-		t.Fatalf("probe must run once with the freshly-scanned pinned known_hosts: %+v", prober)
-	}
-	if inst.calls != 0 {
-		t.Fatal("installer must NOT run when the existing key authenticates")
-	}
-	if en.calls != 1 || en.gotPriv != "EXISTINGPEM" || en.gotKnownHost != "[h]:23 ssh-ed25519 AAAAKEY" {
-		t.Fatalf("enabler must reconfigure with the EXISTING key + fresh pin: %+v", en)
-	}
-	if b.readMarker() != descriptorHash(b.Cfg.Offsite) {
-		t.Fatal("marker must be updated after a key-auth-first apply")
-	}
-}
-
-// Scenario C — key-auth-first must NOT weaken the fresh path: probe fails → the full
-// verify→consume→install path runs unchanged (with the freshly GENERATED key).
-func TestBridge_FreshGuestFallsThroughToFullPath(t *testing.T) {
-	b, cons, inst, en, _ := newBridge(t, goodOffsite())
-	b.Prober = &fakeProber{ok: false} // fresh guest: no key / auth refused
-	if err := b.Reconcile(context.Background()); err != nil {
-		t.Fatalf("fresh-guest reconcile: %v", err)
-	}
-	if cons.calls != 1 || inst.calls != 1 {
-		t.Fatalf("fresh guest must consume+install exactly once: cons=%d inst=%d", cons.calls, inst.calls)
-	}
-	if en.calls != 1 || en.gotPriv != "PRIVPEM" {
-		t.Fatalf("fresh guest must configure with the GENERATED key: %+v", en)
-	}
-	if b.readMarker() != descriptorHash(b.Cfg.Offsite) {
-		t.Fatal("marker must be persisted after a full-path apply")
-	}
-}
-
-// Scenario B — host-key mismatch → refuse: no consume, no install, no configure, no marker.
-func TestBridge_HostKeyMismatchRefuses(t *testing.T) {
-	b, cons, inst, en, _ := newBridge(t, goodOffsite())
-	b.Scanner = &fakeScanner{fp: "SHA256:ATTACKER", line: "[h]:23 ssh-ed25519 EVIL"}
-	b.Prober = &fakeProber{panics: true} // the probe must NEVER run when the identity check failed
-	err := b.Reconcile(context.Background())
-	if err == nil || !strings.Contains(err.Error(), "MISMATCH") {
-		t.Fatalf("mismatch must refuse, got %v", err)
-	}
-	if cons.calls != 0 || inst.calls != 0 || en.calls != 0 {
-		t.Fatalf("nothing may proceed on a host-key mismatch: cons=%d inst=%d en=%d", cons.calls, inst.calls, en.calls)
-	}
-	if b.readMarker() != "" {
-		t.Fatal("no marker may be written on a mismatch")
-	}
-}
-
-// Scenario C — idempotent: marker already matches → no-op, consume is NOT called.
-func TestBridge_IdempotentNoReconsume(t *testing.T) {
-	b, cons, inst, en, _ := newBridge(t, goodOffsite())
-	cons.panics = true // Consume must not be called
-	// pre-seed the marker with the current descriptor hash
-	_ = os.MkdirAll(filepath.Dir(b.MarkerPath), 0o700)
-	if err := os.WriteFile(b.MarkerPath, []byte(descriptorHash(b.Cfg.Offsite)), 0o600); err != nil {
+	// The pre-v0.289.0 marker for this exact descriptor:
+	if err := os.WriteFile(b.MarkerPath, []byte(legacyDescriptorHash(o)), 0o600); err != nil {
 		t.Fatal(err)
 	}
 	if err := b.Reconcile(context.Background()); err != nil {
-		t.Fatalf("idempotent reconcile must be a clean no-op, got %v", err)
+		t.Fatalf("reconcile: %v", err)
 	}
-	if cons.calls != 0 || inst.calls != 0 || en.calls != 0 {
-		t.Fatal("an already-applied descriptor must be a full no-op")
+	if reg.calls != 1 || reg.gotPub != otherPub {
+		t.Fatalf("the upgraded box must re-register its EXISTING key: calls=%d pub=%q", reg.calls, reg.gotPub)
+	}
+	if en.gotPriv != otherPriv {
+		t.Fatal("the existing key was not kept")
 	}
 }
 
-// Scenario D — install fails → fail-safe: marker NOT persisted, offbox NOT configured, loud log.
-func TestBridge_InstallFailIsFailSafe(t *testing.T) {
-	b, cons, inst, en, logbuf := newBridge(t, goodOffsite())
-	inst.err = errors.New("ssh-copy-id refused")
-	err := b.Reconcile(context.Background())
-	if err == nil {
-		t.Fatal("install failure must error")
+// A restart / descriptor change on a box whose key is already pinned: no hub write at all.
+func TestBridge_AlreadyPinnedNeedsNoRegister(t *testing.T) {
+	b, reg, pr, en, _ := newBridge(t, goodOffsite())
+	b.Existing = func() string { return testPriv }
+	pr.pinnedInitially = true
+	if err := b.Reconcile(context.Background()); err != nil {
+		t.Fatalf("reconcile: %v", err)
+	}
+	if reg.calls != 0 || en.calls != 1 {
+		t.Fatalf("register=%d enable=%d", reg.calls, en.calls)
+	}
+}
+
+// The registrar answered but the key still does not reach the PINNED server (e.g. an unpinned line
+// won): refuse — never configure a key that can delete.
+func TestBridge_RegisteredButNotPinnedRefuses(t *testing.T) {
+	b, reg, pr, en, _ := newBridge(t, goodOffsite())
+	pr.pinnedAfterRegister = false
+	if err := b.Reconcile(context.Background()); err == nil {
+		t.Fatal("a key that does not reach the pinned server must refuse")
+	}
+	if reg.calls != 1 || en.calls != 0 {
+		t.Fatalf("register=%d enable=%d", reg.calls, en.calls)
+	}
+	if _, err := os.Stat(b.MarkerPath); err == nil {
+		t.Fatal("marker persisted after a refusal")
+	}
+}
+
+func TestBridge_HubInstalledADifferentKeyRefuses(t *testing.T) {
+	b, reg, _, en, _ := newBridge(t, goodOffsite())
+	reg.wrongFP = true
+	if err := b.Reconcile(context.Background()); err == nil || en.calls != 0 {
+		t.Fatalf("err=%v enable=%d", err, en.calls)
+	}
+}
+
+// Host-key mismatch → refuse before anything touches the hub.
+func TestBridge_HostKeyMismatchRefuses(t *testing.T) {
+	b, reg, pr, en, _ := newBridge(t, goodOffsite())
+	b.Scanner = &fakeScanner{fp: "SHA256:EVIL", line: "x"}
+	reg.panics = true
+	if err := b.Reconcile(context.Background()); err == nil || !strings.Contains(err.Error(), "MISMATCH") {
+		t.Fatalf("err = %v", err)
+	}
+	if pr.calls != 0 || en.calls != 0 {
+		t.Fatal("probe/configure ran after a host-key mismatch")
+	}
+}
+
+func TestBridge_IdempotentMarker(t *testing.T) {
+	b, reg, _, _, _ := newBridge(t, goodOffsite())
+	if err := b.Reconcile(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	reg.panics = true
+	if err := b.Reconcile(context.Background()); err != nil {
+		t.Fatalf("second reconcile: %v", err)
+	}
+}
+
+func TestBridge_RegisterFailIsFailSafe(t *testing.T) {
+	b, reg, _, en, _ := newBridge(t, goodOffsite())
+	reg.err = errors.New("hub down")
+	if err := b.Reconcile(context.Background()); err == nil {
+		t.Fatal("want error")
 	}
 	if en.calls != 0 {
-		t.Fatal("offbox must NOT be configured when install fails")
+		t.Fatal("configured after a failed register")
 	}
-	if b.readMarker() != "" {
-		t.Fatal("marker must NOT be persisted on a failed apply (fail-safe)")
-	}
-	if cons.calls != 1 {
-		t.Fatal("the password was consumed (spent) before install")
-	}
-	if !strings.Contains(logbuf.String(), "password is spent") {
-		t.Fatal("a consumed-but-failed install must log the loud 'password is spent' signal")
+	if _, err := os.Stat(b.MarkerPath); err == nil {
+		t.Fatal("marker persisted after failure")
+	}
+}
+
+// A failed confirm leaves an extra PINNED line (cannot delete) — not a failure of the apply.
+func TestBridge_ConfirmFailureIsNotFatal(t *testing.T) {
+	b, reg, _, en, logbuf := newBridge(t, goodOffsite())
+	reg.confirmEr = errors.New("hub blip")
+	if err := b.Reconcile(context.Background()); err != nil {
+		t.Fatalf("reconcile: %v", err)
+	}
+	if en.calls != 1 || !strings.Contains(logbuf.String(), "confirm key") {
+		t.Fatalf("enable=%d log=%s", en.calls, logbuf.String())
 	}
 }
 
-// Disabled → no-op (no consume/install/configure).
 func TestBridge_DisabledNoOp(t *testing.T) {
 	o := goodOffsite()
 	o.Enabled = false
-	b, cons, inst, en, _ := newBridge(t, o)
-	if err := b.Reconcile(context.Background()); err != nil {
-		t.Fatal(err)
-	}
-	if cons.calls+inst.calls+en.calls != 0 {
-		t.Fatal("disabled offsite must be a no-op")
+	b, reg, _, en, _ := newBridge(t, o)
+	reg.panics = true
+	if err := b.Reconcile(context.Background()); err != nil || en.calls != 0 {
+		t.Fatalf("err=%v enable=%d", err, en.calls)
 	}
 }
+
+// legacyDescriptorHash is the pre-v0.289.0 marker key, byte for byte as v0.288.0 computed it.
+func legacyDescriptorHash(o config.OffsiteConfig) string {
+	sum := sha256.Sum256([]byte(fmt.Sprintf("%s|%s|%s|%d|%s|%s|%d", o.Type, o.Host, o.User, o.Port, o.RepoPath, o.HostFingerprint, o.QuotaGB)))
+	return hex.EncodeToString(sum[:])
+}