filterframe: a DDoS mitigation control plane that only ever adds BGP objects - #1
Open
lunarthegrey wants to merge 26 commits into
Open
filterframe: a DDoS mitigation control plane that only ever adds BGP objects#1lunarthegrey wants to merge 26 commits into
lunarthegrey wants to merge 26 commits into
Conversation
… toolchain pinned exactly Establishes the workspace filterframe will grow into, with nothing in it yet that touches a router. `filterframe version` runs, `make lint` and `make test` are green, and CI gates on both plus a per-triple cross-build. The layout mirrors packetframe deliberately: `crates/common` holds shared types and trait definitions with no async runtime and no I/O, so that tier modules can depend on it without inheriting the weight of backends they never touch, and `crates/cli` is the only crate that will know all of them. Tier modules will depend only on `-common` and never on each other. Three choices that diverge from packetframe, and why: Edition 2024 against its 2021. packetframe's edition is held back by aya-ebpf's nightly floor, which does not exist here. It buys `unsafe_op_in_unsafe_fn` as a hard error, which matters for a daemon that will call libc for signals and pidfile identity, and let-chains, which collapse the nested `if let` ladders a hand-written config parser is mostly made of. MSRV is set at 1.90 — above edition 2024's floor of 1.85 and let-chains' 1.88, and deliberately below the toolchain pin so a contributor a few releases back still builds. `ipnet` rather than hand-rolled prefix types. packetframe rolled its own because its BPF LPM maps needed a specific in-memory repr; filterframe has no such constraint, and the same values travel through NLRI encoding, the state journal's serde, and metrics labels. One type across all of that beats a config type plus conversions. No `anyhow`. packetframe declares it in four manifests and uses it in zero .rs files; carrying the dead declaration forward would only reproduce the drift. Errors are scoped thiserror enums whose variants exist to drive a decision. Dependencies land with the slice that needs them rather than up front, so `cargo tree` always reflects what the binary actually does. The exit-code contract is declared the same way — only `EXIT_OK` exists, because a constant nothing returns is a dead-code allow wearing a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…kaging breakage now fails the PR that caused it Packaging normally lands last, which means the first time anyone builds a package is also the first time it can break, at the worst moment. Putting it first inverts that: every pull request from here on produces an installable artifact, CI installs it in a clean Debian container, and a change that breaks the package fails the change rather than the release. The install verification is what makes this a test rather than a build artifact. It asserts the unit is valid to systemd itself via `systemd-analyze verify` — not merely that the file was copied — that the service is installed disabled, that the reference config and binary are where the docs say, and that argument parsing works with no configuration present. The unit ships with an empty CapabilityBoundingSet and AmbientCapabilities. filterframe holds BGP sessions and reads sysfs, and needs no privilege for either. That emptiness is what makes the additive-only invariant enforceable rather than merely intended: a daemon with no CAP_NET_ADMIN cannot install a route, rewrite a firewall rule, or take an interface down, however badly it is compromised or however wrong its inputs. A comment in the unit says so, because the natural response to a future feature that seems to need privilege is to add it here rather than to ship it as an opt-in drop-in. Installed disabled and stopped: filterframe cannot do anything useful without an operator-supplied peer list and policy-engine token, so auto-start would only produce a first-boot failure in the journal that everyone learns to ignore. Releases are the same code path with publishing appended, triggered on a v*.*.* tag, with a workflow_dispatch dry run that builds the whole matrix and skips publishing so a release can be rehearsed. Signing is conditional on the secret existing, so a fork still produces a complete verifiable release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ops the daemon at load, not at 3am
The grammar is line-based rather than YAML, matching packetframe. Two
properties earn that: a parse error can cite the exact source line the operator
is looking at, and the shipped reference config can carry a comment above every
directive explaining its default, its reload semantics and why it exists. That
makes conf/example.conf a primary document rather than a sample, and a test
parses it verbatim so it stays one.
The parser is pure — a &str in, a Config out, no filesystem, no clock, no
network. Everything needing the world lives in preflight. That split is what
lets the grammar be exercised exhaustively on a laptop that is not a filter
node, and it is why every refusal below has a named test.
Unknown directives are fatal. Silently ignoring one means an operator who
mistyped a safety guard believes it is in force.
The cross-section validators are the interesting part, because each encodes a
failure that is otherwise silent:
- `originate-prefix` is required. It is the authority boundary: without it a
confused or compromised policy engine could have this node announce space
it does not hold.
- A peer allowing tier `rtbh` with no `community` is refused. A blackhole
announced without one is just a host route, forwarded normally — the
mitigation appears to work and does nothing.
- Role and tier must agree. A scrubber peer that accepts blackholes is a
config error, and the right time to learn that is now.
- An unconditional tier-rule with rules after it is refused, because those
rules can never fire.
- A rule selecting a tier no peer can serve is refused, because it would fire
into nothing.
Communities are parsed into canonical form at load rather than carried as
strings, and well-known values are spelled by name. That is deliberate: a
mistyped `65535:665` reads as plausible in review, where `blackhoel` fails at
config load.
restart_only_delta is pure over two Configs so the reload rules are testable
without a running daemon, and every refusal names the directive and says why —
"restart required" without saying which line caused it is the least useful
message a daemon can emit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oad — a broken config can never take a running daemon down The daemon runs in the foreground and never forks; systemd is the supervisor. Signals are polled at 250ms rather than handled on their own thread, because the same loop will own the reconcile tick, module health and the metrics writer, and one place where the daemon decides to act beats shared mutable state between a handler and a loop. The cost is a quarter second of latency answering a signal, which nobody notices. Reload is the part worth arguing about. SIGHUP re-reads the file, checks it against restart_only_delta, and writes the outcome to an acknowledgement marker in the state directory. `filterframe reconfigure` reads the marker *before* signalling — so a stale one is not mistaken for this answer — then signals and polls, and exits non-zero with the daemon's own explanation when an edit is refused. An operator gets an exit code instead of having to read the journal. A refused or malformed reload never takes the daemon down. The running configuration stays in force and the refusal is logged with its line number. A daemon that exits because someone mistyped a duration during an incident is worse than one that carries on with what it had. Presence is three-valued, and that is not pedantry. Pids are recycled, and a pidfile that outlived its daemon will eventually name somebody else's process; a two-valued check will one day send SIGHUP to whatever inherited the number. So the pidfile records pid plus process start time, a mismatch reads as Gone rather than Running, and `is_running()` exists so that no caller writes `!matches!(p, Gone)` and silently treats Unknown as alive. On Linux the start token comes from /proc; macOS has no cheap equivalent, so it degrades to pid liveness and says so rather than claiming a guarantee it does not have. Atomic writes refuse to follow a symlink and fsync the containing directory, not just the file. The directory fsync is the part people leave out, and it is what makes the rename durable on a node that has just lost power — which is exactly when the journal mattered. Logging is installed before argument parsing, because a config that fails to parse is itself worth logging. RUST_LOG beats the config file, for the life of the process, and the daemon says so once — otherwise an operator edits log-level, reloads, sees no change and has no way to know why. A malformed RUST_LOG warns and hands control back rather than refusing to start. Verified end to end: a hot change reloads and exits 0, a bgp-section change is refused by name with the running config unchanged, a second daemon over a live one is refused with the live pid, and SIGTERM leaves announcements in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… list endpoint lies — a failed poll can never look like an empty world
The client is paranoid because every way this goes wrong produces a *plausible*
answer rather than an error. "The engine is down" and "nothing is under attack"
both look like an empty list, and acting on the second when the first is true
withdraws protection during exactly the kind of event that takes a policy
engine offline.
So every failure resolves to MitigationView::Stale, which carries no list. The
planner takes &[Mitigation], not a view, so there is no expression anywhere that
computes a teardown from a failed poll. It is a type error rather than a
code-review finding.
Each guard exists for observed behaviour, not a hypothetical:
- the status filter is a compile-time constant, because unrecognised tokens
are dropped server-side and a typo returns zero rows with HTTP 200
- `pop` is never sent: the server accepts it and ignores it, so sending it
would create a false belief that results were scoped to this node
- the page size cannot be zero — asserted at compile time, because a zero
limit returns has_more with a null cursor and spins a poll loop forever
- has_more with no cursor aborts the whole poll: a truncated page set must
never be mistaken for a complete world
- the cursor must strictly advance, or pagination is looping
- time filters are never sent, because they filter on creation time, which
does not change when a mitigation is withdrawn
- one structurally bad item fails the whole page rather than shortening it
- unknown vectors, actions and statuses survive: forward compatibility is a
safety property here, since an engine upgrade must not blind filterframe
- an empty result is confirmed over three polls before it is believed, but
only once a non-empty one has been seen, so a quiet node is never stuck
Seventeen integration tests walk those against a stub server, including
assertions on the query string filterframe actually builds.
Rates come from the per-IP event history, not from the mitigation record: the
record's rate_bps is the policer rate the playbook chose, and it is null for a
discard action — null exactly when the attack is largest. The history endpoint
has no time predicate, so it returns the newest events for an address *ever*;
samples older than five minutes are discarded, and a victim with only stale
events has no rate at all. Lookups are budgeted per tick and spent
oldest-sample-first, because the rate limit is shared with the dashboard and
every detector.
A rule with a rate condition does not fire without a sample. That is what keeps
"large and persistent" from quietly degrading into "persistent".
`filterframe plan --from-file` runs the whole decision layer against a recorded
response and prints which rule line decided each mitigation, including the ones
left unhandled. Offline, no router, and the same pure function the daemon uses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…annot withdraw — the invariant is now structural, not aspirational The loop re-derives desired state from the whole mitigation list every tick and compares it with what the speaker holds. Nothing reacts to events, so there are no missed messages to recover from, a steady state is silent, and a restart costs nothing because there is no accumulated position to rebuild. The branch that matters is `converge_hold_only`. It is reached only from a stale view, and a stale view carries no mitigation list — so there is no expression in it that could compute a withdrawal. The withdraw loop lives inside the fresh arm, behind `view.fresh()`, and moving it would mean handing the stale branch a list it does not have. That is the invariant enforced by structure rather than by review. `stale_never_tears_down` is table-driven over all seven stale reasons and asserts zero withdrawals across twenty consecutive failed polls. Adding an eighth reason without adding a row should feel uncomfortable. It is the test that must never be deleted. The BGP trait pair splits by consumer: the fast tier needs only RouteOriginator, so its blast radius is provable from the signature, and RibObserver carries the confirmation surface the divert sequence will need. Two things keep that honest. `Submitted` is #[must_use] and carries no verdict, so a receipt discarded next to a transit withdrawal is visibly a bug — it fired on eleven of the mock's own tests, which is the attribute working. And `Fidelity` records what a positive answer actually established, because BGP has no application-layer acknowledgement and no backend can honestly claim one. The mock reports Synthetic and therefore satisfies no real quorum, which is why observe mode can never be mistaken for a working diversion. `mode enforce` refuses to start. No real backend exists yet, and refusing is better than announcing nothing while reporting success. Verified end to end against a stub policy engine, and the log is the argument: two mitigations announced as they appeared; the engine returning 503 held both for six ticks and withdrew nothing; the engine recovering with an empty list was refused twice as unconfirmed before being believed on the third; both were then released; the engine disappearing entirely held again; and SIGTERM left everything in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…easures the right clock RTBH is one announcement away from dropping a customer's traffic on purpose, so every guard here refuses rather than clamps, and every refusal is counted with its own label rather than merged into a number. The defining behaviour is asymmetry: announcements are immediate, withdrawals dwell. The fast tier has to be fast in the direction that protects and slow in the direction that exposes. A detector oscillating around its threshold must not oscillate a BGP announcement at every transit it touches, but it also must not wait out a timer before dropping an attack. Two bugs were found by writing the tests, and the second was a design error rather than a test error: `max-lifetime` originally measured from first announcement. That expired a mitigation the policy engine was still actively confirming — and the admission loop immediately re-announced it, producing churn dressed as a safety guard. It now measures time *unconfirmed*, from the last time the engine asked for it. A continuously confirmed engagement never trips it, which is asserted over a simulated hour. That correction is also what makes the ceiling work during an outage. Modules gained `hold_only(now)`, which takes no desired set — because on a stale view there is none — and can therefore release nothing for lack of demand. The only thing it may remove is work the ceiling has expired, which is the bounded operator-configured exception that stops an unreachable policy engine leaving an address dark forever. Its signature is the argument: there is no list to pass, so there is nothing to release against. Adoption exists because without it a restarted daemon sees paths no module claims, computes them as surplus, and withdraws the lot — every restart a mass teardown. Clocks start from now rather than from any persisted timestamp, so a crash loop cannot accumulate hold credit and then release everything at once. Adoption runs on the first tick rather than at construction, because the speaker may not be reachable at startup and refusing to start over that would be worse. The tier abstraction that came with it keeps modules pure over (desired, now): no I/O, no clock of their own, monotonic instants handed in by the tick. That is what makes a damping bug findable by advancing a number in a unit test rather than at three in the morning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…afety property proved by exhaustion The sequence is a state machine with no async, no I/O and no clock: step(state, event) -> (state, actions), and nothing else. That is the whole testing strategy. The safety property is checked by enumerating every state against every event, then walking every reachable sequence to depth six, then injecting a crash at every index of five representative sequences — which is a far stronger argument than sampling a running system could ever give. The invariant is `signal_up implies scrubber_up`. filterframe never withdraws the protected prefix because it never announces it: the edge does that unconditionally, and filterframe raises a signal route whose presence makes the edge's export policy stop advertising toward transit. So a signal without a scrubber announcement means the prefix is announced by nobody. Announced to *both* is always safe — traffic splits, none is lost — and every failure path therefore stalls in that direction by construction rather than by care. The order follows from that. Engage: scrubber, quorum, dwell, then signal — transit is untouched until the scrubbing path exists, so aborting before the signal costs nothing. Disengage: restore transit, dwell, then withdraw from the scrubber — reachability before cleanliness. Both dwells earn their place. A quorum establishes that the reflectors took the path; it says nothing about the provider's own propagation, and raising the signal the instant a quorum appears creates a gap no adjacency query can see. The teardown dwell is longer on purpose: an overlap costs a little asymmetric routing, a gap costs an outage. Losing quorum while diverted, and the return path dying while diverted, are the same emergency and get the same response — drop the signal immediately, let traffic come back dirty rather than not at all. Recovery resolves toward reachability rather than trusting the journal. Mid-engage resumes at Announcing rather than Settling, because quorum evidence does not survive a restart — the sessions that gave it are gone. Mid-teardown resumes at Restoring rather than Draining, because resuming later would shorten the dwell that protects against the gap. Both are asserted. The journal exists for exactly one thing: which *direction* a sequence was moving when the process died. A prefix announced two ways is simultaneously "engaging, step one done" and "tearing down, step one done", and no amount of reading the RIB distinguishes them. Everything else is re-derived every tick, so persisting it would only create a chance for the file and the world to disagree. Modules own their own format; the journal carries opaque blobs keyed by module, so adding a module never changes it. It is rewritten only when intent actually changes — at a two-second tick, writing unconditionally would be forty thousand fsyncs a day to record nothing. Timers are deliberately not persisted. Restoring a half-elapsed dwell would shorten the window it protects, and restoring hold credit would let a crash loop release everything at once. Every clock restarts on recovery, which biases toward holding. Verified: the shipped conf/example.conf now loads and runs with both modules, six peers and five tier rules, and the token-file permission check refuses anything looser than 0600. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hment that finally lets the divert tier fire Three commands an operator actually reaches for, plus the wiring that makes the slow tier reachable at all. `explain` answers the question that gets asked during an incident: not "what is announced" but "why". It dry-runs the rule table against a hypothetical mitigation and prints which rule matched, on which line, which facts satisfied it, and what the resulting engagement would be — or why there is none. It also states outright when there is no rate sample, because that is the single most common reason a divert rule silently does not fire and it otherwise looks like nothing at all in the output. `status` reads a snapshot the daemon publishes each tick rather than talking to a live process, because an operator reaching for it is often doing so precisely because they suspect there isn't one. The daemon's presence is reported as the three-valued thing it is, and a report built from a snapshot with no daemon running says "snapshot, not live" rather than implying the numbers are current. The ATTENTION block is computed rather than left for someone to spot an anomaly in a table at three in the morning. Metrics go to a Prometheus textfile, not an HTTP endpoint: a daemon whose whole safety story rests on needing no privilege should not open a listening socket to report on itself. Emitted by hand with one writer, which is what makes the atomic rename meaningful — a scrape sees the previous complete file or the next one, never a mixture. `filterframe_policy_stale_seconds` is the series to alert on; it is what distinguishes "quiet because nothing is happening" from "quiet because we cannot see anything", which are identical in every other metric here. Never having polled reports elapsed time rather than zero, because zero reads as healthy. A final write happens on shutdown so a scrape afterwards sees real numbers. Label values are escaped, because an unescaped quote in an operator-chosen node id produces a file the scraper rejects wholesale — losing every metric on the box rather than one. The rate enricher is now called from the poll path, which is what makes the divert tier reachable: a rule requiring a rate cannot fire without a sample, and until now nothing produced one. Enrichment failures stay silent by design. A victim without a sample simply does not satisfy rate rules, where taking the whole view stale over an auxiliary lookup would trade a better decision for no decision at all. Verified end to end: a short attack takes the fast tier, a large sustained one takes divert to three scrubber peers, and both appear in status, the metrics textfile and the log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s script Wiring the enricher into the poll path made the client issue a per-victim history request after every successful poll. The test stub advanced its scripted response index on *every* request, so those auxiliary lookups silently consumed pages, and every test scripting a sequence drifted by however many victims happened to be in the last one. The stub now serves the history endpoint separately and does not count it. Worth recording how this was caught: the total test count dropped from 201 to 180 between two runs. The suite itself was reporting a failure the whole time, but the summary being used to eyeball progress was summing passed-counts across suites and happily added up a partial run. The count moving in the wrong direction was the signal, not the summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… kernel actually sets Diverting traffic to a scrubber is only half of it. The cleaned traffic comes back over a tunnel, and if that tunnel is not carrying, the diversion is not a mitigation — it is a hole. So the divert tier is gated on this, and a path that dies while diverted is the most urgent condition in the daemon. The check reads `IFF_UP` from `flags`, not `operstate`. GRE tunnels have no carrier, so the kernel never sets an RFC 2863 operational state for them and reports `unknown` forever — which the kernel's own documentation defines as meaning the interface must be considered usable. A gate written the obvious way would have refused to divert on a perfectly healthy tunnel, in production, only during an attack. There is a test named for exactly that case. The return-path signal is three-valued, and collapsing it to a boolean was a bug I introduced and then had to fix: `Blocked` and `Down` both prevent a new diversion, but only `Down` justifies undoing a working one. An unreadable sysfs file is a bad reason to move a customer's prefix across the Internet, and a two-valued signal made it one. Hysteresis is asymmetric — three failures to distrust, five successes to trust again — because a flapping tunnel that is trusted quickly produces a divert loop, and every cycle of that is real BGP churn at every peer involved. A second real bug, caught by its own test: recovery from `Degraded` returned straight to `Up` on a single success. A path alternating down/up therefore recovered on every other reading, never accumulated failures, and would never have been declared down — a tunnel flapping once a second would have read as healthy forever. Any state other than `Up` now requires sustained success. The probe runs on its own cadence rather than the reconcile tick, because waiting out a tick interval to notice a dead return path is a second too long. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n proves The default backend, chosen for process separation rather than protocol elegance: GoBGP owns the sessions and outlives filterframe, so a restart is invisible to every router involved. An in-process speaker drops every session when the daemon dies, which is precisely when a half-completed divert is most fragile. Two weaknesses in the reference implementation are fixed here. It connected eagerly at startup and exited if the sidecar was not yet up, turning a boot ordering problem into a daemon that will not start; and it stored the channel once and never reconnected, so a sidecar restart left it handing out a dead client forever. `connect_lazy` with HTTP/2 keepalives fixes both. The confirmation semantics are the part worth reading. GoBGP's adjacency-out is *derived on demand* — `ListPath` builds a fresh view per call and runs the peer's export policy over it — so it can establish that a path is best and passes policy, and cannot establish that any UPDATE was written, because there is no record of transmission to consult. This backend therefore reports PolicyEligible and no higher, and a quorum built on it means "N peers would send this", not "N peers received it". The runbook has to say that in those words. Queries are scoped to one prefix and set `enable_filtered`, which is what distinguishes "policy says no, waiting is futile" from "not there yet, keep waiting" — a full-table scan per confirmation poll would be unacceptable at the rate the gate runs. Clippy caught a real logic error: the verdict loop returned on the first path, so a destination carrying several would report a policy rejection while another path was being advertised perfectly well. One unfiltered path is now enough to answer yes. `list_originated` deliberately returns Unknown rather than a guess. Recovering ownership from a live RIB means matching the origin community across both families and reconstructing a tier from a prefix, and getting that subtly wrong means withdrawing another controller's paths. Until it is written and tested against a real sidecar, reporting nothing is the conservative answer. The protos are vendored with a SHA-256 manifest that CI verifies, because a field number that moved is not a compile error — it is a runtime disagreement. `protoc` comes from `protoc-bin-vendored`, so codegen needs nothing installed and every cross target gets it. The whole backend is behind a feature that is off by default: observe mode runs against the mock and should not pay for a gRPC stack. Compile-verified and unit-tested only. Everything that needs a live sidecar — adjacency readback, epoch behaviour across a real flap — is untested until there is one to test against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ning, plus tunnel-render Four runbooks, each written for someone who has four minutes and a problem. policy-loss explains why holding is correct when the policy source is unreachable, and reads the stale reasons as distinct operator stories rather than one failure. It states plainly that there is no age at which "I cannot reach my policy engine" becomes "there is no attack", and that adding a give-up timer would be a bug. edge-policy documents the transit-suppression mechanism per platform — FRR, Junos, IOS-XR — and names the prerequisite filterframe cannot check for itself. Its verification section ends with the only test that matters: announce the signal, kill -9 the thing announcing it, and confirm the prefix comes back on its own. It also says what taking the weaker fallback costs, so that choice is made deliberately rather than by omission. bgp-backends states the thing most likely to be misread: BGP has no application-layer acknowledgement, so on the GoBGP backend a 2-of-3 quorum means "two reflectors would send this", not "two received it". The fidelity table makes that concrete, and the section on what happens when filterframe dies covers the asymmetry between the backends that is otherwise buried in a docstring. return-path documents the operstate trap in the place an operator will look for it, and the three-valued gate: an unreadable sysfs file blocks a new diversion and must never undo a working one. `filterframe tunnel-render` emits the systemd-networkd units for the return path, with the three things that are easy to get wrong already handled — the MTU arithmetic including the four bytes a GRE key costs, `rp_filter` and its max(all, iface) trap, and MSS clamping. It generates configuration and does not apply it, because creating a tunnel needs CAP_NET_ADMIN, and that capability is not "may create tunnels" but root over the whole dataplane. The daemon ships with an empty capability set, and that emptiness is what makes its additive-only rule enforceable rather than merely intended. The rendered output says so, where whoever installs it will read it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ted" was doing double duty
End-to-end verification surfaced two things worth fixing.
`Refusal::Protected` was being used both for "an operator put this on the
never-blackhole list" and for "the return path is down". Those are unrelated
conditions with unrelated responses, and merging them made them
indistinguishable in `status` and in the metrics — a refusal count that lumps a
deliberate operator guard together with a broken tunnel is not an answer anyone
can act on. They now have their own variants and their own metric labels.
And a refusal carried only its metric label as far as `status`, so an operator
reading it saw the word "protected" with no indication of which prefix, which
list, or why. Every refusal now describes itself in a sentence, and the tick
carries that sentence alongside the label. The label is for the metric; the
sentence is for the person.
Verified: a node whose return tunnel does not exist now reports
REFUSED
divert:198.51.100.0/24 the return path is not usable, so diversion is
blocked (the fast tier still applies)
which is the whole story, including the part that stops it reading as an outage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
…ow a lint goes green while the build goes red
The cross-build job linted with bare `cargo clippy --target <triple>` and then
built with `cross build`. Those are two different environments, and the lint one
has no cross C toolchain.
`reqwest` 0.13's rustls feature resolves to aws-lc-rs, which compiles
aws-lc-sys through cc and cmake, so the lint step needed
`aarch64-linux-musl-gcc` and friends on the runner:
error occurred in cc-rs: failed to find tool "aarch64-linux-musl-gcc"
Three of the four cross targets failed on it. The build step would have been
fine — `cross` supplies the toolchain in its container — so this was purely a
lint running somewhere the code cannot be compiled.
`cross clippy` fixes it by running the lint in the same environment as the
build, which is the right place for it regardless.
Dropping `--all-features` there would also have made it pass, and would have
been the wrong fix: the whole reason this step exists is to compile cfg-gated
and test-only code for each triple, and narrowing the features narrows exactly
what it was added to catch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
CI: cross-target clippy — fixed in 414ff32Three of four The job linted with bare Fixed by running the lint through Dropping Socket:
|
| tokio 1.53.1 | hyper-util 0.1.20 | |
|---|---|---|
repository in manifest |
github.com/tokio-rs/tokio |
github.com/hyperium/hyper-util |
| Source files | 555 .rs |
51 .rs |
| Longest source line | 214 chars | 163 chars |
| Files with a >2000-char line (minification) | 0 | 0 |
include_bytes! of opaque blobs |
0 | 0 |
| String literals >200 chars (base64-ish) | 0 | 0 |
Both resolve from registry+https://github.com/rust-lang/crates.io-index with
checksums pinned in Cargo.lock, so cargo verifies the artifact against the
registry on every build. Neither shows any marker of actual obfuscation:
ordinary, readable, line-wrapped Rust from the canonical upstream repositories.
tokio's 1119 unsafe blocks are expected for an async runtime implementing I/O
drivers and synchronisation primitives — not obfuscation, and not something a
scanner heuristic distinguishes well.
Neither is optional in any meaningful sense: tokio is the async runtime, and
hyper-util arrives transitively through reqwest → hyper. Both Socket checks
report pass, so these are advisory rather than blocking.
If you want them silenced for this PR, the mechanism is a comment of
@SocketSecurity ignore cargo/tokio@1.53.1 and
@SocketSecurity ignore cargo/hyper-util@0.1.20. I have deliberately not posted
those — suppressing a security alert on a maintainer's behalf, when nothing is
blocked by it, is not mine to do.
Codex review bot
Reported its own usage limit rather than any finding. Nothing to address.
🤖 Addressed by Claude Code
…ath it rewrites
Packaging failed outright:
cargo-deb: Unable to parse crates/cli/Cargo.toml
because: can't load root workspace
because: unknown variant `3`, expected `1` or `2`
cargo-deb 2.7.0 predates `resolver = "3"` — its cargo_toml only knew resolvers 1
and 2. 3.7.0 parses it. Keeping resolver 3 rather than downgrading the workspace
is the right way round: it is the edition-2024 default and it makes resolution
MSRV-aware, which is the point of declaring rust-version at all.
Fixing that surfaced a second bug the parse failure had been masking, and this
one would have shipped a wrong package rather than no package:
Only source paths starting with exactly 'target/release/' are detected as
Cargo target dir. '../../target/release/filterframe' does not match
`target/release/` is the literal prefix cargo-deb rewrites to
`target/<triple>/release/` when invoked with `--target`. Written as
`../../target/release/`, the rewrite never happens — so a cross-built .deb would
have quietly packaged the host binary, or nothing. An aarch64 package containing
an x86_64 binary is the kind of thing that installs fine and fails at the worst
moment.
Verified locally with cargo-deb 3.7.0: the package now builds and contains
/usr/bin/filterframe, /etc/filterframe/example.conf, the systemd unit, and the
docs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ependencies are actually resolved
`systemd-analyze verify` failed the install check:
filterframe.service: Command /bin/kill is not executable: No such file or
directory
It was right, and this is a packaging gap rather than a container quirk.
`ExecReload=/bin/kill -HUP $MAINPID` shells out to a binary from procps, and
`depends = "$auto"` derives dependencies from linked shared libraries — it
cannot see a binary a unit file invokes. So the package never declared something
it genuinely needs. Most hosts have procps because it is priority: important,
which is exactly why this would have gone unnoticed until it landed somewhere
minimal.
The verification step also installed with `dpkg -i`, which does not resolve
dependencies. That would have kept passing even after the dependency was
declared — checking that a package unpacks is not checking that it installs. It
now goes through apt, so a missing or unsatisfiable dependency fails the check
the way it would fail an operator.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… a fixed path
Two bugs, one of which only appeared once the other was fixed.
`depends = "$auto"` is not reproducible across architectures. It shells out to
dpkg-shlibdeps, which cannot inspect an aarch64 ELF on an x86_64 runner, so the
arm64 package came out with *different* dependency metadata from the amd64 one
while both reported success. Differing silently is worse than either answer, so
the dependencies are now stated: libc6 >= 2.31, because cross's gnu containers
link against 2.31 deliberately for backward compatibility, and procps for the
/bin/kill that ExecReload invokes.
The second was the step failing rather than the package:
Unable to process file command 'output' successfully
Invalid format 'target/aarch64-unknown-linux-gnu/debian/filterframe_...deb'
`target/` is restored from cache, and the .deb filename carries the commit sha,
so a package built by an earlier commit was still sitting there. `ls *.deb` then
returned two lines, and a two-line value corrupts $GITHUB_OUTPUT. It only
surfaced now because this is the first time the aarch64 job got far enough to
build a second one.
Both workflows now pass an explicit `--output`, so nothing is globbed and
nothing accumulates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit fixed the .deb output path in ci.yml and silently failed to apply the same change to release.yml — the patch pattern did not match and the push went ahead with only half the fix in place. Same reasoning as before: `target/` can carry a .deb from an earlier build, and copying by glob would put two packages claiming the same version into one release. The release path now passes an explicit --output. Worth noting the failure mode, since release.yml only runs on a tag: this would not have been caught until the first release, and the symptom would have been a duplicate or wrong-sha artifact attached to it rather than a failed build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…refix walked straight past it
`is_protected` only asked whether a never-entry contained the engagement's
address. Blackholing a prefix blackholes every address inside it, so the
interesting direction was the other one, and it was not checked.
With `max-prefix-length 24` and `never-blackhole 198.51.100.1/32`, an
engagement for 198.51.100.0/24 was tested as
`198.51.100.1/32.contains(198.51.100.0)` — false — so no guard fired at all:
effective={198.51.100.0/24}, refused=[]. The protected host was blackholed
silently by the one lever an operator reaches for during a mistaken
mitigation.
Also hoists the `max-lifetime` > `withdraw-hold` check out of the
`if let Some(..)` that only ran when `max-lifetime` was present. Setting only
`withdraw-hold 45m` was accepted against the 30m default, and every
undemanded engagement then tripped the ceiling before its dwell could release
it — which is the exact failure the error message describes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p serialising rate lookups Three things, all in the client. `ca-file` was parsed, validated and documented, and never installed. The connection validated against the system trust store while the operator believed a private CA was pinned — the one failure mode a TLS setting must not have. Now loaded via `add_root_certificate`, and a path that cannot be read or is not PEM refuses the start rather than falling back. `EMPTY_CONFIRMATIONS` is documented as *consecutive* empty results, but every failure path returned through `stale()` without touching `empty_streak`. So empty/503/empty/503/empty walked the counter to three and believed the empty set — withdrawing protection on the strength of a flapping engine, which is precisely when withdrawing is worst. Failures now return through `lost()`, which resets it. Rate lookups awaited one at a time: `LOOKUP_BUDGET * request-timeout` is 30s at the defaults, inside a `block_on` on the thread that also polls signals and the return-path probe. The budget is now a concurrency cap, so a tick costs one timeout rather than ten. The sample cache is also pruned to `MAX_SAMPLE_AGE`, since entries past it were already unreadable and the map otherwise grew one entry per distinct victim forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ad gave CLAUDE.md requires anything touching sysfs to sit behind `#[cfg(target_os = "linux")]`, and this did not. On macOS every path under /sys/class/net is absent, which the probe read as a confirmed failure: three polls to `Gate::Down`, `ReturnPath::Down`, every diverted machine torn down and every new diversion refused. The unit tests never saw it because they point `sysfs_root` at a fabricated tree, so the gate stayed green while the daemon was broken on the platform the project promises to develop on. A missing interface is now `Unknown` rather than `Down`. It still blocks a new diversion — a name that is simply wrong should — but it can no longer undo a working one, which is the module's own rule: an unreadable sysfs file is a bad reason to move a customer's prefix across the Internet, and a typo in `return-tunnel` is the same class of thing. `Gate::Down` and `Gate::Degraded` now carry the reason too. `let _ = why;` discarded it, so the daemon logged a bare `state=down` for what this module calls the most urgent condition it can report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s build ignores Module sections are bound at start: each module parses its section once into its own state, and the reload path replaces only the shared `Config`. So an edit was accepted, reported "OK reloaded", and did nothing. Adding `never-blackhole` mid-incident exited 0 while the guard stayed absent, and adding a `divertible-prefix` split the planner's view from the module's, so every affected diversion was refused as not-divertible until a restart. Refused rather than applied: applying it means rebuilding a module, and a module's state *is* its damping — every dwell, hold clock and ceiling would restart at once, mid-incident. The comparison is on content rather than `==`, because `ModuleSection` carries source lines and a plain equality check called a module changed whenever an edit above shifted it down. Separately, five directives were parsed, validated with tailored errors, documented per-directive in example.conf, and read by nothing. `failure-threshold`, `policy-loss-grace` and `converge-deadline` are now refused at load; `on-policy-loss` accepts `hold` and refuses `drain`, which would withdraw protection because a policy engine was unreachable and cannot be reconciled with the additive-only invariant. An accepted inert directive is strictly worse than a typo: a typo is caught, where this leaves an operator believing a guard is in force. example.conf now documents each refusal instead of a feature that is not there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e, so nothing ever diverted The slow tier could not complete an engagement. `set_quorum` was the only source of QuorumMet/QuorumImpossible/QuorumLost, it was not on `TierModule`, and nothing called it — no daemon path, no test. `advance_timers` has no dwell for `Announcing`, so a demanded prefix announced to the scrubber and waited there: 600s of continuous demand still reported `[(198.51.100.0/24, Announcing, 600)]`, transit still carrying, nothing scrubbed, and no error anywhere saying so. Wires it up. `PathEvidence` and `TierModule::observe` in common, a `RibObserver` bound on the reconciler, and verdicts gathered per held path before the modules decide — confirmation is an input to the sequence, not a report on it. Peers are matched by address, since a sidecar names neighbours by address and knows nothing of the names an operator chose. `MIN_QUORUM_FIDELITY` is `PolicyEligible`, so `Synthetic` never counts and observe mode cannot half-believe a diversion. Three separate routes reached `Diverted` without a quorum, all now closed: - `(Restoring | Draining, Demanded)` raised the signal again immediately. The machine is in a teardown because the quorum was lost or the return path failed, and neither is undone by the demand returning. Now resumes at `Announcing`, keeping the scrubber path but re-running the confirmation and the dwell. - `adopt` inserted `Machine::recover(State::Diverted)`, which is the identity, so any path in the RIB at startup was assumed fully diverted and put the signal straight up. Now resumes at `Announcing` — the earliest state consistent with "a path exists", which is what its comment already claimed. - `Machine::recover(Diverted)` trusted a journaled diversion. The quorum evidence died with the sessions that gave it, so it re-confirms too. That also makes the journal's write order non load-bearing, which is a better place for the guarantee than a comment about write order. `invariant_holds` cannot see any of this: `scrubber_up()` means "we are announcing", not "the scrubber accepted it", so all three satisfied it. Its docstring now says so, and two exhaustive tests carry the operational half — `the_signal_only_ever_rises_out_of_settling` and `no_recovery_path_raises_the_signal`. Also: an inherited diversion was never released, because `to_release` required a `demanded` flag that `adopt` and `restore` set false and only the engage path set true. Ten hours of zero demand left it `Diverted`, with no `max-lifetime` here to catch it. The gate is gone, and so is the field, which was left write-only. `MockSpeaker::list_originated` ignored injected faults and always returned `Ok`, so the reconciler's unreadable-RIB branch — the guard against computing a difference from nothing — was unreachable from any test, and the test claiming to cover it asserted nothing. It now honours `Unreachable`, and the tick reads the speaker once instead of twice: two reads let a transient failure on the first defer adoption while the second succeeded, converging with no module having adopted and every inherited path computed as surplus. Finally, `states_for_status` was uncalled. `TierModule::progress` surfaces it as a SEQUENCES block in `status`, and a sequence stuck announcing raises an ATTENTION line — it is not mitigating anything, and it looks finished in the engagement list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…UNSPECIFIED `LOOKUP_EXACT` was a hand-written `0`. In this proto that is `TYPE_UNSPECIFIED`; `TYPE_EXACT` is 1. Every confirmation query was asking for the wrong lookup option, and nothing caught it because an `i32` matches any `i32`. This file already argued that GoBGP v4's typed oneof means "a field that moves is a compile error here rather than a wire disagreement discovered during an incident" — a numeric literal opts straight back out of that. All six protocol constants now come from the generated enums, and a test fails if anyone writes a literal back in. It matters more than it did: `advertised` was unreachable code until the quorum was wired to it, and now it decides whether transit gets suppressed. `withdraw` also built its delete request through `build_path`, which attached the origin community but not the tier's — `PathKey` does not carry them and cannot reconstruct them — so the delete advertised a different attribute set than the origination. Sending a near-miss set to a matcher is worse than sending none: a field that is wrong can fail to match, where one that is absent is not part of the match. It now carries family and NLRI only. And `list_originated` says what actually happens. The comment claimed "the reconciler will re-originate what it wants and never withdraw what it does not recognise", which reads like a safe degraded mode. `Reconciler::tick` treats a failed read as no information about the world and returns without touching anything, so a daemon on this backend logs one warning per tick, forever, and converges nothing — a no-op, not a degraded mode. The docstring now names the actual blocker too: a path's tier is not recoverable from its prefix and origin community, and `divert` and `divert-signal` are the indistinguishable pair whose confusion suppresses transit for a prefix nobody is scrubbing. Nothing reaches it today, since `mode enforce` refuses to start, but it should be the first thing whoever wires enforce up reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he real journal write order CLAUDE.md said "There is a `clippy.toml` rule enforcing this" about monotonic damping timers. There is no clippy.toml in the tree and no `disallowed-methods` config anywhere, so a contributor could read the rule as mechanically enforced, add a `SystemTime::now()` to a damping path, and watch CI pass. Restated as the review rule it actually is, with a note on what enforcing it would cost — an `#[allow]` at each of the half-dozen legitimate uses — so the option stays on the table rather than being quietly dropped. journal.rs claimed the journal is "written before the actions it authorises", which the daemon has never done: a tick decides and acts as one step, and the journal is written when it returns. So a process killed mid-tick left an entry that *under*-stated progress — the direction the comment said was dangerous. Recovery no longer depends on it either way, so the docstring now describes the ordering that exists and says why it is not load-bearing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds filterframe from an empty repository to a daemon that runs the full
mitigation loop end to end in observe mode: 14 commits, 253 tests,
make lintclean on the pinned Rust 1.97.1.
What this is
A DDoS mitigation control plane for Linux. It reads the active mitigation set
from a policy engine and carries out the decisions as BGP: blackholing a host
upstream for short sharp attacks, or diverting a prefix to a scrubbing provider
and back for large sustained ones. It runs on any Linux box that can hold BGP
sessions and is agnostic about which router or provider is on the other end.
The rule everything derives from
filterframe only ever adds BGP objects. It never withdraws a route that is
carrying traffic.
This is why it does not originate protected prefixes. The edge keeps announcing
those unconditionally; filterframe announces a separate signal route, and the
edge's export policy suppresses the prefix while it is present. If filterframe
crashes, is killed, or is simply stopped, the signal ages out and normal routing
returns on its own.
Verified behaviour
Run against a stub policy engine, the log is the argument:
The engine failing held both engagements. The engine recovering with an empty
list was refused twice as unconfirmed before being believed. SIGTERM left
everything standing.
Three things worth a reviewer's attention
The safety invariant is structural, not aspirational.
MitigationView::Stalecarries no mitigation list, so there is no expression anywhere that computes a
withdrawal from a failed poll — "unreachable policy engine causes a teardown" is
a type error rather than a code-review finding.
stale_never_tears_downistable-driven over all seven failure reasons across twenty consecutive failed
polls, and should never be deleted.
The divert sequence is proved by exhaustion. It is a pure state machine with
no I/O, so the safety property (
signal_up implies scrubber_up) is checked byenumerating every state against every event, walking every reachable sequence to
depth six, and injecting a crash at every index of five representative
sequences.
Four real bugs were caught by tests rather than by review, and each is
described in the commit that fixed it: a ceiling measuring from the wrong clock
that expired live mitigations and re-announced them in the same tick; a
return-path gate that recovered on a single success, so a flapping tunnel would
have read as healthy forever; a verdict loop reporting a policy rejection while
another path was being advertised fine; and a three-valued signal collapsed to a
boolean, which would have torn down a working diversion over an unreadable sysfs
file.
What is deliberately not done
mode enforcerefuses to start. The GoBGP backend is compile- andunit-tested but has never met a live sidecar, and
list_originatedreturnsUnknownrather than guessing which paths in a shared RIB are ours — gettingthat wrong means withdrawing another controller's work. Announcing nothing while
reporting success would be worse than refusing.
The embedded BGP speaker is not implemented.
netgauze-bgp-speaker0.13.0has no public route-origination path; the workable route is driving
Peerthrough a six-parameter generic whose cancel-safety is undocumented, on a crate
with 24 downloads at that version. Being the first production user of a Rust BGP
originator inside a DDoS mitigation path is a bad place to be first.
rustybgpimplements GoBGP's gRPC API, so a pure-Rust speaker later is a configuration
change rather than a rewrite. Both decisions are written up in
docs/runbooks/bgp-backends.md.Blocking prerequisite before enforce
The edge's export policy must suppress the protected prefix when it sees the
signal route. filterframe cannot self-check this, and says so rather than
implying a clean bill of health.
docs/runbooks/edge-policy.mdhas the FRR,Junos and IOS-XR forms; the test that matters is announce the signal,
kill -9the thing announcing it, and confirm the prefix comes back on its own.
FRR has had bugs in conditional advertisement, so verify on the exact build.
Structure
Mirrors packetframe:
crates/{common,bgp,policy,cli}plus feature-gated tiermodules under
crates/modules/. Same conventions — thiserror only, hand-writtenline-based config grammar, Prometheus textfile rather than an HTTP endpoint,
systemd and
.debwith no Docker.Packaging lands in the second commit rather than the last, so CI has been
producing installable
.debartifacts and install-verifying them in a cleancontainer from the beginning. The systemd unit ships with an empty capability
set — filterframe needs no privilege to hold BGP sessions and read sysfs, and
that emptiness is what makes the additive-only rule enforceable rather than
merely intended.
Reviewing
Start with
conf/example.conf. It is a primary document, annotated perdirective with defaults and reload semantics, and a test parses it verbatim so
it stays accurate. Then
crates/modules/scrub-divert/src/machine.rsfor theordered sequence, and
crates/common/src/mitigation.rsfor why the stale viewhas no list in it.