fix(core): verify federation peer certificates against a trusted allowlist - #39
Merged
Conversation
FederationManager accepted any certificate on inbound or outbound TLS connections — rejectUnauthorized: false is required since these are self-signed with no CA, but nothing verified which self-signed cert was presented, so any certificate was treated as a valid federation peer. Add fingerprintDer(), which hashes a live tls.PeerCertificate's raw DER the same way getCertificateFingerprint() hashes a PEM certificate, so a fingerprint pinned from one form compares equal to the other presented live over a socket.
…owlist Neither direction of a federation link checked the peer's certificate fingerprint: connect() accepted whatever the remote server presented, and handleInbound() accepted whatever the remote client presented and immediately synced full mesh state (every visible agent, every federated room's membership) to it. Federation trusted nobody in particular and everybody at once. Add a per-instance trusted-fingerprint allowlist (empty by default — federate with nobody until an operator explicitly pins a remote mesh's fingerprint, the same no-CA pin-the-key model ordinary peer connections already use) and verify the presented certificate against it before a link is created in either direction. An unpinned certificate gets the socket destroyed before any handshake is processed. Also add listen()/stopListening(): a real production TLS server for inbound federation connections. Nothing previously stood one up — handleInbound() existed only as a function the integration test called against its own hand-rolled tls.createServer.
Extend the CommsStore interface with the operations needed to actually use federation's new fingerprint allowlist: getFederationFingerprint, fedTrust/fedUntrust/fedTrustedFingerprints, and fedListen/fedStopListening. MeshStore delegates to the corresponding FederationManager methods. FileStore, which doesn't support networking at all, follows its existing not-supported pattern for the mutating operations and returns empty/no-op results for the read-only ones.
mesh_fed_fingerprint, mesh_fed_trust, mesh_fed_untrust, mesh_fed_trusted, mesh_fed_listen, and mesh_fed_stop_listening give an agent the operations needed to actually establish a federation link under the new fingerprint-verification requirement: read this instance's own fingerprint to hand to the other side, pin the other side's fingerprint once received out of band, and start accepting inbound links.
…in CI Rewrite the federation integration test to establish links through the real production path (store.fedListen(), replacing the test's own hand-rolled tls.createServer) and to pin fingerprints on both sides before connecting, matching the new verification requirement. Add a case confirming an inbound connection with no pinned fingerprint is rejected and creates no link. Wire the test into package.json as test:federation, included in test:all — previously nothing ran it at all, which is how handleInbound() went unwired for this long without anything catching it.
|
🎉 This PR is included in version 1.27.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Fixes #38.
Federation links accepted any certificate on either side, and nothing in the shipped product accepted an inbound federation connection at all — three gaps named as P0 in #37 and never actually closed.
What changed
FederationManagergets a per-instance trusted-fingerprint allowlist, empty by default (federate with nobody until an operator explicitly pins a remote mesh's fingerprint — the same no-CA, pin-the-key trust model ordinary peer connections already use).connect()(outbound) andhandleInbound()(inbound) verify the peer's presented certificate against that allowlist before a link is created. An unpinned certificate gets the socket destroyed before any handshake is processed — no state sync, no ready link.listen()/stopListening(): a real production TLS server for inbound federation connections, replacing the fact thathandleInbound()previously had no production caller at all — only the integration test exercised it, against its own hand-rolledtls.createServer.CommsStoregainsgetFederationFingerprint,fedTrust/fedUntrust/fedTrustedFingerprints, andfedListen/fedStopListening, implemented onMeshStoreand stubbed not-supported onFileStore.mesh_fed_fingerprint,mesh_fed_trust,mesh_fed_untrust,mesh_fed_trusted,mesh_fed_listen,mesh_fed_stop_listening) so an agent can actually drive the trust flow: read its own fingerprint, hand it to the other side out of band, pin what comes back, then connect.fedListen/fedTrustpath instead of the hand-rolled server, and gains a case confirming an unpinned connection is rejected outright. It's now wired intopackage.jsonastest:federation, included intest:all— previously nothing ran it, which is how the unwired listener went unnoticed this long.Verification
pnpm typecheck,pnpm lint,pnpm build,pnpm test(23/23),pnpm test:delivery(7/7), andpnpm test:federation(including the new rejection case) all pass locally.Deliberately out of scope
While tracing this down I found the same gap one level deeper: ordinary (non-federation) peer connections also never verify a connecting peer's certificate against its claimed
peerId—grep -r getPeerCertificate src/returns nothing outside this PR.tls-transport.tshas a comment claiming fingerprint verification happens post-handshake; it doesn't. That's a distinct, arguably more significant issue affecting the default mesh path rather than the optional federation feature, and fixing it means touching the core peer-connection flow every bridge relies on — deliberately not folded into this PR. Will file separately.