Push-mirror SSH host-key trust: persist accept-new + surface status (Fix #625) #626

Merged

crueber merged 2 commits from fix/issue-625 into main

2026-09-16 16:43:30 +00:00

crueber commented

2026-09-16 16:37:07 +00:00

Owner

Implements Forgejo #625.

What: after an accept-new sync, harvest the learned host-key lines from the per-fire known_hosts file, merge into the secret sidecar (same CAS discipline, same redaction contract), and surface fingerprint(s) + first-accepted-at in the view, summary, and Settings row.

Key decisions (docs/features/13_push_mirror.md decision k): merge-never-overwrite with operator-pin precedence on conflicts; fingerprints derived at read (single source of truth, covers pre-#625 pinned sidecars); harvest miss keeps outcome ok + narrates + retries next fire; stored trust auto-pins later syncs (StrictHostKeyChecking=yes).

Verification: go test -race pushmirror/api/cmd green; cover 95.8% pushmirror / 95.4% api; web node--test 1612 pass; vite+esbuild build green; real-Chromium proof of the Settings host-key row (screenshot, no overflow); stub-ssh headless e2e proves learn→persist→surface→repin. 390px: row reuses the existing status-row idiom (no new CSS/widths).

Implements Forgejo #625. **What:** after an accept-new sync, harvest the learned host-key lines from the per-fire known_hosts file, merge into the secret sidecar (same CAS discipline, same redaction contract), and surface fingerprint(s) + first-accepted-at in the view, summary, and Settings row. **Key decisions (docs/features/13_push_mirror.md decision k):** merge-never-overwrite with operator-pin precedence on conflicts; fingerprints derived at read (single source of truth, covers pre-#625 pinned sidecars); harvest miss keeps outcome ok + narrates + retries next fire; stored trust auto-pins later syncs (StrictHostKeyChecking=yes). **Verification:** go test -race pushmirror/api/cmd green; cover 95.8% pushmirror / 95.4% api; web node--test 1612 pass; vite+esbuild build green; real-Chromium proof of the Settings host-key row (screenshot, no overflow); stub-ssh headless e2e proves learn→persist→surface→repin. 390px: row reuses the existing status-row idiom (no new CSS/widths).

crueber added 1 commit

2026-09-16 16:37:07 +00:00

Push-mirror SSH host-key trust: persist accept-new + surface status (Fix #625 ) 9958fb6d62

After an accept-new sync the runner returns the post-push known_hosts
content; runPush harvests it at one shared point (scheduled/on-push/
sync-now all funnel through it): merge into ssh_known_hosts under the
secret CAS (operator pins win conflicts), stamp
ssh_known_hosts_accepted_at once, narrate the SHA256 fingerprint.
Harvest miss keeps outcome ok + retries next fire. View/summary gain
host_key_fingerprint + host_key_accepted_at (presence-style, ~p ETag
covers them); Settings shows the host-key row; docs/features/13 §2/§5
+ decision (k) and 04_git.md ssh pin amended in the same change.

crueber referenced this pull request from a commit

2026-09-16 16:42:34 +00:00

Review fixes (Forgejo #626): HashKnownHosts=no, harvest CAS re-merge, stamp reset on clear

crueber added 1 commit

2026-09-16 16:42:34 +00:00

Review fixes (Forgejo #626 ): HashKnownHosts=no, harvest CAS re-merge, stamp reset on clear ccb55f2761

- sshCommand pins HashKnownHosts=no: distro ssh_config often ships
  HashKnownHosts=yes, and salted |1| tokens would defeat the
  host+keytype harvest dedupe with a fresh token per fire.
- RecordHostKeyTrust is a read-merge-CAS loop (retry re-loads and
  re-merges): a concurrent operator edit folds in instead of being
  last-writer-win clobbered by the blind SaveSecretCAS retry.
- Clearing ssh_known_hosts back to accept-new resets
  ssh_known_hosts_accepted_at with the trust it names.
- Tests: HashKnownHosts pin, CAS-race merge survival, clear-resets-stamp.
- Docs: 13_push_mirror §2 + decision (k) amendment, 04_git argv line.

crueber commented

2026-09-16 16:42:50 +00:00

Author

Owner

Independent review of 9958fb6 (fix/issue-625) vs #625 — APPROVED with three small fixes applied (ccb55f2, pushed). All nine scrutiny points verified against code, not claims:

HARVEST CORRECTNESS — ordering sound: Push() defers scratch cleanup at entry (git.go:129) and reads known_hosts (189-193) before return, so the read precedes cleanup on every path; failure paths return "" before the read (no harvest on failed push — correct). Learned bytes come from the real file ssh wrote (accept-new append), never echoed config. Hashed-host lines PARSE (parseKnownHostLine accepts |1| tokens, pinned by TestParseKnownHostLineSkipsNoise) so learning never silently drops — BUT the merge keyed id on the verbatim token, and distro ssh_config often ships HashKnownHosts=yes, which would salt a fresh token per fire and defeat the dedupe (dupe growth + conflict-miss vs operator pins). FIXED in ccb55f2: sshCommand now passes HashKnownHosts=no (stable plaintext hostnames; matching of pre-existing hashed operator pins is unaffected — the option governs writing, not matching) + test pin + 04_git argv line.
MERGE SEMANTICS — verified in code: MergeKnownHosts never drops/replaces stored lines (exact dup skipped, host+keytype conflict keeps stored line, cross-keytype appends). Steady state returns stored,false with no write (version-stable, asserted in TestRecordHostKeyTrustLearnThenSteady). first-accepted-at stamped once (=="" guard) and preserved. FIXED in ccb55f2: operator clearing known_hosts to "" now also resets the stamp (was stale stamp + empty trust in the view).
FINGERPRINT — SHA256 over the raw key blob, base64-raw, "SHA256:" prefix, 43 chars; TestFingerprintMatchesKeygenHelper asserts equality with the keygen helper (equivalent encodings: TrimRight(Std,"=") == RawStd). Views (View/PushMirrorView/summary), narration, and scrub-path carry fingerprint only — harvest_test asserts key bytes absent from log tail and view JSON.
ETAG — pushMirrorHash covers HostKeyFingerprint + HostKeyAcceptedAt (summary.go:348); TestSummaryPushMirrorHostKeyETag proves learned-trust flips the ETag (200, no 304) and the new ETag then 304s.
FAILURE — harvestHostKey: Record error -> outcome stays ok, narrates "not recorded (...; will retry next sync)", next accept-new fire re-learns. Proven by TestHarvestFailureDoesNotFailSync (failSecretPutStore, outcome ok, failures counter 0, no partial persist).
SINGLE HARVEST POINT — exactly one call site (service.go:382 in runPush); scheduled/on-push/sync-now all funnel through runPush. No forks.
CAS — REAL DEFECT FOUND AND FIXED in ccb55f2: RecordHostKeyTrust delegated to SaveSecretCAS, whose 412-retry re-reads the version but rewrites the same stale body — a concurrent operator PUT between load and write would be last-writer-win clobbered (harvest runs at machine rate, widening the race). RecordHostKeyTrust is now a read-merge-CAS loop: on 412 it re-loads and re-merges learned lines over fresh trust. New TestRecordHostKeyTrustMergesOverConcurrentEdit (casRaceStore injects an operator write + 412) proves both lines survive; it fails against the old delegation.
SCHEDULE/OFF/INDEPENDENCE/PUBLISH — diff touches none of the scheduler, backoff, lease, or publish paths (19 files: pushmirror core + api view/summary + cmd mapping + docs + web row). Docs updated in-change (13_push_mirror §2 + decision (k) amendment incl. review notes, 04_git argv) — law 12 satisfied. Coverage re-measured after my fixes: pushmirror 95.5%, api 95.4% (gate >=95 holds). No go.mod/go.sum change; no x/crypto client use (stdlib sha256/base64 only; x/crypto mention is a pre-existing doc comment).
E2E STUB + SCREENSHOT — harvest_test.go exists: stub-git learn->persist->surface->repin, operator-pin-conflict, and harvest-miss suites all pass -race (verified locally). node --test pushmirror suite: 8 pass. Chromium screenshot claim not re-verifiable here — noted as author-attested; the Settings row reuses the existing status-row idiom (no new CSS/widths), low risk.

Fix commit: ccb55f2 on fix/issue-625 (7 files, +184/-36). Post-fix: gofmt clean, go vet clean, pushmirror/api/cmd -race green, web unit green. Verdict: APPROVE.

Independent review of 9958fb6 (fix/issue-625) vs #625 — APPROVED with three small fixes applied (ccb55f2, pushed). All nine scrutiny points verified against code, not claims: 1. HARVEST CORRECTNESS — ordering sound: Push() defers scratch cleanup at entry (git.go:129) and reads known_hosts (189-193) before return, so the read precedes cleanup on every path; failure paths return "" before the read (no harvest on failed push — correct). Learned bytes come from the real file ssh wrote (accept-new append), never echoed config. Hashed-host lines PARSE (parseKnownHostLine accepts |1| tokens, pinned by TestParseKnownHostLineSkipsNoise) so learning never silently drops — BUT the merge keyed id on the verbatim token, and distro ssh_config often ships HashKnownHosts=yes, which would salt a fresh token per fire and defeat the dedupe (dupe growth + conflict-miss vs operator pins). FIXED in ccb55f2: sshCommand now passes HashKnownHosts=no (stable plaintext hostnames; matching of pre-existing hashed operator pins is unaffected — the option governs writing, not matching) + test pin + 04_git argv line. 2. MERGE SEMANTICS — verified in code: MergeKnownHosts never drops/replaces stored lines (exact dup skipped, host+keytype conflict keeps stored line, cross-keytype appends). Steady state returns stored,false with no write (version-stable, asserted in TestRecordHostKeyTrustLearnThenSteady). first-accepted-at stamped once (=="" guard) and preserved. FIXED in ccb55f2: operator clearing known_hosts to "" now also resets the stamp (was stale stamp + empty trust in the view). 3. FINGERPRINT — SHA256 over the raw key blob, base64-raw, "SHA256:" prefix, 43 chars; TestFingerprintMatchesKeygenHelper asserts equality with the keygen helper (equivalent encodings: TrimRight(Std,"=") == RawStd). Views (View/PushMirrorView/summary), narration, and scrub-path carry fingerprint only — harvest_test asserts key bytes absent from log tail and view JSON. 4. ETAG — pushMirrorHash covers HostKeyFingerprint + HostKeyAcceptedAt (summary.go:348); TestSummaryPushMirrorHostKeyETag proves learned-trust flips the ETag (200, no 304) and the new ETag then 304s. 5. FAILURE — harvestHostKey: Record error -> outcome stays ok, narrates "not recorded (...; will retry next sync)", next accept-new fire re-learns. Proven by TestHarvestFailureDoesNotFailSync (failSecretPutStore, outcome ok, failures counter 0, no partial persist). 6. SINGLE HARVEST POINT — exactly one call site (service.go:382 in runPush); scheduled/on-push/sync-now all funnel through runPush. No forks. 7. CAS — REAL DEFECT FOUND AND FIXED in ccb55f2: RecordHostKeyTrust delegated to SaveSecretCAS, whose 412-retry re-reads the version but rewrites the same stale body — a concurrent operator PUT between load and write would be last-writer-win clobbered (harvest runs at machine rate, widening the race). RecordHostKeyTrust is now a read-merge-CAS loop: on 412 it re-loads and re-merges learned lines over fresh trust. New TestRecordHostKeyTrustMergesOverConcurrentEdit (casRaceStore injects an operator write + 412) proves both lines survive; it fails against the old delegation. 8. SCHEDULE/OFF/INDEPENDENCE/PUBLISH — diff touches none of the scheduler, backoff, lease, or publish paths (19 files: pushmirror core + api view/summary + cmd mapping + docs + web row). Docs updated in-change (13_push_mirror §2 + decision (k) amendment incl. review notes, 04_git argv) — law 12 satisfied. Coverage re-measured after my fixes: pushmirror 95.5%, api 95.4% (gate >=95 holds). No go.mod/go.sum change; no x/crypto client use (stdlib sha256/base64 only; x/crypto mention is a pre-existing doc comment). 9. E2E STUB + SCREENSHOT — harvest_test.go exists: stub-git learn->persist->surface->repin, operator-pin-conflict, and harvest-miss suites all pass -race (verified locally). node --test pushmirror suite: 8 pass. Chromium screenshot claim not re-verifiable here — noted as author-attested; the Settings row reuses the existing status-row idiom (no new CSS/widths), low risk. Fix commit: ccb55f2 on fix/issue-625 (7 files, +184/-36). Post-fix: gofmt clean, go vet clean, pushmirror/api/cmd -race green, web unit green. Verdict: APPROVE.

crueber merged commit 7906a33b9f into main

2026-09-16 16:43:30 +00:00

crueber referenced this pull request from a commit

2026-09-16 16:43:31 +00:00

Merge pull request 'Push-mirror SSH host-key trust: persist accept-new + surface status (Fix #625)' (#626) from fix/issue-625 into main

crueber referenced this pull request

2026-09-16 16:43:40 +00:00

Push-mirror SSH host keys: persist accept-new trust + surface host-key status #625