Secure connection with noise encrypted traffic

[kaichaosun]

Fix #33

[msotnikov]

Read through the diff carefully. The direction looks right and the implementation is careful: NK with the server's static key pinned, fresh session keys per connection, a per-tunnel token as client authentication, an ack byte so a rejection surfaces immediately instead of later as a mysteriously dead proxied request, constant-time token comparison, and the handshake moved off the accept loop. A few things below, three of which I think are worth addressing before merge.

Note: this is a read-only review — I did not get a cargo test --workspace run to complete, so nothing here is backed by an actual build.

Blocking

1. server/src/state.rs — in-flight handshakes are unbounded

The handshake now runs in its own tokio::spawn, and nothing caps how many run concurrently. max_sockets only bounds the pool. The pre-check

if sockets.lock().await.len() >= max_sockets as usize {

is almost always false at accept time, because the pool is still empty while the handshakes are in progress. So anyone who knows a tunnel's port can open thousands of connections and hold a file descriptor plus snow state for up to HANDSHAKE_TIMEOUT (10s) each, per tunnel. Before this PR the accept loop was serial and an excess socket was dropped immediately.

A Semaphore sized at something like max_sockets * 2, with the permit acquired before the spawn and held until the handshake resolves, would restore the bound.

2. Trusting the server key means trusting the API channel

NK pins the server's static key, but the client learns that key from the registration response. The API is plain HTTP by default — in the CLI, in server/tests/e2e.rs, and in the README examples. An active man-in-the-middle on that path can substitute its own server_public_key and session_token and terminate the "encrypted tunnel" itself.

So the guarantee is real against a passive observer on the tunnel port, but not against an active attacker, and the README currently says "all tunnel connections between client and server are encrypted" without that qualifier. At minimum this deserves an explicit note that the API must be served over TLS for the handshake to mean anything. Better would be an option to supply the server's public key out of band (a ClientConfig field / CLI flag), so the pin doesn't depend on the channel it arrives over.

3. The session token isn't bound to whoever registered the subdomain

ClientManager::put still inserts unconditionally (server/src/api.rs:67). With require_auth = false, anyone can GET /<someone-elses-subdomain>: that allocates a fresh Client with a new port and a new token and replaces the map entry, the previous Client is dropped along with its pooled sockets, and public traffic goes to whoever registered last.

The doc comment on secure_accept says the token is "what stops an arbitrary peer that found the port from receiving proxied traffic" — that is true, and it is a genuine improvement — but the front door is still open. Since this is the security PR, it seems worth either fixing here or calling out as explicitly known-open.

Worth considering

4. The keypair is generated per process (server/src/lib.rs)

Every restart invalidates every live tunnel's pinned key. Clients do recover through re-registration, but the handshake-failure path sleeps 10s per attempt and has to wait out reregister_after before the key is refreshed. Persisting the keypair (config or file) would avoid that; otherwise the restart cost is worth documenting.

5. secure_connect reads the ack byte but ignores its value (client/src/lib.rs)

The server always writes 1, so the client currently accepts any byte. Checking it is one line and keeps the door open for a meaningful ack later.

6. A rejection is indistinguishable from a network drop

A wrong token gets a silent close, so the client loops on Tunnel handshake failed every 10s with no way to tell "the server refused me" from "the network is down". A single reason byte before closing would make this much easier to diagnose in the field.

7. unauthenticated_tunnel_connections_are_rejected has no timeout

If the server ever fails to close the garbage connection, the read hangs and the test dies on the harness timeout rather than failing with its own message. Wrapping the reads in tokio::time::timeout would keep the failure legible.

8. free_port() is bind-then-drop

It is a TOCTOU: the port is released and re-bound later on another thread. Two tests in the same binary can collide, and the server thread .unwrap()s the bind, so a collision panics a detached thread and the test then fails 10s later with an unrelated "server did not start". Passing the listener through, or at least not unwrapping inside the thread, would make the real cause visible.

Nits

• start_tunnel_connections does Arc::new(server.clone()) even though the caller owns current_info — the key and token get copied once per round for no reason.
• token_matches is hand-rolled. subtle::ConstantTimeEq is already in the dependency tree transitively through snow, and it exists precisely so the compiler cannot optimize the fold away.
• The README section could mention that the public-facing side (browser to proxy_port) is unaffected and still plain HTTP unless fronted by TLS. Readers are likely to conflate the two.
• server/Cargo.toml has localtunnel-client = { path = "../client" } with no version. Fine for publishing since dev-dependencies are stripped, just worth being aware of.
• Version bumps to 0.2.0 are consistent across the workspace, and cli was moved off the stale 0.1.6 requirement.

Interaction with #47

Both PRs rewrite tunnel_one_connection and proxy_through in client/src/lib.rs: this one changes the stream type to NoiseStream<TcpStream>, #47 replaces copy_bidirectional with two io::copy halves plus an explicit shutdown. They will conflict textually, and beyond that it is worth checking whether NoiseStream::poll_shutdown propagates the write-half close down to the underlying TcpStream — if it does not, half-close signalling to the local service stops working once the tunnel is wrapped. Happy to rebase #47 on top of this one if this lands first.