916b7a3 · skrepka · git.iko.soy

git.iko.soy/skrepka

Fix bug #17: enforce isBadServerName at federation egress

The denylist's comment claimed it ran at egress, but only handleRecvGossip (ingress) checked it — peer state persisted before a denylist tightening could still steer outbound fetches at internal hosts. Now sendBatchedGossip and forkForwardsTo skip denylisted servers, and retryPendingForwards drops outbox entries aimed at them (they would never be attempted, so they'd otherwise sit in the outbox forever). Note: the outbox cleanup evaluates isBadServerName outside the atomic filter predicate — pushing it down sends the current knot compiler into unbounded recursion (stack overflow); only plain equality filters go inside atomic.

commit916b7a3810f47c5481711606645b14a75bcf34e9 authoriko <mail@iko.soy> dateThu 11 Jun 2026, 14:00 (4 months ago) parentfbf23aa

1 file changed+21−10

compare with master

unifiedsplit

server.knot+21−10

server.knot+21−10

@@ -406,8 +406,11 @@ else (if take 1 s == c then 1 else 0) + countChar c (drop 1 s)

406406-- 169.254/16, the IPv4 zero net, bare IPv6 literals (any string with more than one `:` — covers loopback

407407-- `::`/`::1`, link-local `fe80::/10`, ULA `fc00::/7`, globals like `2001:db8::1`),

408408-- bracketed IPv6 literals, and the `.local` / `.internal` / `.arpa` / `.onion`

409−-- suffixes. Called at federation egress (so we never `fetch` a peer pubkey from

410−-- loopback) and on inbound requests (so a peer can't claim a bogus `fromServer`).

409+-- suffixes. Enforced on inbound requests (`handleRecvGossip`, so a peer can't

410+-- claim a bogus `fromServer`) and again at every federation egress site

411+-- (`sendBatchedGossip`, `retryPendingForwards`, `forkForwardsTo`) as

412+-- defense-in-depth — state persisted before a denylist tightening, or any

413+-- future ingress gap, must not steer a `fetch` at an internal host.

411414-- A legitimate `host:port` server name has exactly one `:`, so the multi-colon

412415-- check leaves that case intact.

413416isBadServerName : Text -> Bool

@@ -597,14 +600,14 @@ pruneExpired = \t -> atomic do

597600-- a peer record into `forwardFailures` (shared with forward retries) so both

598601-- subsystems back off together.

599602

600−-- Fan out one bundle of gossip events to each peer, skipping peers in backoff.

601−-- No-op when `events` is empty so callers don't need to guard. Records or

602−-- clears federation-failure counters per call.

603+-- Fan out one bundle of gossip events to each peer, skipping denylisted

604+-- peers and peers in backoff. No-op when `events` is empty so callers don't

605+-- need to guard. Records or clears federation-failure counters per call.

603606sendBatchedGossip : [Text] -> [GossipEventType] -> IO {rw *forwardFailures, network, clock} {}

604607sendBatchedGossip = \peers events -> when (count events > 0) (do

605608 t <- now

606609 failures <- *forwardFailures

607− forEach peers (\server -> when (shouldRetryServer server t failures) (do

610+ forEach peers (\server -> when (not (isBadServerName server) && shouldRetryServer server t failures) (do

608611 result <- fetch (federationUrl server) (RecvGossip {events, fromServer: serverHost})

609612 case result of

610613 Err _ -> recordFailure server t

@@ -628,7 +631,15 @@ retryPendingForwards = do

628631 t <- now

629632 fwds <- *forwards

630633 failures <- *forwardFailures

631− forEach (filter (\s -> shouldRetryServer s t failures) (collectPeers fwds (\f -> f.toServer))) (\server -> forEach (take maxBatchSize (filter (\f -> f.toServer == server) fwds)) (\f -> do

634+ forEach (collectPeers fwds (\f -> f.toServer)) (\server -> if isBadServerName server

635+ -- Denylisted destination (state persisted before a denylist tightening):

636+ -- its forwards would never be attempted, so drop them instead of letting

637+ -- them sit in the outbox forever. The denylist check stays outside the

638+ -- atomic predicate — only the plain equality filter is pushed down.

639+ then atomic (do

640+ cur <- *forwards

641+ *forwards = filter (\x -> x.toServer != server) cur)

642+ else when (shouldRetryServer server t failures) (forEach (take maxBatchSize (filter (\f -> f.toServer == server) fwds)) (\f -> do

632643 result <- fetch (federationUrl server) (ForwardMessage {f.toKey, f.encryptedBlob})

633644 success <- case result of

634645 Err _ -> do

@@ -648,7 +659,7 @@ retryPendingForwards = do

648659 msgs <- *messages

649660 *messages = map (\m -> if m.toKey == f.toKey && m.encryptedBlob == f.encryptedBlob && not elem server m.deliveredTo

650661 then {m.toKey, m.encryptedBlob, m.receivedAt, deliveredTo: union m.deliveredTo [server]}

651− else m) msgs))))

662+ else m) msgs)))))

652663

653664-- Forward queued messages to a server when a remote key comes online.

654665-- Skips messages already delivered to `server` (tracked via Message.deliveredTo)

@@ -870,7 +881,7 @@ handleSendMessages = \messages authorization clientIp -> do

870881-- have the message. On failure the forwards row stays so

871882-- `retryPendingForwards` can re-attempt with backoff.

872883forkForwardsTo : PubkeyHex -> BlobHex -> [Text] -> IO {rw *forwardFailures, rw *forwards, rw *messages, network, clock} {}

873−forkForwardsTo = \to encryptedBlob servers -> forEach servers (\server -> do

884+forkForwardsTo = \to encryptedBlob servers -> forEach servers (\server -> when (not isBadServerName server) (do

874885 t <- now

875886 result <- fetch (federationUrl server) (ForwardMessage {toKey: to, encryptedBlob})

876887 let same = forwardMatches encryptedBlob server

@@ -884,7 +895,7 @@ forkForwardsTo = \to encryptedBlob servers -> forEach servers (\server -> do

884895 msgs <- *messages

885896 *messages = map (\m -> if m.toKey == to && m.encryptedBlob == encryptedBlob && not elem server m.deliveredTo

886897 then {m.toKey, m.encryptedBlob, m.receivedAt, deliveredTo: union m.deliveredTo [server]}

887− else m) msgs))

898+ else m) msgs)))

888899

889900-- ============================================

890901-- Handler: federation receive