#347 - Push guardrails: SSH key must belong to the repo owner or an org member — host-wide write flag must not be the push gate - crueber/walhub

crueber commented

2026-09-11 18:47:46 +00:00

Owner

⚠️ Sequencing: #348 must land first

This ticket depends on #348 (organizations: create-org flow, public org profile, management). The "org member" half of the push guardrail below has no meaning until organizations can be created and joined through the UI — today an org can only come into existence via the API (POST /api/v1/orgs), with no create-org UI, no org profile, and no member-management discoverability (#348 covers all of that). Implementing the repo-scoped push check before #348 would mean testing org-membership resolution against orgs users cannot create or manage.

Implementation order: #348 → #346 → #347 (the admission helper this guardrail reuses is specified in #346).

What's requested

Push guardrails: the SSH key (or HTTP credential) pushing to a repo must belong to a user who owns that repo, or a user attached to the owning org (member/owner per bindings), or holds an explicit role binding on it. A host-wide "write" flag must not be the gate.

Current state (code evidence) — the push gate is host-wide, not repo-scoped

SSH auth resolves key → principal → host-wide flags: PrincipalForName (internal/server/auth.go:907-947) returns Write/Admin booleans (token mode: the aggregate of the principal's static tokens; oidc mode: the browser-login admission flags).
The SSH exec gate (internal/sshd/sshd.go:334-339) checks only p.Write — a single host-wide boolean stamped from the key's permissions (sshd.go:275, perms.Extensions["write"]) — before dispatching git-receive-pack.
SSHReceivePack (internal/server/bind_ssh.go:91-103) then deliberately leaves Write/Admin unset on the principal (:97-102, documented 17_ssh.md §3 constraint) and never consults repo-scoped access — so any key whose principal holds host-wide write can push to ANY repo, including repos owned by other users/orgs.
HTTP receive-pack (internal/server/smart.go) gates on read-class checks + AnonymousRead; verify and align — the fix must cover both transports with one repo-scoped rule.
AutoCreateOnPush widens the hole: a push to any <owner>/<newrepo> path can create that repo under an owner string the pusher has no relationship with (see #346 for the creation-side rule).

Proposed design

Resolve the principal's role on the target repo at the push gate. At receive-pack dispatch (SSH exec gate in sshd.go and the HTTP receive-pack handler), call the identity surface's CheckRole/Resolve for (owner, repo, principal) requiring write — the existing P6 resolution (internal/identity/access.go:215-250: bindings, org-owner, team membership) already computes exactly this.
Respect the bind_ssh.go:97-102 constraint: Write/Admin are deliberately unset on the constructed principal; the repo-scoped check must therefore use the resolved principal name before/at that construction (e.g. CheckRole(ctx, id.Owner, id.Name, resolvedPrincipal, RoleWrite)), not the flag-stripped struct. Amending 17_ssh.md §3 with the new check is part of the change (docs amendment duty, law 12).
Auto-create-on-push must satisfy #346's admission rule for the owner segment (own username or member org) before creating the namespace — reference CheckCreateAccess from #346 rather than duplicating the rule.
Failure UX: reject with a clear stderr line naming the repo and the required relationship ("write access to other/repo requires owner or org membership") — not a generic write-denied.
Upload-pack (read) side: align with #345's visibility semantics (public repos readable anonymously; private repos need read role) — same gate, lower role. Keep the two checks adjacent so the matrix is testable.

Acceptance criteria

SSH push to an existing repo succeeds only when the key's principal holds write on THAT repo (owner, org member/owner, or explicit binding); a host-wide-write-only outsider is rejected on other users' repos.
HTTP push enforces the identical rule.
Auto-create-on-push obeys #346's owner-admission rule (self or member org only).
Host admins bypass (documented).
Rejection message names the repo and the required relationship.
Read (upload-pack) path aligned with #345 visibility semantics in the same gate.
Tests: push matrix (SSH + HTTP × owner / org member / binding-holder / host-write-only outsider / anonymous × existing repo, auto-create path).
17_ssh.md §3 amended in the same change (the :97-102 documented constraint evolves); 01_identity_permissions.md matrix updated if the resolution entry points change.

## ⚠️ Sequencing: #348 must land first **This ticket depends on #348 (organizations: create-org flow, public org profile, management).** The "org member" half of the push guardrail below has no meaning until organizations can be **created and joined through the UI** — today an org can only come into existence via the API (`POST /api/v1/orgs`), with no create-org UI, no org profile, and no member-management discoverability (#348 covers all of that). Implementing the repo-scoped push check before #348 would mean testing org-membership resolution against orgs users cannot create or manage. Implementation order: **#348 → #346 → #347** (the admission helper this guardrail reuses is specified in #346). --- ## What's requested Push guardrails: the SSH key (or HTTP credential) pushing to a repo must belong to a user who **owns that repo**, or a user **attached to the owning org** (member/owner per bindings), or holds an explicit role binding on it. A host-wide "write" flag must not be the gate. ## Current state (code evidence) — the push gate is host-wide, not repo-scoped - SSH auth resolves key → principal → host-wide flags: `PrincipalForName` (`internal/server/auth.go:907-947`) returns `Write`/`Admin` booleans (token mode: the aggregate of the principal's static tokens; oidc mode: the browser-login admission flags). - The SSH exec gate (`internal/sshd/sshd.go:334-339`) checks **only `p.Write`** — a single host-wide boolean stamped from the key's permissions (`sshd.go:275`, `perms.Extensions["write"]`) — before dispatching `git-receive-pack`. - `SSHReceivePack` (`internal/server/bind_ssh.go:91-103`) then deliberately leaves Write/Admin unset on the principal (:97-102, documented 17_ssh.md §3 constraint) and never consults repo-scoped access — so **any key whose principal holds host-wide write can push to ANY repo**, including repos owned by other users/orgs. - HTTP receive-pack (`internal/server/smart.go`) gates on read-class checks + `AnonymousRead`; verify and align — the fix must cover both transports with one repo-scoped rule. - `AutoCreateOnPush` widens the hole: a push to any `<owner>/<newrepo>` path can create that repo under an owner string the pusher has no relationship with (see #346 for the creation-side rule). ## Proposed design 1. **Resolve the principal's role on the target repo at the push gate.** At receive-pack dispatch (SSH exec gate in `sshd.go` and the HTTP receive-pack handler), call the identity surface's `CheckRole`/`Resolve` for `(owner, repo, principal)` requiring write — the existing P6 resolution (`internal/identity/access.go:215-250`: bindings, org-owner, team membership) already computes exactly this. 2. **Respect the bind_ssh.go:97-102 constraint**: Write/Admin are deliberately unset on the constructed principal; the repo-scoped check must therefore use the **resolved principal name** before/at that construction (e.g. `CheckRole(ctx, id.Owner, id.Name, resolvedPrincipal, RoleWrite)`), not the flag-stripped struct. Amending 17_ssh.md §3 with the new check is part of the change (docs amendment duty, law 12). 3. **Auto-create-on-push** must satisfy #346's admission rule for the owner segment (own username or member org) before creating the namespace — reference `CheckCreateAccess` from #346 rather than duplicating the rule. 4. **Failure UX**: reject with a clear stderr line naming the repo and the required relationship ("write access to `other/repo` requires owner or org membership") — not a generic write-denied. 5. **Upload-pack (read) side**: align with #345's visibility semantics (public repos readable anonymously; private repos need read role) — same gate, lower role. Keep the two checks adjacent so the matrix is testable. ## Acceptance criteria - [ ] SSH push to an existing repo succeeds only when the key's principal holds write on THAT repo (owner, org member/owner, or explicit binding); a host-wide-write-only outsider is rejected on other users' repos. - [ ] HTTP push enforces the identical rule. - [ ] Auto-create-on-push obeys #346's owner-admission rule (self or member org only). - [ ] Host admins bypass (documented). - [ ] Rejection message names the repo and the required relationship. - [ ] Read (upload-pack) path aligned with #345 visibility semantics in the same gate. - [ ] Tests: push matrix (SSH + HTTP × owner / org member / binding-holder / host-write-only outsider / anonymous × existing repo, auto-create path). - [ ] `17_ssh.md` §3 amended in the same change (the :97-102 documented constraint evolves); `01_identity_permissions.md` matrix updated if the resolution entry points change.

crueber added the

git storage

label

2026-09-11 18:48:31 +00:00

crueber added this to the v1 milestone

2026-09-11 18:48:31 +00:00

crueber referenced this issue

2026-09-11 18:52:47 +00:00

Organizations: create-org flow, public org profile page, and discoverable owner-only management (profile edit + member add/remove) #348

crueber referenced this issue

2026-09-11 18:58:15 +00:00

Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346

crueber referenced this issue

2026-09-11 18:58:59 +00:00

Organizations: create-org flow, public org profile page, and discoverable owner-only management (profile edit + member add/remove) #348

crueber referenced this issue from a commit

2026-09-12 00:07:21 +00:00

Fix #346: create/import owner binding (self/org-member/admin)

crueber referenced this issue

2026-09-12 00:07:24 +00:00

Fix #346: create/import owner binding #356

crueber referenced this issue

2026-09-12 00:07:28 +00:00

Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346

crueber referenced this issue

2026-09-12 00:10:17 +00:00

Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346

crueber referenced this issue

2026-09-12 00:10:37 +00:00

Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346

crueber referenced this issue from a commit

2026-09-12 00:42:01 +00:00

Fix #347: push guardrails (repo-scoped push rule on both transports)

crueber referenced this issue from a pull request that will close it,

2026-09-12 00:42:03 +00:00

Fix #347: push guardrails #357

crueber commented

2026-09-12 00:42:05 +00:00

Author

Owner

Fix ready for review: #357 (branch fix/issue-347). Repo-scoped push rule on both transports + #346 admission for auto-create, per the issue design. Not merging.

Fix ready for review: https://git.packden.us/crueber/walhub/pulls/357 (branch fix/issue-347). Repo-scoped push rule on both transports + #346 admission for auto-create, per the issue design. Not merging.

crueber commented

2026-09-12 00:46:50 +00:00

Author

Owner

Review: PR #357 (fix/issue-347) — push guardrails

Verdict: READY TO MERGE (no blocking findings; two non-blocking notes below). Verified in a scratch worktree at 61849d9 (removed afterward); main worktree left clean; no live instance/docker/browser touched (tests + reasoning only, per brief — nothing here is browser-facing).

Adversarial checklist (all pass)

No push path bypasses the gate. SSH: exec host-write pre-check deleted (sshd.go:340-348), every receive-pack dispatches into SSHReceivePack which runs gatePush first (bind_ssh.go:111-120). HTTP: discovery (smart.go:124-133), direct POST receivePack (374-379), and broker-fallback receivePackLocal (435-440) all gate; broker-forwarded requests re-gate on the broker via receivePackLocal. Only SSHReceivePack implements the Transport interface — no second implementer to drift. Nil-gate legacy path is unreachable in production wiring (serve.go wires ident whenever it exists; nil only in setup-only mode where push routes 503 before any handler).
Deny shapes. Anonymous→real 401 (identity/pushgate.go:43-45), foreign→403 naming owner/repo + required relationship, Sync-probe error→503+Retry-After (never fail-open; TestPushGuardProbeError). 401/403-not-404 matches the #345 existence model (filtered listings, not status codes).
Host-write flag stripped. p.Write gone from sshd.go entirely; CheckPush resolves over auth.Principal{Name:} only (flags unset), so P6 step 3 cannot re-grant; CheckCreateOwner likewise never reads Write. Residual requireWrite calls are eventsNotify (internal bridge) and LFS upload (see note 2) — no git push path.
17_ssh §3 honored. Pipeline principal carries name only (bind_ssh.go:121); gate consults name + admin bypass, never connection flags. Transport signature principal-string→sshd.Principal is the only shape change, documented in 17.7.
Auto-create admission first. gatePush: write-deny + createOn + Sync-NotFound → CheckCreateOwner verbatim (#346 reuse, no fork); discovery re-checks admission after its Sync (closes the gate-probe→Sync race). No wider hole; deny-path TOCTOU reviewed (a deny stands regardless of interleaving creates).
Slug-namespace self rule sound. normPrincipal (lower+trim, identity.go:133) exact-matches owner==name in BOTH CheckPush and CheckCreateOwner — same folding as #346, grants only one's own segment, cannot reach another user's repo.
Git argv unchanged. Zero diff in internal/git, internal/wal.
Budgets. Push-budget test (cmd/walhub, auth-none admin bypass, zero reads) passes unmodified with the live gate wired; Sync probe runs on deny path only.
E2E real-git both transports. TestPushGuardEndToEndHTTP + TestPushGuardEndToEndSSH pass (foreign denied with named message, self/member/auto-create allowed, public-read alignment intact).
Coverage/-race/fmt/vet/docs. identity 97.2, sshd 96.5, server 98.4 (all ≥95); -race green on identity/sshd/server/cmd; gofmt/vet clean; go build clean; internal/e2e green (45s). Docs: 06 §4.4, 17.7, 01 §5.3 + decisions all accurate to the code.

Pre-existing failure (not this PR)

TestUIAssetConcepts fails identically on origin/main (verified in a second scratch worktree): needs concepts/.gif from a newer make web than the checked-in dist. TestPushGuard suites themselves are green.

Non-blocking notes (follow-ups, not merge gates)

Double-gate cost on same-process direct HTTP push: receivePack and receivePackLocal each run gatePush→CheckPush→Resolve, i.e. 2× Resolve reads per token-mode push (auth-none/admin short-circuit to zero, so budgets still pin). Consider threading an already-gated marker for the same-process fallback entry (broker entry must keep its own gate).
LFS upload still host-flag gated (lfs.go:61 lfsAuth write branch). Harmless today — orphan blobs cannot become refs without passing the push gate — but flag it so a future LFS-pointer check does not assume repo-scoped upload auth.

No fixes pushed (nothing correctness-grade to fix). MERGE RECOMMENDATION: ready to merge.

## Review: PR #357 (fix/issue-347) — push guardrails **Verdict: READY TO MERGE** (no blocking findings; two non-blocking notes below). Verified in a scratch worktree at 61849d9 (removed afterward); main worktree left clean; no live instance/docker/browser touched (tests + reasoning only, per brief — nothing here is browser-facing). ### Adversarial checklist (all pass) 1. **No push path bypasses the gate.** SSH: exec host-write pre-check deleted (sshd.go:340-348), every receive-pack dispatches into SSHReceivePack which runs gatePush first (bind_ssh.go:111-120). HTTP: discovery (smart.go:124-133), direct POST receivePack (374-379), and broker-fallback receivePackLocal (435-440) all gate; broker-forwarded requests re-gate on the broker via receivePackLocal. Only SSHReceivePack implements the Transport interface — no second implementer to drift. Nil-gate legacy path is unreachable in production wiring (serve.go wires ident whenever it exists; nil only in setup-only mode where push routes 503 before any handler). 2. **Deny shapes.** Anonymous→real 401 (identity/pushgate.go:43-45), foreign→403 naming owner/repo + required relationship, Sync-probe error→503+Retry-After (never fail-open; TestPushGuardProbeError). 401/403-not-404 matches the #345 existence model (filtered listings, not status codes). 3. **Host-write flag stripped.** p.Write gone from sshd.go entirely; CheckPush resolves over auth.Principal{Name:} only (flags unset), so P6 step 3 cannot re-grant; CheckCreateOwner likewise never reads Write. Residual requireWrite calls are eventsNotify (internal bridge) and LFS upload (see note 2) — no git push path. 4. **17_ssh §3 honored.** Pipeline principal carries name only (bind_ssh.go:121); gate consults name + admin bypass, never connection flags. Transport signature principal-string→sshd.Principal is the only shape change, documented in 17.7. 5. **Auto-create admission first.** gatePush: write-deny + createOn + Sync-NotFound → CheckCreateOwner verbatim (#346 reuse, no fork); discovery re-checks admission after its Sync (closes the gate-probe→Sync race). No wider hole; deny-path TOCTOU reviewed (a deny stands regardless of interleaving creates). 6. **Slug-namespace self rule sound.** normPrincipal (lower+trim, identity.go:133) exact-matches owner==name in BOTH CheckPush and CheckCreateOwner — same folding as #346, grants only one's own segment, cannot reach another user's repo. 7. **Git argv unchanged.** Zero diff in internal/git, internal/wal. 8. **Budgets.** Push-budget test (cmd/walhub, auth-none admin bypass, zero reads) passes unmodified with the live gate wired; Sync probe runs on deny path only. 9. **E2E real-git both transports.** TestPushGuardEndToEndHTTP + TestPushGuardEndToEndSSH pass (foreign denied with named message, self/member/auto-create allowed, public-read alignment intact). 10. **Coverage/-race/fmt/vet/docs.** identity 97.2, sshd 96.5, server 98.4 (all ≥95); -race green on identity/sshd/server/cmd; gofmt/vet clean; go build clean; internal/e2e green (45s). Docs: 06 §4.4, 17.7, 01 §5.3 + decisions all accurate to the code. ### Pre-existing failure (not this PR) - TestUIAssetConcepts fails identically on origin/main (verified in a second scratch worktree): needs concepts/*.gif from a newer make web than the checked-in dist. TestPushGuard* suites themselves are green. ### Non-blocking notes (follow-ups, not merge gates) 1. **Double-gate cost on same-process direct HTTP push:** receivePack and receivePackLocal each run gatePush→CheckPush→Resolve, i.e. 2× Resolve reads per token-mode push (auth-none/admin short-circuit to zero, so budgets still pin). Consider threading an already-gated marker for the same-process fallback entry (broker entry must keep its own gate). 2. **LFS upload still host-flag gated** (lfs.go:61 lfsAuth write branch). Harmless today — orphan blobs cannot become refs without passing the push gate — but flag it so a future LFS-pointer check does not assume repo-scoped upload auth. No fixes pushed (nothing correctness-grade to fix). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-12 00:47:04 +00:00

crueber referenced this issue from a commit

2026-09-12 00:47:05 +00:00

Merge pull request 'Fix #347: push guardrails' (#357) from fix/issue-347 into main

crueber commented

2026-09-12 00:47:05 +00:00

Author

Owner

Fixed by PR #357 (review clean — all 10 adversarial checks pass, both transports e2e green, budgets hold), merged. Closing.

crueber referenced this issue

2026-09-12 13:35:13 +00:00