Networking: test anchors for un-pinned architectural properties (SSRF rebinding, Connection: close, happy-path TLS) #483

New issue

Closed

opened 2026-05-13 17:03:56 +00:00 by navicore · 1 comment

navicore commented

2026-05-13 17:03:56 +00:00

Owner

Networking — test anchors for un-pinned architectural properties

Three test gaps from the PR1–PR5 reviews where the property holds in current code but no test would catch a regression. Reviewers flagged these as "deferrable but worth pinning" or "the strongest security win of the design with no test anchoring it."

Items

SSRF DNS-rebinding-closure regression test (deferred from PR4 #480)

"The architectural property — that addrs are validated then dialled without a second DNS lookup — is the actual security win here. … If a future refactor passes host: String to connect instead of addrs: Vec<IpAddr>, the property silently regresses with no test catching it."
— PR4 second-pass review, "Still open" section

Approach: add a #[cfg(test)] injection point in dns::resolve that returns scripted responses on successive calls. Test mocks the resolver to return a safe IP on the first call (which the SSRF check accepts) and a dangerous IP (e.g. 127.0.0.1) on the second. Then issue a request and assert that the dial went to the first IP — the validated one — not the second. A regression that passes host: String to connect and re-resolves there would fail this test loudly. Small.
Connection: close pool-eviction test (deferred from PR4 #480)
Today's integration test server (http_client/integration_tests.rs::spawn_test_server) only emits Connection: keep-alive. Would catch a future regression where the response's keep_alive=false is silently ignored on the pool release path.

Approach: add a CloseOnce variant to ServerMode that emits Connection: close and closes the socket after one response. Test issues two requests to the same host and asserts accept_count == 2 (proving the pool didn't try to reuse a doomed entry). ~15 lines on top of the existing ServerMode enum. Small.
Happy-path TLS integration test with same-process rustls server (deferred from PR3 #479)

"If you can stand up a same-process rustls server with a self-signed cert and a custom RootCertStore in the test (it's ~30 lines of glue), the happy-path coverage closes that gap without external network. Optional — but the longer the success path goes unexercised the more room for handshake-shape regressions to slip in."
— PR3 review

PR4's HTTPS coverage exercises real-world TLS implicitly (manual verification against https://example.com), but a hermetic test against a same-process rustls server with a fixture self-signed cert + custom RootCertStore would pin the handshake-shape against rustls upgrades. Needs:
- Fixture cert + key files in tests/fixtures/ or generated at test time.
- A cfg(test) path that lets the runtime use a non-default RootCertStore (custom CA) instead of webpki-roots.
- Same-process server glue using rustls server-side.
Medium.

Rough scope

One day. One PR. Mostly test code; the only production change is whatever harness needs to land for the rebinding test (probably a #[cfg(test)] injection point in dns::resolve) and the cfg(test) trust-root override for the TLS happy-path test.

## Networking — test anchors for un-pinned architectural properties Three test gaps from the PR1–PR5 reviews where the property holds in current code but no test would catch a regression. Reviewers flagged these as "deferrable but worth pinning" or "the strongest security win of the design with no test anchoring it." ### Items - **SSRF DNS-rebinding-closure regression test** (deferred from PR4 #480) > "The architectural property — that `addrs` are validated *then* dialled without a second DNS lookup — is the actual security win here. … If a future refactor passes `host: String` to connect instead of `addrs: Vec<IpAddr>`, the property silently regresses with no test catching it." > — PR4 second-pass review, "Still open" section Approach: add a `#[cfg(test)]` injection point in `dns::resolve` that returns scripted responses on successive calls. Test mocks the resolver to return a safe IP on the first call (which the SSRF check accepts) and a dangerous IP (e.g. `127.0.0.1`) on the second. Then issue a request and assert that the dial went to the *first* IP — the validated one — not the second. A regression that passes `host: String` to connect and re-resolves there would fail this test loudly. **Small.** - **`Connection: close` pool-eviction test** (deferred from PR4 #480) Today's integration test server (`http_client/integration_tests.rs::spawn_test_server`) only emits `Connection: keep-alive`. Would catch a future regression where the response's `keep_alive=false` is silently ignored on the pool release path. Approach: add a `CloseOnce` variant to `ServerMode` that emits `Connection: close` and closes the socket after one response. Test issues two requests to the same host and asserts `accept_count == 2` (proving the pool didn't try to reuse a doomed entry). ~15 lines on top of the existing `ServerMode` enum. **Small.** - **Happy-path TLS integration test with same-process rustls server** (deferred from PR3 #479) > "If you can stand up a same-process rustls server with a self-signed cert and a custom `RootCertStore` in the test (it's ~30 lines of glue), the happy-path coverage closes that gap without external network. Optional — but the longer the success path goes unexercised the more room for handshake-shape regressions to slip in." > — PR3 review PR4's HTTPS coverage exercises real-world TLS implicitly (manual verification against `https://example.com`), but a hermetic test against a same-process rustls server with a fixture self-signed cert + custom `RootCertStore` would pin the handshake-shape against rustls upgrades. Needs: - Fixture cert + key files in `tests/fixtures/` or generated at test time. - A `cfg(test)` path that lets the runtime use a non-default `RootCertStore` (custom CA) instead of `webpki-roots`. - Same-process server glue using rustls server-side. **Medium.** ### Rough scope One day. One PR. Mostly test code; the only production change is whatever harness needs to land for the rebinding test (probably a `#[cfg(test)]` injection point in `dns::resolve`) and the `cfg(test)` trust-root override for the TLS happy-path test.

navicore referenced this issue from a commit

2026-05-14 12:25:11 +00:00

● All three test anchors landed. CI fully green.

navicore referenced this issue

2026-05-14 12:26:17 +00:00

● All three test anchors landed. CI fully green. #487

navicore commented

2026-05-14 12:27:51 +00:00

Author

Owner

#487

https://git.navicore.tech/navicore/patch-seq/pulls/487