M6: Reject non-loopback gRPC bind addresses; add a consolidated auth-coverage canary test #201

New Issue

2026-07-28T14:04:47Z

KofTwentyTwo commented

2026-07-28 14:04:47 +00:00

(Migrated from github.com)

Context

Story for milestone M6. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work. crates/nunciod/src/grpc.rs::serve (grpc.rs:1113-1128) binds the nuncio.v1 gRPC server's TcpListener to whatever address grpc_addr_from_env() (crates/nuncio-proto/src/addr.rs:19-21, re-exported at grpc.rs:55) resolves to — by default 127.0.0.1:9420, but overridable via the NUNCIO_GRPC_ADDR env var with no validation at all. Verified in the tree: the doc comment on serve (grpc.rs:1110-1112) says addr "MUST be a loopback address... callers are responsible for passing loopback-only addresses" — this is a comment-only convention, not an enforced invariant; setting NUNCIO_GRPC_ADDR=0.0.0.0:9420 (or any routable interface) today binds the daemon's unencrypted, bearer-token-only gRPC API to the network with zero warning or rejection. Separately, serve_on_listener_with_overrides (grpc.rs:1175-1245) mounts six services (System, Accounts, Mail, Filters, Export, Audit), each wrapped in its own BearerAuthInterceptor — verified correct today — but the only thing enforcing "every future service must also be wrapped" is a doc comment (grpc.rs:1134-1141); there is no single test whose failure would flag a newly-added, un-intercepted service (reconciles #165).

Goal

Make serve fail closed with a clear error when asked to bind a non-loopback address, so a misconfigured NUNCIO_GRPC_ADDR can never silently expose the daemon's API to the network.
Add one consolidated "canary" test that dials every currently-mounted service without a bearer token and asserts Unauthenticated for all of them in a single place, so any PR that adds a 7th service without a matching entry here is an obvious, reviewable gap rather than a silent one. (Rust/tonic cannot introspect Server::builder()'s registered service count at runtime without unsafe, which this workspace forbids — so this is a review-time guard, not a runtime-enforced one; do not attempt an unsafe reflection hack to make it "fully automatic".)

Files to touch / patterns to mirror

crates/nunciod/src/grpc.rs:57-72 (GrpcServeError) — add a new variant, e.g. NonLoopbackAddress(String), mirroring the existing Bind { addr, source } variant's #[error(...)] style.
crates/nunciod/src/grpc.rs:1113-1128 (pub async fn serve) — before calling TcpListener::bind(addr), parse addr with addr.parse::<std::net::SocketAddr>() and reject (return Err(GrpcServeError::NonLoopbackAddress(addr.to_string()))) if parsing fails OR socket_addr.ip().is_loopback() is false. std::net::IpAddr::is_loopback() is stable and covers both 127.0.0.0/8 (IPv4) and ::1 (IPv6) correctly — no manual CIDR math needed here (unlike M6-S6's webhook SSRF checks, which need more than is_loopback()).
crates/nunciod/src/grpc.rs:1148-1245 (serve_on_listener / serve_on_listener_with_overrides) — these take an already-bound TcpListener (used by tests to bind ephemeral 127.0.0.1:0 ports, grpc.rs:1312-1314), so they intentionally do NOT re-validate the address — only serve (the production entry point called from crates/nunciod/src/main.rs:197-206) does. Do not add the loopback check to the listener-based functions; doing so would break the test harness pattern documented at grpc.rs:1143-1147.
crates/nunciod/src/grpc.rs:1247-1250-ish (#[cfg(test)] mod tests) — add the canary test described in Acceptance Criteria, placed near the existing serve_binds_and_starts_on_a_valid_loopback_address / serve_fails_closed_when_bind_address_is_invalid tests (grpc.rs:1443, 1472).
crates/nunciod/src/grpc.rs:1130-1141 (doc comment on serve_on_listener) — extend this doc comment to also point at the new canary test by name, so a future author adding a service is told exactly two places to touch: the add_service chain in serve_on_listener_with_overrides, and the canary test.

Implementation steps

Add GrpcServeError::NonLoopbackAddress(String) with an error message that names the rejected address and states it must be loopback.
In serve, parse and validate addr before binding; return the new error variant on failure. Keep the existing Bind error path unchanged for the case where the address IS loopback but binding still fails (port in use, etc.).
Add a unit test (mirror serve_fails_closed_when_bind_address_is_invalid, grpc.rs:1472) that calls serve with a non-loopback address (e.g. "0.0.0.0:0" or a routable-looking address like "10.0.0.5:9420") and asserts Err(GrpcServeError::NonLoopbackAddress(_)).
Add a test that a loopback IPv6 address ("[::1]:0") is accepted (parses + passes the loopback check) to prove the fix isn't IPv4-only.
Write the consolidated canary test: spawn a test server via the existing spawn_test_server helper (grpc.rs:1264-1274), then for each of the six services (System, Accounts, Mail, Filters, Export, Audit) connect a client with NO authorization metadata and call exactly one representative RPC per service, asserting Code::Unauthenticated for every one — in one test function, in one place, distinct from (and in addition to) the existing per-service exhaustive tests at grpc.rs:1357 (System), 1662/1685 (Accounts), 1943 (Mail), 2654 (Filters), 3018 (Export), 3199 (Audit). Name it clearly, e.g. all_mounted_services_reject_unauthenticated_calls, and give it a doc comment stating explicitly that it is the single place a reviewer should check against serve_on_listener_with_overrides's add_service calls whenever a new service is added.
Update the doc comment on serve_on_listener per the Files list above.

Acceptance criteria

serve("0.0.0.0:9420", ...) (and any other non-loopback address) returns Err(GrpcServeError::NonLoopbackAddress(_)) without ever calling TcpListener::bind.
serve("127.0.0.1:0", ...) and serve("[::1]:0", ...) both still succeed (binding, not just address validation) — loopback IPv4 and IPv6 both work.
A single consolidated test dials all six currently-mounted services (System, Accounts, Mail, Filters, Export, Audit) with no bearer token and asserts Code::Unauthenticated for one RPC on each, in one test function.
The existing per-service exhaustive unauth-rejection tests (grpc.rs:1357,1372,1544,1662,1685,1943,2654,3018,3199) are untouched and still pass.
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m6-grpc-bind-hardening off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

Not a [PROTO] story. Reconciles #165. Touches crates/nunciod/src/grpc.rs, which M6-S2 (zeroize) also touches (different regions of the file — AccountsGrpcService::add_account vs. serve/tests — low conflict risk, but coordinate with the PO on landing order). Size: M.

## Context Story for milestone M6. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `crates/nunciod/src/grpc.rs::serve` (`grpc.rs:1113-1128`) binds the `nuncio.v1` gRPC server's `TcpListener` to whatever address `grpc_addr_from_env()` (`crates/nuncio-proto/src/addr.rs:19-21`, re-exported at `grpc.rs:55`) resolves to — by default `127.0.0.1:9420`, but overridable via the `NUNCIO_GRPC_ADDR` env var with **no validation at all**. Verified in the tree: the doc comment on `serve` (`grpc.rs:1110-1112`) says `addr` "MUST be a loopback address... callers are responsible for passing loopback-only addresses" — this is a comment-only convention, not an enforced invariant; setting `NUNCIO_GRPC_ADDR=0.0.0.0:9420` (or any routable interface) today binds the daemon's unencrypted, bearer-token-only gRPC API to the network with zero warning or rejection. Separately, `serve_on_listener_with_overrides` (`grpc.rs:1175-1245`) mounts six services (`System`, `Accounts`, `Mail`, `Filters`, `Export`, `Audit`), each wrapped in its own `BearerAuthInterceptor` — verified correct today — but the only thing enforcing "every future service must also be wrapped" is a doc comment (`grpc.rs:1134-1141`); there is no single test whose failure would flag a newly-added, un-intercepted service (reconciles #165). ## Goal 1. Make `serve` fail closed with a clear error when asked to bind a non-loopback address, so a misconfigured `NUNCIO_GRPC_ADDR` can never silently expose the daemon's API to the network. 2. Add one consolidated "canary" test that dials every currently-mounted service without a bearer token and asserts `Unauthenticated` for all of them in a single place, so any PR that adds a 7th service without a matching entry here is an obvious, reviewable gap rather than a silent one. (Rust/tonic cannot introspect `Server::builder()`'s registered service count at runtime without `unsafe`, which this workspace forbids — so this is a review-time guard, not a runtime-enforced one; do not attempt an `unsafe` reflection hack to make it "fully automatic".) ## Files to touch / patterns to mirror - `crates/nunciod/src/grpc.rs:57-72` (`GrpcServeError`) — add a new variant, e.g. `NonLoopbackAddress(String)`, mirroring the existing `Bind { addr, source }` variant's `#[error(...)]` style. - `crates/nunciod/src/grpc.rs:1113-1128` (`pub async fn serve`) — before calling `TcpListener::bind(addr)`, parse `addr` with `addr.parse::<std::net::SocketAddr>()` and reject (return `Err(GrpcServeError::NonLoopbackAddress(addr.to_string()))`) if parsing fails OR `socket_addr.ip().is_loopback()` is `false`. `std::net::IpAddr::is_loopback()` is stable and covers both `127.0.0.0/8` (IPv4) and `::1` (IPv6) correctly — no manual CIDR math needed here (unlike M6-S6's webhook SSRF checks, which need more than `is_loopback()`). - `crates/nunciod/src/grpc.rs:1148-1245` (`serve_on_listener` / `serve_on_listener_with_overrides`) — these take an already-bound `TcpListener` (used by tests to bind ephemeral `127.0.0.1:0` ports, `grpc.rs:1312-1314`), so they intentionally do NOT re-validate the address — only `serve` (the production entry point called from `crates/nunciod/src/main.rs:197-206`) does. Do not add the loopback check to the listener-based functions; doing so would break the test harness pattern documented at `grpc.rs:1143-1147`. - `crates/nunciod/src/grpc.rs:1247-1250`-ish (`#[cfg(test)] mod tests`) — add the canary test described in Acceptance Criteria, placed near the existing `serve_binds_and_starts_on_a_valid_loopback_address` / `serve_fails_closed_when_bind_address_is_invalid` tests (`grpc.rs:1443`, `1472`). - `crates/nunciod/src/grpc.rs:1130-1141` (doc comment on `serve_on_listener`) — extend this doc comment to also point at the new canary test by name, so a future author adding a service is told exactly two places to touch: the `add_service` chain in `serve_on_listener_with_overrides`, and the canary test. ## Implementation steps 1. Add `GrpcServeError::NonLoopbackAddress(String)` with an error message that names the rejected address and states it must be loopback. 2. In `serve`, parse and validate `addr` before binding; return the new error variant on failure. Keep the existing `Bind` error path unchanged for the case where the address IS loopback but binding still fails (port in use, etc.). 3. Add a unit test (mirror `serve_fails_closed_when_bind_address_is_invalid`, `grpc.rs:1472`) that calls `serve` with a non-loopback address (e.g. `"0.0.0.0:0"` or a routable-looking address like `"10.0.0.5:9420"`) and asserts `Err(GrpcServeError::NonLoopbackAddress(_))`. 4. Add a test that a loopback IPv6 address (`"[::1]:0"`) is accepted (parses + passes the loopback check) to prove the fix isn't IPv4-only. 5. Write the consolidated canary test: spawn a test server via the existing `spawn_test_server` helper (`grpc.rs:1264-1274`), then for each of the six services (`System`, `Accounts`, `Mail`, `Filters`, `Export`, `Audit`) connect a client with NO authorization metadata and call exactly one representative RPC per service, asserting `Code::Unauthenticated` for every one — in one test function, in one place, distinct from (and in addition to) the existing per-service exhaustive tests at `grpc.rs:1357` (`System`), `1662`/`1685` (`Accounts`), `1943` (`Mail`), `2654` (`Filters`), `3018` (`Export`), `3199` (`Audit`). Name it clearly, e.g. `all_mounted_services_reject_unauthenticated_calls`, and give it a doc comment stating explicitly that it is the single place a reviewer should check against `serve_on_listener_with_overrides`'s `add_service` calls whenever a new service is added. 6. Update the doc comment on `serve_on_listener` per the Files list above. ## Acceptance criteria - [ ] `serve("0.0.0.0:9420", ...)` (and any other non-loopback address) returns `Err(GrpcServeError::NonLoopbackAddress(_))` without ever calling `TcpListener::bind`. - [ ] `serve("127.0.0.1:0", ...)` and `serve("[::1]:0", ...)` both still succeed (binding, not just address validation) — loopback IPv4 and IPv6 both work. - [ ] A single consolidated test dials all six currently-mounted services (`System`, `Accounts`, `Mail`, `Filters`, `Export`, `Audit`) with no bearer token and asserts `Code::Unauthenticated` for one RPC on each, in one test function. - [ ] The existing per-service exhaustive unauth-rejection tests (`grpc.rs:1357,1372,1544,1662,1685,1943,2654,3018,3199`) are untouched and still pass. - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m6-grpc-bind-hardening` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes Not a [PROTO] story. Reconciles #165. Touches `crates/nunciod/src/grpc.rs`, which M6-S2 (zeroize) also touches (different regions of the file — `AccountsGrpcService::add_account` vs. `serve`/tests — low conflict risk, but coordinate with the PO on landing order). Size: M.

KofTwentyTwo commented

2026-07-30 16:25:34 +00:00

(Migrated from github.com)

Delivered via PR #240 (squash-merged to dev). serve now fails closed with GrpcServeError::NonLoopbackAddress when NUNCIO_GRPC_ADDR is non-loopback (parses SocketAddr, rejects before TcpListener::bind; covers IPv4 127.0.0.0/8 and IPv6 ::1 via is_loopback()) — a misconfigured bind can no longer silently expose the API. Added a single consolidated canary test all_mounted_services_reject_unauthenticated_calls that dials all EIGHT mounted services (System/Accounts/Mail/Filters/Export/Audit/Calendar/Contacts — corrected from the issue's stale six) with no bearer token and asserts Unauthenticated for each, with a doc comment naming it the review checkpoint for any new service. CI fully green. Reconciles #165.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#201