Skip to content

fix: the four self-contained findings left from the logic-bug audit - #175

Merged
passcod merged 4 commits into
mainfrom
fix/audit-easy-sweep
Sep 9, 2026
Merged

passcod merged 4 commits into
mainfrom
fix/audit-easy-sweep

Conversation

@passcod

@passcod passcod commented Sep 9, 2026

Copy link
Copy Markdown
Member

🤖 The logic-bug audit had 23 critical/high findings; the eight theme PRs closed 16 of them, all the ones that were instances of a recurring pattern. Seven single-site findings were left over. These are the four that are self-contained; the other three are on cards.

Each commit changes the spec first where the spec was silent, then the code, then tests.

An unresolvable listen interface silently dropped the management plane

resolve_oi_addrs warned and continued when a named interface had no addresses, so an interface that had not come up yet — a boot-time race — left the daemon binding whichever subset resolved, or nothing at all when that was the only interface. It then logged seedling ready with no OI listener, which from outside is indistinguishable from a daemon that is up but unreachable. The flag's own doc comment already called this fatal.

The decision moved into select_oi_addrs, which receives the host's interface list instead of reading it, so it is testable without a process exit. That also drops a get_if_addrs call per interface.

The WebTransport certificate rotated only once it had expired

w[wt.cert.rotation] already required rotating before expiry, but the swap was gated on not_after <= now and reconsidered hourly, so the endpoint served an expired certificate for up to a full tick. Advertising both hashes across the overlap does not help — a browser enforces the validity period even when the hash arrives through serverCertificateHashes, so every new session failed TLS for as long as the window lasted.

The tick interval moved into wt_cert as ROTATION_TICK so the swap margin's dependency on it is a test rather than a comment.

The advertised WebTransport port ignored explicit listen addresses

The advertised port was args.wt_port with no explicit addresses and DEFAULT_WT_PORT otherwise — so under --wt-listen or --listen, where that argument is not the port anything binds, clients were told 7893 regardless. It reaches them twice, in wt_url and in the CSP connect-src origin, and both were wrong together, leaving the documented explicit-address combinations with no working WebTransport at all.

It now derives from the addresses actually bound. Addresses disagreeing on a port are rejected at startup, since one port reaches the client and picking one would misadvertise the rest.

An expired certificate was served in preference to a valid one

find_active_for_hostname matched on state = 'active' alone. The exact-hostname path takes the highest id, so a newer expired row shadowed an older valid one and the SAN-coverage scan beneath it never ran — the proxy got the expired certificate and nothing on the serve path checked notAfter.

Both paths now skip expired rows. A row with no recorded not_after is still served, because unparsed and expired are not the same thing. The sibling matcher in state keeps its behaviour, since the renewal scheduler reaches an expiring cert through it; its comment now records the divergence instead of claiming the two mirror each other.

…reduction

`resolve_oi_addrs` warned and continued when a named interface had no
addresses, so an interface that had not come up yet — a boot-time race —
left the daemon binding whichever subset did resolve, or nothing at all
when that was the only interface. It then logged `seedling ready` with no
management plane, which looks identical from outside to a daemon that is
up but unreachable. The flag's own documentation already called this
fatal.

The decision moves into `select_oi_addrs`, which takes the host's
interface list rather than reading it, so it is testable without a
process exit; `resolve_oi_addrs` keeps the I/O and the `fatal!`. That
also drops a `get_if_addrs` call per named interface.
`w[wt.cert.rotation]` requires rotating before expiry, but the swap was
gated on `not_after <= now` and reconsidered hourly, so the endpoint
served an already-expired certificate for up to a full tick. Advertising
both hashes during the overlap does not help: a browser enforces the
validity period even when the hash arrives via
`serverCertificateHashes`, so every new session failed TLS for the
duration.

The swap now happens `SWAP_LOOKAHEAD` ahead of expiry, and the tick
interval moves into `wt_cert` as `ROTATION_TICK` so the margin's
dependency on it is checked by a test rather than asserted in a comment.
Two existing tests primed `next` with a not_after 60 s out, which is
inside the new swap window; they now use a time clear of it, and the
60-second case became the test that the swap pre-empts expiry.
@github-code-quality

github-code-quality Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript, Rust

TypeScript / code-coverage/vitest

The overall line coverage in commit 9fd4d89 in the fix/audit-easy-sweep branch remains at 66%, unchanged from commit cc93403 in the main branch.

Rust / code-coverage/rust

The overall line coverage in commit 9fd4d89 in the fix/audit-easy-sweep branch remains at 60%, unchanged from commit cc93403 in the main branch.

Show a line coverage summary of the most impacted files.
File main cc93403 fix/audit-easy-sweep 9fd4d89 +/-
crates/core/src/oi/server.rs 60% 59% -1%
crates/core/src...me/tls/store.rs 92% 92% 0%
crates/web/src/wt_cert.rs 97% 98% +1%
crates/daemon/src/main.rs 0% 8% +8%
crates/web/src/main.rs 26% 37% +11%

Updated September 09, 2026 11:17 UTC

…ns on

The advertised port was `args.wt_port` when no explicit addresses were
given and `DEFAULT_WT_PORT` otherwise — so with `--wt-listen` or
`--listen`, where the argument is not the port anything binds, clients
were handed 7893 regardless. That reaches them twice: in `wt_url` from
`POST /connect`, and in the CSP's `connect-src` origin. Both were wrong
together, so the documented explicit-address combinations had no working
WebTransport at all.

It now derives from the addresses actually bound. Addresses that disagree
on a port are rejected at startup, since only one port reaches the
client and picking one would misadvertise the others.
`find_active_for_hostname` matched on `state = 'active'` alone. Because
the exact-hostname path takes the highest id, a newer expired row
shadowed an older one that was still valid, and the SAN-coverage scan
below it never ran — so the proxy was handed the expired certificate and
nothing on the serve path checked `notAfter`.

Both paths now skip rows whose expiry has passed. A row with no recorded
`not_after` is still served: unparsed and expired are not the same, and
conflating them would withhold a usable cert.

The sibling matcher in `state` keeps its behaviour — the renewal
scheduler reaches an expiring cert through it, so filtering there would
stop renewals — and its comment now says so rather than claiming the two
mirror each other. The only other caller, the Tailscale fetch guard,
already fell through on an expired cert, so it is unaffected.
@passcod
passcod force-pushed the fix/audit-easy-sweep branch from ae4051d to 9fd4d89 Compare September 9, 2026 11:15
@passcod
passcod enabled auto-merge September 9, 2026 11:18
@passcod
passcod added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit 7df84ad Sep 9, 2026
14 checks passed
@passcod
passcod deleted the fix/audit-easy-sweep branch September 9, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant