node,router: reconcile peer bans from the control plane instead of a local store - #356
node,router: reconcile peer bans from the control plane instead of a local store#356HosniBelfeki wants to merge 6 commits into
Conversation
The connection gater is what stops a node dialling or accepting a banned peer (InterceptPeerDial, InterceptSecured). It consults two things: the in-memory revocation cache and Store.IsBanned. The cache is an LRU bounded at RevocationCacheSize and it dies with the process. Store.IsBanned reads the banned_peers bucket -- which nothing in the node ever writes. The bucket is created and read, and both gater methods consult it, but the writer was never added, so in production IsBanned always answers false. Both of the gater's ban checks are therefore empty after a restart. The control plane publishes MeshEvent_BANNED exactly once, at the moment of the ban (/admin/revoke), and gossip has no replay, so nothing re-tells a restarted node. It resumes dialling and accepting the peer for as long as the peer's already-issued biscuit is valid. The control plane does record the ban durably (SetNodeBanned); only the nodes forget it. The sibling mesh event already has this durability: handleKeyRotationEvent -> addTrustedKey -> persistTrustedKeys -> SaveTrustedKeys, whose own comment says it exists "so keys learned from rotation events or /keys survive restarts". handleBannedEvent updated only the cache. This adds the missing writer and calls it from handleBannedEvent, so the bucket the gater already reads is actually populated: - Store.SaveBannedPeer keys the record by the decoded peer's canonical String(), the same form IsBanned looks up, so a differently encoded peer id in an event cannot write a record the reader would miss. - persistBannedPeer mirrors persistTrustedKeys: a write failure is logged and does not abort the in-memory eviction, which is still the fastest way to stop honouring the peer. TestBannedEventSurvivesRestart delivers a BANNED event, asserts the gater denies the peer, then closes and reopens the store with a fresh cache -- a restart -- and asserts it still denies. Both assertions fail without this change. TestConnectionGater passes either way: its banned-peer case writes the bucket directly through store.db.Update, because until now there was no API to write it, so the existing coverage could not observe the gap. Deliberately out of scope, and raised separately: a node that was offline when the event was published still never learns the ban (there is no startup reconciliation against the control plane), and internal/router keeps its bans in an in-memory sync.Map with the same lifetime problem. Both need a design decision rather than a missing writer. No new dependency: bbolt and peer are already imported in both files.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that peer bans survive node restarts by persisting them to the local store when a BANNED mesh event is handled. It introduces the SaveBannedPeer method to write the ban to the database, along with a corresponding integration test. The review feedback suggests validating that the peer ID is not empty before executing the database transaction to prevent potential errors, and using tx.Bucket instead of tx.CreateBucketIfNotExists for consistency with other store operations.
| func (s *Store) SaveBannedPeer(p peer.ID) error { | ||
| return s.db.Update(func(tx *bbolt.Tx) error { | ||
| b, err := tx.CreateBucketIfNotExists([]byte(bucketBannedPeers)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return b.Put([]byte(p.String()), []byte("true")) | ||
| }) | ||
| } |
There was a problem hiding this comment.
To prevent unnecessary database transactions and potential database errors (such as bbolt.ErrKeyRequired when attempting to write an empty key), we should validate that the peer ID is not empty before starting the update transaction. Additionally, since bucketBannedPeers is already created during NewStore initialization, we can use tx.Bucket directly instead of tx.CreateBucketIfNotExists to remain consistent with other write operations in this file (e.g., SaveIdentity and SaveTrustedKeys).
| func (s *Store) SaveBannedPeer(p peer.ID) error { | |
| return s.db.Update(func(tx *bbolt.Tx) error { | |
| b, err := tx.CreateBucketIfNotExists([]byte(bucketBannedPeers)) | |
| if err != nil { | |
| return err | |
| } | |
| return b.Put([]byte(p.String()), []byte("true")) | |
| }) | |
| } | |
| func (s *Store) SaveBannedPeer(p peer.ID) error { | |
| if p == "" { | |
| return errors.New("empty peer ID") | |
| } | |
| return s.db.Update(func(tx *bbolt.Tx) error { | |
| b := tx.Bucket([]byte(bucketBannedPeers)) | |
| return b.Put([]byte(p.String()), []byte("true")) | |
| }) | |
| } |
There was a problem hiding this comment.
Both applied in 47ab0e7 — thanks, they're good catches.
Empty peer: correct that bbolt answers ErrKeyRequired, so the old version would have spent a write transaction to produce a confusing error. Rejected up front now. Worth noting it wasn't reachable from handleBannedEvent, which only calls this inside a successful peer.Decode — but SaveBannedPeer is exported, so this is about the contract rather than the one call site.
tx.Bucket: I checked the "already created in NewStore" claim before taking it, since a nil bucket would turn b.Put into a nil dereference. It holds: NewStore is the only constructor and creates banned_peers unconditionally, and ResetMeshIdentity deletes keys inside bucketIdentity rather than any bucket. So the defensive form wasn't buying anything real, and keeping it would only have made this writer fail differently from the other five if that ever changed. Switched to match SaveIdentity/SaveTrustedKeys.
Also added TestSaveBannedPeer, covering the round trip and the empty-peer rejection. The round-trip assertion is the one that matters: if the writer and IsBanned ever disagreed on the key format, the gater would read straight past a ban that had been recorded — which is the same class of gap this PR exists to close.
…cket access Review feedback on google#356. Both points check out: - bbolt answers ErrKeyRequired for an empty key, so an empty peer would have cost a write transaction to produce a confusing error. It is a caller bug, so it is now rejected up front. SaveBannedPeer is exported, so this is about the contract, not just the one call site -- handleBannedEvent only reaches it inside a successful peer.Decode. - tx.CreateBucketIfNotExists is replaced by tx.Bucket, matching SaveIdentity, SaveTrustedKeys and every other writer in this file. Verified the bucket is always there: NewStore is the only constructor and creates banned_peers unconditionally, and ResetMeshIdentity deletes keys inside bucketIdentity rather than any bucket. Keeping the special case would only have made this one writer fail differently from the other five if that ever changed. TestSaveBannedPeer covers the round trip -- asserting IsBanned observes what SaveBannedPeer wrote, since a key-format disagreement between the two would read straight past a recorded ban -- and the empty-peer rejection.
|
Thanks for digging into this and catching the unused To give some background: persisting bans in bbolt was the initial idea early, but it was never finalized because storing bans on disk in the node has fundamental issues with stale state and unban, offline nodes and the new routers role.t problem. Because of this, we should remove that stale bbolt code ( I think the right way to handle startup reconciliation without introducing a new endpoint is to extend the existing endpoint
If you'd like to take this on, feel free to update this PR (or open a new one) to remove the stale bbolt code and implement the |
MeshEvent_BANNED is broadcast once, at the moment of the ban, and gossip has no replay. A node or router that restarted, or that was down when the event went out, therefore never learns the ban -- and nothing re-tells it, even though the control plane holds the ban durably in its own database. /info is already fetched by sam-node at startup and by sam-router every 30s, so it is where that catch-up belongs. This adds the whole ban set to ControlPlaneInfoResponse rather than an incremental feed, because the full set is also the only signal an *unban* has: there is no unban event, so a consumer that merged deltas could never let a peer back in. A failure to read the ban set fails the request rather than answering without the field. An empty list is a meaningful value -- it means nothing is banned -- so serving one on error would tell every consumer to clear its blocklist. api/sam.pb.go is regenerated with the pinned toolchain the committed file records (protoc v3.21.12, protoc-gen-go v1.36.12), verified beforehand to reproduce it byte for byte so this diff is only the new field. No consumers yet; they follow.
Nodes and routers only learned bans from MeshEvent_BANNED, which is published once with no replay, so a restart or any downtime left them enforcing nothing. Both now reconcile against the ban set in /info, which they already fetch: sam-node at startup via SyncMeshConfig, sam-router in runFederationLoop. The event stays as the sub-second path for peers that are already up. Reconciliation replaces the local blocklist instead of merging into it. That is what makes an unban work -- the control plane can clear a ban and there is no event for it, so a peer that drops off the list has to drop out of the blocklist too. Replacing raises a race the router hits on startup, where the first /info is still in flight while gossip is already live: a stale answer would immediately undo a ban that had just arrived. The control plane writes the ban to its database before publishing the event, so an answer *issued after* a ban is guaranteed to carry it, while one issued before cannot speak to it at all. reconcileBannedPeers therefore takes the time the request was issued and leaves alone any ban recorded since. bannedPeers stores that timestamp; every reader only tests presence. - node: SyncMeshConfig returns the ban set alongside the key and router addresses, and Options.BannedPeerIDs seeds the revocation cache in NewSamNode so the gater enforces existing bans from the first connection. A nil result means the control plane was not reached, which is not the same as "nothing is banned", so it clears nothing. - router: reconcileBannedPeers evicts any prior admission for a newly banned peer, and skips undecodable entries rather than discarding the valid ones. TestReconcileBannedPeers covers the unban case, which is the point of replacing rather than merging, and the malformed-entry case. TestRouterGossipSubBannedEvent already covered the startup race described above; it fails without the fetch timestamp.
The banned_peers bbolt bucket was created and read -- Store.IsBanned was wired into both connection gater methods, peerIsRevoked and the debug connect handler -- but nothing in the node ever wrote it, so IsBanned always answered false. The only writer was TestConnectionGater, which put a key in through store.db.Update directly, so the test proved the gater read the bucket while nothing proved anything filled it. Persisting bans locally was an early idea that was never finished, and it should not be: a ban on disk cannot be cleared by the control plane, since there is no unban event, and it says nothing about a node that was offline when the ban was published. Reconciling against the ban set in /info on every start handles both, and now does (previous commit). So this drops the bucket, IsBanned and its four call sites. peerIsRevoked keeps the revocation cache, which is now seeded from /info at startup, and the gater keeps its cache check -- the only one of its two checks that was ever able to answer true. TestConnectionGater's third case becomes what it can now assert: a peer the control plane does not ban stays reachable. TestGaterEnforcesSeededBans replaces what the store case was reaching for, checking the gater denies a peer seeded from the control plane's ban set and does not deny unrelated peers. TestStore_IsBanned goes with the method it covered.
HosniBelfeki
changed the title
|
Thanks @aojea — that background was the part I was missing, and you were right to push back on the bbolt route. The unban problem in particular is a real flaw in what I proposed and I should have caught it: Implemented your design, pushed as three commits so each is reviewable and builds on its own:
One thing I hit that's worth your eye. Reconciling by replacement — which is what makes unban work — opens a window on router startup, where the first Since the control plane writes the ban before publishing the event, an answer issued after a ban must carry it, while one issued before can't speak to it. So I left the router-persistence question open in #355 rather than deciding it here — this takes the reconcile-only route for both node and router, which I read as your intent, but happy to change it. Also updated the PR title and description, since the old ones described the approach you rejected. |
Resolves a conflict in internal/controlplane/server_test.go, where both sides appended tests at the end of the file: TestHandleInfoPublishesBanSet here, TestInitRegisterRoutesEmbedded and TestAdminBootstrapTokensList upstream. Git factored out the closing braces the two blocks shared, so both are kept and the first is closed explicitly. No test logic changed. Nothing in this branch collides semantically with what landed upstream. The closest change is the revocation fix that gates /routers/lease with CheckAdmission: that stops a revoked router renewing its own lease, while this branch stops nodes and routers honouring a banned peer they already know about. Same 24h offline-verification window, opposite ends of it. api/sam.proto and api/sam.pb.go both auto-merged and were verified to still agree: regenerating from the merged proto reproduces the merged generated file exactly, so the generated-code check still passes.
2 participants
Fixes #355.
Implements the design @aojea laid out in review: drop the unfinished on-disk ban store, and reconcile bans against the control plane's ban set carried in
GET /info.The problem
The connection gater is what stops a node dialling or accepting a banned peer. It consulted the in-memory revocation cache and
Store.IsBanned, and both were empty after a restart:revokedPeersis anlru.Cache, bounded and held only in memory.Store.IsBannedread thebanned_peersbucket, which nothing in the node ever wrote. It was created and read, and wired into both gater methods,peerIsRevokedand the debug connect handler, but the writer was never added — so in production it always answeredfalse. The only writer wasTestConnectionGater, putting a key in throughstore.db.Updatedirectly, which is why the gap was invisible: the test proved the gater read the bucket while nothing proved anything filled it.MeshEvent_BANNEDis published once, at the moment of the ban, and gossip has no replay, so nothing re-tells a restarted node. It resumes serving a revoked peer for as long as its already-issued biscuit is valid. The control plane holds the ban durably in its own database; only the nodes forget it.This is separate from the
/refreshenforcement incontrol-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.Why not just persist it
That was this PR's first attempt, and @aojea was right to reject it. A ban on disk cannot be cleared by the control plane —
SetNodeBannedtakes 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 a node that was offline when the ban was published, which is the harder half of the problem.Reconciling against a full set on every start handles both, and is the reason the field is the whole ban list rather than an incremental feed: absence from that list is the only signal an unban has.
The change
Three commits, each building on its own:
api,controlplane—ControlPlaneInfoResponse.banned_peer_ids(field 5), populated inHandleInfofromnodeswherebanned = truevia a newstorage.Store.ListBannedPeerIDs. A failure to read it fails the request rather than answering without the field: an empty list means "nothing is banned", so serving one on error would tell every consumer to clear its blocklist. No consumers yet.node,router—sam-nodereconciles at startup inSyncMeshConfig, which now returns the ban set;Options.BannedPeerIDsseeds the revocation cache inNewSamNodeso the gater enforces from the first connection.sam-routerreconciles inrunFederationLoop, which already polls/infoevery 30s. The event stays as the sub-second path.node— removes the bucket,Store.IsBannedand its four call sites.api/sam.pb.gois regenerated with the toolchain the committed file records — protoc v3.21.12, protoc-gen-go v1.36.12 — which I verified reproduces the existing file byte for byte first, so the diff is only the new field.One race worth flagging
Replacing rather than merging introduces a window the router hits on startup, where the first
/infois still in flight while gossip is already live: a stale answer would immediately undo a ban that had just arrived.The control plane writes the ban to its database before publishing the event, so an answer issued after a ban is guaranteed to carry it, while one issued before cannot speak to it at all.
reconcileBannedPeerstherefore takes the time the request was issued and leaves alone any ban recorded since;bannedPeersstores that timestamp, and every reader only tests presence.This was not hypothetical —
TestRouterGossipSubBannedEventfailed without it, which is how I found it.Tests
TestReconcileBannedPeers— the unban case, which is the point of replacing rather than merging, plus the malformed-entry case.TestHandleInfoPublishesBanSet—/infocarries exactly the banned node, and an unban shows up as absence.TestGaterEnforcesSeededBans— the gater denies a peer seeded from the ban set, and seeding does not deny unrelated peers. This replaces what the deleted store case was reaching for.TestRouterGossipSubBannedEvent(existing) — now also covers the startup race; it fails without the fetch timestamp.TestConnectionGater's third case becomes what it can honestly assert: a peer the control plane does not ban stays reachable.Each of the three commits was checked out in a separate worktree and built and vetted on its own, so the branch is bisectable.
Still open in #355
Whether routers should persist anything at all, versus always reconciling centrally, is the one question I left in the issue — this takes the reconcile-only route for both, which I believe matches what you described, but say the word if you'd rather routers behaved differently.
Local verification caveat
Seven tests fail on my Windows workstation both with and without this change — the six Unix-socket and subprocess-backend tests in
internal/node, plusTestNodeProactiveTokenRefresh, which fails inTempDircleanup because Windows will not unlink an open bbolt file. I verified the identical set fails on a clean checkout of this branch's merge base.make lintandmake e2e-testneed a Linux toolchain I don't have locally, so CI is the authority for the full gated matrix.