fix(relay): enforce real signing key on inbound tickets (C-001 critical) - #22
Open
David Mireles (louzt) wants to merge 2 commits into
Open
fix(relay): enforce real signing key on inbound tickets (C-001 critical)#22David Mireles (louzt) wants to merge 2 commits into
David Mireles (louzt) wants to merge 2 commits into
Conversation
Adds §Layer 0: SnapPipe-gated QUIC to OPERATIONAL-DEPLOYMENT.md. Cross-links to the 5-tier gist case study and to src/relay/listener.rs. Documents the ordered operations: 5-tier fallback → SnapPipe relay accept loop → server_handshake on stream 0 → streams 1..N forwarded. No code changes.
- src/relay/listener.rs:122 server now calls conn.accept_bi() on the
client's bidi stream. Previously the listener called conn.open_bi()
which opened a separate stream with no peer; recv.read_exact() then
blocked forever on the handshake, silently dropping every connection.
This was a pre-existing production bug unmasked by the new
c001_subject_validation integration test below.
- src/relay/mod.rs Relay::signing_key() exposes the relay's identity
key, and RelayConfig.signing_key: Arc<SigningKey> is now a required
field. src/relay/listener.rs:138 passes relay.signing_key().verifying_key()
as server_handshake's expected_subject — a zeroed
ed25519_dalek::SigningKey::from_bytes(&[0; 32]) had previously been
used there, which made the subject-binding check (which compares
claims.subject to expected_subject's NodeId-derived id) pass against
any subject, bypassing the entire signed-ticket subject guarantee.
The issuer signature was the only thing actually enforced.
- src/session.rs::server_handshake: the four sites that previously did
`let _ = futures_block_on_send(...)` now do
`.inspect_err(|e| eprintln!("...")).ok()` so failed handshakes log
via stderr instead of vanishing. Closes B-002.
- src/relay/listener.rs handle_connection now accepts the shared
RelayStats and feeds handle_connection's returned ConnectionLog into
guard.record(&log). Without this the stats snapshot was always 0.
Closes B-003.
- src/rate_limit.rs doc comment with f64 floating-point caveats
(multiply-then-divide, subnormal edge cases). Closes B-006.
- tests/c001_subject_validation.rs NEW (218 lines, integration). Two
tests against a real quinn::Endpoint on loopback:
* ticket_with_unrelated_subject_is_rejected_with_subject_mismatch —
expects HandshakeResponse::Err(SubjectMismatch) OR a connection
close with reason `b"handshake failed"` (the production listener
closes the conn after handshake error; the close can race ahead
of the response body).
* ticket_with_matching_subject_is_accepted — positive control, the
canonical single-issuer happy path.
Both register relay_key with TrustStore::add() so the handshake
reaches the subject check rather than failing on IssuerNotTrusted.
- tests/relay_listener_e2e.rs uses RelayConfig::with_generated_key() in
place of ::new(...) (signature change propagation).
- 68 tests pass, 0 failures, 0 warnings. Carried-over gaps documented
in /home/lou/.claude/projects/-home-lou/memory/snappipe-c001-critical-bypass-2026-07-10.md:
* server_handshake issuer_verifying_key is hardcoded to relay.vk
(single-issuer model works, multi-issuer needs a resolver fn).
* listener conn.close() passes error_code 0 while reason says
`b"handshake failed"` — Quinn treats 0 as graceful, codifying
a non-zero code is a separate hardening item.
Refs: PR slicing batch for SnapPipe, ultracode 2026-07-10.
📝 WalkthroughWalkthroughThe relay gains signing-key identity, client-accepted Stream 0 handshakes, subject validation, connection statistics recording, improved handshake error logging, integration coverage, and deployment documentation. ChangesRelay handshake validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QUICClient
participant run_listener
participant server_handshake
participant RelayStats
QUICClient->>run_listener: Accept Stream 0
run_listener->>server_handshake: Validate ticket subject
server_handshake-->>QUICClient: Return handshake response
QUICClient->>run_listener: Open application streams
run_listener->>RelayStats: Record ConnectionLog
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/relay/listener.rs (1)
172-178: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBreak out of the accept loop on
ConnectionError— once the peer disconnects,conn.accept_bi()stops yielding new streams, socontinuejust spins this spawned task in a tight log/retry loop until cancellation.breakhere lets the task exit cleanly.🤖 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 `@src/relay/listener.rs` around lines 172 - 178, The accept loop in the listener’s bidirectional stream handling spins after the peer disconnects because all accept_bi() errors use continue. Update the Err branch to break when the error is a ConnectionError, while preserving the existing logging and retry behavior for other errors.
🤖 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 `@docs/OPERATIONAL-DEPLOYMENT.md`:
- Around line 173-176: Update the `run_listener` description in the operational
deployment documentation to state that the relay accepts incoming QUIC
connections and then validates the SnapPipe ticket on Stream 0, removing the
claim that peers complete the handshake beforehand.
- Around line 197-203: Update the handshake description in the deployment guide
to explicitly include claims.subject validation against the relay signing key as
a required gate, and add subject mismatch to the listed handshake failure cases
alongside untrusted issuer, replay, and expired tickets.
In `@tests/c001_subject_validation.rs`:
- Around line 78-82: Fix the cargo fmt check failures in
tests/c001_subject_validation.rs by running cargo fmt --all; ensure the
quinn::Endpoint::server calls in the setup blocks, the timeout(...).await match
expression, and the wait_for_close_reason function signature use rustfmt’s
expected line wrapping and indentation.
---
Outside diff comments:
In `@src/relay/listener.rs`:
- Around line 172-178: The accept loop in the listener’s bidirectional stream
handling spins after the peer disconnects because all accept_bi() errors use
continue. Update the Err branch to break when the error is a ConnectionError,
while preserving the existing logging and retry behavior for other errors.
🪄 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 Plus
Run ID: c18949a8-ab98-47f7-9fdd-29b7108e0611
📒 Files selected for processing (7)
docs/OPERATIONAL-DEPLOYMENT.mdsrc/rate_limit.rssrc/relay/listener.rssrc/relay/mod.rssrc/session.rstests/c001_subject_validation.rstests/relay_listener_e2e.rs
Comment on lines
+173
to
+176
| The SnapPipe relay (`run_listener` in [`src/relay/listener.rs`](../src/relay/listener.rs)) | ||
| is the **Layer 0** in the 5-tier fallback stack: it runs on the VPS and | ||
| accepts incoming QUIC connections from peers that have already completed | ||
| a SnapPipe ticket handshake on stream 0. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the documented handshake order.
The relay accepts the QUIC connection first and then performs the Stream 0 ticket handshake; peers have not already completed it before run_listener accepts them. Please describe this as “accepts connections and validates the ticket on Stream 0” to avoid misleading deployment instructions.
🤖 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 `@docs/OPERATIONAL-DEPLOYMENT.md` around lines 173 - 176, Update the
`run_listener` description in the operational deployment documentation to state
that the relay accepts incoming QUIC connections and then validates the SnapPipe
ticket on Stream 0, removing the claim that peers complete the handshake
beforehand.
Comment on lines
+197
to
+203
| 1. `ssh-proxy` races Tier 1 (QUIC) → Tier 2 (Hysteria2) → Tier 3 (gost) → Tier 4 (tls-direct) → Tier 5 (direct-ssh). | ||
| 2. On the VPS side, `run_listener` accepts the QUIC connection. | ||
| 3. Stream 0 performs `server_handshake` (ticket validation + trust check). | ||
| 4. If the handshake succeeds, streams 1…N are forwarded to the application. | ||
| 5. If the handshake fails (untrusted issuer, replay, expired ticket), the | ||
| connection is closed — the 5-tier chain degrades gracefully without | ||
| leaking sessions. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document subject validation as a handshake gate.
The new contract also rejects tickets whose claims.subject does not match the relay signing key, but the documented handshake step only mentions ticket validation and trust checks, and the failure examples omit subject mismatch. Include this case explicitly so the deployment guide reflects the behavior covered by tests/c001_subject_validation.rs and implemented in src/session.rs.
🧰 Tools
🪛 LanguageTool
[grammar] ~197-~197: Ensure spelling is correct
Context: ...1 (QUIC) → Tier 2 (Hysteria2) → Tier 3 (gost) → Tier 4 (tls-direct) → Tier 5 (direct...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/OPERATIONAL-DEPLOYMENT.md` around lines 197 - 203, Update the handshake
description in the deployment guide to explicitly include claims.subject
validation against the relay signing key as a required gate, and add subject
mismatch to the listed handshake failure cases alongside untrusted issuer,
replay, and expired tickets.
Comment on lines
+78
to
+82
| ); | ||
| let mut server_cfg = server_cfg; | ||
| server_cfg.transport_config(transport); | ||
| let server_ep = | ||
| quinn::Endpoint::server(server_cfg, dev_bind()).expect("server endpoint"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix cargo fmt failures blocking CI.
The pipeline reports cargo fmt --all -- --check failures at multiple locations in this file. Run cargo fmt --all to auto-fix all of them:
- Lines 81-82:
quinn::Endpoint::server(...)call should be collapsed to a single line (fits within line limit). - Lines 150-155:
timeout(...).awaitmatch expression needs reflowed indentation/brace layout. - Lines 231-233:
wait_for_close_reasonfunction signature should be condensed onto one line. - Lines 260-261: Same
quinn::Endpoint::server(...)wrapping issue as lines 81-82.
Also applies to: 147-155, 228-233, 257-261
🧰 Tools
🪛 GitHub Actions: CI / 0_test.txt
[error] 78-78: cargo fmt --all -- --check failed (diff indicates formatting changes needed for match/timeout block formatting and indentation).
🪛 GitHub Actions: CI / test
[error] 78-78: cargo fmt --check failed due to formatting of quinn::Endpoint::server(...) call (collapsed to single line).
🤖 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 `@tests/c001_subject_validation.rs` around lines 78 - 82, Fix the cargo fmt
check failures in tests/c001_subject_validation.rs by running cargo fmt --all;
ensure the quinn::Endpoint::server calls in the setup blocks, the
timeout(...).await match expression, and the wait_for_close_reason function
signature use rustfmt’s expected line wrapping and indentation.
Source: Pipeline failures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(relay): enforce real signing key on inbound tickets (C-001 critical)
Summary
The QUIC relay listener was passing a zeroed
ed25519_dalek::SigningKey::from_bytes(&[0u8; 32])as theexpected_subjecttoserver_handshake. Because the dummy key's verifying key is all zeros, anyclaims.subjectvalue would pass the subject-binding check — bypassing the entire subject-binding guarantee of the signed-ticket format. Only the issuer signature was being enforced.This change also fixes a pre-existing production bug exposed by the new integration test: the listener was calling
conn.open_bi()to accept the handshake stream instead ofconn.accept_bi(), causing the server handshake to hang forever onrecv.read_exact(Quinn bi-streams are paired but stream IDs are independently allocated per peer).API change
RelayConfig::new(...)now takes a 5th argumentsigning_key: Arc<ed25519_dalek::SigningKey>andRelay::signing_key() -> &SigningKeyis the new accessor used as the canonicalexpected_subject.RelayConfig::with_generated_key(...)is a builder for tests/dev loops that mints a fresh key.Migration:
Changes
src/relay/mod.rsRelayConfig.signing_key+ new accessor +with_generated_keybuildersrc/relay/listener.rsopen_bi()→accept_bi()(pre-existing production bug); useRelay::signing_key()asexpected_subject(C-001 fix); feedConnectionLogintoRelayStats(B-003)src/session.rslet _ = ...silent send-fail drops with.inspect_err(...).ok()(B-002)src/rate_limit.rstests/c001_subject_validation.rstests/relay_listener_e2e.rswith_generated_key()after signature changeVerification
cargo test --tests→ 68 passed, 0 failed, 0 warnings:relay::tests::relay_exposes_signing_keytests/c001_subject_validation.rs:ticket_with_unrelated_subject_is_rejected_with_subject_mismatchticket_with_matching_subject_is_acceptedtests/integration_trust_sync.rstests/quic_e2e.rs(includingtrusted_issuer_is_acceptedfor the multi-issuer model)tests/relay_listener_e2e.rs(signature change propagation)The negative test asserts EITHER a
HandshakeResponse::Err(SubjectMismatch)arrives on the wire OR the connection is closed with reasonb"handshake failed"— both are valid rejections of the C-001 attack.Sanitization checklist
open_bihang) fixed in the same commitfix(relay))lzt-branch-claimCarried-over gaps (NOT in this PR)
These surfaced during the audit but are out of scope for C-001:
src/relay/listener.rs:142-148hard-codes&relay_verifying_keyasissuer_verifying_key. The single-issuer model works; a multi-issuer trust-store requires extendingserver_handshaketo accept a resolver fn (|NodeId| -> Option<VerifyingKey>).b"handshake failed":src/relay/listener.rs:155-157passes0u8as close code, which Quinn interprets as graceful. Clients can't distinguish clean disconnect from handshake rejection by code alone. Reserve a non-zero code (e.g.0x4747).Tracked in memory
snappipe-c001-critical-bypass-2026-07-10.Related
~/.claude/projects/-home-lou/memory/snappipe-c001-critical-bypass-2026-07-10.mdSummary by CodeRabbit
Security
Monitoring
Documentation
Testing