Skip to content

Commit 0a15e76

Browse files
feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31) (#68)
* feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31) A valid did:key signature on git-receive-pack is authentication, not authorization: any party can generate a key, derive its DID, sign, and push to an unprotected branch. Owner checks only ran on protected branches, so a non-owner could write to unprotected branches of an arbitrary repo. Add an opt-in flag GITLAWB_ENFORCE_OWNER_PUSH (default false, mirroring GITLAWB_REQUIRE_SIGNED_PEER_WRITES) that requires the authenticated pusher to be the repo owner on every branch. Default-off keeps live nodes unaffected. Design follows the maintainer's review on #31: - Authorize on the canonical AuthenticatedDid injected by require_signature, via Extension<AuthenticatedDid> on the handler — not a re-parse of the headers. The legacy branch-protection check is left as-is. - Fail closed: an absent identity or a non-owner caller is rejected. The policy lives in a pure fn (owner_push_rejection) so it is unit-tested without a DB. - The gate runs before branch protection, so one rejection yields one error body. - Factor the duplicated owner-match into auth::is_repo_owner, reused by receive-pack, protect.rs, and visibility.rs (each keeps its own message). events.rs / peers.rs are left out — they aren't owner checks. - Name the push gate by intent (caller_authorized_to_push) so the planned Phase 2 UCAN git/push capability is a pure addition, not a rewrite. Tests: owner (full + short DID) allowed, non-owner rejected, missing-DID rejected, flag-off legacy. Docs: .env.example + docs/RUN-A-NODE.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(node): address review on #68 — 403 status, finish owner-match consolidation Per @beardthelion's review: - Return 403 Forbidden (not 400) for authorization denials: the owner-push gate, the branch-protection denial, and the owner checks in protect.rs / visibility.rs. 400 reads as a malformed request and some git/CI clients retry it. - Finish the is_repo_owner consolidation: route the branch-protection check and the repos list filter through the helper. The branch-protection check now uses the verified AuthenticatedDid (auth.0) instead of re-parsing the headers. - Drop the second identity source on the push path entirely: the post-push event recording also uses auth.0, so extract_did_from_auth (and the handler's HeaderMap param + import) are removed. - Tests assert the Forbidden variant (so a future status regression fails loudly) and add direct coverage for is_repo_owner / caller_authorized_to_push. - Docs: warn that enabling the flag blocks delegated and CI agents until the Phase 2 UCAN git/push capability is honored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: FrozenRaspberry <3733873+FrozenRaspberry@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 2153b0b commit 0a15e76

5 files changed

Lines changed: 205 additions & 56 deletions

File tree

‎.env.example‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,12 @@ GITLAWB_BOOTSTRAP_DISABLE_SEEDS=false
7272
# rolling upgrades so existing live nodes can still communicate.
7373
GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false
7474

75+
# Require the authenticated pusher to be the repo owner on git-receive-pack.
76+
# A valid did:key signature is authentication, not authorization: anyone can
77+
# sign as their own DID. When true, pushes from a non-owner DID are rejected.
78+
# Keep false until the repo owner is ready for owner-only writes.
79+
GITLAWB_ENFORCE_OWNER_PUSH=false
80+
7581
# Comma-separated libp2p multiaddrs.
7682
# Example: /ip4/1.2.3.4/udp/7546/quic-v1/p2p/12D3KooW...
7783
GITLAWB_P2P_BOOTSTRAP=

‎crates/gitlawb-node/src/api/repos.rs‎

Lines changed: 151 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
use axum::extract::{Extension, Path, Query, State};
2-
use axum::http::{HeaderMap, StatusCode};
2+
use axum::http::StatusCode;
33
use axum::response::Response;
44
use axum::Json;
55
use bytes::Bytes;
66
use std::sync::Arc;
77

8-
use crate::auth::AuthenticatedDid;
8+
use crate::auth::{caller_authorized_to_push, AuthenticatedDid};
99
use chrono::Utc;
1010
use serde::{Deserialize, Serialize};
1111
use uuid::Uuid;
@@ -162,8 +162,7 @@ pub async fn list_repos(
162162
.iter()
163163
.filter(|(r, _)| {
164164
if let Some(owner) = &query.owner {
165-
let short = r.owner_did.split(':').next_back().unwrap_or(&r.owner_did);
166-
short == owner.as_str() || r.owner_did == owner.as_str()
165+
crate::api::did_matches(owner.as_str(), &r.owner_did)
167166
} else {
168167
true
169168
}
@@ -474,11 +473,38 @@ pub async fn git_upload_pack(
474473
Ok(resp)
475474
}
476475

476+
/// Decide whether the owner-push gate rejects a `git-receive-pack` request.
477+
///
478+
/// Returns `Some(error)` when the push must be rejected, `None` when it may
479+
/// proceed. Pure function so the policy is unit-testable without a database or a
480+
/// live git backend.
481+
///
482+
/// Fails closed: when `enforce` is on, an absent identity (`None`) or a caller
483+
/// that is not authorized to push is rejected. When `enforce` is off it always
484+
/// allows, preserving the legacy (authentication-only) behavior.
485+
fn owner_push_rejection(
486+
enforce: bool,
487+
record: &crate::db::RepoRecord,
488+
caller: Option<&str>,
489+
) -> Option<AppError> {
490+
if !enforce {
491+
return None;
492+
}
493+
match caller {
494+
Some(did) if caller_authorized_to_push(record, did) => None,
495+
_ => Some(AppError::Forbidden(
496+
"push rejected — only the repo owner may push to this repository \
497+
(GITLAWB_ENFORCE_OWNER_PUSH is enabled)"
498+
.into(),
499+
)),
500+
}
501+
}
502+
477503
/// POST /:owner/:repo.git/git-receive-pack (AUTH REQUIRED — enforced by middleware)
478504
pub async fn git_receive_pack(
479505
State(state): State<AppState>,
480506
Path((owner, repo)): Path<(String, String)>,
481-
headers: HeaderMap,
507+
Extension(auth): Extension<AuthenticatedDid>,
482508
body: Bytes,
483509
) -> Result<Response> {
484510
let name = repo.trim_end_matches(".git");
@@ -496,9 +522,30 @@ pub async fn git_receive_pack(
496522
"parsed ref updates from pack"
497523
);
498524

525+
// ── Owner-only push enforcement (opt-in: GITLAWB_ENFORCE_OWNER_PUSH) ──
526+
// Runs before branch protection on purpose: when enabled, a non-owner is
527+
// rejected here regardless of whether the target branch is protected, so a
528+
// single rejection never yields two different error bodies. The identity is
529+
// the canonical DID injected by `require_signature`, not a re-parse of the
530+
// request headers. Fails closed (see `owner_push_rejection`).
531+
if let Some(err) = owner_push_rejection(
532+
state.config.enforce_owner_push,
533+
&record,
534+
Some(auth.0.as_str()),
535+
) {
536+
tracing::warn!(
537+
repo = %name,
538+
pusher = %auth.0,
539+
owner_did = %record.owner_did,
540+
"owner-push enforcement: rejecting push from non-owner"
541+
);
542+
return Err(err);
543+
}
544+
499545
// ── Branch protection check ──────────────────────────────────────────
500-
let pusher_did_for_check = extract_did_from_auth(&headers);
501-
tracing::debug!(pusher_did = ?pusher_did_for_check, "extracted pusher DID from auth headers");
546+
// Uses the same verified identity as the owner-push gate above. (When that
547+
// gate is enabled a non-owner never reaches here; this still applies when it
548+
// is off, gating only the branches an owner has explicitly protected.)
502549
for update in &ref_updates {
503550
// Strip refs/heads/ prefix to get plain branch name
504551
let branch = update
@@ -510,27 +557,17 @@ pub async fn git_receive_pack(
510557
.is_branch_protected(&record.id, branch)
511558
.await
512559
.unwrap_or(false)
560+
&& !crate::api::did_matches(&auth.0, &record.owner_did)
513561
{
514-
let owner_short = record
515-
.owner_did
516-
.split(':')
517-
.next_back()
518-
.unwrap_or(&record.owner_did);
519-
let is_owner = pusher_did_for_check
520-
.as_deref()
521-
.map(|did| did == record.owner_did || did == owner_short)
522-
.unwrap_or(false);
523-
if !is_owner {
524-
tracing::warn!(
525-
branch = %branch,
526-
pusher = ?pusher_did_for_check,
527-
owner_did = %record.owner_did,
528-
"branch protection: rejecting push from non-owner"
529-
);
530-
return Err(AppError::BadRequest(format!(
531-
"branch '{branch}' is protected — only the repo owner can push to it"
532-
)));
533-
}
562+
tracing::warn!(
563+
branch = %branch,
564+
pusher = %auth.0,
565+
owner_did = %record.owner_did,
566+
"branch protection: rejecting push from non-owner"
567+
);
568+
return Err(AppError::Forbidden(format!(
569+
"branch '{branch}' is protected — only the repo owner can push to it"
570+
)));
534571
}
535572
}
536573

@@ -566,9 +603,11 @@ pub async fn git_receive_pack(
566603
crate::metrics::record_push(&record.id);
567604
crate::metrics::observe_pack_size(body_len as f64);
568605

569-
// Record push event for trust score and issue a signed ref certificate
570-
let pusher_did = extract_did_from_auth(&headers);
571-
if let Some(ref did) = pusher_did {
606+
// Record push event for trust score and issue a signed ref certificate.
607+
// The route is behind `require_signature`, so the verified pusher identity is
608+
// always present; use it directly rather than re-parsing the headers.
609+
let did = auth.0.as_str();
610+
{
572611
// Use the first new commit hash we parsed, fall back to timestamp
573612
let commit_hash = ref_updates
574613
.first()
@@ -626,7 +665,7 @@ pub async fn git_receive_pack(
626665
"created": update.old_sha == "0000000000000000000000000000000000000000",
627666
"forced": false,
628667
"pusher": {
629-
"did": pusher_did.as_deref().unwrap_or("unknown"),
668+
"did": did,
630669
},
631670
"repository": {
632671
"id": record.id,
@@ -806,7 +845,7 @@ pub async fn git_receive_pack(
806845
.map(|u| (u.ref_name.clone(), u.new_sha.clone()))
807846
.collect::<Vec<_>>();
808847
let p2p_handle = state.p2p.clone();
809-
let pusher_did_clone = pusher_did.clone().unwrap_or_default();
848+
let pusher_did_clone = did.to_string();
810849
let db_for_peers = state.db.clone();
811850
let ref_update_tx = state.ref_update_tx.clone();
812851
let irys_url = state.config.irys_url.clone();
@@ -1246,30 +1285,6 @@ fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
12461285
updates
12471286
}
12481287

1249-
/// Extract the DID from RFC 9421 Signature-Input header (keyid="...").
1250-
/// Falls back to draft-cavage Authorization header for old clients.
1251-
fn extract_did_from_auth(headers: &HeaderMap) -> Option<String> {
1252-
// RFC 9421: Signature-Input: sig1=(...);keyid="did:key:z6Mk...";...
1253-
if let Some(sig_input) = headers.get("signature-input").and_then(|v| v.to_str().ok()) {
1254-
if let Some(start) = sig_input.find("keyid=\"") {
1255-
let rest = &sig_input[start + 7..];
1256-
if let Some(end) = rest.find('"') {
1257-
return Some(rest[..end].to_string());
1258-
}
1259-
}
1260-
}
1261-
// Fallback: draft-cavage Authorization: Signature keyId="..."
1262-
if let Some(auth) = headers.get("authorization").and_then(|v| v.to_str().ok()) {
1263-
if let Some(start) = auth.find("keyId=\"") {
1264-
let rest = &auth[start + 7..];
1265-
if let Some(end) = rest.find('"') {
1266-
return Some(rest[..end].to_string());
1267-
}
1268-
}
1269-
}
1270-
None
1271-
}
1272-
12731288
// ── Helpers ───────────────────────────────────────────────────────────────
12741289

12751290
fn to_response(record: &crate::db::RepoRecord, state: &AppState, star_count: i64) -> RepoResponse {
@@ -1300,3 +1315,83 @@ fn to_response(record: &crate::db::RepoRecord, state: &AppState, star_count: i64
13001315
forked_from: record.forked_from.clone(),
13011316
}
13021317
}
1318+
1319+
#[cfg(test)]
1320+
mod tests {
1321+
use super::owner_push_rejection;
1322+
use crate::auth::caller_authorized_to_push;
1323+
use crate::error::AppError;
1324+
1325+
const OWNER_DID: &str = "did:key:z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH";
1326+
const OWNER_SHORT: &str = "z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH";
1327+
const STRANGER_DID: &str = "did:key:z6Mkffonly5tranger0000000000000000000000000000000";
1328+
1329+
fn repo_owned_by(owner_did: &str) -> crate::db::RepoRecord {
1330+
let now = chrono::Utc::now();
1331+
crate::db::RepoRecord {
1332+
id: "repo-id".into(),
1333+
name: "demo".into(),
1334+
owner_did: owner_did.into(),
1335+
description: None,
1336+
is_public: true,
1337+
default_branch: "main".into(),
1338+
created_at: now,
1339+
updated_at: now,
1340+
disk_path: "/tmp/demo".into(),
1341+
forked_from: None,
1342+
machine_id: None,
1343+
}
1344+
}
1345+
1346+
/// A rejection must be a 403 Forbidden (authenticated but not authorized),
1347+
/// not a 400 — some git/CI clients retry 400s.
1348+
fn assert_forbidden(rejection: Option<AppError>) {
1349+
assert!(
1350+
matches!(rejection, Some(AppError::Forbidden(_))),
1351+
"expected Some(Forbidden), got {rejection:?}"
1352+
);
1353+
}
1354+
1355+
#[test]
1356+
fn enforced_allows_owner_full_did() {
1357+
let repo = repo_owned_by(OWNER_DID);
1358+
assert!(owner_push_rejection(true, &repo, Some(OWNER_DID)).is_none());
1359+
}
1360+
1361+
#[test]
1362+
fn enforced_allows_owner_short_did() {
1363+
// Owners are accepted in bare-multibase form, matching the rest of the
1364+
// codebase's owner comparisons.
1365+
let repo = repo_owned_by(OWNER_DID);
1366+
assert!(owner_push_rejection(true, &repo, Some(OWNER_SHORT)).is_none());
1367+
}
1368+
1369+
#[test]
1370+
fn enforced_rejects_non_owner_with_forbidden() {
1371+
let repo = repo_owned_by(OWNER_DID);
1372+
assert_forbidden(owner_push_rejection(true, &repo, Some(STRANGER_DID)));
1373+
}
1374+
1375+
#[test]
1376+
fn enforced_rejects_missing_did_with_forbidden() {
1377+
// Fail closed: an absent authenticated identity is rejected, not allowed.
1378+
let repo = repo_owned_by(OWNER_DID);
1379+
assert_forbidden(owner_push_rejection(true, &repo, None));
1380+
}
1381+
1382+
#[test]
1383+
fn disabled_allows_non_owner_and_missing_did() {
1384+
// Flag off → legacy behavior: authentication-only, no owner gate.
1385+
let repo = repo_owned_by(OWNER_DID);
1386+
assert!(owner_push_rejection(false, &repo, Some(STRANGER_DID)).is_none());
1387+
assert!(owner_push_rejection(false, &repo, None).is_none());
1388+
}
1389+
1390+
#[test]
1391+
fn caller_authorized_to_push_is_owner_only_in_phase_1() {
1392+
let repo = repo_owned_by(OWNER_DID);
1393+
assert!(caller_authorized_to_push(&repo, OWNER_DID));
1394+
assert!(caller_authorized_to_push(&repo, OWNER_SHORT));
1395+
assert!(!caller_authorized_to_push(&repo, STRANGER_DID));
1396+
}
1397+
}

‎crates/gitlawb-node/src/auth/mod.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,18 @@ use crate::state::AppState;
1717
#[derive(Clone, Debug)]
1818
pub struct AuthenticatedDid(pub String);
1919

20+
/// Whether `caller` is authorized to push to `record`.
21+
///
22+
/// Phase 1 (`GITLAWB_ENFORCE_OWNER_PUSH`): owner-only, via the canonical
23+
/// [`crate::api::did_matches`] owner comparison (DID-safe on both sides). This is
24+
/// intentionally a distinct, intent-named gate rather than a bare owner check so
25+
/// that Phase 2 can extend it to honor a verified UCAN `git/push` capability as a
26+
/// pure addition (`did_matches(..) || ucan_grants_push(..)`) without rewriting
27+
/// call sites.
28+
pub fn caller_authorized_to_push(record: &crate::db::RepoRecord, caller: &str) -> bool {
29+
crate::api::did_matches(caller, &record.owner_did)
30+
}
31+
2032
use gitlawb_core::http_sig::{
2133
build_signing_string, compute_content_digest, HttpSignature, COVERED_COMPONENTS,
2234
};

‎crates/gitlawb-node/src/config.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ pub struct Config {
4949
)]
5050
pub require_signed_peer_writes: bool,
5151

52+
/// Require the authenticated pusher to be the repo owner on `git-receive-pack`.
53+
/// Authentication (a valid did:key signature) is not authorization on its own:
54+
/// any party can sign as their own DID. When true, pushes whose authenticated
55+
/// DID is not the repo owner are rejected. Keep false during rolling upgrades;
56+
/// flip it on once owners are ready for owner-only writes.
57+
#[arg(long, env = "GITLAWB_ENFORCE_OWNER_PUSH", default_value_t = false)]
58+
pub enforce_owner_push: bool,
59+
5260
/// URL of local IPFS/Kubo node HTTP API (e.g. http://127.0.0.1:5001)
5361
#[arg(long, env = "GITLAWB_IPFS_API", default_value = "")]
5462
pub ipfs_api: String,

‎docs/RUN-A-NODE.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,34 @@ During the cooldown your node still earns rewards if it keeps heartbeating.
141141

142142
---
143143

144+
## Hardening: owner-only push
145+
146+
By default the node authenticates every `git-receive-pack` push (a valid RFC 9421
147+
`did:key` signature) but does **not** check that the pusher owns the repo, except
148+
on branches that are explicitly protected. Because `did:key` is self-certifying,
149+
any party can generate a key, derive its DID, sign, and push to an unprotected
150+
branch — authentication is not authorization.
151+
152+
To require the authenticated pusher to be the repo owner on **every** branch, set:
153+
154+
```bash
155+
GITLAWB_ENFORCE_OWNER_PUSH=true
156+
```
157+
158+
- **Default `false`** — preserves current behavior so live nodes are unaffected by
159+
an upgrade. Turn it on once you're ready for owner-only writes.
160+
- **When `true`** — a push whose authenticated DID is not the repo owner is
161+
rejected (HTTP 403) before any ref update is applied. The owner is matched in
162+
both the full `did:key:z6Mk…` form and its bare `z6Mk…` suffix.
163+
- **Caution: this blocks every non-owner pusher, including your own delegated and
164+
CI agents.** Push authorization is owner-only today — a UCAN `git/push`
165+
capability is verified but not yet honored for authorization, so delegated keys
166+
cannot push while this is on. Don't enable it until every identity that pushes
167+
to your repos is the owner, or you'll lock out your own automation. Scoped
168+
collaborator / UCAN-delegated push rights are a planned follow-up.
169+
170+
---
171+
144172
## Operational checklist
145173

146174
| Concern | Recommendation |

0 commit comments

Comments
 (0)