feat(transport): TCP-over-QUIC tunnel (tunnel serve / tunnel connect) - #24
Conversation
Adds a transparent TCP-over-QUIC tunnel layer that ships any TCP
protocol through the existing identity-gated SnapPipe relay.
Reuses SignedTicket + TrustStore + RateLimiter (no second auth
surface), and runs on a dedicated ALPN (/snappipe/tunnel/0) so
tunnel traffic stays on its own wire.
Surface
- src/transport/tunnel.rs: serve + connect_with + connect,
per-stream TCP↔QUIC bridge with bidirectional copy via
tokio::join!. ALPN constant TUNNEL_ALPN distinct from
DEFAULT_ALPN.
- src/main.rs: new 'tunnel {serve,connect}' subcommand.
- examples/relay.sample.toml: quic_bind 0.0.0.0:4443
(defensible default — outside the typical ISP drop-list and not
captured by HTTP/3 SO_REUSEPORT on UDP/443).
- examples/snappipe-tunnel.service (relay host skeleton) +
snappipe-tunnel-client.service (trusted-peer skeleton) +
snappipe-tunnel-client.ps1 (Windows scheduled-task installer).
- tests/tunnel_e2e.rs: in-process TCP echo + tunnel round-trip,
plus ALPN-distinct and config-validation unit tests.
- README.md: 'TCP-over-QUIC tunnel (v0.3.0)' section with
topology diagram, CLI examples, production caveats. Examples
use generic placeholders (<operator-keys>, <relay-public-host>)
so the README stays useful to fork operators without leaking
per-deploy IPs or paths.
- docs/OPERATIONAL-DEPLOYMENT.md: 'TCP-over-QUIC tunnel
deployment' section with UDP/4443 rationale and firewall
expectations, again using placeholders for operator-specific
values.
- Bump version 0.2.1 -> 0.3.0 (new public CLI surface).
Tested
- 70 tests pass (61 unit + 1 integration_trust_sync + 4 quic_e2e
+ 1 relay_listener_e2e + 3 tunnel_e2e). No regressions.
- clippy -D warnings clean on tunnel.rs + main.rs.
Scope-boundary note
This PR ships the GENERIC transport feature. Operator-specific
deployment details (public IPs, internal paths, namespace names,
DNS, ticket TTLs) deliberately stay outside the public repository
in a private deployment manifest.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds TCP-over-QUIC tunneling with authenticated QUIC handshakes, TCP stream bridging, ChangesTCP-over-QUIC tunnel
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TCPClient
participant TunnelClient
participant TunnelServer
participant TCPBackend
TCPClient->>TunnelClient: Connect to local TCP listener
TunnelClient->>TunnelServer: Authenticate over QUIC stream 0
TunnelClient->>TunnelServer: Open one QUIC stream per TCP connection
TunnelServer->>TCPBackend: Dial configured backend
TunnelClient->>TunnelServer: Proxy bidirectional bytes
TCPBackend-->>TCPClient: Return backend response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
README.md-212-212 (1)
212-212: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced diagram block.
Use a tag such as
textornoneso Markdown tooling can process the block consistently.🤖 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 `@README.md` at line 212, Add a language identifier such as text or none to the fenced diagram block in README.md, preserving its existing diagram content.Source: Linters/SAST tools
README.md-88-89 (1)
88-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale crate-release statement.
This table now marks v0.3.0 as current, but the later README text still says the published metadata mirrors the v0.2.1 tag. Update that statement to match the v0.3.0 release.
🤖 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 `@README.md` around lines 88 - 89, Update the later README statement describing which release tag the published metadata mirrors, changing the stale v0.2.1 reference to v0.3.0 while preserving the surrounding wording and links.docs/OPERATIONAL-DEPLOYMENT.md-23-23 (1)
23-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the transport diagram to v0.3.0.
Cargo.tomland the surrounding documentation identify v0.3.0 as current, but this diagram still labels the transport v0.2.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` at line 23, Update the transport diagram entry in OPERATIONAL-DEPLOYMENT so its SnapPipe version label uses v0.3.0 instead of v0.2.1, while preserving the existing transport feature descriptions.docs/OPERATIONAL-DEPLOYMENT.md-332-341 (1)
332-341: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicated deployment-notes section.
The “Why these deploy notes live outside the repo” section already appears earlier in this document. Merge the additional “probe fixtures” detail into the first section instead of repeating the heading and paragraph.
🤖 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 332 - 341, Remove the later “Why these deploy notes live outside the repo” heading and paragraph from the deployment documentation, then add “probe fixtures” to the corresponding list in the first occurrence. Preserve the remaining deployment guidance without duplicating the section.README.md-300-306 (1)
300-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh the documented test count.
This adds the tunnel end-to-end test, while the existing README test summary still reports 63 tests. Update the count and test-scope description to match the reported 70 passing tests.
🤖 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 `@README.md` around lines 300 - 306, Update the README test summary near the tunnel_e2e documentation to report 70 passing tests instead of 63, and revise the test-scope description so it accurately reflects the expanded suite including the tunnel end-to-end test.examples/relay.sample.toml-8-10 (1)
8-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the contradictory UDP allowlist rationale.
If UDP/4443 is outside a carrier’s allowlist, it is not “unfettered” on that network. This guidance can cause failed deployments in the exact scenario it describes.
🤖 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 `@examples/relay.sample.toml` around lines 8 - 10, Correct the UDP/4443 rationale in the comments near the relay configuration: remove the contradictory claim that port 4443 is unfettered when it is outside the carrier allowlist, and describe its availability only for freshly provisioned hosts or networks where it is actually permitted.examples/snappipe-tunnel-client.ps1-12-14 (1)
12-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the credential path the script actually uses.
The script checks
$env:USERPROFILE\snappipe\friend.ticket.jsonandfriend.secret, not files beside the script. A user following these instructions will immediately get a “file missing” error.Proposed documentation fix
- operator. Drop it next to this script as `friend.ticket.json` and - `friend.secret` (Ed25519, base64url, single-line) before running. + operator. Save it as + `$env:USERPROFILE\snappipe\friend.ticket.json` and + `$env:USERPROFILE\snappipe\friend.secret` before running.🤖 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 `@examples/snappipe-tunnel-client.ps1` around lines 12 - 14, Update the prerequisite documentation to instruct users to place friend.ticket.json and friend.secret under $env:USERPROFILE\snappipe, matching the credential paths used by the script; do not describe them as files beside the script.src/transport/tunnel.rs-59-70 (1)
59-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI
cargo fmt --all -- --checkis failing across both new files. The shared root cause is thatcargo fmtwas not run on the new tunnel code; rustfmt reports import ordering, multiline boolean reflow, and wrapped-call diffs.
src/transport/tunnel.rs#L59-L70: runcargo fmt --allto fix the reported diffs at lines 63, 95, 142, 159, 227, 331, and 448.tests/tunnel_e2e.rs#L52-L71: runcargo fmt --allto fix the reported diffs at lines 55, 71, and 118.🤖 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/transport/tunnel.rs` around lines 59 - 70, Run cargo fmt --all and commit the resulting rustfmt changes in src/transport/tunnel.rs (including the import ordering, boolean formatting, and wrapped calls) and tests/tunnel_e2e.rs; no behavioral changes are needed.Source: Pipeline failures
src/main.rs-158-160 (1)
158-160: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReplace the concrete operator IP in the help text with a placeholder.
167.88.38.25:4443is a real routable address baked into user-visible CLI help, which contradicts the PR's stated intent to keep operator-specific values out of the public repo.✏️ Proposed fix
- /// Remote relay host:port (e.g. `167.88.38.25:4443`). + /// Remote relay host:port (e.g. `relay.example.com:4443`).🤖 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/main.rs` around lines 158 - 160, Update the documentation comment for the relay argument to replace the concrete routable address with a generic host-and-port placeholder, while preserving the existing description and CLI behavior of the relay field.tests/tunnel_e2e.rs-156-172 (1)
156-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
read_exactand don't swallow the task join results.A single
readcan return a short payload;read_exactmakes the assertion deterministic. More importantly, the threelet _ = timeout(...)calls at Lines 170-172 discard both the timeout outcome and anyJoinError, so a panic insidetunnel::serveorconnect_withpasses the test silently. Note also thatserve_handlewill always hit the 2 s timeout today because the cancel flag can't interruptendpoint.accept()(see the comment onsrc/transport/tunnel.rs), so this teardown currently verifies nothing.💚 Proposed change
- let mut received = [0u8; 64]; - let n = client.read(&mut received).await.expect("read payload"); - assert_eq!( - &received[..n], - TUNNEL_PAYLOAD, - "echoed bytes must match the request" - ); + let mut received = [0u8; TUNNEL_PAYLOAD.len()]; + client + .read_exact(&mut received) + .await + .expect("read payload"); + assert_eq!(&received, TUNNEL_PAYLOAD, "echoed bytes must match the request");and once cancellation actually interrupts the accept loop, assert on the join results instead of discarding them.
🤖 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/tunnel_e2e.rs` around lines 156 - 172, Update the test payload read to use read_exact with a buffer sized for TUNNEL_PAYLOAD, then assert the complete buffer matches. In the teardown of the tunnel test, stop discarding timeout and JoinError results from serve_handle, client_handle, and echo_handle: await each with timeout and assert successful completion, while preserving the existing cancellation flow. Ensure serve_handle can actually exit when cancellation is requested by fixing the accept-loop cancellation behavior in the relevant tunnel serving logic before asserting its join result.
🧹 Nitpick comments (4)
examples/snappipe-tunnel.service (1)
36-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDrop the unnecessary capability grant for this unprivileged QUIC port.
4443is not a privileged port, and there is no standardCAP_NET_BIND_SERVICErequirement or systemd/raw-QUIC bind capability to enable. If you need additional hardening around the granted capability, add the matchingCapabilityBoundingSet=CAP_NET_BIND_SERVICEto limit what the ambient capability can open the path to; otherwise remove it.🤖 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 `@examples/snappipe-tunnel.service` around lines 36 - 39, Remove the unnecessary AmbientCapabilities=CAP_NET_BIND_SERVICE setting and its explanatory comments from the service unit, since the QUIC listener uses unprivileged port 4443. Do not add a capability bounding set unless the capability grant is intentionally retained.src/main.rs (1)
400-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead code after
tunnel::connect, plus a redundant key decode.
tunnel::connecteither loops forever or returnsErr(propagated by?), sodrop(signing_key)andstd::future::pending()are unreachable.signing_keyis also decoded here and then decoded again from the same path insideconnect_with(src/transport/tunnel.rs:315-319) — the local copy serves no purpose. Also worth callingcfg.validate()before spawning the runtime so bad--listen/--relayvalues fail early.♻️ Proposed cleanup
- // Read client secret key (currently only used for ticket issuer - // re-verification). - let secret_raw = fs::read_to_string(&args.secret_key) - .with_context(|| format!("read secret key {}", args.secret_key.display()))?; - let signing_key = decode_secret_key(secret_raw.trim()) - .map_err(|err| anyhow::anyhow!("decode secret key: {}", err))?; - let cfg = tunnel::TunnelConfig { quic_bind: "0.0.0.0:0".parse().unwrap(), target_addr: "127.0.0.1:0".parse().unwrap(), listen_addr: args.listen.parse::<SocketAddr>()?, relay_addr: args.relay.parse::<SocketAddr>()?, }; + cfg.validate()?; let runtime = tokio::runtime::Builder::new_multi_thread() .enable_all() .build()?; - runtime.block_on(async move { - tunnel::connect(cfg, &args.ticket, &args.secret_key).await?; - // `tunnel::connect` never returns Ok normally (it runs the - // listener forever). The signing_key binding only exists to - // re-verify the ticket in `connect`. - drop(signing_key); - std::future::pending::<()>().await; - Ok(()) - }) + runtime.block_on(tunnel::connect(cfg, &args.ticket, &args.secret_key))Note
validate()in its current form only rejects the all-zero case — see the separate comment onTunnelConfig::validate.🤖 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/main.rs` around lines 400 - 423, Remove the redundant secret-key decoding and signing_key binding from the startup flow, and delete the unreachable drop/signing_key and pending calls after tunnel::connect in the runtime closure. Before building the runtime, invoke cfg.validate() so invalid tunnel configuration is rejected early, while preserving error propagation through the existing Result flow.tests/tunnel_e2e.rs (2)
52-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPredictable shared
/tmppath for secret-key material; prefertempfile.
temp_dir().join(format!("snappipe-tunnel-{}", now))is second-granular and world-writable-parent, so two test binaries starting in the same second collide (and Line 174 then deletes the other's directory mid-run). The secret keys at Lines 58 and 67 also land with default0644permissions.tempfile::tempdir()gives a unique0700directory with RAII cleanup that survives a panicking test.♻️ Proposed change
- let tmp = std::env::temp_dir().join(format!("snappipe-tunnel-{}", now)); - std::fs::create_dir_all(&tmp).expect("mkdir"); + let tmp_dir = tempfile::tempdir().expect("tempdir"); + let tmp = tmp_dir.path();and drop the manual
remove_dir_allat Line 174. Addtempfileto[dev-dependencies].🤖 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/tunnel_e2e.rs` around lines 52 - 71, Replace the predictable temp_dir path setup in the tunnel end-to-end test with tempfile::tempdir(), retaining the TempDir handle for the test’s lifetime so the directory is unique and automatically cleaned up. Update the issuer/subject key and ticket paths to use the TempDir path, remove the manual remove_dir_all cleanup, and add tempfile to dev-dependencies.
147-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixed 300 ms sleep will flake under CI load.
If the QUIC handshake hasn't completed,
TcpStream::connectsucceeds anyway (the listener is already bound) but the subsequentopen_bifails and the read at Line 157 hangs or returns 0. Retry the TCP connect + round-trip in a bounded loop, or haveconnect_withsignal readiness, rather than relying on a wall-clock guess.🤖 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/tunnel_e2e.rs` around lines 147 - 151, The fixed 300 ms delay before the TCP round-trip in the tunnel end-to-end test is race-prone. Replace the sleep and one-shot TcpStream connection around the client setup with a bounded retry loop that repeats the TCP connect and payload round-trip until the QUIC path is ready, preserving the existing assertions and failing clearly when retries are exhausted.
🤖 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 221-234: Align both deployment guides with the documented ISP port
constraint: in docs/OPERATIONAL-DEPLOYMENT.md lines 221-234, recommend an
operator-verified/allowlisted tunnel port or explicitly state that UDP/4443 is
not guaranteed; in README.md lines 240-245, remove the claim that UDP/4443 is
outside the typical ISP drop list while preserving the surrounding deployment
guidance.
- Around line 239-243: The deployment examples must preserve the
already-provisioned relay identity instead of regenerating keys. In
docs/OPERATIONAL-DEPLOYMENT.md lines 239-243, remove the server-start keygen
invocation or clearly mark it as one-time bootstrap only; in README.md lines
252-255, update the example to consume the existing relay key files without
invoking keygen.
- Around line 294-298: Update the “Cold start latency” guidance in
OPERATIONAL-DEPLOYMENT.md to remove the instruction to hold the relay-side TCP
connection or delay forwarding until backend readiness. Keep the documented JVM
cold-start latency, and describe only behavior supported by the tunnel CLI
contract.
- Around line 247-251: Update the tunnel serve examples at
docs/OPERATIONAL-DEPLOYMENT.md lines 247-251 and README.md lines 259-263 to
include the required --trust-store <path> argument, and add an explicit warning
not to omit it because omission enables allow-all issuer trust.
In `@examples/snappipe-tunnel-client.service`:
- Around line 18-24: Update the commented snappipe ticket command to use
relay.public as the --subject-public-key, matching the NodeId derived from the
server unit’s --public-key; leave the other command options unchanged.
- Around line 9-10: Update the service template configuration around User and
Group so it uses a valid peer account: either convert the unit into an instance
template by renaming it to include @ and document enabling a named instance, or
replace both %i values with the intended explicit account and ensure %h resolves
against that account.
In `@README.md`:
- Around line 313-316: Update the README’s snappipe tunnel production guidance
so the client cannot be presented as an unpinned public deployment path: either
document and require the available certificate-pinning mechanism before public
exposure, or clearly mark the shown command as development-only and state that
public deployments must fail closed without pinning.
- Around line 294-298: Revise the README tunnel guidance near the readiness
discussion to describe backend readiness as an external prerequisite, since the
supplied tunnel contract provides no relay-side readiness gate or retry
handling. Remove claims that it holds TCP connections open or delays forwarding
until readiness, unless the documented command is changed to implement that
behavior.
In `@src/main.rs`:
- Around line 348-356: The tunnel serve flow should no longer read or decode the
issuer secret key; replace that input with an --issuer-public-key path and
decode it using decode_public_key. Update the related argument definition and
the issuer_key usage around verifying_key() to use the decoded issuer public key
directly, while preserving the existing --public-key handling.
- Around line 164-166: The tunnel connect flow parses alpn but never uses it.
Update tunnel_connect and tunnel::connect to propagate args.alpn into the client
configuration used by quic::default_client_config, preserving the default
tunnel::TUNNEL_ALPN when unspecified; alternatively remove the unused --alpn
argument and its help text.
- Around line 367-369: Replace the relay_backhaul profile used in the tunnel
setup around relay_backhaul and build_transport_config with a dedicated tunnel
transport profile whose maximum concurrent bidirectional streams is sized for
the expected connection count, rather than the relay default of 32. Track the
handshake and proxied TCP streams and return a clear error when the configured
stream limit is reached instead of allowing open_bi() to block indefinitely.
- Line 358: Update the trust initialization near allow_all_trust() to read
args.trust_store: use the existing trust-store loader when a path is provided,
or reject Some(_) with an explicit error if loading is not supported; retain
allow_all_trust() only when no trust store is configured.
In `@src/transport/tunnel.rs`:
- Around line 32-41: The threat-model documentation in the module-level notes
incorrectly claims that RateLimiter caps apply. Since tunnel.rs and tunnel_serve
do not construct or consult a limiter, remove or reword that claim to accurately
describe the current unbounded stream-task behavior, without implying a
protection that is not implemented.
- Around line 386-398: Update connect() and the tunnel connect CLI flow to
obtain the server certificate or its fingerprint/CA through an explicit
out-of-band CLI option, then build the client trust configuration from that
supplied value instead of generating a new certificate with
quic::self_signed_dev_cert(&[]). Ensure the configured certificate includes or
validates the "localhost" server name used by connect_with, while preserving the
existing endpoint setup and connection flow.
- Around line 97-104: Update TunnelConfig::validate so it enforces the
documented requirement for the required listen_addr and relay_addr roles instead
of requiring all four ports to be zero before failing. Preserve the intentional
:0 values for quic_bind and target_addr used by tunnel_connect, and keep the
existing error result for configurations missing the required addresses.
- Around line 179-201: Bound the unauthenticated handshake flow in the
connection handler with tokio::time::timeout, covering both accept_bi() and
server_handshake() (or apply the timeout directly around server_handshake as
indicated), using the intended handshake deadline. Handle timeout as a failed
handshake by logging it and closing the connection, while preserving existing
error handling for normal failures.
- Around line 123-138: Replace the Mutex<bool>-based cancellation checks in the
endpoint accept loop and per-connection stream loop with a wakeable cancellation
primitive such as Arc<tokio::sync::Notify>. Use tokio::select! to race
cancellation notification against endpoint.accept() and the stream read/receive
operation, ensuring both loops exit promptly when cancellation is signaled while
preserving normal connection and end-of-stream handling.
- Around line 314-340: Update the tunnel connection flow around verify_ticket so
local ticket verification uses an explicitly supplied issuer verifying key from
the CLI or trust-store configuration, rather than _signing_key.verifying_key()
derived from the client secret. Preserve the client key for its existing
client-authentication purpose, and extend tests/tunnel_e2e.rs with distinct
issuer and subject keys so non-self-issued tickets are verified successfully.
---
Minor comments:
In `@docs/OPERATIONAL-DEPLOYMENT.md`:
- Line 23: Update the transport diagram entry in OPERATIONAL-DEPLOYMENT so its
SnapPipe version label uses v0.3.0 instead of v0.2.1, while preserving the
existing transport feature descriptions.
- Around line 332-341: Remove the later “Why these deploy notes live outside the
repo” heading and paragraph from the deployment documentation, then add “probe
fixtures” to the corresponding list in the first occurrence. Preserve the
remaining deployment guidance without duplicating the section.
In `@examples/relay.sample.toml`:
- Around line 8-10: Correct the UDP/4443 rationale in the comments near the
relay configuration: remove the contradictory claim that port 4443 is unfettered
when it is outside the carrier allowlist, and describe its availability only for
freshly provisioned hosts or networks where it is actually permitted.
In `@examples/snappipe-tunnel-client.ps1`:
- Around line 12-14: Update the prerequisite documentation to instruct users to
place friend.ticket.json and friend.secret under $env:USERPROFILE\snappipe,
matching the credential paths used by the script; do not describe them as files
beside the script.
In `@README.md`:
- Line 212: Add a language identifier such as text or none to the fenced diagram
block in README.md, preserving its existing diagram content.
- Around line 88-89: Update the later README statement describing which release
tag the published metadata mirrors, changing the stale v0.2.1 reference to
v0.3.0 while preserving the surrounding wording and links.
- Around line 300-306: Update the README test summary near the tunnel_e2e
documentation to report 70 passing tests instead of 63, and revise the
test-scope description so it accurately reflects the expanded suite including
the tunnel end-to-end test.
In `@src/main.rs`:
- Around line 158-160: Update the documentation comment for the relay argument
to replace the concrete routable address with a generic host-and-port
placeholder, while preserving the existing description and CLI behavior of the
relay field.
In `@src/transport/tunnel.rs`:
- Around line 59-70: Run cargo fmt --all and commit the resulting rustfmt
changes in src/transport/tunnel.rs (including the import ordering, boolean
formatting, and wrapped calls) and tests/tunnel_e2e.rs; no behavioral changes
are needed.
In `@tests/tunnel_e2e.rs`:
- Around line 156-172: Update the test payload read to use read_exact with a
buffer sized for TUNNEL_PAYLOAD, then assert the complete buffer matches. In the
teardown of the tunnel test, stop discarding timeout and JoinError results from
serve_handle, client_handle, and echo_handle: await each with timeout and assert
successful completion, while preserving the existing cancellation flow. Ensure
serve_handle can actually exit when cancellation is requested by fixing the
accept-loop cancellation behavior in the relevant tunnel serving logic before
asserting its join result.
---
Nitpick comments:
In `@examples/snappipe-tunnel.service`:
- Around line 36-39: Remove the unnecessary
AmbientCapabilities=CAP_NET_BIND_SERVICE setting and its explanatory comments
from the service unit, since the QUIC listener uses unprivileged port 4443. Do
not add a capability bounding set unless the capability grant is intentionally
retained.
In `@src/main.rs`:
- Around line 400-423: Remove the redundant secret-key decoding and signing_key
binding from the startup flow, and delete the unreachable drop/signing_key and
pending calls after tunnel::connect in the runtime closure. Before building the
runtime, invoke cfg.validate() so invalid tunnel configuration is rejected
early, while preserving error propagation through the existing Result flow.
In `@tests/tunnel_e2e.rs`:
- Around line 52-71: Replace the predictable temp_dir path setup in the tunnel
end-to-end test with tempfile::tempdir(), retaining the TempDir handle for the
test’s lifetime so the directory is unique and automatically cleaned up. Update
the issuer/subject key and ticket paths to use the TempDir path, remove the
manual remove_dir_all cleanup, and add tempfile to dev-dependencies.
- Around line 147-151: The fixed 300 ms delay before the TCP round-trip in the
tunnel end-to-end test is race-prone. Replace the sleep and one-shot TcpStream
connection around the client setup with a bounded retry loop that repeats the
TCP connect and payload round-trip until the QUIC path is ready, preserving the
existing assertions and failing clearly when retries are exhausted.
🪄 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: 06f6aa8e-d2b1-4d8f-a71d-e5ce450b11c5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlREADME.mddocs/OPERATIONAL-DEPLOYMENT.mdexamples/relay.sample.tomlexamples/snappipe-tunnel-client.ps1examples/snappipe-tunnel-client.serviceexamples/snappipe-tunnel.servicesrc/lib.rssrc/main.rssrc/transport/mod.rssrc/transport/tunnel.rstests/tunnel_e2e.rs
| snappipe tunnel serve \ | ||
| --secret-key <relay-keys>/relay.secret \ | ||
| --public-key <relay-keys>/relay.public \ | ||
| --quic-bind 0.0.0.0:4443 \ | ||
| --target <tcp-backend-host>:25565 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files of interest:\n'
git ls-files | rg '^(docs/OPERATIONAL-DEPLOYMENT\.md|README\.md)$' || true
printf '\nRelevant docs snippets:\n'
for f in README.md docs/OPERATIONAL-DEPLOYMENT.md; do
if [ -f "$f" ]; then
echo "--- $f"; sed -n '240,268p' "$f" | cat -n
fi
done
printf '\nSearch for tunnel serve trust-store/cli options/usages:\n'
rg -n --hidden --no-heading 'tunnel serve|trust.store|trust store|trust-store|allow_all|allow-all|snappipe' . | sed -n '1,220p'Repository: LOUST-PRO/SnapPipe
Length of output: 16995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'src/main.rs TunnelServe args/parsing and tunnel serve body:\n'
sed -n '100,145p' src/main.rs | cat -n
sed -n '320,385p' src/main.rs | cat -n
printf '\nsrc/session.rs allow_all_trust:\n'
sed -n '60,80p' src/session.rs | cat -n
printf '\nSystemd tunnel unit ExecStart:\n'
sed -n '1,60p' examples/snappipe-tunnel.service | cat -n
printf '\nSnippets mentioning allow_all or empty store default behavior:\n'
sed -n '34,48p' README.md | cat -n
sed -n '11,13p' docs/SECURITY-MODEL.md | cat -n
sed -n '300,335p' docs/OPERATIONAL-DEPLOYMENT.md | cat -n
printf '\nStructured CLI option scan:\n'
rg -n --hidden --no-heading 'trust_store|allow_all_trust\(|trust store|absent store|absent' src/main.rs src/session.rs README.md docs/OPERATIONAL-DEPLOYMENT.md | cat -nRepository: LOUST-PRO/SnapPipe
Length of output: 12255
Do not publish allow-all tunnel deployment examples.
snappipe tunnel serve --trust-store ... currently describes loading a trust store, but absent --trust-store hardcodes allow_all_trust(). These examples omit that required argument and do not warn against it.
README.md#L259-L263docs/OPERATIONAL-DEPLOYMENT.md#L247-L251
Add --trust-store <path> to both commands and explicitly warn operators not to omit it; otherwise the documented deployment accepts any issuer.
📍 Affects 2 files
docs/OPERATIONAL-DEPLOYMENT.md#L247-L251(this comment)README.md#L259-L263
🤖 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 247 - 251, Update the tunnel
serve examples at docs/OPERATIONAL-DEPLOYMENT.md lines 247-251 and README.md
lines 259-263 to include the required --trust-store <path> argument, and add an
explicit warning not to omit it because omission enables allow-all issuer trust.
| User=%i | ||
| Group=%i |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
systemd-analyze verify examples/snappipe-tunnel-client.service
grep -nE '^(User|Group)=%i$|%h/snappipe' examples/snappipe-tunnel-client.serviceRepository: LOUST-PRO/SnapPipe
Length of output: 209
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- repository files matching snappipe-tunnel-client.service ---\n'
git ls-files | grep -F 'snappipe-tunnel-client.service' || true
printf '\n--- example unit content ---\n'
if [ -f examples/snappipe-tunnel-client.service ]; then
cat -n examples/snappipe-tunnel-client.service
fi
printf '\n--- related service/unit files ---\n'
git ls-files | grep -E '(^|/)snappipe-tunnel-client(|[^.]*\.service|[^A-Za-z0-9_.-])' || true
printf '\n--- usage/docs references for examples/snappipe-tunnel-client.service ---\n'
rg -n "snappipe-tunnel-client(@[^'\"]*)?(service)?|User=\%i|Group=\%i|systemctl .*snappipe-tunnel-client" . || trueRepository: LOUST-PRO/SnapPipe
Length of output: 247
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- repository files matching snappipe-tunnel-client.service ---'
git ls-files | grep -F 'snappipe-tunnel-client.service' || true
printf '%s\n' ''
printf '%s\n' '--- example unit content ---'
if [ -f examples/snappipe-tunnel-client.service ]; then
cat -n examples/snappipe-tunnel-client.service
fi
printf '%s\n' ''
printf '%s\n' '--- related service/unit files ---'
git ls-files | grep -E '(^|/)snappipe-tunnel-client(|[^.]*\.service|[^A-Za-z0-9_.-])' || true
printf '%s\n' ''
printf '%s\n' '--- usage/docs references for examples/snappipe-tunnel-client.service ---'
rg -n "snappipe-tunnel-client(@[^'\"]*)?(service)?|User=\%i|Group=\%i|systemctl .*snappipe-tunnel-client" . || trueRepository: LOUST-PRO/SnapPipe
Length of output: 2196
🌐 Web query:
systemd unit templates %i specifier behavior User=%i non-template unit systemd.unit=
💡 Result:
In systemd, unit templates are designed for parameterized instantiation, where a single configuration file (e.g., service@.service) serves as a template for multiple unit instances [1][2]. %i Specifier Behavior The %i specifier is used within a template unit file to refer to the "instance name"—the string between the @ character and the unit type suffix [1][3]. When systemd instantiates a unit from a template (e.g., starting service@foo.service from service@.service), it replaces %i with the instance name ("foo") [1][3]. Crucially, %i is escaped; special characters (such as / or spaces) within the instance name are replaced (e.g., / becomes -) to ensure the resulting string is a valid unit name component [4][5]. A related specifier, %I, functions similarly but provides the unescaped instance name [4][5]. User=%i Behavior Using User=%i in a non-template (static) unit is not supported as intended because the %i specifier is only resolved for instantiated units [1][5]. In a non-template unit, there is no instance name, so %i will fail to resolve or remain empty, likely causing the unit to fail or run as the wrong user [1][5]. If you need to dynamically set a user, you must use a template unit and pass the username as the instance parameter (e.g., my-service@username.service), which then allows User=%i to correctly resolve to that username [5]. systemd.unit= Behavior The systemd.unit= parameter is a kernel command line option used to override the default unit (usually default.target) that systemd activates upon boot [6][7]. It is not related to unit templates or specifier resolution; rather, it directs PID 1 to start a specific target or service at the end of the boot process [6][8]. Top results: [1][4][5][6]
Citations:
- 1: https://www.freedesktop.org/software/systemd/man/systemd.unit
- 2: https://freedesktop.org/software/systemd/man/devel/systemd.service.html
- 3: https://man.archlinux.org/man/systemd.unit.5.en.txt
- 4: https://unix.stackexchange.com/questions/396978/specifier-resolution-i-and-i-difference
- 5: https://unix.stackexchange.com/questions/777080/systemd-unit-file-with-multiple-instance-parameters-including-user
- 6: https://www.freedesktop.org/software/systemd/man/latest/systemd.html
- 7: https://man7.org/linux/man-pages/man7/systemd.special.7.html
- 8: https://www.freedesktop.org/software/systemd/man/latest/kernel-command-line.html
🌐 Web query:
systemd man systemd.unit User Group specifiers %i non-template empty
💡 Result:
In systemd unit files, the %i specifier refers to the "instance name" of a unit [1][2][3]. This instance name is the string located between the "@" character and the unit type suffix in the filename of an instantiated unit (e.g., in service@foo.service, the instance name is "foo") [2][3][4]. Regarding its behavior in non-template units: 1. Definition: The %i specifier is intended for use in "template" units (units ending in @.service, @.socket, etc.) [1][2]. 2. Value in Non-Template Units: In non-instantiated (non-template) units, the %i specifier is empty [3][5]. Because there is no instance name to resolve, any configuration option referencing %i in a non-template unit will resolve to an empty string [3][5]. For settings like User= or Group=, using %i is generally ineffective in a non-template unit because it will resolve to nothing, potentially causing the setting to be invalid or behave unexpectedly depending on how the application or systemd interprets the resulting empty value [3]. Specifiers like %u (user name) or %g (group name) should be used instead if the goal is to reference the user or group running the service manager [5][6].
Citations:
- 1: https://www.freedesktop.org/software/systemd/man/systemd.unit
- 2: https://www.man7.org/linux/man-pages/man5/systemd.unit.5.html
- 3: https://manpages.debian.org/buster/systemd/systemd.unit.5.en.html
- 4: https://man.archlinux.org/man/systemd.unit.5
- 5: https://dokk.org/manpages/debian/10/systemd/systemd.unit.5.en
- 6: https://manpages.ubuntu.com/manpages/xenial/man5/systemd.unit.5.html
Make this an actual template unit or use a concrete account.
%i is empty for examples/snappipe-tunnel-client.service because the filename has no @ instance, so User=%i/Group=%i will not select the intended peer account and %h will not use that user’s ~ directory. Rename it to examples/snappipe-tunnel-client@.service and document enabling an instance, or replace both values with an explicit account.
🤖 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 `@examples/snappipe-tunnel-client.service` around lines 9 - 10, Update the
service template configuration around User and Group so it uses a valid peer
account: either convert the unit into an instance template by renaming it to
include @ and document enabling a named instance, or replace both %i values with
the intended explicit account and ensure %h resolves against that account.
| loop { | ||
| // Check cancellation flag cooperatively. | ||
| if *cancel.lock().await { | ||
| break; | ||
| } | ||
|
|
||
| // Accept the next incoming connection. | ||
| let incoming = endpoint.accept().await; | ||
|
|
||
| let incoming = match incoming { | ||
| Some(i) => i, | ||
| None => { | ||
| // Endpoint permanently closed; exit gracefully. | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancellation never fires while blocked in endpoint.accept().
The flag is only read between accepts, so once the loop parks on accept().await the task stays alive indefinitely regardless of cancel. Same pattern in the per-connection stream loop at Line 209-216. A Mutex<bool> can't wake a waiter; use a notification primitive and select!.
♻️ Suggested shape using `tokio::sync::Notify`
- // Accept the next incoming connection.
- let incoming = endpoint.accept().await;
-
- let incoming = match incoming {
- Some(i) => i,
- None => {
- // Endpoint permanently closed; exit gracefully.
- break;
- }
- };
+ let incoming = tokio::select! {
+ _ = cancel.notified() => break,
+ incoming = endpoint.accept() => match incoming {
+ Some(i) => i,
+ // Endpoint permanently closed; exit gracefully.
+ None => break,
+ },
+ };with cancel: Arc<tokio::sync::Notify> (or a watch::Receiver<bool> if late subscribers must also observe the shutdown).
🤖 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/transport/tunnel.rs` around lines 123 - 138, Replace the
Mutex<bool>-based cancellation checks in the endpoint accept loop and
per-connection stream loop with a wakeable cancellation primitive such as
Arc<tokio::sync::Notify>. Use tokio::select! to race cancellation notification
against endpoint.accept() and the stream read/receive operation, ensuring both
loops exit promptly when cancellation is signaled while preserving normal
connection and end-of-stream handling.
| let (hs_send, hs_recv) = conn | ||
| .accept_bi() | ||
| .await | ||
| .map_err(|err| anyhow::anyhow!("accept handshake stream: {}", err))?; | ||
|
|
||
| let now = crate::now_unix_seconds(); | ||
| let summary = match server_handshake( | ||
| hs_send, | ||
| hs_recv, | ||
| &issuer_key, | ||
| &expected_subject, | ||
| trust, | ||
| now, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(summary) => summary, | ||
| Err(err) => { | ||
| eprintln!("tunnel: handshake failed: {}", err); | ||
| conn.close(0u8.into(), b"handshake failed"); | ||
| return Ok(()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on the pre-auth handshake — unauthenticated peers can pin resources.
accept_bi() and server_handshake() run before any authentication with no deadline. A peer that completes the QUIC handshake and then stalls holds a spawned task and connection state until the idle timeout (30s from relay_backhaul, refreshed by keep-alives). Wrap the handshake in tokio::time::timeout and close on expiry.
🛡️ Proposed fix
- let (hs_send, hs_recv) = conn
- .accept_bi()
- .await
- .map_err(|err| anyhow::anyhow!("accept handshake stream: {}", err))?;
+ const HANDSHAKE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
+ let (hs_send, hs_recv) = match tokio::time::timeout(HANDSHAKE_TIMEOUT, conn.accept_bi()).await {
+ Ok(Ok(pair)) => pair,
+ Ok(Err(err)) => return Err(anyhow::anyhow!("accept handshake stream: {}", err)),
+ Err(_) => {
+ conn.close(0u8.into(), b"handshake timeout");
+ return Ok(());
+ }
+ };Apply the same bound around the server_handshake(...) call below.
🤖 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/transport/tunnel.rs` around lines 179 - 201, Bound the unauthenticated
handshake flow in the connection handler with tokio::time::timeout, covering
both accept_bi() and server_handshake() (or apply the timeout directly around
server_handshake as indicated), using the intended handshake deadline. Handle
timeout as a failed handshake by logging it and closing the connection, while
preserving existing error handling for normal failures.
CI fmt --check was failing on the prior commit (3 files needed
reformat). Running cargo fmt --all now produces a clean tree and
cargo test --release still passes 70/70 (61 unit + 9 integration,
0 regressions).
Changes are pure whitespace:
- src/main.rs: import order swap (verify_ticket / transport::tunnel)
- src/transport/tunnel.rs: multi-line block reformat of
verify_ticket + ticket_error_label match + a few wrap points
- tests/tunnel_e2e.rs: single-line / multi-line wrap fixes
No semantic change.
Resolves 4 Critical + Major + Minor findings from CodeRabbit PR review. Critical - tunnel::serve with --trust-store now emits a clear stderr warning that the flag is currently a no-op (allow_all until TrustStore wiring is shipped in a follow-up PR). - examples/snappipe-tunnel-client.service: ticket issuance now uses --subject-public-key <relay-public-key> (was ./peer.public, which would issue a self-ticket the server rejects). - connect_with now requires issuer_public_key: &VerifyingKey and uses it to verify the ticket locally before presenting it on the wire (defense-in-depth against self-issued tickets). - tunnel::connect() now requires server_cert_der: &[u8] and pins it into the client rustls root store via the new quic::pinned_client_config helper (no more silent reliance on webpki defaults). Major - README + OPS-DEPLOY: v0.2.1 -> v0.3.0 across all references. - README server-side: removed misleading 'snappipe keygen' guidance; added explicit warning that the relay identity is provisioned ONCE during v0.2.x bootstrap and must NOT be regenerated. - README client-side: now documents the 4 files the operator ships to the trusted peer (peer.ticket.json, peer.secret, relay.public, relay.cert.der). - README production caveats: --server-cert now listed as REQUIRED for production (was previously advisory). - README wire model + threat model note: RateLimiter is NOT yet wired into tunnel::serve; operators must layer external rate caps until a follow-up PR threads it through. - TunnelConfig gains validate() (strict, fails on any zero port) + validate_server() / validate_client() (role-aware); callers pick the helper that matches their role. - README + OPS-DEPLOY topology diagrams converted from ASCII to mermaid (per operator feedback). - OPS-DEPLOY 'Why UDP/4443' rewritten: UDP/4443 is now framed as a defensible default candidate, NOT a guarantee; operators MUST probe the peer path with nc -uvz / lzt-tunnel-probe before locking the choice in. - OPS-DEPLOY 'Cold start latency' wording aligned with the README Production Caveats: tunnel cannot hold a connection open while backend is cold; operators must layer their own readiness gate. Minor - README test count updated from 63 -> reflects current suite. - OPS-DEPLOY removed duplicate 'Why these deploy notes live outside the repo' section. - quic::mod.rs re-exports pinned_client_config + pinned_client_config_with_alpn. - Removed the IP-167.88.38.25 docstring example from main.rs tunnel_connect; replaced with the generic 127.0.0.1:4443. Tests - tests/tunnel_e2e.rs updated to pass issuer_public_key through to connect_with (5-arg -> 6-arg signature). - src/transport/tunnel.rs: tunnel_config_rejects_fully_empty_population extended to assert all 3 validate helpers fail on zero ports. Verification - cargo fmt --all -- --check: clean - cargo test: all tests pass - cargo clippy --all-targets -- -D warnings: clean Refs: PR #24
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
README.md (1)
205-207: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not claim that tunnel serving currently reuses
TrustStoremachinery.The tunnel server does not yet wire
TrustStoreinto serving, and--trust-storeis currently a no-op. This wording can make operators assume relay-side peer authorization is enforced when it is not. Limit the claim to the mechanisms actually used, and explicitly document the TrustStore limitation alongside the existingRateLimitercaveat.Proposed wording
- The tunnel reuses the existing `SignedTicket` / - `TrustStore` / `RateLimiter` machinery — no new auth layer — + The tunnel reuses existing `SignedTicket` verification and + introduces no new authentication layer. Tunnel serving does not + currently consume `TrustStore` or `RateLimiter`; see the + operational caveats below.🤖 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 `@README.md` around lines 205 - 207, Update the README tunnel description to remove the claim that serving reuses TrustStore machinery, retain only mechanisms actually used such as SignedTicket and RateLimiter, and explicitly state that --trust-store is currently a no-op and does not enforce relay-side peer authorization, alongside the existing RateLimiter caveat.src/transport/tunnel.rs (3)
99-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
validate_server()/validate_client()fix the logic but are never called.Neither role-aware helper is invoked from the actual production paths:
connect()/connect_with()never callself.validate_client(), andserve()doesn't even accept aTunnelConfig(seesrc/main.rstunnel_serve, which buildsbind/targetdirectly), sovalidate_server()has no real caller at all today. A misconfiguredlisten_addr/relay_addr(e.g. accidental:0) will silently bind to an OS-assigned ephemeral port instead of failing fast with a clear message.♻️ Minimal wiring fix
pub async fn connect_with( endpoint: Endpoint, listener: TcpListener, relay_addr: SocketAddr, ticket_path: &Path, issuer_public_key: &ed25519_dalek::VerifyingKey, client_secret_key_path: &Path, ) -> Result<()> { + // (call site should validate before constructing the listener/endpoint)Call
cfg.validate_client()at the top ofconnect()beforeTcpListener::bind(cfg.listen_addr).🤖 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/transport/tunnel.rs` around lines 99 - 147, Call cfg.validate_client() at the beginning of connect(), before TcpListener::bind(cfg.listen_addr), so invalid client listen_addr or relay_addr ports fail fast. Do not modify validate_server() or add wiring for serve(), since the requested change is limited to the client connection path.
383-394: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHardcoded SNI
"localhost"conflicts with the new pinned-cert production path.
connect_withalways dials with server name"localhost", butconnect()now pins whatever cert the operator actually ships (--server-cert). Rustls' default verifier (used byrustls_pinned_client_configinsrc/quic/endpoint.rs) checks the presented cert's SAN against this SNI, so any real production cert withoutlocalhostin its SAN will fail hostname verification — the "should override this with the operator's hostname" comment (Line 386-388) describes an override that doesn't exist anywhere in the API/CLI. This is the second half of a previously-flagged critical finding; the trust-anchor half is fixed, this half is not.Thread a
server_name: &strparameter throughconnect_with/connect(and a--server-nameCLI flag) instead of hardcoding it.🤖 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/transport/tunnel.rs` around lines 383 - 394, Update the tunnel connection flow around connect_with and connect to accept and propagate a server_name: &str, using it for QUIC endpoint.connect instead of hardcoded "localhost". Add a --server-name CLI option, thread its value through the existing call path, and preserve localhost as the development default while allowing production deployments to match the certificate SAN.
450-480: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTunnel ALPN configurability was only wired through the client pinning helper — the server-side crypto config and both
--alpnCLI flags never actually affect the negotiated ALPN.Root cause:
default_server_config(used bytunnel_serve) has no ALPN parameter, andtunnel::connect()callsquic::pinned_client_config(hardcodedDEFAULT_ALPN) instead of the already-availablepinned_client_config_with_alpn. As a result, the module's stated goal of a dedicated/snappipe/tunnel/0ALPN distinct from relay traffic is never actually presented in the TLS handshake — both sides silently fall back toDEFAULT_ALPN, so default installs still connect, but operators overriding--alpnfor isolation/routing purposes get no error and no effect.
src/transport/tunnel.rs#L450-L480: add analpn: &strparameter toconnect()/connect_with()and callquic::pinned_client_config_with_alpn(server_cert_der, alpn)instead ofquic::pinned_client_config(server_cert_der).src/quic/endpoint.rs#L163-L230: add a server-side equivalent (e.g.default_server_config_with_alpn(dev_cert, alpn)) sotunnel_servehas a way to actually set the negotiated ALPN; today it's structurally impossible viadefault_server_config.src/main.rs#L401-L410: once the server-side helper exists, passargs.alpninto it instead of only intoQuicTransportProfile::relay_backhaul, and keep the status-line log honest in the meantime.src/main.rs#L429-L480: threadargs.alpnthrough totunnel::connect(...)once it accepts an ALPN parameter.🤖 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/transport/tunnel.rs` around lines 450 - 480, The tunnel ALPN setting is not propagated to both QUIC endpoints, so custom --alpn values have no effect. In src/transport/tunnel.rs lines 450-480, update connect and connect_with to accept an ALPN string and use pinned_client_config_with_alpn; in src/quic/endpoint.rs lines 163-230, add an ALPN-aware counterpart to default_server_config; in src/main.rs lines 401-410, pass args.alpn to the server configuration while keeping the status log accurate; and in src/main.rs lines 429-480, thread args.alpn through tunnel::connect. Ensure both client and server negotiate the configured ALPN while preserving the default behavior.src/main.rs (1)
399-405: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a persistably loaded TLS identity for
tunnel serve.
tunnel_serve()currently callsself_signed_dev_cert(&[])insrc/main.rs:399, andTunnelServeArgshas no--tls-cert/--tls-keypath, so each restart presents a new certificate. Since production clients use--server-cert,tunnel connectwill keep rejecting a restarted relay until the operator redistributes the new DER certificate.🤖 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/main.rs` around lines 399 - 405, Update TunnelServeArgs and tunnel_serve() so the relay uses a persistently loaded TLS certificate and private key instead of generating one via self_signed_dev_cert(&[]). Add --tls-cert and --tls-key inputs, load and validate the identity before constructing server_cfg, and preserve the existing transport and endpoint setup.
♻️ Duplicate comments (1)
src/main.rs (1)
369-381: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
tunnel servestill requires the operator's private issuance key just to derive its public half.
issuer_keyis decoded from--secret-keyand only.verifying_key()(Line 416) is ever used — the signing capability itself is unused. This was flagged previously;tunnel_connectwas updated to take--issuer-public-key+decode_public_keyfor the analogous need, buttunnel_servewasn't given the same treatment, so the edge/relay host still needs the master issuance private key on disk, unnecessarily widening the blast radius if that host is compromised.🔒 Suggested fix (mirrors `tunnel_connect`'s pattern)
-struct TunnelServeArgs { - /// Path to the operator's secret key (Ed25519, base64url). Used - /// to verify the signed ticket presented by the client. - #[arg(long)] - secret_key: PathBuf, +struct TunnelServeArgs { + /// Path to the operator's ISSUING public key (Ed25519, base64url). + /// Used to verify the signed ticket presented by the client. + #[arg(long)] + issuer_public_key: PathBuf,and in
tunnel_serve, replace thedecode_secret_key+.verifying_key()dance with a directdecode_public_keyread.🤖 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/main.rs` around lines 369 - 381, Update tunnel_serve to stop reading or decoding the private issuance key; read the public issuer key from the appropriate public-key argument, decode it with decode_public_key, and use that value directly wherever issuer_key.verifying_key() is currently used. Preserve expected_subject decoding from the separate public key.
🤖 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.
Outside diff comments:
In `@README.md`:
- Around line 205-207: Update the README tunnel description to remove the claim
that serving reuses TrustStore machinery, retain only mechanisms actually used
such as SignedTicket and RateLimiter, and explicitly state that --trust-store is
currently a no-op and does not enforce relay-side peer authorization, alongside
the existing RateLimiter caveat.
In `@src/main.rs`:
- Around line 399-405: Update TunnelServeArgs and tunnel_serve() so the relay
uses a persistently loaded TLS certificate and private key instead of generating
one via self_signed_dev_cert(&[]). Add --tls-cert and --tls-key inputs, load and
validate the identity before constructing server_cfg, and preserve the existing
transport and endpoint setup.
In `@src/transport/tunnel.rs`:
- Around line 99-147: Call cfg.validate_client() at the beginning of connect(),
before TcpListener::bind(cfg.listen_addr), so invalid client listen_addr or
relay_addr ports fail fast. Do not modify validate_server() or add wiring for
serve(), since the requested change is limited to the client connection path.
- Around line 383-394: Update the tunnel connection flow around connect_with and
connect to accept and propagate a server_name: &str, using it for QUIC
endpoint.connect instead of hardcoded "localhost". Add a --server-name CLI
option, thread its value through the existing call path, and preserve localhost
as the development default while allowing production deployments to match the
certificate SAN.
- Around line 450-480: The tunnel ALPN setting is not propagated to both QUIC
endpoints, so custom --alpn values have no effect. In src/transport/tunnel.rs
lines 450-480, update connect and connect_with to accept an ALPN string and use
pinned_client_config_with_alpn; in src/quic/endpoint.rs lines 163-230, add an
ALPN-aware counterpart to default_server_config; in src/main.rs lines 401-410,
pass args.alpn to the server configuration while keeping the status log
accurate; and in src/main.rs lines 429-480, thread args.alpn through
tunnel::connect. Ensure both client and server negotiate the configured ALPN
while preserving the default behavior.
---
Duplicate comments:
In `@src/main.rs`:
- Around line 369-381: Update tunnel_serve to stop reading or decoding the
private issuance key; read the public issuer key from the appropriate public-key
argument, decode it with decode_public_key, and use that value directly wherever
issuer_key.verifying_key() is currently used. Preserve expected_subject decoding
from the separate public key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ae810cd-8250-48e4-90d7-4311ada0d578
📒 Files selected for processing (9)
README.mddocs/OPERATIONAL-DEPLOYMENT.mdexamples/snappipe-tunnel-client.serviceexamples/snappipe-tunnel.servicesrc/main.rssrc/quic/endpoint.rssrc/quic/mod.rssrc/transport/tunnel.rstests/tunnel_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/snappipe-tunnel.service
- examples/snappipe-tunnel-client.service
- docs/OPERATIONAL-DEPLOYMENT.md
- tests/tunnel_e2e.rs
Critical - tunnel serve --trust-store now bails with a clear error instead of silently degrading to allow_all. The trust-store loader is not implemented yet; supplying the flag refuses to start so operators do not assume the issuer allowlist is being enforced. - tests/tunnel_e2e.rs now uses distinct issuer + subject keys and exercises the cross-key verify_ticket path that the previous self-issued-ticket degenerate case masked. Major - tunnel serve CLI flag renamed --secret-key -> --issuer-public-key. The serve side only verifies ticket signatures, so the secret key was unnecessary exposure; the secret now stays on the offline ticket-issuance host. Examples and docs updated. - tunnel::connect now takes an explicit alpn: &str parameter and threads it through pinned_client_config_with_alpn, so the --alpn flag on tunnel connect is honoured (was previously parsed but silently ignored). Tests - e2e test renamed from 'subject = &issuer' to distinct subject_key, with a separate subject.public file feeding the server's expected_subject. This catches the client-key-as-issuer regression CodeRabbit flagged. Verification - cargo fmt --all -- --check: clean - cargo test: all tests pass (3/3 tunnel_e2e) - cargo clippy --all-targets -- -D warnings: clean Deferred (follow-up PRs) - cancel propagation via tokio::select! / Notify (was a Mutex<bool> check; CR flagged the primitive doesn't wake the accept waiter) - handshake timeout via tokio::time::timeout on accept_bi + server_handshake (pre-auth resource pinning) - dedicated tunnel transport profile with stream-cap raised above relay_backhaul's 32 bidi streams Refs: PR #24 round 2
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/OPERATIONAL-DEPLOYMENT.md (2)
228-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not document an unshipped probe binary.
lzt-tunnel-probeis not a SnapPipe command introduced by this PR, so operators cannot follow the primary recommendation. Replace it with a shipped tool, or make the generic manual probe the documented option.🤖 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 228 - 230, Update the operational deployment guidance around the peer outbound-path probe to remove the unshipped lzt-tunnel-probe recommendation; document only the shipped manual nc probe, or replace it with another tool confirmed to be shipped by this PR.
309-313: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the unsupported trust-store override guidance.
Trust-store loading is not implemented, yet this note instructs operators to configure a trust-store rate-limit override. Replace it with a supported setting or document the fixed
100/minlimit and its operational consequence.🤖 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 309 - 313, Update the “Rate limit headroom” guidance in OPERATIONAL-DEPLOYMENT to remove instructions to configure a trust-store per-minute override. Instead, document the fixed 100/min-per-node-id limit and its operational consequence, or reference a currently supported configuration setting.
♻️ Duplicate comments (1)
docs/OPERATIONAL-DEPLOYMENT.md (1)
300-304: 🩺 Stability & Availability | 🟠 MajorThe cold-start instruction is still unsupported.
The tunnel CLI has no documented backend-readiness gate, so operators cannot hold the connection and wait for a
Readysignal as described. This repeats the previous review finding and remains a production failure mode.🤖 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 300 - 304, Update the cold-start guidance in the operational deployment documentation to describe only readiness behavior supported by the tunnel CLI. Remove the unsupported instruction to hold the TCP connection and wait for a backend “Ready” signal, and replace it with the documented operational procedure or explicitly state that no readiness gate is available.
🤖 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.
Outside diff comments:
In `@docs/OPERATIONAL-DEPLOYMENT.md`:
- Around line 228-230: Update the operational deployment guidance around the
peer outbound-path probe to remove the unshipped lzt-tunnel-probe
recommendation; document only the shipped manual nc probe, or replace it with
another tool confirmed to be shipped by this PR.
- Around line 309-313: Update the “Rate limit headroom” guidance in
OPERATIONAL-DEPLOYMENT to remove instructions to configure a trust-store
per-minute override. Instead, document the fixed 100/min-per-node-id limit and
its operational consequence, or reference a currently supported configuration
setting.
---
Duplicate comments:
In `@docs/OPERATIONAL-DEPLOYMENT.md`:
- Around line 300-304: Update the cold-start guidance in the operational
deployment documentation to describe only readiness behavior supported by the
tunnel CLI. Remove the unsupported instruction to hold the TCP connection and
wait for a backend “Ready” signal, and replace it with the documented
operational procedure or explicitly state that no readiness gate is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8485432c-325e-417d-b096-26e702e803fc
📒 Files selected for processing (6)
README.mddocs/OPERATIONAL-DEPLOYMENT.mdexamples/snappipe-tunnel.servicesrc/main.rssrc/transport/tunnel.rstests/tunnel_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/snappipe-tunnel.service
- tests/tunnel_e2e.rs
- README.md
Reflows the read_to_string line for the new subject_public_path file (introduced in the round-2 fix to exercise distinct issuer vs subject keys). No functional change.
1 participant
Summary
Adds a transparent TCP-over-QUIC tunnel to SnapPipe so operators
running self-hosted relays can ship any TCP-based protocol (RDP,
raw SSH, internal database wire protocols, operator-defined
backends) through the existing identity-gated transport.
The tunnel reuses the existing
SignedTicket+TrustStore+RateLimitermachinery — no second auth surface — and runs on adedicated ALPN (
/snappipe/tunnel/0) so tunnel traffic stayson its own wire from regular relay traffic.
Surface added
src/transport/tunnel.rs(460 LOC):serve,connect_with, andconnectfunctions. Per-stream TCP↔QUIC bridge with bidirectionalcopy via
tokio::join!. NewTUNNEL_ALPNconstant distinct fromDEFAULT_ALPN.src/main.rs: newsnappipe tunnel {serve,connect}subcommand.examples/relay.sample.toml:quic_bind = "0.0.0.0:4443"(UDP/443is captured by HTTP/3 listeners via
SO_REUSEPORT; 4443 is thedefensible default outside the typical ISP drop-list).
examples/snappipe-tunnel.service(relay host skeleton),examples/snappipe-tunnel-client.service(trusted-peer skeleton),examples/snappipe-tunnel-client.ps1(Windows scheduled-taskinstaller).
tests/tunnel_e2e.rs: in-process TCP echo + tunnel round-tripover QUIC, plus ALPN-distinct and config-validation unit tests.
README.md/docs/OPERATIONAL-DEPLOYMENT.md: new sectiondocumenting topology, CLI examples, and firewall expectations.
All examples use generic placeholders (
<operator-keys>,<relay-public-host>,<tcp-backend-host:port>) so the publicdocs stay useful to fork operators without leaking per-deploy
IPs or paths.
0.2.1 → 0.3.0(new public CLI surface).Why this lives next to
relayReusing the existing relay ALPN would silently start routing
tunnel traffic through the regular relay code path. The dedicated
/snappipe/tunnel/0ALPN preserves the wire-level separation.Validation
Scope boundary
This PR ships the GENERIC transport feature. Operator-specific
deployment details (public IPs, internal paths, namespace names,
DNS, ticket TTLs, absolute-path conventions) deliberately stay
outside the public repository in the operator's private
deployment manifest. See:
~/<…>/_staging/MANIFEST-2026-07-26-snappipe-tunnel-private.md(operator-local split notes — what goes public, what stays
private, with rationale citing
stage-local-first+automation-risk-asymmetry)."Why these deploy notes live outside the repo" subsection of
docs/OPERATIONAL-DEPLOYMENT.md.Production caveats
stream. Intentional: keeps failure semantics simple.
exposing the relay publicly. The
snappipe tunnelclientcurrently trusts whatever the server presents.
quic_bind = 0.0.0.0:4443must be verified withss -lnu/ss -lntbefore locking in: some hosting panels occupynon-standard ports.
Checklist
public docs.
cargo test --releasegreen (70/70).Cargo.toml.Summary by CodeRabbit
snappipe tunnel serveandsnappipe tunnel connectCLI commands with tunnel-specific ALPN support.