Dstack-TEE / Dstack-TEE/dstack
gateway: robustness hardening plan for WaveKV-synced state (fault isolation, blast-radius control)
- Dominant language
- Rust
- Stars
- 544
- Forks
- 96
- Avg merge
- 17h 57m
- Merged PRs (30d)
- 117
Description
## Context
dstack-gateway replicates its state (instances, nodes, certificates, DNS credentials, ACME account) across nodes via WaveKV. The gateway runs in a TEE and must stay robust when the KV contains bad data: a corrupt or malformed record for one CVM must never take down other instances or the whole cluster.
This issue tracks the findings of a robustness review of `gateway/src/kv/`, `gateway/src/web_routes/wavekv_sync.rs`, the KV→ProxyState consumption paths in `gateway/src/main_service.rs`, and the wavekv crate itself. The wavekv protocol redesign that came out of the same review is tracked separately in **Phala-Network/wavekv#2 (RFC 0001: delta-state synchronization)**; the migration-relevant parts are summarized at the bottom.
What is already right (keep): ProxyState is the primary read path so the data plane does not depend on KV availability; per-key decode failures are warn-and-skip; `get_acme_credentials()` fails closed on corruption; peer URLs are validated; the sync endpoint enforces RA-TLS same-app-id mTLS, a 16 MiB body cap, and rejects `node_id == 0`.
---
## P0 — paths where one bad input becomes a global outage
### 1. Decompression bomb on the sync endpoint
`web_routes/wavekv_sync.rs`: `data.open(16.mebibytes())` caps the **compressed** size only; `GzDecoder::read_to_end` is unbounded. Gzip expands up to ~1000:1, so a single 16 MiB request from a compromised or buggy peer can force a ~16 GiB allocation → OOM kill.
- [ ] Cap decompressed size (`decoder.take(limit)` + error on overflow); also cap entry count and per-entry value size in the decoded `SyncMessage`.
### 2. One bad instance record can break the whole WireGuard config
`reload_instances_from_kv_store` imports KV instances into ProxyState verbatim; `reconfigure()` renders `templates/wg.conf` with `escape = "none"` and runs `wg syncconf`.
- A malformed `public_key` makes `wg syncconf` reject the **entire** config file — all instances lose wg updates (error is only logged). A key containing newlines can inject `Endpoint=`/`AllowedIPs=` directives.
- `valid_ip()` (client range / broadcast / reserved nets) is enforced at registration, but **not** on the KV import path: a synced instance can carry the gateway's own wg IP, a reserved-net IP, or an IP duplicating another instance (LWW cannot enforce cross-key invariants).
- Public-key uniqueness is checked in `new_client_by_id` but not on import.
- [ ] Re-run all registration-path semantic validation at the KV→ProxyState import boundary (pubkey is valid base64 32 bytes; IP in range; IP/pubkey uniqueness); skip only the offending instance, never abort the batch.
- [ ] Assert key/IP formats once more before rendering `wg.conf`.
### 3. `unwrap_or_default()` on corrupt global keys silently changes global behavior
The fail-closed pattern of `get_acme_credentials()` (absent ≠ tombstone ≠ corrupt) is not applied to its siblings:
- `get_certbot_config()`: a corrupt record silently falls back to defaults, i.e. switches `acme_url` to Let's Encrypt production and resets renewal intervals.
- `get_default_dns_credential_id`, `get_acme_attestation`, etc. treat corruption as absence.
- [ ] Extract the three-state helper (missing / deleted / corrupt→`Err`) and apply it to every global key whose corruption must not silently change behavior; alert on the corrupt case.
### 4. Wall-clock LWW + clock skew: one bad write can poison state cluster-wide
- A node with a future clock (or a corrupted `i64::MAX`-ish timestamp) wins every LWW conflict; the key becomes unfixable until real time catches up. No admin override exists.
- Gateway aggregations take `max` across nodes: `get_instance_latest_handshake` / `get_node_latest_last_seen`. One node writing future handshake timestamps keeps dead CVMs "alive" cluster-wide — `recycle()` never fires and top-N routing is distorted.
- [ ] Clamp/reject timestamps beyond `local_now + max_drift` on ingest and in aggregations.
- [ ] Admin "force put" escape hatch (writes with `max(existing.ts)+1`).
- (Long-term: HLC in wavekv v2 — see RFC.)
### 5. Local WAL/snapshot corruption prevents startup, though state is fully replicated
`Node::new_with_persistence`: `read_all_ops()` hard-fails on a checksum/deserialize error (while `find_last_sequence` tolerates the same); a corrupt 4-byte length prefix can trigger a multi-GiB allocation; a corrupt snapshot is fatal with no `.bak` fallback. Torn WAL tails are the *normal* crash artifact and should never brick the gateway.
- [ ] Wrap `KvStore::new`: on init failure, quarantine the data dir (rename `.corrupt`), start empty, re-bootstrap from peers.
- [ ] Upstream (wavekv): tolerant tail recovery, length sanity checks, snapshot generation fallback — tracked in RFC §3.10.
### 6. Global-lock amplifiers
- `Node::read/write` uses `.expect()` on a poisonable `std::sync::RwLock`: any panic while holding the write lock turns into a permanent crash loop. Switch to `parking_lot` (upstream).
- `persist_if_dirty` serializes + fsyncs the snapshot **inside** the global write lock, stalling registration/sync paths as state grows. Clone `CoreState` under the lock, write outside.
---
## P1 — containment and correctness
### 7. "Decode failure = silently invisible" needs quarantine + visibility
A corrupt `inst/` record makes that CVM vanish from routing with only a warn log.
- [ ] Maintain a quarantine list (key → error → entry meta) surfaced via admin/status; per-prefix decode-failure metrics + alerting.
### 8. No schema/quota enforcement on ingest
Any same-app peer can replicate arbitrary keys of arbitrary size to every node, persisted forever.
- [ ] Key-prefix whitelist (`inst/`, `node/`, `cert/`, `dns_cred/`, `global/`, `__peer_addr/`, `conn/`, `handshake/`, `last_seen/`), per-prefix value-size caps, global key-count/byte caps.
- [ ] GC for `cert/{domain}/attestation/{timestamp}` history (unbounded today).
- [ ] Note: `cleanup_expired_tombstones` is never called — tombstones grow forever; if ever enabled, it must be watermark-coordinated first (resurrection risk; see RFC §6).
### 9. Remote deletions never remove instances from ProxyState
`reload_instances_from_kv_store` only upserts. An instance recycled on node A stays in node B's ProxyState/wg config until B's own recycle timeout — a deregistered CVM remains routable in the window.
- [ ] On reload, explicitly remove instances present locally but absent/tombstoned in KV (with a reg_time grace window for not-yet-synced local registrations).
### 10. Schema evolution in a mixed-version cluster
Values use `rmp_serde` positional encoding. New-data→old-decoder fails during rolling upgrades/rollbacks → instances silently vanish on old nodes (compounds item 7).
- [ ] Written policy: additive `#[serde(default)]` fields only; bidirectional decode compat tests with old-version fixtures.
- [ ] Consider `to_vec_named` or a `{version, body}` envelope for new key types.
### 11. Minor consistency checks
- [ ] `list_zt_domain_configs`: assert value.domain matches the domain in the key; mismatch → quarantine.
- [ ] Document the worst-case behavior of the best-effort LWW locks (`cert/{domain}/lock`, rotation lock) — concurrent renewal is bounded by ACME idempotency today.
- [ ] Per-peer sync-lag metrics (a peer stuck in gap-drop today only produces warn spam).
---
## Design-level follow-ups
- **Formalize the import boundary**: a single module through which all KV→ProxyState/CertStore/wg data flows, with decode → semantic → invariant validation layers and quarantine on failure (gives items 2/7/9/11 one home).
- **Failure-domain separation**: routing state, cert/keys, DNS creds, and attestation history share one lock/WAL/snapshot; consider per-domain stores so a poisoned cert subtree cannot stall instance sync.
- **Secrets in KV**: cert private keys, Cloudflare tokens, and the ACME account key are replicated in plaintext to every peer and included in full-dump responses. Consistent with the RA-TLS same-app trust model, but it makes any single gateway compromise a total credential compromise. Audit debug/admin endpoints for raw-KV exposure; consider KMS-derived encryption for sensitive values.
- **Observability minimum set**: per-prefix decode failures, quarantined keys, per-peer sync lag/last-success, KV size, `wg syncconf` failure alert.
---
## WaveKV upgrade plan (tracked in Phala-Network/wavekv#2)
The protocol-level items (delta-state sync replacing op-logs; state digest for silent-divergence detection; ingest admission hooks/quotas; WAL/snapshot recovery hardening; coordinated tombstone GC; HLC deferred) are specified in the RFC. Gateway-side integration follows its staged plan:
- **Phase 0 (wavekv 1.x prep)**: adopt `state_digest()` + per-peer sync metrics; compare digests via the admin plane; gate = digest equality across the production cluster.
- **Phase 1 (wavekv 2.0 dual-stack)**: rolling upgrade one CVM at a time; gateway adds the `/wavekv/sync2/{store}` route with probe-and-fallback negotiation; promotion gate per node = cluster-wide digest equality + shim counters clean. Any single node can roll back (snapshot/WAL formats stay v1-loadable).
- **Phase 2 (cleanup)**: after ≥14 days of digest equality on all-v2, drop the v1 sync route; only then take wire-breaking follow-ups (HLC).
- Mixed-version e2e matrix runs on `gateway/test-run/e2e/` (3-node harness); test list in RFC §8.5.
Suggested sequencing overall: **P0.1 → P0.2 → P0.5 → P0.3+P0.4**, then the import-boundary refactor + observability, with the wavekv Phase 0/1 work proceeding in parallel under the RFC.
Contributor guide
Research direction
Start with the suggested P0 sequence, reading gateway/src/web_routes/wavekv_sync.rs, gateway/src/kv/, gateway/src/main_service.rs, and the wavekv crate; use gateway/test-run/e2e/ for the mixed-version matrix. Done requires the listed validation, containment, recovery, observability, and synchronization safeguards, with the relevant checks passing and no bad record taking down unrelated state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend, databases, distributed-systems, observability, security
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100