Skip to content

Served-git concurrency cap is a single global pool: an unauthenticated caller can shed authenticated pushes (#174 follow-up) #175

Description

@beardthelion

Follow-up to #174 (part of #62). Not a regression: the served-git concurrency cap #174 added is a net improvement over unbounded PID/thread exhaustion. This is a residual fairness gap in that cap.

Gap

The cap is a single global tokio::sync::Semaphore (state.git_semaphore) shared by all three served-git handlers, with no per-caller or per-class sub-limit. git_info_refs and git_upload_pack are anonymous-reachable (auth: Option<Extension<AuthenticatedDid>>) and acquire a permit at the top of the handler, before any auth/authorization. git_receive_pack (authenticated push) draws from the same pool.

So sustained anonymous load (enough concurrent info/refs or upload-pack to hold all permits, default GITLAWB_MAX_CONCURRENT_GIT_OPS=128) sheds authenticated pushes with a 503. An unauthenticated caller can degrade or deny authenticated writes across the auth boundary.

Reproduction (executed)

Capacity forced to 1 to make the contention deterministic; one held permit represents an in-flight anon op (git_permit does sem.clone().try_acquire_owned(), and the anon handlers call it):

Scenario Status Retry-After
anon info/refs, slot held 503 (anon draws from the same pool)
authenticated git-receive-pack, anon holds the slot 503 Some("1") (the Overloaded cap shed)
authenticated git-receive-pack, slot freed 500 None (reached the DB, past the cap gate)

The Overloaded shed of the authenticated push happens only while the anon op holds the slot; freeing it lets the same push through the cap gate. Counterfactual confirmed, so the anon-held slot is the cause, not a coincident 503.

Impact

Permissionless, unauthenticated callers can deny service to authenticated writes by occupying the shared pool. It is graceful (a retryable 503, not a crash or a leak), but it crosses the auth boundary: low-privilege load starves high-privilege operations.

This compounds with the gap already documented on #174: info/refs and the withheld-blob (upload_pack_excluding) path are not duration-bounded and do not reap their git child on client disconnect, so a hung anonymous op holds its slot until it exits, making the slots cheap to occupy and hold.

Directions (not prescriptive)

  • Reserve capacity for authenticated writes: a separate (smaller) semaphore for receive-pack, or a reserved fraction of the pool the read paths can't consume.
  • A per-caller / per-origin sub-limit so no single source can hold all permits.
  • Class-based pools (anon read vs authenticated write) sized independently.

Any of these wants a handler-layer regression test in the shape of the reproduction above (anon-held pool must not shed an authenticated push).

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:securityVulnerability fix or hardeningsev:mediumDegraded but workaround existssubsystem:apiNode REST API request/response surface

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions