SSH transport for git: clone/fetch and push over ssh:// #2

Merged
crueber merged 7 commits from feat/ssh-transport into main 2026-09-03 20:24:12 +00:00
Owner

Closes #1.

What this adds

walhub serves git over SSH in both standard directions: git-upload-pack (clone/fetch) and git-receive-pack (push), through a new internal/sshd package built directly on golang.org/x/crypto/ssh — the same primitive Gitea and Forgejo build on.

The SSH wire layer owns auth and command parsing only; the git pipeline is shared. internal/server implements a small sshd.Transport interface, so an SSH session runs the exact same pipeline as the HTTP routes (sync → upload-pack; parse → ingest → connectivity → publish → report), with the same gates: drain (phase 2), placement (§4.3), max_push_bytes, and the per-repo semaphore.

Auth

Public keys live in config ([[server.ssh.keys]]: principal, key/key_env, write, admin — mirroring the static-token pattern), validated fail-closed at boot (parse errors, duplicate fingerprints, unset env). The matched key's principal carries its write/admin flags; push requires write. Host key: host_key_env → host_key path → auto-generated ed25519 persisted under <data-dir>/ssh/ (TOFU).

The framing traps (why this is not "just another listener")

  • upload-pack --stateless-rpc waits for stdin EOF; an SSH channel never gets it (the client waits for the response). Fetch runs upload-pack interactively (Layer.UploadPackSSH).
  • receive-pack opens with the server's ref advertisement — without it both sides hang. Always v0 (protocol v2 does not cover push; the client's GIT_PROTOCOL is honored for fetch).
  • git send-pack never closes its side before reading the report — the push path parses the command section incrementally (ParsePushRequestStream) and streams the pack through IngestStream (straight into index-pack, which stops at the pack trailer; the feed goroutine is released at child exit). Pure-delete pushes send no pack and skip ingest.
  • max_push_bytes covers the whole request; an over-cap push reports "pack exceeds max_bytes" as band-2 + unpack-ng on the wire.

Security

Strictly two verbs; option-bearing argv refused (injection guard); repo paths via ParseRepoId; no PTY/shell/subsystem/forwarding; per-connection session cap (16) plus the process-wide exec semaphore (64); key-fingerprint warnings on refusals; host key never silently regenerated when configured-but-unreadable.

Dependency note (Law 1)

golang.org/x/crypto joins as the fourth backend module, amended in AGENTS.md Law 1 and docs/go/17_ssh.md decision 17.1 per the explicit request for SSH support. Hand-rolling SSH was not on the table.

Tests

  • internal/sshd: command parsing tables (quoting, injection, extra words), auth matrix over real loopback handshakes, GIT_PROTOCOL passthrough, session limiter, PTY/shell refusal, transport-error → stderr mapping, host-key generation/load matrix.
  • internal/server: gate matrix (drain phases 1/2, placement not-served + maintain-only + empty ServedBy, repo errors), over-max_push_bytes push reporting band-2 + unpack-ng on the wire, and the end-to-end proof: real git clone / git push over ssh://127.0.0.1:<random> against the in-process server, both directions, with a generated client key.
  • make ci green; every package ≥ 95% statement coverage (sshd 95.8%, server 95.2%, git 95.1%).

Self review

A full reviewer pass ran before this PR (verdict fix-first); all findings were addressed in b12f2f9: placement/drain gate parity with HTTP, per-repo semaphore, index-pack stderr diagnostics restored (the ingest refactor had dropped them), per-connection session cap, host-key TOFU protection (unreadable configured key fails the boot instead of silently regenerating), dead sentinels removed, capReader race made airtight, and the two test gaps (over-cap push, maintain-only placement) closed.

Docs

New docs/go/17_ssh.md (spec + decisions 17.1–17.4, concurrency section); AGENTS.md Law 1 amendment + package list; docs/go/11_config_cli.md key table + validation rule 11; docs/go/04_git.md interface additions; setup UI field examples (server.ssh.*).

Closes #1. ## What this adds walhub serves git over **SSH** in both standard directions: `git-upload-pack` (clone/fetch) and `git-receive-pack` (push), through a new `internal/sshd` package built directly on `golang.org/x/crypto/ssh` — the same primitive Gitea and Forgejo build on. The SSH wire layer owns auth and command parsing only; the git pipeline is shared. `internal/server` implements a small `sshd.Transport` interface, so an SSH session runs the exact same pipeline as the HTTP routes (sync → upload-pack; parse → ingest → connectivity → publish → report), with the same gates: drain (phase 2), placement (§4.3), `max_push_bytes`, and the per-repo semaphore. ## Auth Public keys live in config (`[[server.ssh.keys]]`: principal, key/key_env, write, admin — mirroring the static-token pattern), validated fail-closed at boot (parse errors, duplicate fingerprints, unset env). The matched key's principal carries its write/admin flags; push requires `write`. Host key: `host_key_env` → `host_key` path → auto-generated ed25519 persisted under `<data-dir>/ssh/` (TOFU). ## The framing traps (why this is not "just another listener") - `upload-pack --stateless-rpc` waits for stdin EOF; an SSH channel never gets it (the client waits for the response). Fetch runs upload-pack interactively (`Layer.UploadPackSSH`). - receive-pack **opens with the server's ref advertisement** — without it both sides hang. Always v0 (protocol v2 does not cover push; the client's `GIT_PROTOCOL` is honored for fetch). - `git send-pack` never closes its side before reading the report — the push path parses the command section incrementally (`ParsePushRequestStream`) and streams the pack through `IngestStream` (straight into index-pack, which stops at the pack trailer; the feed goroutine is released at child exit). Pure-delete pushes send no pack and skip ingest. - `max_push_bytes` covers the whole request; an over-cap push reports "pack exceeds max_bytes" as band-2 + unpack-ng on the wire. ## Security Strictly two verbs; option-bearing argv refused (injection guard); repo paths via `ParseRepoId`; no PTY/shell/subsystem/forwarding; per-connection session cap (16) plus the process-wide exec semaphore (64); key-fingerprint warnings on refusals; host key never silently regenerated when configured-but-unreadable. ## Dependency note (Law 1) `golang.org/x/crypto` joins as the fourth backend module, amended in `AGENTS.md` Law 1 and `docs/go/17_ssh.md` decision 17.1 per the explicit request for SSH support. Hand-rolling SSH was not on the table. ## Tests - `internal/sshd`: command parsing tables (quoting, injection, extra words), auth matrix over real loopback handshakes, GIT_PROTOCOL passthrough, session limiter, PTY/shell refusal, transport-error → stderr mapping, host-key generation/load matrix. - `internal/server`: gate matrix (drain phases 1/2, placement not-served + maintain-only + empty ServedBy, repo errors), over-`max_push_bytes` push reporting band-2 + unpack-ng on the wire, and the end-to-end proof: real `git clone` / `git push` over `ssh://127.0.0.1:<random>` against the in-process server, both directions, with a generated client key. - `make ci` green; every package ≥ 95% statement coverage (sshd 95.8%, server 95.2%, git 95.1%). ## Self review A full reviewer pass ran before this PR (verdict fix-first); all findings were addressed in `b12f2f9`: placement/drain gate parity with HTTP, per-repo semaphore, index-pack stderr diagnostics restored (the ingest refactor had dropped them), per-connection session cap, host-key TOFU protection (unreadable configured key fails the boot instead of silently regenerating), dead sentinels removed, capReader race made airtight, and the two test gaps (over-cap push, maintain-only placement) closed. ## Docs New `docs/go/17_ssh.md` (spec + decisions 17.1–17.4, concurrency section); `AGENTS.md` Law 1 amendment + package list; `docs/go/11_config_cli.md` key table + validation rule 11; `docs/go/04_git.md` interface additions; setup UI field examples (`server.ssh.*`).
internal/sshd (new): a golang.org/x/crypto/ssh listener that parses the
client's command string (git-upload-pack / git-receive-pack only), maps
configured public keys to principals (write/admin flags), and dispatches
to a Transport the server implements — the same git pipeline HTTP uses.
No PTY/shell/subsystem; option-bearing argv refused; GIT_PROTOCOL v2
passthrough for fetch; host key from config or auto-generated ed25519
under <data-dir>/ssh/.

The framing traps this design answers (17_ssh.md §2):
- stateless-rpc waits for stdin EOF, which never comes over SSH — fetch
  runs upload-pack interactively (UploadPackSSH).
- receive-pack OPENS with the server's ref advertisement; without it both
  sides hang.
- git send-pack never closes its side before the report — the pack is
  parsed incrementally (ParsePushRequestStream) and streamed through
  IngestStream (index-pack stops at the pack trailer; the feed goroutine
  is released at child exit, not awaited). Pure-delete pushes send no
  pack at all and skip ingest.

Gates (drain, placement, max_push_bytes) apply to SSH exactly as HTTP.
Config: [server.ssh] listen/host_key/host_key_env + [[server.ssh.keys]]
(principal/key/key_env/write/admin), validation fail-closed. Law 1
amended for golang.org/x/crypto (explicit user request, issue #1).

Tests: sshd command/auth/dispatch over real loopback handshakes, the
session-limiter, PTY refusal, GIT_PROTOCOL passthrough; server gates;
real git clone+push over ssh:// against the in-process server (both
directions); IngestStream/ParsePushRequestStream/UploadPackSSH at the
git layer. Coverage floors hold for every package.
Review findings (self review, reviewer pass):
- placement gate now matches HTTP placementOK exactly: serve only when
  pl.Serve (a Maintain-only host refuses SSH object work), no placement
  info serves; metrics counter mirrored.
- drain gate refuses at phase 2 only (HTTP serves through phase 1); the
  test pins both phases.
- SSH git work takes the per-repo semaphore (MaxConcurrentPerRepo) like
  the HTTP route; busy → 'repository busy'.
- ingestFed harvests index-pack stderr on both feed paths —
  PackRejectedError.Detail and cancel-path diagnostics are populated
  again (the refactor had silently dropped them).
- capReader made race-safe (mutex on overErr; feed goroutine writes it).
- hostSigner: only a missing host key file falls through to generation;
  a configured-but-unreadable key fails the boot (TOFU protection);
  the impossible parse-again branch removed.
- per-connection session cap (16 channels) with a 17-sessions test.
- dead sentinels removed (ErrDenied, errPushTooLarge); NewMaxBytesReader
  exported for the SSH transport; counting reader tracks request bytes.
- tests: over-max_push_bytes SSH push (band-2 + unpack ng on the wire),
  maintain-only placement refusal, counting-reader edges, hostSigner
  file-error matrix.
The [[server.ssh.keys]] config shape is gone. Keys belong to principals
and live in the object store (Law 4 — no state outside the bucket):

  ssh-keys/k/<fp>                the key record (auth lookup: 1 GET by
                                 fingerprint — no LIST on the SSH hot path)
  ssh-keys/u/<principal>/<fp>    the per-principal listing entry (UI list)

- internal/server/sshkeys.go: the store-backed registry — Add (PutCreate,
  duplicate fingerprints 409, listing-entry rollback on failure), List
  (torn index entries skipped), Get/Delete (ownership enforced), and
  LookupByFingerprint, which resolves the principal's rights at SSH-auth
  time through the mode-aware PrincipalForName (none -> anon-all; oidc ->
  admission lists; token -> the aggregate of the principal's tokens).
- AuthService.PrincipalForName + the typed-nil fix on the oidc branch
  (returning principalFromEmail directly made every successful oidc
  lookup a non-nil error interface holding a nil *AuthError).
- /api/v1/ssh-keys: GET (list), POST (add), DELETE /{id} — read-gated
  (identity self-service: a no-write principal registers a read-only
  key; its SSH rights still resolve per principal at auth time).
- internal/sshd: Config.Keys -> Config.KeyLookup (the registry's lookup
  behind the auth callback); the server wires the registry in.
- web/src/pages/Keys.jsx + /keys route + nav: list/add/delete with the
  short-fingerprint display; the page is plain fetch over the API (the
  SDK is repo-scoped).
- config: [[server.ssh.keys]] removed; checkSSH validates only the
  listener shape. The setup page's ssh.keys field is gone with it.
- Docs: 17_ssh.md §3 rewritten (store layout, principal resolution,
  self-service), 11_config_cli.md ssh.keys rows removed.

Tests: registry CRUD/ownership/duplicate/rollback, PrincipalForName
branches per mode, API surface (none-mode add/list/delete/duplicate/
not-configured), and the e2e story: key added via the HTTP API, used
for a clone and a push, removal revokes the access.
Author
Owner

Pushed fb7e5bc: SSH keys are now user-managed per the review discussion.

  • Keys moved out of [[server.ssh.keys]] config into the object store: ssh-keys/k/<fp> (the auth lookup — one GET by fingerprint, no LIST on the hot path) and ssh-keys/u/<principal>/<fp> (the UI listing). Duplicate fingerprints are refused with a listing-entry rollback; deleting a key frees the fingerprint for re-registration.
  • Rights are resolved at SSH-auth time through the mode-aware PrincipalForName: auth none -> the anon-all principal (anyone can add keys and they push as anon); oidc -> the same admission/write/admin rules the browser login applies to that email; token -> the aggregate of the principal's static tokens. A principal whose credentials are gone is denied at lookup time.
  • The /api/v1/ssh-keys surface (list/add/delete) is read-gated identity self-service: a no-write principal can register a read-only key (clone works, push is refused by the per-principal rights).
  • The /keys SPA page: list, add, remove with short-fingerprint display.
  • The config now carries only the server: listen + host_key/host_key_env.

make ci green; the e2e now covers the full story: key added via the HTTP API, used for a clone and a push, removal revokes the access.

Pushed fb7e5bc: **SSH keys are now user-managed** per the review discussion. - Keys moved out of `[[server.ssh.keys]]` config into the object store: `ssh-keys/k/<fp>` (the auth lookup — one GET by fingerprint, no LIST on the hot path) and `ssh-keys/u/<principal>/<fp>` (the UI listing). Duplicate fingerprints are refused with a listing-entry rollback; deleting a key frees the fingerprint for re-registration. - Rights are resolved **at SSH-auth time** through the mode-aware `PrincipalForName`: auth `none` -> the anon-all principal (anyone can add keys and they push as anon); oidc -> the same admission/write/admin rules the browser login applies to that email; token -> the aggregate of the principal's static tokens. A principal whose credentials are gone is denied at lookup time. - The `/api/v1/ssh-keys` surface (list/add/delete) is read-gated identity self-service: a no-write principal can register a read-only key (clone works, push is refused by the per-principal rights). - The `/keys` SPA page: list, add, remove with short-fingerprint display. - The config now carries only the server: `listen` + `host_key`/`host_key_env`. `make ci` green; the e2e now covers the full story: key added via the HTTP API, used for a clone and a push, removal revokes the access.
Both compose stacks publish 2222 and set WALHUB__SERVER__SSH__LISTEN
(the transport is opt-in per deploy). The host key auto-generates into
the data volume, so client TOFU pins survive container replacement.

README: the compose section documents the SSH port, the /keys page for
adding keys, and the ssh:// clone URL. 17_ssh.md gains a deployment
section (§8).
docs/EVIDENCE.md: performance evidence document with method standards
(measured on the real code path, named backend, both population sizes,
reproduction harness committed to the repo). Entry E1: SSH key registry
at 10k and 1M keys — auth flat (O(1) per handshake, fingerprint = the
storage key), keys page linear in per-user keys, writes constant.

internal/devtools/keybench: the committed reproduction harness for E1
(skipped unless WALHUB_EVIDENCE=1; ~40s, ~2GB memory store).

internal/server/sshkeys.go: the SSHKeyRegistry now has a proper
constructor (NewSSHKeyRegistry) so the keybench and tests share it.
Additional tests: store-error injection (list/put/delete/get failures),
torn index entries, placement lookup errors, advertisement errors,
host_key_env resolution branches, and the counting-reader cap edges.
Coverage: server 95.2%, config 95.4%, sshd 95.5% — all above the floor.

AGENTS.md: working-rules pointer to the evidence doc.

Also in this commit: the /api/v1/ssh-keys handlers, /keys SPA page,
PrincipalForName (with the typed-nil fix on the oidc branch), and the
SSH transport work from the prior commits on this branch.
crueber left a comment

Review verdict: fix-first (do not merge as-is)

The architecture is right -- the SSH wire layer owns auth/parsing only, the git pipeline is genuinely shared (pushPipeline, uploadPackPrepare), gates have HTTP parity (phase-2-only drain, pl.Serve placement, per-repo semaphore), and the framing traps are handled correctly (no --stateless-rpc over SSH, advertisement-first receive-pack, incremental command parse + IngestStream, delete-push skips ingest). Verified locally: go vet clean; sshd, config, git -race, server -race all pass. Drain parity via router.go drainGate checks out, and the TOFU protection on the host key is the secure choice.

But the six inline findings block the merge: a session-limiter scope bug + counter leak (sshd.go), a capReader data race in stream mode (ingest.go), a max_push_bytes bypass edge (bind_ssh.go), stale normative docs in two files (Law 12), and the keybench skip vs Law 11. Details inline.

Nits folded in here: 17_ssh.md section 6 omits the per-conn 16 cap; SSHReceivePack drops Write/Admin flags (safe today since write is enforced pre-dispatch and Publish takes only the name, but fragile -- a comment would help); principal names containing / would break the ssh-keys/u/ listing prefix.

Fix list: (1) rewrite 17_ssh.md section 3 + 11_config_cli.md SSH rows/rule 11 for store-managed keys; (2) fix limiter scope + leak; (3) mutex on capReader; (4) clamp remaining >= 0 / early-refuse; (5) reconcile the keybench skip with Law 11. Happy to re-review after.

## Review verdict: fix-first (do not merge as-is) The architecture is right -- the SSH wire layer owns auth/parsing only, the git pipeline is genuinely shared (`pushPipeline`, `uploadPackPrepare`), gates have HTTP parity (phase-2-only drain, `pl.Serve` placement, per-repo semaphore), and the framing traps are handled correctly (no `--stateless-rpc` over SSH, advertisement-first receive-pack, incremental command parse + `IngestStream`, delete-push skips ingest). Verified locally: `go vet` clean; `sshd`, `config`, `git -race`, `server -race` all pass. Drain parity via `router.go` `drainGate` checks out, and the TOFU protection on the host key is the secure choice. But the six inline findings block the merge: a session-limiter scope bug + counter leak (`sshd.go`), a `capReader` data race in stream mode (`ingest.go`), a `max_push_bytes` bypass edge (`bind_ssh.go`), stale normative docs in two files (Law 12), and the `keybench` skip vs Law 11. Details inline. Nits folded in here: `17_ssh.md` section 6 omits the per-conn 16 cap; `SSHReceivePack` drops Write/Admin flags (safe today since write is enforced pre-dispatch and `Publish` takes only the name, but fragile -- a comment would help); principal names containing `/` would break the `ssh-keys/u/` listing prefix. Fix list: (1) rewrite `17_ssh.md` section 3 + `11_config_cli.md` SSH rows/rule 11 for store-managed keys; (2) fix limiter scope + leak; (3) mutex on `capReader`; (4) clamp `remaining >= 0` / early-refuse; (5) reconcile the `keybench` skip with Law 11. Happy to re-review after.
@ -61,1 +61,4 @@
| `server.tls.cert/key/hostnames` | — | files-mode PEM; self-signed SANs (default localhost, *.localhost, 127.0.0.1, ::1 + public_url host) |
| `server.ssh.listen` | `""` | SSH git transport bind address; empty = disabled (17_ssh.md) |
| `server.ssh.host_key` / `host_key_env` | — | OpenSSH/PEM private key: path / env var NAME; auto-generated ed25519 under `<data-dir>/ssh/` when unset |
| `server.ssh.keys[]` | — | `{principal, key \| key_env, write, admin}`; public keys allowed to clone/fetch (and push with `write`) over SSH |
Author
Owner

Stale row: server.ssh.keys[] was removed (must fix, Law 12)

fb7e5bc deleted config-managed SSH keys, but this row (and rule 11 below, which describes keys[] entry validation) still documents them. checkSSH now validates listener-shape only. Please drop the row and rewrite rule 11 to match -- see also the 17_ssh.md section 3 comment.

**Stale row: `server.ssh.keys[]` was removed (must fix, Law 12)** `fb7e5bc` deleted config-managed SSH keys, but this row (and rule 11 below, which describes `keys[]` entry validation) still documents them. `checkSSH` now validates listener-shape only. Please drop the row and rewrite rule 11 to match -- see also the `17_ssh.md` section 3 comment.
@ -0,0 +54,4 @@
listen = "" # e.g. "0.0.0.0:2222"; empty = disabled (default)
host_key = "" # path to an OpenSSH/PEM private key
host_key_env = "" # env var NAME holding the private key; overrides host_key
[[server.ssh.keys]]
Author
Owner

Stale section 3: [[server.ssh.keys]] no longer exists (must fix, Law 12)

fb7e5bc removed config-managed keys (config.go ServerSSH is listener-only; validate.go checkSSH validates just the listener), but section 3 still documents the [[server.ssh.keys]] TOML shape, boot-time key validation, and "keys are a credential class of their own". Code and doc disagree -- please rewrite section 3 for what shipped: the ssh-keys/k/ + ssh-keys/u/ store layout, PrincipalForName mode-aware resolution at auth time, and the self-service API. Same staleness at docs/go/11_config_cli.md:64 (server.ssh.keys[] row) and rule 11 (describes keys[] validation that no longer exists).

**Stale section 3: `[[server.ssh.keys]]` no longer exists (must fix, Law 12)** `fb7e5bc` removed config-managed keys (`config.go` `ServerSSH` is listener-only; `validate.go` `checkSSH` validates just the listener), but section 3 still documents the `[[server.ssh.keys]]` TOML shape, boot-time key validation, and "keys are a credential class of their own". Code and doc disagree -- please rewrite section 3 for what shipped: the `ssh-keys/k/` + `ssh-keys/u/` store layout, `PrincipalForName` mode-aware resolution at auth time, and the self-service API. Same staleness at `docs/go/11_config_cli.md:64` (`server.ssh.keys[]` row) and rule 11 (describes `keys[]` validation that no longer exists).
@ -0,0 +36,4 @@
// one GET of latency per lookup and per list entry (see E1 in the evidence doc).
func TestSSHKeyRegistryScale(t *testing.T) {
if os.Getenv("WALHUB_EVIDENCE") != "1" {
t.Skip("set WALHUB_EVIDENCE=1 to run the evidence benchmark (~40s, ~2GB RAM)")
Author
Owner

Skipped-by-default test vs Law 11 (please reconcile)

Law 11 says "never merge with skipped tests", and this t.Skips unless WALHUB_EVIDENCE=1 (plus a testing.Short skip), so CI will report it skipped. As an evidence harness that is arguably the right shape -- but then it should not look like a unit test: a //go:build evidence tag (or a main harness under internal/devtools/) expresses "harness, not CI test" without tripping the law. At minimum, record it in the PR as an accepted Law-11 exception so the next reader does not have to re-litigate it.

**Skipped-by-default test vs Law 11 (please reconcile)** Law 11 says "never merge with skipped tests", and this `t.Skip`s unless `WALHUB_EVIDENCE=1` (plus a `testing.Short` skip), so CI will report it skipped. As an evidence harness that is arguably the right shape -- but then it should not look like a unit test: a `//go:build evidence` tag (or a `main` harness under `internal/devtools/`) expresses "harness, not CI test" without tripping the law. At minimum, record it in the PR as an accepted Law-11 exception so the next reader does not have to re-litigate it.
@ -57,0 +112,4 @@
func newCapReader(r io.Reader, max int64) *capReader { return &capReader{r: r, max: max} }
func (c *capReader) over() error {
Author
Owner

capReader data race in stream mode (must fix)

total/overErr are written by the feed goroutine in Read and read here in over(). The staged path (waitFeed: true) awaits the feed before returning, so it is safe -- but IngestStream (waitFeed: false, the only mode SSH uses) returns from cmd.Run() while the feed goroutine may still be in Read. over() is then called concurrently with Read: -race will flag overErr, and in production it is a real torn-read. The PR description claims "capReader race made airtight (mutex on overErr)" but no mutex landed. Please guard total/overErr with a sync.Mutex (or atomics).

**`capReader` data race in stream mode (must fix)** `total`/`overErr` are written by the feed goroutine in `Read` and read here in `over()`. The staged path (`waitFeed: true`) awaits the feed before returning, so it is safe -- but `IngestStream` (`waitFeed: false`, the only mode SSH uses) returns from `cmd.Run()` while the feed goroutine may still be in `Read`. `over()` is then called concurrently with `Read`: `-race` will flag `overErr`, and in production it is a real torn-read. The PR description claims "capReader race made airtight (mutex on overErr)" but no mutex landed. Please guard `total`/`overErr` with a `sync.Mutex` (or atomics).
@ -0,0 +127,4 @@
if perr != nil {
return fmt.Errorf("malformed push request: %w", perr)
}
remaining := int64(s.cfg.Server.MaxPushBytes) - lr.n
Author
Owner

Negative remaining disables the pack cap (should fix)

If the command section alone reaches max_push_bytes (lr.n >= max), remaining goes negative and IngestStream -> newCapReader(pack, negative) treats max <= 0 as unlimited (if c.max > 0 guard in ingest.go) -- an over-cap command section silently disables the pack cap. Unreachable at the 64 GiB default, but real with a small max_push_bytes. Suggested: refuse early when lr.n >= max (band-2 + unpack-ng shape, like the pack path) and clamp remaining >= 0.

Related nit: a cap hit inside the command section currently surfaces as malformed push request: ...ErrMaxBytes (transport error -> stderr + exit 1) rather than the band-2 + unpack-ng refusal the pack path uses. Worth mapping for consistency.

**Negative `remaining` disables the pack cap (should fix)** If the command section alone reaches `max_push_bytes` (`lr.n >= max`), `remaining` goes negative and `IngestStream` -> `newCapReader(pack, negative)` treats `max <= 0` as **unlimited** (`if c.max > 0` guard in `ingest.go`) -- an over-cap command section silently disables the pack cap. Unreachable at the 64 GiB default, but real with a small `max_push_bytes`. Suggested: refuse early when `lr.n >= max` (band-2 + unpack-ng shape, like the pack path) and clamp `remaining >= 0`. Related nit: a cap hit *inside* the command section currently surfaces as `malformed push request: ...ErrMaxBytes` (transport error -> stderr + exit 1) rather than the band-2 + unpack-ng refusal the pack path uses. Worth mapping for consistency.
@ -0,0 +258,4 @@
return
}
s.mu.Lock()
if s.live[sconn]++; s.live[sconn] > maxSessionsPerConn {
Author
Owner

Session limiter: wrong scope + counter leak (must fix)

Two bugs here:

  1. The pre-check above (s.liveSessions() >= maxSessionsPerConn) sums sessions across all connections, so the intended "per-connection cap of 16" is actually a server-global 16-session cap -- two connections with 8 sessions each block a third connection's first session, and effective global concurrency becomes 16 instead of the documented MaxSessions 64.

  2. This rejection path increments s.live[sconn] and returns without decrementing. The entry is only deleted on connection close, so every rejected attempt permanently burns one slot on that connection -- after a few rejections the connection can open no sessions at all.

Suggested fix: drop the liveSessions() pre-check (the locked s.live[sconn] check below is already the race-safe per-conn cap) and decrement before rejecting:

s.mu.Lock()
s.live[sconn]++
if s.live[sconn] > maxSessionsPerConn {
	s.live[sconn]--
	s.mu.Unlock()
	_ = newCh.Reject(gossh.Prohibited, "walhub: too many sessions on this connection")
	return
}
s.mu.Unlock()
**Session limiter: wrong scope + counter leak (must fix)** Two bugs here: 1. The pre-check above (`s.liveSessions() >= maxSessionsPerConn`) sums sessions across **all** connections, so the intended "per-connection cap of 16" is actually a **server-global 16-session cap** -- two connections with 8 sessions each block a third connection's first session, and effective global concurrency becomes 16 instead of the documented `MaxSessions` 64. 2. This rejection path increments `s.live[sconn]` and returns **without decrementing**. The entry is only deleted on connection close, so every rejected attempt permanently burns one slot on that connection -- after a few rejections the connection can open no sessions at all. Suggested fix: drop the `liveSessions()` pre-check (the locked `s.live[sconn]` check below is already the race-safe per-conn cap) and decrement before rejecting: ```go s.mu.Lock() s.live[sconn]++ if s.live[sconn] > maxSessionsPerConn { s.live[sconn]-- s.mu.Unlock() _ = newCh.Reject(gossh.Prohibited, "walhub: too many sessions on this connection") return } s.mu.Unlock() ```
All six inline findings from the 2026-09-03 review:

1. sshd session limiter (must fix): dropped the server-global liveSessions
   pre-check that serialized unrelated connections — the cap is per
   connection, checked under the same lock that counts it, and a refused
   attempt decrements before rejecting (no burned slot). Regression test
   holds 16 sessions on one connection, refuses the 17th, and proves a
   second connection and a post-refusal attempt both open.

2. capReader race in stream mode (must fix): total/overErr are now
   mutex-guarded; IngestStream's over() reads concurrently with the feed
   goroutine's Read. -race clean on the sshd+server suites.

3. Negative remaining disabled the pack cap (should fix): the command
   section alone reaching max_push_bytes now refuses on the wire
   (band-2 + unpack ng, "push exceeds max_push_bytes"), remaining is
   clamped >= 0, and a cap hit inside the command section maps to the
   git-wire refusal shape instead of a transport error — ParsePushRequestStream
   now propagates ErrMaxBytes unwrapped so errors.Is finds it.

4+5. Docs match the shipped shape (Law 12): 17_ssh.md §3 rewritten for
   store-managed keys (ssh-keys/k/ + ssh-keys/u/, PrincipalForName at
   auth time, /api/v1/ssh-keys + /keys); §4 documents both refusal
   shapes; §6 documents the per-connection 16-session cap;
   11_config_cli.md drops the server.ssh.keys[] row and rule 11 now
   describes listener-shape validation only. web/src/lib/setup.js drops
   the stale server.ssh.keys field.

6. keybench is a harness, not a unit test: moved behind the
   build tag so go test ./... never compiles it (nothing is reported
   skipped — Law 11 holds). EVIDENCE.md documents the new invocation.

Nits folded in: SSHReceivePack documents why Write/Admin stay unset
(enforced pre-dispatch by the sshd exec gate); principals must be a
single path segment (ErrKeyBadPrincipal, 400) so ssh-keys/u/ listings
cannot escape their prefix.

Dead NewMaxBytesReader removed (no callers since the countingReader).
make ci green: git 95.1%, server 95.2%, sshd 96.5% — all above the floor.
Author
Owner

Pushed 29c47e1 addressing all six inline findings:

  1. sshd session limiter — global pre-check dropped; per-connection cap checked under the counting lock; the refused path decrements before rejecting. Regression test (TestSessionCapIsPerConnection) fills one connection to 16, refuses the 17th, then proves both that a second connection opens freely and that the first connection still has its full budget after the refusal.
  2. capReader race — total/overErr mutex-guarded; sshd + server suites -race clean.
  3. Negative remaining — the command section alone consuming max_push_bytes now refuses on the wire (band-2 + unpack ng, "push exceeds max_push_bytes"), remaining is clamped >= 0, and a cap hit inside the command section maps to the git-wire refusal shape instead of a transport error (ParsePushRequestStream propagates ErrMaxBytes unwrapped so errors.Is finds it). Two new tests cover both cap-hit sites.
    4+5. Docs — 17_ssh.md §3 rewritten for store-managed keys (ssh-keys/k/ + ssh-keys/u/, PrincipalForName at auth time, the self-service API); §4 documents both refusal shapes; §6 now names the per-connection 16 cap; 11_config_cli.md row + rule 11 rewritten; the setup page's stale server.ssh.keys field removed.
  4. keybench vs Law 11 — moved behind the evidence build tag: go test ./... never compiles the file, so nothing is ever reported skipped. EVIDENCE.md documents the new invocation; the harness re-verified under the tag (32s for 10k+1M).

Nits folded in: SSHReceivePack carries a comment naming both enforcement points (auth-time KeyLookup → PrincipalForName, pre-dispatch sshd exec gate) so the unset Write/Admin flags read as intentional; principals must now be a single path segment (ErrKeyBadPrincipal → 400) so ssh-keys/u/ listings cannot escape their prefix.

Also removed dead NewMaxBytesReader (no callers since the countingReader landed). make ci green: git 95.1%, server 95.2%, sshd 96.5%.

Pushed 29c47e1 addressing all six inline findings: 1. **sshd session limiter** — global pre-check dropped; per-connection cap checked under the counting lock; the refused path decrements before rejecting. Regression test (TestSessionCapIsPerConnection) fills one connection to 16, refuses the 17th, then proves both that a second connection opens freely and that the first connection still has its full budget after the refusal. 2. **capReader race** — total/overErr mutex-guarded; sshd + server suites -race clean. 3. **Negative remaining** — the command section alone consuming max_push_bytes now refuses on the wire (band-2 + unpack ng, "push exceeds max_push_bytes"), remaining is clamped >= 0, and a cap hit inside the command section maps to the git-wire refusal shape instead of a transport error (ParsePushRequestStream propagates ErrMaxBytes unwrapped so errors.Is finds it). Two new tests cover both cap-hit sites. 4+5. **Docs** — 17_ssh.md §3 rewritten for store-managed keys (ssh-keys/k/ + ssh-keys/u/, PrincipalForName at auth time, the self-service API); §4 documents both refusal shapes; §6 now names the per-connection 16 cap; 11_config_cli.md row + rule 11 rewritten; the setup page's stale server.ssh.keys field removed. 6. **keybench vs Law 11** — moved behind the `evidence` build tag: `go test ./...` never compiles the file, so nothing is ever reported skipped. EVIDENCE.md documents the new invocation; the harness re-verified under the tag (32s for 10k+1M). Nits folded in: SSHReceivePack carries a comment naming both enforcement points (auth-time KeyLookup → PrincipalForName, pre-dispatch sshd exec gate) so the unset Write/Admin flags read as intentional; principals must now be a single path segment (ErrKeyBadPrincipal → 400) so ssh-keys/u/ listings cannot escape their prefix. Also removed dead NewMaxBytesReader (no callers since the countingReader landed). `make ci` green: git 95.1%, server 95.2%, sshd 96.5%.
crueber left a comment

Re-review of 29c47e1: all six findings fixed, one new blocker

Verified each fix against the code and ran the suites locally (-race: sshd, config, git, server, api all green; go vet ./... clean; evidence-tagged keybench excluded from the default build and vets clean with -tags evidence).

  • Limiter: exact fix, and TestSessionCapIsPerConnection pins scope plus leak-freedom (one timing nit inline).
  • capReader mutex: correct; holding the lock across the underlying Read is fine (single reader by construction).
  • Cap refusals: remaining clamp plus zero-allowance refuse close the bypass; ErrMaxBytes propagating unwrapped through parsePushCommands is the right call, and both new gate tests pin the wire shape plus no-publish.
  • Docs: 17_ssh.md section 3 rewrite matches the shipped registry; section 4 documents both refusal shapes; section 6 documents the per-conn cap; 11_config_cli.md row plus rule 11 and the setup.js field are gone. Two cosmetic nits: section 5 dropped the ParsePushRequestStream bullet while section 2 still references it, and there is a doubled blank line before section 7.
  • keybench tag: the Law-11-clean solution; EVIDENCE.md invocation updated.
  • Principal single-segment guard (ErrKeyBadPrincipal): good catch beyond the review, correctly closes the listing-prefix escape I noted.

One new blocker inline: sshKeysAdd lost its default: 500 branch -- any store error is now a silent 200. Restore it (one line plus a handler test) and this is merge-ready from my side.

Assumption I checked and am fine with: the synthesized Caps: report-status plus side-band-64k when the cap fires inside the first command pkt relies on git requesting both whenever offered -- true for stock send-pack, and the comment says so.

## Re-review of 29c47e1: all six findings fixed, one new blocker Verified each fix against the code and ran the suites locally (`-race`: `sshd`, `config`, `git`, `server`, `api` all green; `go vet ./...` clean; evidence-tagged keybench excluded from the default build and vets clean with `-tags evidence`). - **Limiter**: exact fix, and `TestSessionCapIsPerConnection` pins scope plus leak-freedom (one timing nit inline). - **capReader mutex**: correct; holding the lock across the underlying `Read` is fine (single reader by construction). - **Cap refusals**: `remaining` clamp plus zero-allowance refuse close the bypass; `ErrMaxBytes` propagating unwrapped through `parsePushCommands` is the right call, and both new gate tests pin the wire shape plus no-publish. - **Docs**: 17_ssh.md section 3 rewrite matches the shipped registry; section 4 documents both refusal shapes; section 6 documents the per-conn cap; `11_config_cli.md` row plus rule 11 and the setup.js field are gone. Two cosmetic nits: section 5 dropped the `ParsePushRequestStream` bullet while section 2 still references it, and there is a doubled blank line before section 7. - **keybench tag**: the Law-11-clean solution; EVIDENCE.md invocation updated. - **Principal single-segment guard** (`ErrKeyBadPrincipal`): good catch beyond the review, correctly closes the listing-prefix escape I noted. **One new blocker inline**: `sshKeysAdd` lost its `default:` 500 branch -- any store error is now a silent 200. Restore it (one line plus a handler test) and this is merge-ready from my side. Assumption I checked and am fine with: the synthesized `Caps: report-status plus side-band-64k` when the cap fires inside the first command pkt relies on git requesting both whenever offered -- true for stock send-pack, and the comment says so.
@ -0,0 +94,4 @@
switch {
case errors.Is(err, ErrKeyDuplicate):
writePlain(w, http.StatusConflict, err.Error())
case errors.Is(err, ErrKeyBadKeyLine), errors.Is(err, ErrKeyBadPrincipal):
Author
Owner

Regression: store errors now answer silent 200 (must fix)

This commit dropped the default: 500 branch when adding the ErrKeyBadPrincipal case. If SSHKeys.Add fails with anything other than duplicate/bad-line/bad-principal -- a store Put failure, the listing-rollback Delete failure, a marshal error -- the handler now returns without writing any status, so net/http emits 200 OK with an empty body for a failed write. The SPA happens to surface non-201 as an error, but API consumers get a success status for a failure, and the failure is unlogged.

Please restore the default 500 branch, plus a handler test pinning 500 on a failing store (the registry tests cover Add errors, but nothing covers this handler's mapping).

**Regression: store errors now answer silent 200 (must fix)** This commit dropped the `default:` 500 branch when adding the `ErrKeyBadPrincipal` case. If `SSHKeys.Add` fails with anything other than duplicate/bad-line/bad-principal -- a store `Put` failure, the listing-rollback `Delete` failure, a marshal error -- the handler now returns **without writing any status**, so net/http emits `200 OK` with an empty body for a failed write. The SPA happens to surface non-201 as an error, but API consumers get a success status for a failure, and the failure is unlogged. Please restore the default 500 branch, plus a handler test pinning 500 on a failing store (the registry tests cover `Add` errors, but nothing covers this handler's mapping).
@ -0,0 +275,4 @@
// Finish one session, then the same connection must admit a new one: the
// refused attempt above must not have leaked its increment.
tr.releaseOne()
time.Sleep(50 * time.Millisecond) // let the server observe the exit
Author
Owner

Nit: timing-dependent assertion (flake risk)

time.Sleep(50 * time.Millisecond) before re-opening assumes the server observes the channel close within 50 ms -- true on an idle loopback host, less certain under a loaded CI runner (this repo already carries known-flaky tests it would rather not grow). A poll-with-deadline retry around c1.NewSession() would make the no-leak assertion deterministic without changing what it proves. Non-blocking.

**Nit: timing-dependent assertion (flake risk)** `time.Sleep(50 * time.Millisecond)` before re-opening assumes the server observes the channel close within 50 ms -- true on an idle loopback host, less certain under a loaded CI runner (this repo already carries known-flaky tests it would rather not grow). A poll-with-deadline retry around `c1.NewSession()` would make the no-leak assertion deterministic without changing what it proves. Non-blocking.
Blocker: adding ErrKeyBadPrincipal to the switch swallowed the default
branch, so any other Add failure (store put, rollback delete) returned
without writing a status - net/http answered 200 with an empty body for
a failed write. The default 500 mapping is back, and the handler test
now pins both the bad-principal 400 and a store-failure 500.

Review polish: TestSessionCapIsPerConnection polls with a deadline
instead of a fixed 50ms sleep (deterministic under load); 17_ssh.md
section 5 regains the ParsePushRequestStream bullet section 2 still
references, and the doubled blank line before section 7 is gone.

make ci green: all packages above the 95.0% floor (api 95.3%).
Author
Owner

Pushed 328f780:

  • Blocker fixed: the default: 500 branch in sshKeysAdd is restored (my earlier edit replaced it when adding the ErrKeyBadPrincipal case - good catch). The handler test now pins the full mapping: duplicate 409, bad key line 400, bad principal 400, and a generic store failure (addErr = errors.New("put failed")) answering 500 with the error text - never a silent 200.
  • Timing nit: the limiter regression test now polls c1.NewSession() with a 5s deadline (10ms steps) instead of the fixed 50ms sleep - same assertion, deterministic under load.
  • Doc nits: 17_ssh.md section 5 regains the ParsePushRequestStream bullet (section 2 references it; it also now documents the unwrapped ErrMaxBytes propagation), and the doubled blank line before section 7 is gone.

make ci green, all packages above the floor (api 95.3%).

Pushed 328f780: - **Blocker fixed**: the `default:` 500 branch in `sshKeysAdd` is restored (my earlier edit replaced it when adding the `ErrKeyBadPrincipal` case - good catch). The handler test now pins the full mapping: duplicate 409, bad key line 400, bad principal 400, and a generic store failure (`addErr = errors.New("put failed")`) answering 500 with the error text - never a silent 200. - **Timing nit**: the limiter regression test now polls `c1.NewSession()` with a 5s deadline (10ms steps) instead of the fixed 50ms sleep - same assertion, deterministic under load. - **Doc nits**: 17_ssh.md section 5 regains the `ParsePushRequestStream` bullet (section 2 references it; it also now documents the unwrapped `ErrMaxBytes` propagation), and the doubled blank line before section 7 is gone. `make ci` green, all packages above the floor (api 95.3%).
crueber left a comment
No description provided.
## Verified: merge-ready from my side Checked 328f780 against the code and ran it: default 500 branch restored in `sshKeysAdd` with handler tests pinning both 400 (bad principal) and 500 (store failure); `TestSessionCapIsPerConnection` now polls with a 5s deadline instead of the fixed sleep; 17_ssh.md section 5 bullet restored and the doubled blank line gone. `go test -race`: `api`, `sshd` (plus targeted `TestSSHKeysAPI` / `TestSessionCapIsPerConnection` / both cap-gate tests / `TestRegistryBadPrincipal`) green; `go vet` clean. No remaining findings.
Sign in to join this conversation.
No description provided.