Peer bans do not survive a node restart: the connection gater's persistent ban check is never written

[HosniBelfeki]

Summary

A peer ban does not survive a node restart, so the connection gater silently stops enforcing it while the banned peer's biscuit is still valid.

The gater (internal/node/gate.go) is what stops a node dialling or accepting a banned peer. InterceptPeerDial and InterceptSecured each consult two sources:

if g.node.revokedPeers != nil && g.node.revokedPeers.Contains(p.String()) { return false }
if g.node.Store.IsBanned(p) { return false }

Both are empty after a restart:

1. revokedPeers is an lru.Cache bounded at RevocationCacheSize (10,000) and held only in memory, so it dies with the process.
2. Store.IsBanned reads the banned_peers bucket — and nothing in the node ever writes it. The bucket is created (store.go:82) and read (store.go:319), but the writer was never added, so in production IsBanned always returns false.

Nothing re-tells a restarted node either. MeshEvent_BANNED is published exactly once, at the moment of the ban (server.go:1699 and server.go:1976), and gossip has no replay. There is no startup reconciliation. The control plane does record the ban durably via SetNodeBanned, so the truth exists — it is just never propagated again.

Net effect: an operator revokes a compromised peer. Every node learns it over gossip and enforces it. Any node that then restarts — deploy, crash, upgrade, autoscale — resumes dialling and accepting that peer for the remainder of its already-issued biscuit.

This is distinct from the /refresh enforcement documented in control-plane-configuration.md. That stops the banned node renewing its own token; it does nothing to make other nodes refuse a peer whose current biscuit is still valid. Peer-side refusal is the gater's job, and the gater's state is non-durable.

Why this reads as an oversight rather than a design choice

The sibling mesh event already has exactly this durability:

handleKeyRotationEvent → addTrustedKey → persistTrustedKeys → Store.SaveTrustedKeys, whose own comment states the principle:

"persists the full set of control plane public keys the node currently trusts, so keys learned from rotation events or /keys survive restarts"

handleBannedEvent updates only the in-memory cache. Same event source, same durability requirement, different treatment — and the persistent bucket plus the IsBanned reader wired into both gater methods show the intent was there.

Existing coverage cannot see it: TestConnectionGater's banned-peer case writes the bucket directly with store.db.Update(...), because until now there was no API to write it. The test proves the gater reads the bucket; nothing proves anything fills it.

Reproduction

Deliver a BANNED event, confirm the gater denies the peer, then close and reopen the store with a fresh cache (a restart) and ask again:

--- FAIL: TestBannedEventSurvivesRestart
    gate_test.go:351: after a restart the gater still dials a banned peer: the ban did not survive
    gate_test.go:354: after a restart the gater still accepts a banned peer: the ban did not survive

Proposed fix, and what it does not cover

The narrow part is a missing writer, and I've opened a PR for it: persist the ban in handleBannedEvent so the bucket the gater already reads is populated. That closes the restart case.

Two related gaps need a design decision rather than a missing writer, which is why I'm raising them here instead of folding them into that PR:

1. A node that was offline when the event was published never learns the ban at all. Persisting on receipt does not help a node that never received it. Options: reconcile against the control plane at startup, or have nodes fetch a ban list alongside /keys. Both add an API surface and a trust/staleness question, so they seem like yours to choose.

2. internal/router has the same lifetime problem. router.go:612 stores bans in an in-memory sync.Map (r.bannedPeers.Store(...)) with no persistence, so a router restart forgets them the same way. Whether routers should persist bans or always reconcile centrally probably depends on the answer to (1).

Happy to implement either direction once you've said which shape you want.

Environment

main at 935be05.

[HosniBelfeki]

Updating this, because the fix I proposed in the description above is not the one being made, and I don't want the original text read as the plan.

The persistence approach was the wrong call. I suggested writing the ban to the banned_peers bucket in handleBannedEvent. @aojea pointed out why that doesn't work, and he's right: a ban on disk can't be cleared by the control plane, since SetNodeBanned takes a bool but there is no unban event — so the record would have been permanent short of deleting the node's database. It also does nothing for the offline case, which is the harder half. Persisting bans in bbolt was an early idea in this repo that was deliberately never finished.

What #356 does instead is reconcile against the control plane's full ban set, published in GET /info:

• ControlPlaneInfoResponse.banned_peer_ids carries the complete set, so absence from it is the unban signal.
• sam-node reconciles at startup in SyncMeshConfig; sam-router in runFederationLoop, which already polls /info every 30s.
• MeshEvent_BANNED stays as the sub-second path for peers already up.
• The unused banned_peers bucket and Store.IsBanned are removed rather than filled in.

Both items I listed as out of scope are covered by that, so nothing from this issue is left unaddressed:

1. A node that was offline when the event was published now picks the ban up from /info at startup — reconciling against current state doesn't care what was missed.
2. internal/router's in-memory sync.Map is still in memory, but it is now reconciled every 30s, so a router restart recovers within one poll.

One thing worth flagging that I hit while implementing it: reconciling by replacement — which is what makes unban work — opens a window on router startup where the first /info is still in flight while gossip is live, and a stale answer undoes a ban that just arrived. TestRouterGossipSubBannedEvent caught it. Since the control plane writes the ban before publishing the event, an answer issued after a ban must carry it; reconcileBannedPeers therefore takes the request-issue time and leaves alone anything banned since.

What's actually still open here is a question, not a defect: whether routers should persist anything at all, versus always reconciling centrally. #356 takes the reconcile-only route for both node and router, which I read as the intent — happy to change it if not.

Leaving this open; #356 declares the link, so it will close on merge.