Skip to content

fix(gateway): harden the KV→data-plane boundary against bad replicated state - #1035

Open
kvinwang wants to merge 12 commits into
nextfrom
fix/gateway-kv-robustness
Open

fix(gateway): harden the KV→data-plane boundary against bad replicated state#1035
kvinwang wants to merge 12 commits into
nextfrom
fix/gateway-kv-robustness

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes the robustness findings of #1029 that do not depend on the WaveKV protocol upgrade. The wavekv-side items (decompression bound, key-schema admission, state digest, WAL recovery) stay in #1031 / Phala-Network/wavekv#2; there is no overlap between the two diffs.

Eleven fixes plus one cleanup, one per commit — each is readable on its own:

# Issue item Commit
1 P0.2 one bad instance record breaks the whole WireGuard config validate replicated instance records before they reach the data plane
2 P1.9 remote deletions never remove instances from ProxyState drop instances deleted on another node
3 P0.3 unwrap_or_default() on corrupt global keys fail closed on corrupt global KV records
4 P0.4 wall-clock LWW + clock skew poisons state cluster-wide ignore future-dated handshake and last_seen observations
5 P0.5 local WAL/snapshot corruption prevents startup quarantine an unreadable KV data dir instead of refusing to start
6 P1.11 list_zt_domain_configs key/value consistency skip zt-domain configs whose key and value disagree
7 Review: an unreadable record evicted a healthy instance keep instances whose replicated record is unreadable
8 Review: reg_time escaped the P0.4 drift horizon reject instance records dated into the future
9 Review: a corrupt global/certbot_config had no repair path let an operator replace a corrupt certbot config
10 Review: quarantining on a storage fault discarded intact state fail the boot on a storage fault instead of quarantining
11 Review: a stuck record logged an error! on every reload report a stuck KV record once, not on every reload
Review: this series added a third copy of now_secs() keep one epoch-seconds helper

Problem

dstack-gateway replicates instances, nodes, certificates and DNS credentials across nodes with last-writer-wins semantics, then feeds that state straight into ProxyState, the certbot and the rendered wg.conf. A corrupt or hostile record for one CVM could take down unrelated instances or the whole node:

  1. Instance import had no validation. Registration checks (valid_ip, public-key uniqueness) were applied at registration only, never on the sync path. wg syncconf rejects the entire config file when a single peer key is malformed, so one bad inst/ record froze WireGuard updates for every instance on the node — the failure was only logged. A key containing a newline could inject Endpoint=/AllowedIPs= lines (the template renders with escape = "none"), and LWW cannot enforce cross-key invariants, so a synced instance could claim the gateway's own wg IP, a reserved-net address, or an IP/key already held by another instance.
  2. reload_instances_from_kv_store only upserted. An instance recycled on node A stayed routable on node B until B's own recycle timeout — 10h by default.
  3. Corrupt global records read as absent. get_certbot_config() fell back to the defaults, silently switching acme_url to Let's Encrypt production and resetting renewal intervals; the DNS-credential and ACME-attestation readers did the same. Only get_acme_credentials() failed closed.
  4. Aggregations took max over wall-clock timestamps. One node with a fast clock, or one record near u64::MAX, kept a dead CVM "alive" cluster-wide: recycle() never fired and top-N routing kept selecting it, unfixable until real time caught up.
  5. A torn WAL tail bricked startup. read_all_ops() hard-fails on a checksum error, so the normal artifact of a crash stopped the gateway from booting — even though every record is replicated on the peers.
  6. list_zt_domain_configs trusted the value over the key. Issuance, DNS-01 and cert/{domain}/data all key off the value, so a record filed under one domain could drive a certificate for another.

Three more surfaced in review of the first six commits, each a defect in the fix rather than in the original code:

  1. The new removal pass evicted instances it had promised to leave alone. It filtered on !accepted.instances.contains_key(id), and a rejected record is absent from accepted.instances — so a record that stopped validating, or (because load_all_instances folded decode failures into "key not present") stopped decoding, dropped a healthy instance from ProxyState after the 60s grace and took its wg peer with it. That is a regression against the pre-fix behavior, which merely declined to update.
  2. reg_time was left out of the P0.4 horizon. Both the removal pass and recycle() age instances with elapsed().unwrap_or_default(), which reads a future timestamp as zero age, so an instance dated forward is immune to remote deletion and to local recycling until the process restarts — surviving even after an operator deletes the record and evicts the peer that wrote it.
  3. Quarantining fired on storage faults, not just unreadable contents. A full disk, an exhausted fd table or a volume that has not finished mounting says nothing about what is stored, so moving the directory aside discards intact state — and since the condition survives a restart, every boot attempt does it again, burying the real data under a pile of .corrupt.* directories. For a single-node deployment holding the only copy of the ACME account and DNS credentials, that is the difference between a restart and a rebuild.
  4. A refused record logged on every reload. A record is refused for what it contains, so it stays refused until someone rewrites it; re-emitting the whole set each round turns one stuck record into an unbounded stream of identical error! lines at whatever rate peer syncs wake the watch task, burying the first occurrence.
  5. The corrupt-certbot_config guidance pointed at a repair path that did not exist. start_certbot_task says to "wait for an operator to repair the record", but the key is a singleton with no delete RPC, and the only write path — SetCertbotConfig — starts with the now fail-closed get_certbot_config(). Renewal stopped permanently, and because do_rotate_acme_credentials reads the same key, RotateAcmeCredentials was blocked too.

Fix

  • A single import boundary (gateway/src/kv/import.rs) that every inst/ record passes through: decode → per-record checks (32 base64-encoded key bytes, no whitespace/control chars, IP inside the routable network, bounded identifiers) → cross-record invariants (unique IP, unique key). An offending record is skipped and logged at error!; the batch is never aborted. Conflicts resolve by (reg_time, instance_id) so all nodes converge on the same winner, matching how the registration path resolves them (oldest keeps the claim). The renderer re-checks every peer before values reach wg, and WgConf now takes an explicit peer list instead of borrowing the instance map.
  • The reload removes what the KV no longer has, with a 60s grace window for a local registration whose KV write failed.
  • Refusals are typed, because they are not the same thing. load_all_instances returns LoadedInstances, keeping undecodable records distinct from absent ones, and every refusal carries a Rejection: Unusable (fails validation, or does not decode) says nothing about whether the instance still exists, so the instance keeps whatever the data plane already holds; LostConflict (well-formed, but lost an IP or key conflict to an older registration) does say the address belongs to someone else, so the loser stops being routable. The removal pass exempts only the first kind.
  • reg_time is held to the same drift horizon as the handshake observations, which closes the future-dating path at the boundary and leaves the two elapsed().unwrap_or_default() call sites nothing to mishandle. Two cheap neighbors: a record claiming the gateway's own WireGuard public key is refused (matching the existing refusal of its wg IP), and a public key's base64 length is checked before it is decoded.
  • Quarantine is for unreadable contents only. wavekv reports both classes through anyhow, so they are told apart by the error chain: a decode failure, a checksum/header bail!, or a read off the end of a truncated file (an io::Error, but only ever UnexpectedEof/InvalidData) is content; any other io::Error is the storage layer and now fails the boot with that error as the cause, which is also what puts the real fault in front of the operator.
  • One epoch-seconds helper. Item 8 added a third copy of now_secs() next to the two already in admin_service and distributed_certbot, and they had already drifted in spelling (unwrap_or_default() vs unwrap_or(0)). All three, plus encode_ts/decode_ts — the same conversion for an arbitrary SystemTime — now live in crate::time. Call sites that propagate with duration_since(UNIX_EPOCH)? keep doing so: whether a pre-epoch clock is fatal is the caller's call.
  • Refusals are logged as transitions, not as a level — reported when a record starts being refused or its reason changes, and again at info! when it recovers.
  • SetCertbotConfig can replace a corrupt record, but only with a request that states every field. It is a partial update, so merging into the defaults would quietly reset acme_url to Let's Encrypt production for an operator who only meant to tune renew_interval — the exact switch the fail-closed reader exists to prevent. Nothing is inherited from a record we cannot read; readers keep the strict accessor.
  • A decode_strict codec helper encoding the three states (missing/tombstoned → Ok(None), decodable → Ok(Some), corrupt → Err), applied to every global record whose corruption must not silently change behavior. The renewal loop skips the round and retries instead of running with defaults.
  • A 5-minute drift horizon on handshake/ and last_seen/ reads, logged with a drop count. Well above NTP-synced drift, well below the recycle timeout.
  • KvStore::new quarantines an unreadable data dir to <data_dir>.corrupt.<unix_ts> and starts empty, re-fetching state from peers. Nothing is deleted.
  • Key/value agreement asserted for cert/{domain}/config.

The address check says nothing about which pool an address came from. A CVM registers with one gateway but is handed every gateway as a WireGuard server, so each node carries peers for the CVMs registered elsewhere — holding addresses from those nodes' pools. Nothing in a node's config describes the other nodes' pools, and the deployments do not agree on a shape that could be inferred: deploy-to-vmm.sh puts every pool inside one /16 that each interface covers, while test-run/cluster.sh and the e2e configs give each node a /24 that no other node's interface covers. is_valid_client_ip keeps governing allocation, where client_ip_range is the right question; the import boundary and the renderer use is_routable_client_ip, which asserts only that the address is ordinary unicast and not one of this gateway's own. Coherence of the peer list comes from the uniqueness pass instead, which runs over the whole KV contents and so holds cluster-wide.

Verification

cargo test -p dstack-gateway --all-features (137 tests), each of the 12 commits verified independently with git rebase --exec, cargo clippy -- -D warnings -D clippy::expect_used -D clippy::unwrap_used --allow unused_variables, cargo fmt --check --all — all clean.

New tests cover each failure mode as a behavior, not as a unit of the helper:

  • a poisoned peer record (unparsable key with an embedded Endpoint= line, plus an instance claiming the gateway's own wg IP) leaves the healthy instance routable, and the injected directive never appears in the rendered config;
  • duplicate IP/key claims resolve to the older registration, independently of iteration order;
  • an instance deleted on another node stops being routable here, releasing its IP and app entry — while a just-registered local instance survives a reload that cannot see it yet;
  • a corrupt global/certbot_config errors instead of reading as the default (which would move issuance to LE production); same for the ACME attestation and DNS-credential defaults;
  • a u64::MAX handshake observation is ignored while a 30s-old one survives, and drift inside the allowance stays usable;
  • a garbage WAL lets KvStore::new succeed with empty state and leaves exactly one .corrupt.* directory behind;
  • a cert config filed under a foreign domain key is skipped;
  • a record that stops validating, and one that stops decoding, both keep their instance routable — while a record that loses an IP conflict to an older registration does drop out. Both of the first two fail without the exemption, which is how the eviction bug was found;
  • an instance record dated a year ahead never reaches the data plane;
  • a CVM registered on another node is imported and reaches the rendered wg.conf, under both cluster shapes in tree (a /18 pool inside a /16 interface, and a /24 pool equal to the interface), while the gateway's own address, its reserved_net and non-unicast addresses stay refused;
  • a storage fault (data dir path blocked by a regular file) fails startup and leaves no .corrupt.* directory behind, while a garbage WAL still quarantines and starts empty;
  • a record that stays refused is recorded once and re-reported only when its reason changes or it recovers;
  • a corrupt global/certbot_config can be replaced by a complete SetCertbotConfig request, while a partial one is refused with an error naming the fields to resend, and a partial update against a readable record still keeps the fields it does not mention.

Test fixtures that used placeholder strings as WireGuard keys ("pubkey-allow", "top-key-0", …) now use real 32-byte base64 keys, since both the import boundary and the renderer reject what wg would reject; the two affected snapshots change only in those key values.

Not in this PR

Rebased onto next for #1030 (named MessagePack encoding) and #1036 (sync-path hardening). Named encoding removes the rolling-upgrade amplification of item 7 — a new field no longer makes older nodes fail to decode — but not item 7 itself, which reproduces with a well-formed record that merely fails validation.

Items 7-11 are scoped to accidental faults: torn writes, schema drift across a rolling upgrade, a clock that is wrong once before chrony converges, a full disk. Nothing here assumes a peer is trying to cause them.

P0.1 (decompression bound) landed with #1036; P1.8 (key-schema admission) is in #1031, P0.6 needs wavekv-side changes, and P1.7 (quarantine list + per-prefix metrics) / P1.10 (schema-evolution policy) are follow-ups.

Copilot AI lite review requested due to automatic review settings August 10, 2026 11:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens dstack-gateway against malformed or hostile WaveKV-replicated state by introducing a stricter KV→data-plane import boundary, fail-closed decoding for global records, and defensive handling for timestamp skew and local persistence corruption.

Changes:

  • Add kv::import validation to filter/resolve inst/ records before they reach ProxyState/WireGuard rendering, and re-check peers just before rendering wg.conf.
  • Make several global KV reads fail closed (decode_strict) to avoid silently falling back to unsafe defaults; propagate errors through certbot/admin call paths.
  • Ignore future-dated handshake/ and last_seen/ observations; quarantine unreadable WaveKV data dirs to allow startup recovery.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
dstack/gateway/src/kv/import.rs New validation/import boundary for replicated inst/ records (per-record + cross-record invariants).
dstack/gateway/src/main_service.rs Routes KV instance import through the boundary; renders WireGuard peers from an explicitly validated list; fail-closed certbot config read path; remote deletion handling.
dstack/gateway/src/models.rs Replace map-values adapter with explicit WgPeer list for WireGuard template rendering.
dstack/gateway/src/kv/mod.rs Add decode_strict; drop future-dated observations; quarantine corrupt persistence; enforce cert config key/value agreement; propagate errors for global reads.
dstack/gateway/src/distributed_certbot.rs Propagate KV read errors (esp. certbot config) instead of silently defaulting.
dstack/gateway/src/admin_service.rs Update admin RPC paths to handle new Result<Option<_>> KV getters.
dstack/gateway/src/config.rs Centralize WireGuard client-IP validity logic in WgConfig::is_valid_client_ip.
dstack/gateway/src/main_service/tests.rs Update fixtures to use real base64 WG keys; add tests for poisoned peer records, remote deletions, and local-write grace behavior.
dstack/gateway/src/main_service/snapshots/*.snap Snapshot updates reflecting real-looking WG keys in fixtures.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 894 to 898
fn reload_instances_from_kv_store(proxy: &Proxy, store: &KvStore) -> Result<()> {
let instances = store.load_all_instances();
let accepted = import::accept_instances(&proxy.config.wg, store.load_all_instances());
report_rejected_instances(accepted.rejected);
let instances = accepted.instances;
let mut state = proxy.lock();

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both correct, and both fixed.

Removals vs rejections — addressed in 98fcd67cc. The filter tested !accepted.instances.contains_key(id) alone, and a rejected record is absent from accepted.instances, so it read as a deletion. load_all_instances folding decode failures into "key not present" made the same thing happen a second way, before accept_instances even saw the record.

Simply exempting everything in rejected would have been wrong, though: it mixes two opposite cases. A record that lost an IP or key conflict to an older registration does say the address belongs to someone else, and keeping the loser routable would put the same address in wg.conf twice — a config wg will not load. Two gateways allocate IPs from their own local allocated_addresses, so a concurrent duplicate assignment is a real accident, not a hypothetical.

So refusals are now typed. Rejection::Unusable (fails validation, or does not decode) says nothing about whether the instance still exists → the instance keeps whatever the data plane already holds. Rejection::LostConflict → the loser stops being routable. load_all_instances returns LoadedInstances { decoded, undecodable } so an unreadable record is distinguishable from a tombstone, and the removal pass exempts only the first kind. Three tests cover the split; the first two fail without the exemption.

Future-dated reg_time — addressed in 700182a1e. Fixed at the import boundary rather than at the elapsed() call sites, because recycle() has the identical unwrap_or_default() and the instance needs to be immune to both to be stuck. reg_time now gets the same MAX_CLOCK_DRIFT_SECS horizon that P0.4 put on handshake/ and last_seen/, which leaves neither call site anything to mishandle.

Worth spelling out why this is reachable without an adversary: reg_time is the registering node's clock at one instant, so the clock only has to be wrong once — the window before chrony converges, or a time jump — for the future timestamp to be written into the KV. It does not heal when the clock does.

@kvinwang

Copy link
Copy Markdown
Collaborator Author

Review round: six commits added, two amended

Rebased onto next for #1030 (named msgpack) and #1036 (sync hardening) — #1036 landed P0.1, so the "not in this PR" section is updated.

Scope note: items below are reasoned about as accidental faults only — torn writes, schema drift across a rolling upgrade, a clock wrong once before chrony converges, a full disk. Nothing here assumes a peer is trying to cause them. Two justifications from the first round were written against a hostile-peer model and have been rewritten to say what they actually defend against; one check that only had an adversarial rationale (refusing a record that claims the gateway's own WireGuard public key) was dropped rather than kept on a story nobody believes.

Finding Commit
An unreadable record evicted a healthy instance, contradicting the stated intent 98fcd67cc
reg_time escaped the P0.4 drift horizon; a future value pins an instance past deletion and recycling 700182a1e
A corrupt global/certbot_config had no repair path, and blocked RotateAcmeCredentials with it 6c3f2eda3
Quarantining fired on storage faults, discarding intact state once per boot attempt 9aad7f750
A stuck record logged an error! on every reload 66b861e61
This series added a third copy of now_secs() 801b2e86c
The original open error was dropped when quarantining also failed folded into 700182a1e

Two worth expanding on, because the first fix for each was wrong:

certbot_config repair. The obvious fix — read through to the defaults when the record is unreadable — reintroduces the exact failure P0.3 exists to prevent. SetCertbotConfig is a partial update, and acme_url defaults to empty, meaning Let's Encrypt production. An operator who hit a corrupt record and then tuned renew_interval would silently move issuance off their staging or private ACME server. So the record is repairable, but only by a request that states every field: nothing is inherited from a record we cannot read.

Quarantine. Moving the data dir aside is right when the contents are unreadable and wrong when the storage is at fault — a full disk or an unmounted volume says nothing about the contents, and since the condition survives a restart, each boot attempt quarantines again and buries the real data. wavekv reports both through anyhow, so they are separated by the error chain: decode failures, checksum/header bail!s, and reads off the end of a truncated file (io::Error, but only ever UnexpectedEof/InvalidData) are content; any other io::Error now fails the boot with that error as the cause.

Verification: cargo test -p dstack-gateway --all-features — 135 passed. cargo clippy -p dstack-gateway -- -D warnings -D clippy::expect_used -D clippy::unwrap_used --allow unused_variables, cargo fmt --check --all, cargo check --workspace --all-features — all clean.

The two eviction tests were written first and confirmed failing against the old filter; the storage-fault test asserts no .corrupt.* directory is left behind, which is what the old code got wrong.

Known and not addressed here: conflict resolution is "oldest registration wins", which converges deterministically but has no notion of which node wrote a record. P1.7 (quarantine list + per-prefix metrics) and P1.10 (schema-evolution policy — #1030 covers added fields, not renames or type changes) remain follow-ups.

@kvinwang
kvinwang force-pushed the fix/gateway-kv-robustness branch from 801b2e8 to 06e598d Compare August 11, 2026 11:32
@kvinwang

Copy link
Copy Markdown
Collaborator Author

Correction: this PR broke multi-node clusters — fixed in ddb37f6f0

While discussing follow-ups, @kvinwang pointed out that client_ip_range is meant to be disjoint per node. That is right, and it exposed a regression in the first commit of this series.

What was wrong. The import boundary validated a replicated record's address with is_valid_client_ip, i.e. against this node's client_ip_range. But in a cluster that range is only this node's share of the address space. dstack-app/deploy-to-vmm.sh spells out the design:

# Each node gets a /18 client range (16k addresses) within the 10.8.0.0/16 network.
# Gateway IP uses /16 so it can route to all client ranges across the cluster.
WG_IP="10.8.${WG_THIRD_OCTET}.1/16"
WG_CLIENT_RANGE="10.8.${WG_THIRD_OCTET}.0/18"

cluster-deployment.md §2.3 and the 3-node test-run/cluster.sh do the same. And RegisterCvm hands a CVM every active gateway as a WireGuard server, so each node has to carry peers for the CVMs registered on the other nodes — holding addresses from those nodes' shares by design.

So on a 4-node deployment every gateway would have refused every instance registered on the other three: dropped from ProxyState, absent from state.apps so select_top_n_hosts could not reach them, and absent from wg.conf so their handshakes were refused. The cluster degrades to each gateway serving only its own CVMs — no cross-node routing, no failover. Reproduced as a unit test before fixing:

REJECTED peer-node-cvm: ip 10.8.64.5 is outside the WireGuard client range

The fix. Split the predicate along the distinction the deploy script already draws. is_valid_client_ip still governs allocation and stays narrow. A new is_routable_client_ip governs what may appear as a peer — the interface's own network, minus the gateway's address, the broadcast address and reserved_net — and is what the import boundary and the wg renderer use. Folded into ddb37f6f0 rather than added on top, since a commit that breaks clusters should not be in the series at all.

Tests. Two, both confirmed failing against the old predicate: an import-level one built on the real deploy-to-vmm.sh shape (/18 share inside a /16 interface) asserting a peer's address is accepted while a foreign subnet, the gateway's own address and reserved_net are still refused; and an end-to-end one asserting a peer-node CVM reaches the rendered wg.conf.

Why CI did not catch it. gateway-proxy-tests.yml runs only test_proxy.sh. The multi-node suites — cluster.sh, test_suite.sh, e2e/ — are not wired into any workflow, and test_suite.sh has exactly the assertion that would have failed:

# Verify ProxyState sync (node 2 should have loaded instance from KvStore)
if [[ "$ps_instances1" -lt 1 ]] || [[ "$ps_instances2" -lt 1 ]]; then

Worth running in CI separately from this PR — nothing in the Rust test suite covered cross-node ProxyState until the two tests added here.

All 12 commits verified independently (git rebase --exec), 137 tests at tip.

…the data plane

Instance records synced from a peer went into ProxyState — and from there
into the rendered wg.conf — without re-running any of the checks the
registration path applies. That made a single malformed record a node-wide
failure: `wg syncconf` rejects the *entire* config file when one peer key is
malformed, and a key containing a newline can inject `Endpoint=`/`AllowedIPs=`
directives. Last-writer-wins replication also cannot enforce invariants that
span keys, so a synced instance could carry the gateway's own wg IP or an
IP/public key already claimed by another instance.

All KV instance records now pass through `kv::import`, which re-runs the
registration checks (public key is 32 base64-encoded bytes, IP and key unique,
address not one of this gateway's own) and skips only the offending record,
never the batch. Conflicts resolve by registration time so every node reaches
the same decision from the same KV contents. The renderer re-checks each peer
as a last line of defense before values reach `wg`.

The address check deliberately says nothing about which pool an address came
from. A CVM registers with one gateway but is handed *every* gateway as a
WireGuard server, so each node carries peers for the CVMs registered on the
other nodes, and each node allocates from its own `client_ip_range`. Nothing
in a node's config describes the other nodes' pools, and the deployments do not
agree on a shape that could be inferred: `deploy-to-vmm.sh` puts every pool
inside one /16 that each interface covers, while `test-run/cluster.sh` and the
e2e configs give each node a /24 that no other node's interface covers. So
`is_valid_client_ip` keeps governing allocation, and the import boundary and
the renderer use `is_routable_client_ip`, which asserts only what a node can
know on its own: an ordinary unicast address that is not one of its own. What
keeps the peer list coherent is the uniqueness pass, which runs over the whole
KV contents and therefore holds cluster-wide.

Refs #1029
`reload_instances_from_kv_store` only ever upserted. An instance recycled or
deregistered on node A stayed routable on node B — in ProxyState, in the
top-N selection and in B's WireGuard config — until B's own recycle timeout
expired, which is 10h by default.

The reload now also removes instances that are present locally but gone from
the KV store. Records that merely failed validation are left alone: the last
known-good state of an instance is better than no state. A registration newer
than the grace window is also kept, so an instance whose KV write failed is not
evicted before the CVM's next registration refresh.

Refs #1029
`get_acme_credentials()` already distinguished missing from corrupt, but its
siblings folded a corrupt record into `None`, which silently changes global
behavior: `get_certbot_config()` fell back to the defaults, switching
`acme_url` to Let's Encrypt production and resetting every renewal interval;
a corrupt `dns_cred_default` or per-credential record made the certbot issue
through the wrong DNS account or none at all; a corrupt ACME attestation
reported an attested account as unattested.

The three-state read (missing/tombstoned vs. decodable vs. corrupt) is now a
`decode_strict` helper on the KV codec, applied to every global record whose
corruption must not silently change behavior. The renewal loop skips its round
and retries instead of proceeding with defaults.

Refs #1029
`handshake/` and `last_seen/` records are wall-clock seconds written by
whichever node made the observation, and the gateway aggregates them with
`max`. One node with a fast clock — or a single corrupt record near `u64::MAX`
— therefore kept a dead CVM "alive" on every node in the cluster: `recycle()`
never fired and top-N routing kept steering traffic at it, with no way to
correct the record until real time caught up.

Observations dated more than 5 minutes ahead of local time are now dropped on
read, logged with a count. The allowance is well above NTP-synced drift and
well below the recycle timeout.

Refs #1029
…g to start

`Node::new_with_persistence` hard-fails on a checksum or deserialize error in
the WAL, so a torn tail — the normal artifact of a crash — kept the gateway
from starting at all, and the node served no traffic until an operator
intervened. The persistent state is a cache: every record is replicated on the
peers and re-fetched by the sync service.

`KvStore::new` now moves the unreadable directory to
`<data_dir>.corrupt.<unix_ts>` and starts empty, logging loudly. Nothing is
deleted, so the original bytes stay available for post-mortem.

Refs #1029
`list_zt_domain_configs` returned the decoded value without checking it
against the `cert/{domain}/config` key it was filed under. Everything
downstream — certificate issuance, the DNS-01 challenge, `cert/{domain}/data`
— is driven by the value, so one poisoned record could point the certbot at a
domain nobody configured.

Refs #1029
The reload pass claimed that "records that merely failed validation are
left alone", but it filtered on `!accepted.instances.contains_key(id)`
alone, and a rejected record is absent from `accepted.instances`. So a
record that stopped validating — and, because `load_all_instances`
folded decode failures into "key not present", one that stopped decoding
too — evicted a healthy instance from ProxyState after the 60s grace,
taking its wg peer with it. A single corrupt or hostile record was
enough to black-hole a live CVM until it re-registered.

`load_all_instances` now returns `LoadedInstances`, which keeps
undecodable records separate from absent ones, and `import` labels every
refusal with a `Rejection`:

- `Unusable` (fails validation, or does not decode) says nothing about
  whether the instance still exists, so the instance keeps whatever the
  data plane already holds for it;
- `LostConflict` (a well-formed record that lost an IP or key conflict
  to an older registration) does say the address belongs to someone
  else, so the loser stops being routable — keeping it would put the
  same address in `wg.conf` twice.

The removal pass now exempts only the first kind. Three tests cover the
split: a record that stops validating and one that stops decoding both
keep their instance, while a conflict loser is still dropped.
P0.4 put a drift horizon on `handshake/` and `last_seen/`, but left
`reg_time` unchecked, and `reg_time` feeds the same kind of arithmetic.
It is the registering node's clock at one instant, so the clock only has
to be wrong once — during the window before chrony converges, or across
a time jump — for a future timestamp to be written into the KV, and it
does not heal when the clock does.

From then on that record is permanent: the reload's "gone from KV" pass
and `recycle()` both age instances with `elapsed().unwrap_or_default()`,
which reads a future timestamp as zero age. The instance is immune to
remote deletion and to local recycling at the same time, and stays
routable on every node that loaded it until that process restarts, even
after an operator deletes the record.

Import now holds `reg_time` to the same `MAX_CLOCK_DRIFT_SECS` horizon
as the observations, which closes the path at the boundary and leaves
the two `elapsed().unwrap_or_default()` call sites nothing to mishandle.
Drift inside the horizon is still accepted, since nodes are not
perfectly synchronized; the batch samples the clock once so every record
in it is judged against the same instant.

Also check a public key's base64 length before decoding it. `wg` writes
the padded form and accepts nothing else, so 44 characters is part of
the format rather than something to discover from the decode.
`start_certbot_task` refuses to run against an unreadable
`global/certbot_config` and says to "wait for an operator to repair the
record instead" — but there was no way to repair it. The key is a
singleton with no delete RPC, so the only write path is the
read-modify-write inside SetCertbotConfig, whose first statement is the
fail-closed `get_certbot_config()`. One bad record therefore stopped
renewal permanently, and since `do_rotate_acme_credentials` reads the
same key, it took RotateAcmeCredentials down with it: unlike
`global/acme_credentials`, this one had no way back.

SetCertbotConfig is a partial update — a field the operator leaves unset
keeps its stored value — so simply reading through to the defaults would
not do either. `acme_url` defaults to empty, meaning Let's Encrypt
production, so an operator who hit a corrupt record and then tuned
`renew_interval` would silently move issuance off their staging or
private ACME server and start burning real rate limits. That is the very
switch the fail-closed reader exists to prevent.

The merge now happens in `merge_certbot_config`, which keeps the stored
values when the record is readable and, when it is not, requires the
request to state every field before it will replace it. Nothing is ever
inherited from a record we cannot read, so the repair path exists
without any field being guessed. The error names the fields to resend.
Quarantining an unreadable data dir is right when the *contents* cannot
be read: a torn WAL tail is the normal artifact of a crash, every record
is replicated, and a gateway that will not start serves nothing. It is
wrong when the *storage* is at fault. A full disk, an exhausted fd
table, a data volume that has not finished mounting — these say nothing
about the contents, so moving the directory aside discards intact state.
Worse, the condition survives a restart, so each boot attempt quarantines
again and buries the real data under a pile of `.corrupt.*` directories.
For a single-node deployment holding the only copy of the ACME account
and DNS credentials, that is the difference between a restart and a
rebuild.

wavekv reports both classes through `anyhow`, so they are told apart by
what is in the error chain. Unreadable content arrives as a decode
failure, a checksum or header `bail!`, or a read that ran off the end of
a truncated file — the last of which is an `io::Error`, but only ever
`UnexpectedEof` or `InvalidData`. Any other `io::Error` is the storage
layer, and now fails the boot with that error as the cause, which is
also what puts the actual fault in front of the operator instead of a
misleading "started empty" line.
A record is refused for what it contains, so nothing about the next
reload makes it acceptable — it stays refused until someone rewrites it.
Logging the whole refused set every round turns one stuck record into an
unbounded stream of identical `error!` lines, emitted at whatever rate
peer syncs happen to wake the watch task, which buries the first
occurrence exactly when it matters.

The reload now keeps the reason last logged per instance and reports a
record when it starts being refused or its reason changes, and again at
`info!` when it becomes usable. The log carries transitions instead of a
level, and a refusal is still never silent.
`now_secs()` was already copied into `admin_service` and
`distributed_certbot`, and the future-dated-observation fix earlier in
this series added a third to `kv`. Three copies of four lines is not
itself a problem; three copies that quietly differ would be, and they
already had started to — two saturate with `unwrap_or_default()`, the
new one with `unwrap_or(0)`. They agree today by luck.

`main_service` also held `encode_ts`/`decode_ts`, the same conversion
for an arbitrary `SystemTime`, so `now_secs()` is `encode_ts(now)` and
belongs beside them rather than in whichever module needed it next.

All three now live in `crate::time`, and the call sites that had the
expression inlined — the peer last_seen write, the two cert-lock
acquisitions, the proxy cert load, the node last_seen refresh, the debug
service's reg_time — use them. No behaviour changes: every one of those
saturated the same way already.

Left alone: the call sites that write `duration_since(UNIX_EPOCH)?` and
propagate. A clock behind the epoch is a real fault, and whether to
report it or carry on is the caller's decision, not something to bury in
a shared helper.
@kvinwang
kvinwang force-pushed the fix/gateway-kv-robustness branch from 06e598d to 98e1bef Compare August 11, 2026 12:53
@kvinwang

Copy link
Copy Markdown
Collaborator Author

Correction to the correction: the first fix was still too narrow

My first attempt checked a replicated address against the gateway's interface network instead of its client_ip_range, on the strength of deploy-to-vmm.sh:

# Gateway IP uses /16 so it can route to all client ranges across the cluster.
WG_IP="10.8.${WG_THIRD_OCTET}.1/16"
WG_CLIENT_RANGE="10.8.${WG_THIRD_OCTET}.0/18"

That works for the production shape and fails for the one in tree. test-run/cluster.sh and test-run/e2e/configs/gateway-*.toml give each node a /24 interface equal to its own pool:

interface ip client_ip_range
deploy-to-vmm.sh 10.8.N.1/16 /18 — inside the interface network
cluster.sh, e2e/configs 10.0.4N.1/24 10.0.4N.0/24no node's interface covers another's

Under the second shape node 1 routes only 10.0.41.0/24, so node 2's CVMs at 10.0.42.x were still refused and test_suite.sh would still have failed.

The general point, which is what makes this a class rather than an incident: nothing in a node's config describes the other nodes' pools. Local topology is the wrong instrument for judging replicated data, no matter how wide you make the net.

So is_routable_client_ip no longer looks at ranges at all. It asserts only what a node can know on its own — an ordinary unicast address that is not this gateway's own address, its broadcast, or inside its reserved_net. is_valid_client_ip is unchanged and still governs allocation, where client_ip_range is exactly the right question.

What actually keeps the peer list coherent is the uniqueness pass in kv::import — no two instances may claim the same address — and that one is correct cluster-wide because it runs over the whole KV contents rather than over local config.

The regression test now covers both shapes in a loop and fails against either wrong predicate. Re-folded into ddb37f6f0's successor; all 12 commits re-verified with git rebase --exec, 137 tests at tip.

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.

2 participants