|
| 1 | +# S15 A3 builder design — threat-model defences + observability |
| 2 | + |
| 3 | +Branch `quic-s15-a3-builder` (from `034ddf30`). Implements design §A3 |
| 4 | +(`audit/quic/s15-design.md` ~line 739) for the Mode A QUIC passthrough |
| 5 | +datapath. Plan-approval gate per A2 discipline: NO `.rs` edits until the |
| 6 | +lead approves this doc. |
| 7 | + |
| 8 | +## 0. What is ALREADY done (do not re-implement) |
| 9 | + |
| 10 | +Reading the current tree (`034ddf30`) the following A3 items are already |
| 11 | +landed and only need test/coverage + audit-throttle wiring: |
| 12 | + |
| 13 | +- **`min_client_dcid_len` floor** — `PassthroughConfig.min_client_dcid_len` |
| 14 | + (lb-config/src/lib.rs:959, `serde(default)` = 8, `validate` range |
| 15 | + `8..=20` at :1626), `PassthroughParams.min_client_dcid_len` |
| 16 | + (passthrough.rs:99, default 8 at :149), enforced in `handle_initial` |
| 17 | + (passthrough.rs:581-589 — drops below-floor Initials), and wired |
| 18 | + config→params in `spawn_passthrough` (main.rs:1044). **No source change |
| 19 | + needed; A3 only adds the table-driven unit test for it.** |
| 20 | +- **`mint_retry` knob** + `strict_source_binding` + `audit_throttle_window` |
| 21 | + config/params/wiring — all present. |
| 22 | + |
| 23 | +So the residual A3 impl scope is: **(1) Prometheus metrics**, **(2) audit- |
| 24 | +log throttle mechanism** (the window is a knob but nothing throttles yet), |
| 25 | +**(3) unit tests** to lift passthrough.rs coverage to ≥80% and cover each |
| 26 | +defence. |
| 27 | + |
| 28 | +## 1. Prometheus metrics — `quic_passthrough_*` |
| 29 | + |
| 30 | +### 1.1 Registration site & pattern |
| 31 | + |
| 32 | +Follow the **`XdpMetrics`** template (lb-observability/src/xdp_metrics.rs): |
| 33 | +a subsystem struct of `prometheus` handles with a |
| 34 | +`register(&MetricsRegistry) -> Result<Self, MetricsError>` constructor that |
| 35 | +uses the existing get-or-create `counter()` / `gauge()` methods |
| 36 | +(lb-observability/src/lib.rs:144,335). No new registration pattern; no |
| 37 | +`lazy_static`/`once_cell`. |
| 38 | + |
| 39 | +New file **`crates/lb-observability/src/passthrough_metrics.rs`**, exported |
| 40 | +from lib.rs as `pub mod passthrough_metrics;` + a `pub use` of the struct |
| 41 | +(mirrors how `xdp_metrics` is surfaced — verify exact re-export shape and |
| 42 | +match it): |
| 43 | + |
| 44 | +```rust |
| 45 | +#[derive(Clone)] |
| 46 | +pub struct PassthroughMetrics { |
| 47 | + pub flows: IntGauge, // current dispatch-table size |
| 48 | + pub flows_evicted_total: IntCounter, // LRU evictions |
| 49 | + pub retry_minted_total: IntCounter, // Retry packets minted |
| 50 | + pub retry_rejected_total: IntCounter, // token verify failures |
| 51 | + pub header_parse_errors_total: IntCounter, // public-header parse failures |
| 52 | + pub backend_socket_errors_total: IntCounter, // backend send/recv errors |
| 53 | +} |
| 54 | +impl PassthroughMetrics { |
| 55 | + pub fn register(reg: &MetricsRegistry) -> Result<Self, MetricsError> { … } |
| 56 | +} |
| 57 | +``` |
| 58 | + |
| 59 | +Names exactly as the task spec lists them. Help strings one line each. |
| 60 | +The gauge + 5 counters are pre-seeded to 0 by virtue of get-or-create |
| 61 | +(gauge starts 0; counters start 0) so `/metrics` shows the rows from |
| 62 | +spawn — matching the XdpMetrics pre-seed intent. |
| 63 | + |
| 64 | +### 1.2 lb-quic → lb-observability dependency |
| 65 | + |
| 66 | +`PassthroughMetrics` holds raw `prometheus` `IntGauge`/`IntCounter` |
| 67 | +handles. To bump them at the event sites in lb-quic, lb-quic must see the |
| 68 | +type. Dependency direction is **acyclic**: lb-observability depends on |
| 69 | +lb-security + lb-l4-xdp (NOT lb-quic), so `lb-quic → lb-observability` is |
| 70 | +safe (confirmed by reading both Cargo.toml). Add to |
| 71 | +`crates/lb-quic/Cargo.toml`: |
| 72 | + |
| 73 | +```toml |
| 74 | +lb-observability = { path = "../lb-observability" } |
| 75 | +``` |
| 76 | + |
| 77 | +`PassthroughParams` gains: |
| 78 | + |
| 79 | +```rust |
| 80 | +/// Observability handles. `None` in unit tests / when the listener is |
| 81 | +/// spawned without a registry; the event sites become no-ops. |
| 82 | +pub metrics: Option<lb_observability::passthrough_metrics::PassthroughMetrics>, |
| 83 | +``` |
| 84 | + |
| 85 | +Default `None` in `PassthroughParams::new`. `RouterCtx` carries the |
| 86 | +`Option<PassthroughMetrics>` (cloned in; handles are Arc-backed/cheap). |
| 87 | +A tiny helper macro/fn keeps the call sites clean, e.g. |
| 88 | +`if let Some(m) = &ctx.params.metrics { m.retry_minted_total.inc(); }`. |
| 89 | + |
| 90 | +### 1.3 Event-site instrumentation points (file:line on current tree) |
| 91 | + |
| 92 | +| Metric | Site | Action | |
| 93 | +|---|---|---| |
| 94 | +| `header_parse_errors_total` | `handle_inbound` parse `Err` arm, passthrough.rs:524-527 | `.inc()` on parse failure | |
| 95 | +| `retry_minted_total` | `handle_initial` after successful `send_to(&out, from)` of the Retry, passthrough.rs:628-630 | `.inc()` once the Retry is sent | |
| 96 | +| `retry_rejected_total` | `handle_initial` token-verify `Err` arm, passthrough.rs:638-641 | `.inc()` on verify failure | |
| 97 | +| `flows` (gauge) | on insert (passthrough.rs:701) `+= n`; on eviction (`evict_oldest`, :507-512) `-= removed` | keep gauge == dispatch-table size; set to `ctx.table.len()` after each mutation for self-correction | |
| 98 | +| `flows_evicted_total` | `evict_oldest` after removing victim keys, passthrough.rs:516 | `.inc_by(removed as u64)` (or `.inc()` per evicted flow — see Q1) | |
| 99 | +| `backend_socket_errors_total` | forward-pump send `Err` (passthrough.rs:711-713) + `reverse_pump` recv `Err` (:730-732) + reverse send `Err` (:755) + backend `bind`/`connect` `Err` (:667-674) | `.inc()` on each backend socket error | |
| 100 | + |
| 101 | +`flows` gauge: simplest correct approach is to `.set(ctx.table.len() as |
| 102 | +i64)` after each table insert and after each `evict_oldest` — avoids |
| 103 | +drift from the 1-or-2-keys-per-flow asymmetry. (The gauge counts |
| 104 | +dispatch-table entries, consistent with `flows_len()` and the verify-gate |
| 105 | +(iii) assertion that the gauge stays at 1 for a single migrated flow — |
| 106 | +NOTE: gate (iii) expects the gauge == flow count; a single flow with 2 |
| 107 | +keys would read 2. **Q2 below.**) |
| 108 | + |
| 109 | +### 1.4 main.rs wiring |
| 110 | + |
| 111 | +In `spawn_passthrough` (main.rs:1034) add a `metrics: &Arc<MetricsRegistry>` |
| 112 | +parameter (the registry built at main.rs:1682 is in scope at the |
| 113 | +call-site main.rs:1948). Register once and thread into params: |
| 114 | + |
| 115 | +```rust |
| 116 | +let pt_metrics = lb_observability::passthrough_metrics::PassthroughMetrics::register(metrics) |
| 117 | + .map_err(|e| anyhow::anyhow!("passthrough metrics: {e}"))?; |
| 118 | +params.metrics = Some(pt_metrics); |
| 119 | +``` |
| 120 | + |
| 121 | +Pass `&metrics` at the call site (main.rs:1948). |
| 122 | + |
| 123 | +## 2. Audit-log throttle (one line per window, not per event) |
| 124 | + |
| 125 | +`audit_throttle_window` exists as a knob but nothing enforces it. Add a |
| 126 | +small throttle gate so `source_binding_violation` and cap-hit eviction |
| 127 | +emit **one** audit line per window per category, satisfying the verifier's |
| 128 | +saturation test (10_000 events → 1 line per `audit_throttle_window_secs`). |
| 129 | + |
| 130 | +Mechanism: a per-category `AtomicU64` holding the epoch-relative millis of |
| 131 | +the last emitted line, on `RouterCtx`. A helper: |
| 132 | + |
| 133 | +```rust |
| 134 | +/// Returns true if an audit line for `last_emit` may be emitted now |
| 135 | +/// (i.e. ≥ window since the last). Updates `last_emit` on success. |
| 136 | +fn audit_allow(last_emit: &AtomicU64, now_ms: u64, window: Duration) -> bool |
| 137 | +``` |
| 138 | + |
| 139 | +CAS/`fetch_update` so two threads racing the same window emit once. |
| 140 | +Categories (each its own `AtomicU64` on `RouterCtx`, initialized to a |
| 141 | +sentinel so the FIRST event always emits): |
| 142 | + |
| 143 | +- `source_binding_violation` — gates the `tracing::warn!(target: |
| 144 | + "audit", event = "source_binding_violation", …)` line in |
| 145 | + `forward_short_via` (passthrough.rs:826-832). |
| 146 | +- `eviction` — gates an `audit`-target line in `evict_oldest` on cap-hit. |
| 147 | + |
| 148 | +Audit-line convention: design §A3 names these lines by a slash-prefixed |
| 149 | +event string — `audit/source_binding_violation` (line 751) and |
| 150 | +`audit/quic_passthrough_cap_hit` (line 1146). The repo has NO |
| 151 | +`target: "audit"` tracing convention and no audit helper in |
| 152 | +lb-observability (grepped — none exists). So the audit record is a |
| 153 | +`tracing::warn!(event = "audit/source_binding_violation", …)` with the |
| 154 | +slash-prefixed `event` field as the design specifies, which the verifier's |
| 155 | +saturation test can collect/count. The `trace!` already at :827 stays for |
| 156 | +per-event debug visibility; the throttled `warn!` is the audit record. The |
| 157 | +cap-hit eviction line is `audit/quic_passthrough_cap_hit`. |
| 158 | + |
| 159 | +First-event-emits + one-per-window: a window of 60s default means the |
| 160 | +saturation test (which fires 10_000 in well under 60s) sees exactly 1. |
| 161 | + |
| 162 | +## 3. Unit tests — coverage to ≥80% session-scope |
| 163 | + |
| 164 | +Current passthrough.rs llvm-cov = 75.91% (498/656). Uncovered clusters: |
| 165 | +599-674 (Retry-mint + verify + backend-socket open), 787-848 (short-header |
| 166 | +multi-length fallback + strict-source gate + `sample_lb_scid`), 994-1037 |
| 167 | +(retry-secret loader error/NotFound edges). Table-driven `#[cfg(test)] mod` |
| 168 | +additions in passthrough.rs (in-crate so private fns are reachable): |
| 169 | + |
| 170 | +1. **`min_client_dcid_len` floor** (table): rows |
| 171 | + `(dcid_len, floor, expect_dropped)` driving `handle_initial` via a |
| 172 | + helper that builds a `RouterCtx` with a stub backend; assert below-floor |
| 173 | + is dropped (no table insert, no backend socket), at/above-floor proceeds. |
| 174 | +2. **`mint_retry == true`** path: no-token Initial → assert a Retry packet |
| 175 | + is sent to `from` (capture via a loopback listener socket as the |
| 176 | + dataplane) and `retry_minted_total` incremented; assert NO flow inserted |
| 177 | + (Retry is stateless). |
| 178 | +3. **`mint_retry == false`** forward-verbatim path: no-token Initial → |
| 179 | + assert NO Retry minted, flow IS created and the Initial forwarded to the |
| 180 | + backend (counting backend socket). |
| 181 | +4. **token-present verify FAIL** → `retry_rejected_total` inc, dropped. |
| 182 | + **token-present verify OK** → flow created. |
| 183 | +5. **`evict_oldest`** (787-848 + 507-516): seed cap+1 flows with distinct |
| 184 | + `last_seen_ms`; assert oldest victim removed, `flows_evicted_total` inc, |
| 185 | + `flows` gauge decremented; negative control: under-cap → no eviction. |
| 186 | +6. **`forward_short` multi-length fallback + strict_source_binding**: |
| 187 | + drive `forward_short_via` true/false table (strict on/off × peer |
| 188 | + match/mismatch); drive the multi-length fallback loop by registering a |
| 189 | + flow with a non-default `short_dcid_len`. |
| 190 | +7. **`load_or_generate_retry_secret`** (994-1037): NotFound→generate (temp |
| 191 | + dir), wrong-length→Err, correct-length→Ok. Reuses the temp-dir pattern |
| 192 | + already in the bounded-state tests. |
| 193 | +8. **`hash_dcid_for_maglev` / `pick_backend`** determinism (small). |
| 194 | +9. **audit throttle** unit: `audit_allow` returns true once then false |
| 195 | + within the window, true again after the window elapses (drive with an |
| 196 | + explicit `now_ms` so no sleep). |
| 197 | + |
| 198 | +Most tests need an async `RouterCtx` builder; I'll add a `#[cfg(test)]` |
| 199 | +helper `fn test_ctx(params) -> Arc<RouterCtx>` that binds a real loopback |
| 200 | +`UdpDataplane` (the `select_dataplane` Auto tier already used by spawn) so |
| 201 | +`send_to` works and Retry packets can be captured on a sibling socket. |
| 202 | + |
| 203 | +Self-check coverage before hand-off via |
| 204 | +`cargo llvm-cov -p lb-quic --all-features --lcov` filtered to |
| 205 | +passthrough.rs DA lines (the resume's method); the verifier re-measures |
| 206 | +`--workspace` independently. |
| 207 | + |
| 208 | +## 4. Build / gate plan |
| 209 | + |
| 210 | +- Iterate: `CARGO_INCREMENTAL=0 cargo test -p lb-quic -p lb --all-features`. |
| 211 | +- Cross-crate (spans lb-quic + lb-observability + lb): workspace fmt + |
| 212 | + clippy + check ([[cross-crate-gate-scope]]): |
| 213 | + - `cargo fmt --all --check` |
| 214 | + - `cargo clippy --workspace --all-features --all-targets -- -D warnings` |
| 215 | + - `cargo check --workspace --all-features` |
| 216 | +- Export `CARGO_TARGET_DIR=/home/ubuntu/Code/eg-target`, `CARGO_INCREMENTAL=0`. |
| 217 | +- Commit per increment (metrics; throttle; tests), push continuously. |
| 218 | + |
| 219 | +## 5. Open questions for lead (non-blocking; will default if no answer) |
| 220 | + |
| 221 | +- **Q1.** `flows_evicted_total` granularity: increment **per evicted flow** |
| 222 | + (1 per cap-hit, since v1 single-evicts one flow) vs per removed dispatch |
| 223 | + key (1 or 2). Default: **per flow** (`.inc()` once per `evict_oldest` |
| 224 | + that removed ≥1 key) — "evicted" is a flow-level event. |
| 225 | +- **Q2.** `flows` gauge semantics: dispatch-table-entry count |
| 226 | + (== `flows_len()`, may be 2/flow) vs unique-flow count. Verify gate |
| 227 | + (iii) asserts the gauge "stays at 1" for one migrated flow — but a |
| 228 | + migrated flow can hold 2 keys. Default: gauge = **dispatch-table size** |
| 229 | + (`ctx.table.len()`), and I'll flag that gate (iii)'s "==1" holds only |
| 230 | + while the flow has a single key (pre server-SCID registration); if the |
| 231 | + verifier needs unique-flow semantics, that's a +1 atomic counter. Want |
| 232 | + the gate-(iii) reading reconciled now or deferred to the verifier? |
| 233 | +- **Q3.** Audit-log convention: RESOLVED by reading the tree — no |
| 234 | + `target: "audit"` convention or lb-observability audit helper exists; design |
| 235 | + §A3 names the lines `audit/source_binding_violation` (:751) and |
| 236 | + `audit/quic_passthrough_cap_hit` (:1146), so I'll emit |
| 237 | + `tracing::warn!(event = "audit/…")`. Flagging only so the lead can |
| 238 | + redirect to a canonical helper if one is intended. |
| 239 | + |
| 240 | +NOT in scope (deferred, ticket-only): per-IP Retry rate-limit (v1.1), |
| 241 | +CF-S15-PASSTHROUGH-FEATURE-GATING-DEEP, CF-S15-PASSTHROUGH-RETRY-ODCID. |
0 commit comments