test(node): real-node deny harness for trust-boundary regressions (INV-1/2/8)#194
test(node): real-node deny harness for trust-boundary regressions (INV-1/2/8)#194beardthelion wants to merge 11 commits into
Conversation
…can spawn a real node Move the module tree and boot logic from main.rs into a new lib.rs crate root exposing the boot surface (build_router, AppState, Config, migrations) as pub; main.rs becomes a thin #[tokio::main] shim over run(). No behavior change: both targets build and the full node suite (488 tests) stays green. Prerequisite for the real-node deny harness (U1).
…-U4, U5a) Add a feature-gated (test-harness) spawn surface (src/test_harness.rs) that boots a real node on 127.0.0.1:0 over an ephemeral #[sqlx::test] pool through the production axum::serve stack with connect-info, and an integration crate (tests/deny_harness.rs) that drives deny paths with a real reqwest client: - U2 signing client: wraps gitlawb_core::http_sig::sign_request for reqwest; self-checks that a valid signature clears require_signature and a tampered body is rejected (400 content_digest_mismatch). - U3 spawn_node: real socket, p2p disabled, per-test DB, shutdown-on-drop. - U4 assert_denied: 4xx AND body-no-leak AND not-empty-200 (INV-8); pure core unit-tested for clean-403 / empty-200 / leaking-403 / wrong-status. - U5a INV-8: unsigned git-receive-pack is denied 401 with no leak. Widens the three cfg(test) test builders (Db/RepoStore::for_testing, run_migrations) to also compile under the feature. No production behavior change: prod build (no feature) excludes test_harness; node suite stays green (488) and the 7 integration tests pass.
A validly signed non-owner PUT /visibility is rejected 403 by require_owner (no x-ucan, so require_ucan_chain passes through to the gate); the owner's signed PUT reaches the handler (reachability proof, guards against a 404/415 masquerading as a pass). Adds seed_repo/withhold_path seeding helpers to the test harness. Mutation-verified load-bearing: with require_owner forced Ok the non-owner PUT returns 201 and the INV-8 assertion flips the test RED.
Adds seed_bare_repo (shells git to build a real bare repo at the served path,
sha1 or sha256 object format) and two INV-2 deny cases over the real stack:
- U7: a public repo with a /secret/** withhold rule denies an anonymous blob
read of the withheld path (404) with no content/OID leak, while the sibling
public path is served (path-scoped, not blanket).
- U5b: the same withhold denies an anonymous /ipfs/{cid} read of the withheld
blob's content-addressed id (404, no leak), while the public blob's CID is
served. Completes U5 (INV-8) alongside U5a.
Both mutation-verified load-bearing: forcing visibility_check to allow leaks
the secret at 200 and the INV-8 assertion flips each test RED.
Drives the git-upload-pack POST directly (v0 stateless-RPC: want HEAD, flush, done) via a bounded reqwest client rather than a vanilla `git clone` (which negotiates protocol v2 and deadlocks against the node's v0 server, and would otherwise wedge the suite). The served pack is indexed with git index-pack and its objects listed with verify-pack -v: a packfile-aware assertion, since a raw byte scan cannot see an OID inside the zlib-compressed stream. A public repo with a /secret/** withhold rule must serve a pack that omits the withheld blob's object while keeping the sibling public blob. Mutation-verified load-bearing: forcing visibility_check to allow puts the withheld blob back in the pack and flips the test RED. Completes the harness (8 units, 11 integration tests). Prod build (no feature) and the 488-test node suite stay green.
cargo test --workspace skips the harness because it lives behind the test-harness feature (kept off the production binary). Add an explicit step that runs it with the feature and the same Postgres service, so the INV-1/ INV-2/INV-8 trust-boundary regression cases execute on every PR instead of only when run by hand.
Add unit tests for the two remaining check_denied branches: a non-4xx expected status is rejected as a test bug, and an empty withheld token is skipped rather than matching every body. Closes the last unexecuted branches in the deny assertion.
Fan-out of U6 to the security-sensitive owner-gated mutations that had only the source-level authz-table guard and no runtime deny test: protect_branch, unprotect_branch, create_webhook, delete_webhook, remove_visibility. Each rejects a validly-signed non-owner with 403 and lets the owner reach the handler (not 403). Mutation-verified load-bearing on their shared root gate: did_matches forced true opens all five (non-owner protect_branch returns 201) and the test flips RED. Replica register/unregister were intentionally excluded: they are signer-self (you register your own node), not owner-gated, so there is no owner-deny to assert.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe node startup code moves into a reusable library, while a feature-gated ChangesReusable node library and boot lifecycle
Feature-gated harness runtime and CI wiring
Signed requests and denial-path integration tests
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/gitlawb-node/tests/deny_harness.rs (1)
36-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent request timeouts across the suite.
Only the clone test (line 344-347) builds its
reqwest::Clientwith an explicit timeout, with a comment explaining why (avoiding a wedged suite). Every other test here (e.g. this one, and lines 67, 98, 123, 192, 249, 420) usesreqwest::Client::new()with no timeout. If the real node under test ever hangs on any of these paths, the test blocks until the 45-minute CI job timeout instead of failing fast with a clear cause.♻️ Suggested fix: a shared bounded client helper
fn bounded_client() -> reqwest::Client { reqwest::Client::builder() .timeout(std::time::Duration::from_secs(30)) .build() .expect("client builds") }Then swap each
reqwest::Client::new()in this file forbounded_client().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/tests/deny_harness.rs` around lines 36 - 37, Introduce a shared bounded client helper in the deny harness, such as bounded_client, that builds reqwest::Client with a 30-second timeout. Replace every reqwest::Client::new() usage in this file, including the clone test, with the helper while preserving the existing request behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/lib.rs`:
- Around line 539-572: Update the HTTP shutdown flow around axum::serve and the
with_graceful_shutdown future to enforce the configured grace duration, aborting
the server drain when it expires instead of waiting indefinitely for long-lived
requests. Use the existing grace value derived from config.shutdown_grace_secs
and remove the unused grace discard while preserving normal shutdown signaling
and serve_result handling.
- Around line 1007-1027: Update the identity-key creation and loading flow
around Keypair generation and key_path.exists() to eliminate the TOCTOU race and
disclosure window: create the file with OpenOptions::create_new(true) and Unix
mode 0o600, write the PEM through that handle, and handle AlreadyExists by
retrying the existing-key load path. When loading an existing key, validate or
tighten its permissions to 0600 before reading it, while preserving the existing
PEM parsing and error behavior.
---
Nitpick comments:
In `@crates/gitlawb-node/tests/deny_harness.rs`:
- Around line 36-37: Introduce a shared bounded client helper in the deny
harness, such as bounded_client, that builds reqwest::Client with a 30-second
timeout. Replace every reqwest::Client::new() usage in this file, including the
clone test, with the helper while preserving the existing request behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da2a7166-0514-4eaf-9449-ef5be4e258e0
📒 Files selected for processing (11)
.github/workflows/pr-checks.ymlcrates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/lib.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/test_harness.rscrates/gitlawb-node/tests/deny_harness.rscrates/gitlawb-node/tests/support/assert.rscrates/gitlawb-node/tests/support/mod.rscrates/gitlawb-node/tests/support/signing.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Create identity keys atomically with owner-only permissions
crates/gitlawb-node/src/lib.rs:1007
The new library retains the existingexists()→fs::write()flow. On Unix,fs::writecreates the PEM using umask-derived permissions and only then changes it to0600; a local process can read the node private key in that window. The separate existence check also lets concurrent node starts overwrite each other's generated identity. Create the file withcreate_newand mode0600, then handleAlreadyExistsby loading the winning key. -
[P2] Enforce the configured HTTP shutdown grace period
crates/gitlawb-node/src/lib.rs:539
with_graceful_shutdownbegins draining when the signal fires but has no deadline; the computedgraceis explicitly discarded at line 571. A long-lived request can therefore prevent termination until the orchestrator hard-kills the process, defeatingGITLAWB_SHUTDOWN_GRACE_SECSand risking interrupted cleanup. Bound the drain with that duration and force completion once it expires.
- Create the node identity key atomically with create_new + mode 0600, closing the umask-derived 0644 disclosure window and the exists()->write overwrite race; on AlreadyExists load the winner's key (bounded retry so a loser can't read a half-written PEM) and tighten looser perms on load. - Enforce the configured shutdown grace: bound the axum drain by grace measured from the signal (extracted as drive_serve_with_grace), abandoning in-flight requests once it expires instead of waiting indefinitely. Removes the discarded grace value. - Route deny-harness reqwest clients through a shared bounded_client (30s timeout) so a wedged node path fails fast instead of hanging to the CI limit. Tests: 0600-on-create, load-tighten, concurrent-start convergence (create race), and grace-race abandon / normal-drain / signal-gated-clock. All RED-then-GREEN by execution.
- Create the node identity key atomically with create_new + mode 0600, closing the umask-derived 0644 disclosure window and the exists()->write overwrite race; on AlreadyExists load the winner's key (bounded retry so a loser can't read a half-written PEM) and tighten looser perms on load. - Enforce the configured shutdown grace: bound the axum drain by grace measured from the signal (extracted as drive_serve_with_grace), abandoning in-flight requests once it expires instead of waiting indefinitely. Removes the discarded grace value. - Route the deny-harness reqwest clients through a shared bounded_client (30s timeout) so a wedged node path fails fast instead of hanging to the CI limit. Tests: 0600-on-create, load-tighten, concurrent-start convergence (create race), and grace-race abandon / normal-drain / signal-gated-clock. All RED-then-GREEN by execution.
|
Addressed the review feedback in 30672cf. P1 (identity key). Created atomically with P2 (shutdown grace). The drain is now bounded by the configured grace, measured from the signal rather than server start (extracted as Unit tests cover 0600-on-create, tighten-on-load, 8-thread concurrent-start convergence, and the grace abandon / normal-drain / signal-gated paths. Also took CodeRabbit's nitpick: the deny-harness clients now go through a shared 30s |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/lib.rs`:
- Around line 1120-1125: Update the key-writing logic in create_new for both
Unix and non-Unix branches so any write_all failure removes the partially
written file at key_path before returning the error. Preserve the existing
contextual error and successful write behavior, and ensure cleanup is attempted
consistently in both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 620c5ae9-78f1-476e-badb-3e054d1f583a
📒 Files selected for processing (2)
crates/gitlawb-node/src/lib.rscrates/gitlawb-node/tests/deny_harness.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/tests/deny_harness.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Remove a failed first-write identity file
crates/gitlawb-node/src/lib.rs:1123
The newcreate_new(true)path fixes the original permission window, but if the PEM write itself fails after the file has been created, the just-created key path is left behind as an empty or partial PEM. Every later start then takes thekey_path.exists()branch, retries parsing that same bad file inload_racing, and exits withinvalid PEM keyinstead of generating a fresh identity. A transient ENOSPC/EIO/quota failure during first boot can therefore permanently wedge the node until an operator manually deletes the file. Please remove the newly-created file onwrite_allfailure in both the Unix and non-Unix branches before returning the error. -
[P2] Do not ignore failed key permission tightening
crates/gitlawb-node/src/lib.rs:1059
The load path now advertises that loose existing identity keys are tightened to0600, but theset_permissionsresult is discarded. If the file is readable but chmod fails, for example on a read-only mount or an ownership/ACL mismatch, the node still reads and uses a world/group-readable private key while logging a normal "loaded existing identity" path. That leaves the exact key exposure this follow-up is trying to close. Please surface the chmod failure or otherwise verify the final mode before continuing with the key.
…r tightening (#194) F1 (P1): create_new(true) closed the permission window, but a write_all failure after the file was created left an empty/partial PEM behind. Every later start then took the key_path.exists() branch, re-parsed that corrupt file in load_racing, and exited 'invalid PEM key' instead of regenerating — a transient ENOSPC/EIO on first boot permanently wedged the node. Extract write_key_or_cleanup, which removes the just-created file on write failure, and wire it into both the unix and non-unix create branches. F2 (P2): the load path tightened a loose existing key to 0600 but discarded the set_permissions result. A chmod that failed (read-only mount, ownership/ACL mismatch) left a world/group-readable private key in use while logging a normal 'loaded existing identity'. Surface the tighten failure (propagate it) and add ensure_key_mode_0600, which fails closed if the key is not 0600 after the attempt. RED->GREEN: failed_write_removes_the_partial_key_file (a failed write leaves no file; RED without the remove_file). loose_key_mode_is_rejected_not_used (a 0644 key is rejected; RED without the mode check). Existing created_key_is_mode_0600, existing_key_is_loaded_and_tightened, and concurrent_starts_converge_on_one_identity stay green. Full node lib+bin suite 497 passed, fmt + clippy clean.
|
Both addressed on Remove a failed first-write (F1). Extracted Do not ignore a failed permission tighten (F2). The load path now propagates the One behavior note on F2: a loose key that genuinely cannot be tightened is now rejected rather than used, which narrows the original "never reject a loose key" leniency — but only in the exposed-and-unfixable case, which is the exposure this follow-up closes. The real ENOSPC/chmod-fail I/O triggers are not driven end to end (no portable fault injection), but the error handling is proven at the helper level and the wiring is a one-line pass-through of the write/chmod result into it. RED->GREEN for each; no other production behavior changes. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Do not fail concurrent startup after an arbitrary 100 ms key-write window
crates/gitlawb-node/src/lib.rs:1123
create_newexposes the final key path before the winner has completedwrite_all, and every other process that sees that inode gives up after 50 2-ms retries. On a slow or temporarily stalled filesystem, a winner can legitimately take longer than that interval, so all losing node starts returninvalid PEM keyeven though the winning write later succeeds. This reintroduces an availability failure for the concurrent-start case the new code is intended to make safe. Keep retrying until a meaningful startup deadline, or publish a fully written temporary key atomically so readers never observe a partial final file.
2 participants
What
A real-socket, end-to-end security regression harness for gitlawb-node. It boots a real node on
127.0.0.1:0, drives trust-boundary DENY paths through a real reqwest client with real RFC-9421 signing, and asserts both the refusal status and that no withheld data leaks. It turns the per-PR real-node-verify step into executed tests, and covers groundtower::oneshotcan't: the full production middleware stack over an actual socket, plus a systematic no-empty-200 (denial-as-success) assertion.Why the crate split
gitlawb-node was binary-only, so an out-of-crate integration test could not reach
build_router/AppState/Config. The first commit splits it into lib+bin: the module tree and boot logic move tosrc/lib.rs(exposing a minimalrun()plus the boot surface), andsrc/main.rsbecomes a thin#[tokio::main]shim. No behavior change, the 488-test node suite stays green and the production binary is unaffected. The harness itself lives behind atest-harnessfeature so its spawn surface never compiles into the release binary.Coverage
Fourteen executed cases, one strong case per invariant plus a high-value owner-gate fan-out:
/ipfs/{cid}read of a withheld blob denied 404 with no leak./ipfssurface, and the git-upload-pack replication path where the served pack must omit the withheld blob while keeping the sibling public one.Every case is mutation-verified load-bearing: the specific gate was broken, the test observed to go red (the secret leaking, or the withheld object appearing in the pack), then reverted.
Notes for review
git-upload-packPOST directly (v0 stateless-RPC) instead ofgit clone, because a defaultgit clonenegotiates protocol v2 and hangs against the node's v0 server. That hang (rather than a clean error) may be worth a separate look if standard-client interop matters. The assertion is packfile-aware (git index-pack+verify-pack), not a raw byte scan, since a leaked OID would otherwise hide inside the zlib stream.--features test-harness, sincecargo test --workspaceskips it by design.Summary by CodeRabbit