auth: validate the DH server hello instead of trusting and panicking (ibx#276)

[userFRM]

Summary

• process_server_hello (src/auth/dh.rs) indexed fields[0] and fields[1] directly and decoded both with unwrap, and its three call sites passed &parts[2..], which itself panics when the message splits into fewer than three parts. All of it runs during connect, so a short or non-base64 hello aborted the process instead of failing the connection.
• It returns io::Result now. The call sites already had return Err(io::Error::new(...)) immediately above them, so there was somewhere for the failure to go.
• The server's public value is range-checked against [2, N-2] before the modpow. 0 and 1 pin the pre-master secret to a known constant, and the whole 104-byte key block derived from it follows — both AES keys, both IVs, both HMAC keys.

Why

The panics need no adversary; an ordinary malformed frame reaches them on the normal connect path.

The range check is defence in depth: every socket runs inside TLS with certificate validation enabled at all four construction sites, so this is not remotely exploitable against a correct TLS path. It matters because nothing downstream would notice a chosen key — the one consumer that does enforce its MAC would be verifying against the key the peer supplied. That is the reason for the layer to hold on its own rather than to lean on the one above it.

Closes #276.

What is NOT included

The issue notes that do_srp never verifies the server's M2 proof. That needs an M2 function srp.rs does not have, and belongs in its own change. Related: #275 covers the inbound frame MAC never being enforced.

Correction to the security rationale

The issue this closes, and this PR's first description, said every socket runs inside TLS with certificate validation, making the range check defence in depth. That is wrong. It holds for the auth connection only. connect_farm opens a plain TcpStream and runs the key exchange over it — the code's own comment calls it "raw TCP" — so the trading and historical-data farms have no TLS beneath them.

On those channels the DH exchange is the only thing between the client and an active intermediary. The range check removes the degenerate case, not the exposure: an in-range key from an intermediary is still accepted. I have corrected #276 and #275 as well, because both carried the same false premise and it understated them.

Known, and stated rather than hidden

• This is a source-breaking signature change. process_server_hello goes from () to io::Result<()> on an exported type. Every in-repo caller is updated, but a downstream caller using it as a tail expression would not compile. Representing "this hello is unusable" needs a fallible return, so the break is not avoidable — only relocatable to a parallel method, which I would rather not add without your call.
• The call sites' error propagation is not covered by test. Reverting all three to their old ignoring form leaves both new tests passing, because they call the helper directly; the sites live inside connect and need a socket.
• One diagnosis in the first message was wrong: reaching the 533 branch already proves at least two parts, so &parts[2..] was valid and the panic came from fields[0] alone. The .get(2..) is harmless but was not load-bearing.

Test plan

• cargo test --offline --lib — 804 passed. The 2 failures are config::expiry_tests::{named_zone_converts_with_dst, instant_round_trips_to_wire}, which fail on the base commit too: the host has no legacy timezone files (fixed separately in config: resolve the legacy timezone names IB states its times in (ibx#335) #336).
• cargo check --offline clean on every offline target: --lib, --features python, --bins, --examples, --test control_plane, --test scenarios, --test hot_loop_lifecycle.
• Mutation check: dropping the range check fails a_degenerate_public_value_is_refused; restoring the direct field indexing fails a_malformed_hello_is_an_error_not_a_panic. Both by name.
• The range test covers 0, 1, N-1 and N, and carries a positive control — N-2 is still accepted, so the refusals are not passing for want of a working path.

🤖 Generated with Claude Code

[userFRM]

Reviewed. The functional change holds — 0, 1, N-1 and N rejected, both endpoints accepted, malformed hellos returning errors, all three callers propagating — but two things in how I described it were wrong. Corrected in 580630b.

The security rationale was false, and it understated the issue. I wrote that every socket runs inside TLS with certificate validation, so this was defence in depth. That holds for the auth connection only. connect_farm opens a plain TcpStream and runs the key exchange over it — the code's own comment calls it "raw TCP". So on the trading and historical-data farms there is no TLS beneath this at all, and the DH exchange is the only thing between the client and an active intermediary. The range check removes the degenerate case, not the exposure. I have corrected #275 and #276 too, since both carried the same premise.

One diagnosis was wrong in the other direction. I said &parts[2..] panics when the message splits into fewer than three parts. Reaching the 533 branch already proves at least two, and slicing [2..] of a two-element slice is valid. The panic came from fields[0] alone. The .get(2..) is harmless but was not doing the work I credited it with.

Two things I am flagging rather than fixing, because they are yours to decide:

• This is a source-breaking signature change on an exported type. Every in-repo caller is updated, but a downstream caller using process_server_hello as a tail expression would not compile. Representing "this hello is unusable" needs a fallible return, so the break is not avoidable, only relocatable into a parallel method — which I would rather not add unasked.
• The call sites' propagation is not covered: reverting all three to their old ignoring form leaves both new tests passing, because they exercise the helper directly and the sites live inside connect, which needs a socket.

804 pass plus the two config::expiry_tests timezone failures. All offline targets clean.

[userFRM]

Second review agrees the code is merge-ready as it stands: neither guard can refuse a legitimate hello, both are mutation-killed by name, every error path fires before any state mutation so there is no half-keyed channel, and the DH round-trip tests still prove shared-secret agreement through the real generator path.

It confirmed the correction already pushed — the TLS claim was false — and sharpened two details worth having right:

• There are two TLS construction sites, not four. The four I counted were accept_invalid_certs: false settings, which is a different thing.
• The &parts[2..] slice panics below two parts, not three, and that state cannot reach it: all three sites gate on message type 533 first, and that type parses out of parts[1]. So the reachable abort was fields[0] and the two unwraps alone. The conclusion was right and the mechanism attribution was not; .get(2..) is belt and braces rather than the fix.

Both stated gaps stand: the signature change is source-breaking on an exported type, and the call sites' propagation is not covered because they live inside connect.

804 pass plus the two config::expiry_tests timezone failures. All offline targets clean.

[userFRM]

Closing this. It's in #409 along with the rest of the fork, which is easier to take in one piece than sixty separate branches.