Merge pull request #330 from Gitlawb/fix/default-enforce-owner-push

fix(node)!: enforce owner-only push by default

Author: kevincodex1

.env.example

@@ -94,9 +94,14 @@ GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false
 
 # Require the authenticated pusher to be the repo owner on git-receive-pack.
 # A valid did:key signature is authentication, not authorization: anyone can
-# sign as their own DID. When true, pushes from a non-owner DID are rejected.
-# Keep false until the repo owner is ready for owner-only writes.
-GITLAWB_ENFORCE_OWNER_PUSH=false
+# mint a key, derive its DID and sign, so with this off every signed caller may
+# push to every repository, private ones included. On by default.
+#
+# Set to false only for a rolling upgrade whose pushers are not yet the repo
+# owner. Note that delegated and CI keys count as non-owners: UCAN git/push is
+# verified but not yet honored for authorization, so they cannot push while this
+# is on. See docs/RUN-A-NODE.md.
+GITLAWB_ENFORCE_OWNER_PUSH=true
 
 # Comma-separated libp2p multiaddrs.
 # Example: /ip4/1.2.3.4/udp/7546/quic-v1/p2p/12D3KooW...
@@ -191,8 +196,9 @@ GITLAWB_MAX_CONCURRENT_READS_PER_CALLER=16
 #     many WRITE-pool slots. This one is load-bearing for push availability, since
 #     it is acquired before the global write permit: without it, one host minting
 #     disposable did:key identities could open enough slow pushes to monopolize the
-#     write pool and 503 every other source (owner enforcement defaults off, and
-#     the push rate limiter caps arrival rate, not in-flight concurrency).
+#     write pool and 503 every other source (the push rate limiter caps arrival
+#     rate, not in-flight concurrency, and owner enforcement can be turned off for
+#     a rolling upgrade).
 # Keyed on the resolved source IP, never the DID, so a DID farm does not defeat
 # them; keying granularity follows GITLAWB_TRUSTED_PROXY like the read cap above.
 

README.md

@@ -392,7 +392,8 @@ Important node settings:
 | `GITLAWB_BOOTSTRAP_PEERS` | Comma-separated HTTP peer URLs. |
 | `GITLAWB_P2P_BOOTSTRAP` | Comma-separated libp2p multiaddrs. |
 | `GITLAWB_BOOTSTRAP_DISABLE_SEEDS` | Disable embedded seed peers for isolated dev/test networks. |
-| `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. |
+| `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. Defaults to `false` during the staged rollout below. |
+| `GITLAWB_ENFORCE_OWNER_PUSH` | Require the authenticated pusher to be the repo owner on `git-receive-pack`. **Defaults to `true`.** A `did:key` signature is authentication, not authorization — anyone can mint a key and sign — so with this off every signed caller may push to every repository, private ones included. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so they cannot push while this is on. Set `false` only for a rolling upgrade; see [`docs/RUN-A-NODE.md`](docs/RUN-A-NODE.md). |
 | `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. |
 | `GITLAWB_MAX_PACK_BYTES` | Max git pack body size for smart-HTTP routes. |
 | `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` | Max seconds a served git upload-pack, receive-pack, or `info/refs` advertisement may run before it is aborted (504). Default 600. Also bounds the withheld-blob classification walk (on both the upload-pack serve and receive-pack replication paths) and the push-side pin-candidate discovery (`rev-list` / `cat-file`), each reaped via process-group teardown at the deadline. On the path-scoped upload-pack path the classification walk and the pack serve share ONE deadline, so this value bounds their combined duration rather than granting each stage a full budget: a walk that consumes it leaves the serve nothing and the clone gets a 504. Serving large path-scoped repos may therefore need a higher value than they did when each stage was budgeted separately. Accepted range is 1 to 3153600000 (100 years), since the node derives deadlines from this value and a larger one cannot be represented. |

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

@@ -78,10 +78,26 @@ pub struct Config {
 
     /// Require the authenticated pusher to be the repo owner on `git-receive-pack`.
     /// Authentication (a valid did:key signature) is not authorization on its own:
-    /// any party can sign as their own DID. When true, pushes whose authenticated
-    /// DID is not the repo owner are rejected. Keep false during rolling upgrades;
-    /// flip it on once owners are ready for owner-only writes.
-    #[arg(long, env = "GITLAWB_ENFORCE_OWNER_PUSH", default_value_t = false)]
+    /// any party can mint a did:key and sign as it, so with this off every signed
+    /// caller may push to every repository, private ones included. On by default.
+    ///
+    /// Turn it off only for a rolling upgrade whose pushers are not yet the repo
+    /// owner. Both `GITLAWB_ENFORCE_OWNER_PUSH=false` and `--enforce-owner-push
+    /// false` disable it, and the bare `--enforce-owner-push` form still means
+    /// `true`.
+    ///
+    /// The value-taking action is what makes the CLI form parse at all: as a
+    /// presence-only flag, `--enforce-owner-push false` is an "unexpected argument"
+    /// error. The env form resolved correctly either way, so it is the CLI escape
+    /// hatch this buys, not the environment one.
+    #[arg(
+        long,
+        env = "GITLAWB_ENFORCE_OWNER_PUSH",
+        action = clap::ArgAction::Set,
+        num_args = 0..=1,
+        default_value_t = true,
+        default_missing_value = "true"
+    )]
     pub enforce_owner_push: bool,
 
     /// URL of local IPFS/Kubo node HTTP API (e.g. http://127.0.0.1:5001)
@@ -1384,4 +1400,51 @@ mod tests {
             "db_max_connections at the floor (pushes + headroom) must validate"
         );
     }
+
+    /// The DECLARED default, read off the parser rather than out of a parse.
+    ///
+    /// `Config::parse_from` consults the process environment, so on a host that
+    /// exports `GITLAWB_ENFORCE_OWNER_PUSH=false` a parse-based assertion says
+    /// nothing about what this crate declares — it reports the operator's setting.
+    /// Asserting the declaration is the env-independent form, and it is the one that
+    /// actually fails if someone flips `default_value_t` back.
+    #[test]
+    fn enforce_owner_push_is_declared_true_independent_of_the_environment() {
+        use clap::CommandFactory;
+        let cmd = Config::command();
+        let arg = cmd
+            .get_arguments()
+            .find(|a| a.get_id() == "enforce_owner_push")
+            .expect("the argument must exist");
+        assert_eq!(
+            arg.get_default_values(),
+            ["true"],
+            "owner-only push must be the declared default; a node started with no \
+             configuration cannot accept a push from a self-minted key"
+        );
+    }
+
+    /// The flip must not strand an operator mid-upgrade.
+    ///
+    /// Turning the gate on is a breaking change for any deployment whose pushers are
+    /// not yet the repo owner, so the escape hatch has to keep working. As a
+    /// presence-only flag `--enforce-owner-push false` is not "false", it is an
+    /// "unexpected argument" error; the value-taking action is what makes that form
+    /// parse.
+    ///
+    /// This pins the CLI form only. The environment form resolved to `false` under
+    /// the presence-only declaration too, so it is not what this change fixed, and it
+    /// is not exercised here because the process environment is global and these
+    /// tests run in parallel.
+    #[test]
+    fn enforce_owner_push_stays_disableable_for_rolling_upgrades() {
+        assert!(
+            !Config::parse_from(["gitlawb-node", "--enforce-owner-push", "false"])
+                .enforce_owner_push,
+            "operators must still be able to opt out during a rolling upgrade"
+        );
+        assert!(
+            Config::parse_from(["gitlawb-node", "--enforce-owner-push", "true"]).enforce_owner_push
+        );
+    }
 }

crates/gitlawb-node/src/config.rs

@@ -30,6 +30,12 @@ use crate::state::AppState;
 
 /// Build an [`AppState`] over a real, migrated Postgres pool (from `#[sqlx::test]`).
 /// Runs the schema migrations first, because the per-test database starts empty.
+///
+/// **The config here is parsed from the process environment.** `Config` sources 47
+/// fields from `GITLAWB_*` variables, so a developer or CI environment that sets one
+/// silently changes what this state does. Never assert a behaviour that a config
+/// field controls on top of this state — use [`test_state_with`] and set the field,
+/// so the test states the configuration it is about instead of inheriting it.
 pub(crate) async fn test_state(pool: PgPool) -> AppState {
     let db = Arc::new(crate::db::Db::for_testing(pool.clone()));
     db.run_migrations()
@@ -38,6 +44,24 @@ pub(crate) async fn test_state(pool: PgPool) -> AppState {
     build_state(db, pool)
 }
 
+/// [`test_state`] with an explicit config override, so a test that depends on a
+/// setting pins it rather than inheriting whatever the environment supplies.
+///
+/// The shipped default is proved separately and env-independently by
+/// `config::tests::enforce_owner_push_is_declared_true_independent_of_the_environment`,
+/// which reads the declaration off the parser. Splitting the two keeps a parser
+/// question and an authorization question from sharing one failure mode.
+pub(crate) async fn test_state_with(
+    pool: PgPool,
+    configure: impl FnOnce(&mut crate::config::Config),
+) -> AppState {
+    let mut state = test_state(pool).await;
+    let mut cfg = (*state.config).clone();
+    configure(&mut cfg);
+    state.config = Arc::new(cfg);
+    state
+}
+
 /// DB-free [`AppState`] for middleware/auth tests that return before any query.
 /// The pool is lazy and never connects — do NOT use for tests that hit the DB.
 // Harness API consumed by the plan-002/003 middleware and no-auth-rejection tests.
@@ -2302,6 +2326,71 @@ mod tests {
         );
     }
 
+    /// The handler wiring: with the gate on, a signed non-owner push is refused.
+    ///
+    /// The gate's own unit tests pass `enforce` as a literal and never exercise the
+    /// handler, so this is what proves the flag is actually consulted on the request
+    /// path. It sets the field EXPLICITLY rather than leaning on the default: on a
+    /// host exporting `GITLAWB_ENFORCE_OWNER_PUSH=false` — which is exactly what the
+    /// rolling-upgrade guidance tells operators to set — an ambient config would
+    /// build a disabled state, let the push through to git, and fail this test for a
+    /// reason that has nothing to do with the code under test.
+    ///
+    /// The shipped default is proved env-independently in `config::tests`, off the
+    /// parser declaration. Authorization behaviour and parser defaults are separate
+    /// questions and get separate tests.
+    ///
+    /// 403 rather than 401 is the discriminator: the request carries a real RFC 9421
+    /// signature and passes `require_signature`, so it is authenticated and refused on
+    /// authorization — which is the whole distinction the change rests on.
+    #[sqlx::test]
+    async fn enforced_owner_push_refuses_a_signed_non_owner(pool: PgPool) {
+        use gitlawb_core::http_sig::sign_request;
+        use gitlawb_core::identity::Keypair;
+
+        let owner = Keypair::generate();
+        let stranger = Keypair::generate();
+        let owner_did = owner.did().to_string();
+        let short = owner_did.split(':').next_back().unwrap().to_string();
+
+        let state = test_state_with(pool, |cfg| cfg.enforce_owner_push = true).await;
+        state
+            .db
+            .create_repo(&seed_repo(&owner_did, "defrepo"))
+            .await
+            .expect("seed repo");
+
+        let router = Router::new()
+            .route(
+                "/{owner}/{repo}/git-receive-pack",
+                axum::routing::post(crate::api::repos::git_receive_pack),
+            )
+            .layer(axum::middleware::from_fn(crate::auth::require_signature))
+            .with_state(state);
+
+        let path = format!("/{short}/defrepo.git/git-receive-pack");
+        let body = b"0000".to_vec();
+        let signed = sign_request(&stranger, "POST", &path, &body);
+        let req = Request::builder()
+            .method(Method::POST)
+            .uri(&path)
+            .header("content-type", "application/x-git-receive-pack-request")
+            .header("content-digest", signed.content_digest)
+            .header("signature-input", signed.signature_input)
+            .header("signature", signed.signature)
+            .body(Body::from(body))
+            .unwrap();
+
+        let resp = router.oneshot(req).await.unwrap();
+        assert_eq!(
+            resp.status(),
+            StatusCode::FORBIDDEN,
+            "with the gate enabled, a signed push from a non-owner must be refused: \
+             a did:key is self-certifying, so authentication alone is not \
+             authorization"
+        );
+    }
+
     /// A1 Phase-2 contract: the `git-upload-pack` POST (the actual fetch, after
     /// the advertisement) is itself read-visibility gated. An ANONYMOUS upload-pack
     /// POST against a private repo is denied (404), so signing only the Phase-1

crates/gitlawb-node/src/test_support.rs

@@ -44,6 +44,10 @@ services:
       GITLAWB_BOOTSTRAP_PEERS: ${GITLAWB_BOOTSTRAP_PEERS:-}
       GITLAWB_BOOTSTRAP_DISABLE_SEEDS: ${GITLAWB_BOOTSTRAP_DISABLE_SEEDS:-false}
       GITLAWB_REQUIRE_SIGNED_PEER_WRITES: ${GITLAWB_REQUIRE_SIGNED_PEER_WRITES:-false}
+      # Owner-only push. The default matches the node's own, so the container path
+      # behaves like a source build; forwarding it is what makes the documented
+      # rolling-upgrade opt-out reachable for an operator running compose.
+      GITLAWB_ENFORCE_OWNER_PUSH: ${GITLAWB_ENFORCE_OWNER_PUSH:-true}
       GITLAWB_P2P_BOOTSTRAP: ${GITLAWB_P2P_BOOTSTRAP:-}
       GITLAWB_PUBLIC_READ: ${GITLAWB_PUBLIC_READ:-true}
       GITLAWB_MAX_PACK_BYTES: ${GITLAWB_MAX_PACK_BYTES:-2147483648}

docker-compose.yml

@@ -93,7 +93,7 @@ Fixed or staged in this pass:
 Live-network blockers to prioritize:
 
 - GraphQL POST is still open for compatibility; GraphQL mutations should get mutation-aware auth before it becomes a public write API surface.
-- Push authorization is still not capability-complete. A valid DID signature is authentication, not authorization; unprotected repo branches do not yet enforce owner/UCAN capability checks.
+- Push authorization is still not capability-complete. A valid DID signature is authentication, not authorization. Owner checks are now enforced on every branch, protected or not (`GITLAWB_ENFORCE_OWNER_PUSH`, on by default); what remains is that a UCAN `git/push` capability is not yet honored, so a delegated or CI key cannot push.
 - UCAN chain validation is incomplete and UCAN revocation/blocklisting is not implemented as an operator feature.
 - Private repository reads are not enforced. `is_public` and `GITLAWB_PUBLIC_READ` exist, but per-repository private-read behavior is not wired.
 - Peer URLs are self-asserted by DIDs. Signatures prove control of the DID key when present, not ownership/safety of the announced URL.

docs/OSS-READINESS-AUDIT.md

@@ -141,31 +141,43 @@ During the cooldown your node still earns rewards if it keeps heartbeating.
 
 ---
 
-## Hardening: owner-only push
+## Owner-only push
 
-By default the node authenticates every `git-receive-pack` push (a valid RFC 9421
-`did:key` signature) but does **not** check that the pusher owns the repo, except
-on branches that are explicitly protected. Because `did:key` is self-certifying,
-any party can generate a key, derive its DID, sign, and push to an unprotected
-branch — authentication is not authorization.
+The node requires the authenticated pusher to be the repo owner on **every**
+branch. A push whose authenticated DID is not the repo owner is rejected with
+HTTP 403 before any ref update is applied. The owner is matched in both the full
+`did:key:z6Mk…` form and its bare `z6Mk…` suffix.
 
-To require the authenticated pusher to be the repo owner on **every** branch, set:
+This is on by default, and the default is the point. The node authenticates every
+`git-receive-pack` push with a valid RFC 9421 `did:key` signature, but `did:key`
+is self-certifying: any party can generate a key, derive its DID and sign.
+Authentication is not authorization, so without this gate every signed caller can
+push to every repository — private ones included — on any branch that is not
+explicitly protected.
+
+### Turning it off
 
 ```bash
-GITLAWB_ENFORCE_OWNER_PUSH=true
+GITLAWB_ENFORCE_OWNER_PUSH=false
 ```
 
-- **Default `false`** — preserves current behavior so live nodes are unaffected by
-  an upgrade. Turn it on once you're ready for owner-only writes.
-- **When `true`** — a push whose authenticated DID is not the repo owner is
-  rejected (HTTP 403) before any ref update is applied. The owner is matched in
-  both the full `did:key:z6Mk…` form and its bare `z6Mk…` suffix.
-- **Caution: this blocks every non-owner pusher, including your own delegated and
-  CI agents.** Push authorization is owner-only today — a UCAN `git/push`
-  capability is verified but not yet honored for authorization, so delegated keys
-  cannot push while this is on. Don't enable it until every identity that pushes
-  to your repos is the owner, or you'll lock out your own automation. Scoped
-  collaborator / UCAN-delegated push rights are a planned follow-up.
+Both the environment variable and `--enforce-owner-push false` disable it; the
+bare `--enforce-owner-push` flag still means `true`.
+
+Do this only for a rolling upgrade whose pushers are not yet the repo owner, and
+treat it as temporary: while it is off, your node accepts a push to any repository
+from anyone who can generate a keypair.
+
+### Caution: delegated and CI keys are non-owners
+
+Push authorization is owner-only today. A UCAN `git/push` capability is verified
+but **not yet honored for authorization**, so a delegated key cannot push while
+this gate is on, even when it holds a valid capability for the repo.
+
+If your automation pushes under its own DID rather than the owner's, it will start
+getting 403s. Either have it push as the owner, or set
+`GITLAWB_ENFORCE_OWNER_PUSH=false` until scoped collaborator / UCAN-delegated push
+rights land — that work is what removes this trade-off.
 
 ---