Security review remediation: proof of possession, identity bans, type-bound dispatch, gossip validation, control-plane transport trust, secrets hygiene, mobile tokens - #415
Merged
Conversation
Low-risk, decision-free fixes from the 2026-09-16 security audit (audit.md): secrets that leaked sideways and inputs that were unbounded. Secrets: - H4: mask the hub admin token in Actions logs and mint the router bootstrap token over a port-forward from the runner instead of a kubectl-run pod whose spec (and `describe` on failure) carried it. The bootstrap token is masked too. - M4: create the SQLite file 0600 before the driver opens it, so the WAL/SHM side files inherit that mode, and tighten pre-existing files. The keyring table holds the mesh signing private keys. - M5: secrets.FromPathOrEnv unsets the env var once read, and command backends get an environment with SAM_API_TOKEN/SAM_CLIENT_SECRET stripped (backendEnv), so subprocesses cannot inherit the node's own credentials. - M15: OPENROUTER_API_KEY moves from a plain Deployment env value to a Secret with secretKeyRef. - L25: the sam-mesh chart mounts the router bootstrap token as a file and uses --bootstrap-token-path; nothing secret in argv or env. - I12: Cache-Control: no-store on every route that returns biscuits, bootstrap tokens or enrolled-node records. - I14: POST /admin/bootstrap-tokens requires an explicit role; the silent default was router, the most privileged one. - L16: verifiers call GetAllValidPublicKeys; the private halves no longer flow through five handlers that only verify. Bounds: - M19: StdioBridge reads lines up to the 1 MiB request cap (was bufio.Scanner's 64 KiB, after which the reader stopped forever and every later caller hung). When the reader does stop, the bridge kills the child and answers 503 instead of hanging. - M20: remote-controlled strings (request path, catalog text, target) are truncated before logging, the ingress path is no longer logged before authorization, and the debug log ring caps each retained line at 4 KiB so its memory is bounded. - I16: control-plane and IdP response bodies are read through a 1 MiB LimitReader everywhere (enroll, enroll/status, refresh, token). - I17: Options.Validate rejects a control-plane public key that is not 32 bytes, before it can reach ed25519 verification. - L17: the sam-one --policy-file seed runs the same validation as POST /policies (ValidatePolicyConfig). CI / supply chain: - M14/I19: the OpenClaw gateway token is checked for emptiness (base64 -d on empty input exits 0) and masked; every envsubst names its placeholders so runner env and container-runtime $VARs are never inlined into cluster objects. - L28: chart-test.yml checkout no longer persists GITHUB_TOKEN; helm-unittest is installed at a pinned version. - L21: release.yml fires on v* tags only and drops the unused packages: write scope. - L20: .dockerignore keeps .git, local DBs, keys, tokens and build output out of every image build context; dependabot now covers the nano-init and sam-a2a-bridge Go modules, npm, pip, pub and gradle. - L2: vendored js-yaml 4.1.0 -> 4.1.1 (CVE-2025-64718, merge-key prototype pollution), taken from the npm tarball verified against the registry shasum. Tests: SQLite file mode incl. WAL/SHM and DSN parsing; env var removed after FromPathOrEnv; backendEnv strips only exact secret names; bridge delivers a >64 KiB reply and refuses with 503 after backend exit; ring buffer line cap and truncateForLog; Options key-size check; admin token role required + no-store header; chart unit test for the mounted token.
…dit H1, H6) Two control-plane routes trusted a peer_id from the request body. libp2p proves key possession on every peer-to-peer stream, but the control plane is plain HTTP and holds no libp2p host, so on this surface a peer_id is a claim and a biscuit is a bearer token. /enroll, /enroll/status and /refresh already carry a signed sam:<endpoint>:<peer_id>:<unix-ms> challenge for exactly this reason; these two were the ones that never got it. H1 - POST /register. Any OIDC identity with a node binding could submit a victim's peer_id with any public key. The upsert overwrote the victim's record, NULLed owner_id and reset autonomous_recovery, and the victim's next /refresh failed its key challenge. EnrollRequest gains timestamp/challenge_signature over api.RegisterChallenge; the handler requires peer_id to be derived from public_key and the signature to verify with it, before any ban or policy check. Since peer_id is now always the key's own, a record can only ever be overwritten by its owner. H6 - POST /routers/lease. The handler verified the router's biscuit and its role() fact, nothing else. Routers send that biscuit to every peer they authenticate (AuthResponse.Biscuit), so any enrolled node held a copy and could rewrite the router's lease for every joining peer: empty or attacker addresses, false connected_peers/dht_size. RouterLeaseRequest gains timestamp/challenge_signature over api.RouterLeaseChallenge, verified against the key stored at enrollment (routerRecord.PublicKey), so nothing in the request is something to sign with. The handler also requires routerRecord.Role == router, the control plane's own record, next to the biscuit's fact. Clients: node enrollHTTP takes the private key and signs; router enroll() and renewLease() sign with r.privKey. Both fields are hard-required with no grace period (agreed: alpha, and /refresh keeps existing nodes working; only pre-upgrade binaries hitting /register or renewing leases are refused, with a clear 401). Tests, per .gemini/styleguide.md §3.6 (negative inputs) and §3.3 (fail on parent): TestRegisterRequiresProofOfPossession drives the H1 hijack - victim peer_id with attacker key (400), with victim key and no challenge (401), with a challenge signed by the attacker (401), a challenge for another endpoint (401), a stale one (401) - and checks the victim's public key and autonomous_recovery survive and the attacker left no record. TestNodeAndRouterRegistrationFlow gains the H6 poisoning cases - captured router biscuit with no challenge, with the node's signature, with a stale router signature - and checks /info still advertises the router's real addresses. With either guard removed the tests reproduce the audit's outcome (200s, addresses wiped to [], recovery flag reset). Existing tests and the two integration helpers sign the challenge; the alias test signs over the canonical id, as clients do, so the canonicalization it pins still holds. Docs: control-plane-configuration.md lists the five challenges and why they exist next to libp2p's transport-level proof.
Two findings from the 2026-09-16 audit (audit.md) about what an unauthenticated or merely-enrolled peer can make a verifier do. M23 - Datalog CPU pinning through appended blocks. Appending a block to a biscuit needs no root key, so it is the one place a token holder can put Datalog of their own. biscuit-go runs each rule to completion before it checks its deadline, checks the fact cap only after a whole iteration, and on timeout returns while the worker goroutine keeps computing and then blocks forever on an unbuffered channel. A block with ~200 facts and a 3-way self-join therefore pinned a core for the full budget (1 s on nodes and routers, 10 s on the control plane) and leaked two goroutines on every verify path, with the token still verifying. Reproduced here before the fix: took=budget, goroutines 3->5->7->... per call. SAM never mints appended blocks and reads nothing from them (their facts are invisible to the authorizer; RequireAuthorityBinding already ignores them), so the fix is to not evaluate any: identity.UnmarshalInbound rejects a token with BlockCount() > 0 (ErrAppendedBlocks) before an authorizer is built. Every inbound path goes through it: verifyBiscuit (VerifyBiscuit / AndGetKey / AndGetExpiry), extractPeerID (refresh, policies, catalog), VerifyBiscuitRole, and the node's Authorize. The world limits WithMaxFacts/WithMaxIterations are now set explicitly and the comment that claimed they bound the work is corrected. TestPeerBindingRejectsAppendedBlock previously asserted that an attenuated token stays usable for its real owner; it now asserts ErrAppendedBlocks, with RequireAuthorityBinding kept as defence in depth. M6 - Unauthenticated handshakes had no deadline or rate limit. Any internet peer could open /sam/auth streams on a node or router and hold each one (and its goroutine) open indefinitely, or loop handshakes to make the verifier evaluate every frame. Both handlers now refuse a peer over a per-peer budget (5/s, burst 10, LRU-tracked) and set a stream deadline (10 s) before the frame read; the /sam/mcp auth frame gets the same read deadline, lifted once the peer is authorized since that session is long-lived. The limiter moves from internal/node to internal/ratelimit so the router can use it without importing the node (the two components stay independent). Also deleted: cmd/sam-node re-registered HandleAuthHandshake after Start, replacing the panic-recovering wrapper Start had installed, so a panic on untrusted handshake bytes would have taken the process down. Tests: TestInboundVerifyRejectsAppendedBlocksWithoutEvaluatingThem builds the join bomb and checks all six verify paths reject it in well under the budget with no goroutine growth (fails as described above with the guard removed). TestHandleAuthHandshakeBoundsUnauthenticatedPeers in node and router: an idle stream is closed on the server's schedule, and 30 back-to-back valid handshakes are not all answered.
The control plane's identity lifecycle had gaps between what a ban, a
bootstrap token and an issuer claim were meant to say and what the code
enforced. Findings M17, M3+L35, M1, M2, M9, L36, L12, L9, L10, L11, L15,
I13 from audit.md.
M17 - Identity ban not enforced on /user/*. /admin/revoke bans the node
key and the OIDC identity behind it, but authenticateUser never checked
the identity ban, so a revoked user's still-valid ID token minted a
bootstrap token and re-enrolled a fresh device (auto-approve: back on the
mesh immediately, with the banned identity as OwnerID). Now:
authenticateUser refuses a banned issuer|subject (403 via requireUser on
every /user/* and admin route); /enroll and approve refuse a token whose
owner is banned. Users gain an Issuer column (migration 11) so the owner
of a token can be matched against the issuer|subject ban key; rows from
before the migration adopt the issuer on next login. The inverse exists
too: POST /admin/nodes/{peer}/unban lifts node and identity together
(L36), and the CLI ban/unban canonicalizes --peer, fails on an unknown
node instead of printing "Successfully banned" for zero rows, and covers
the identity half (L12), all through one controlplane.SetNodeBan.
M3 + L35 - Token usage was read-then-increment across two round trips;
approve never re-checked the token. ConsumeBootstrapTokenUsage replaces
IncrementBootstrapTokenUsage: one UPDATE whose WHERE clause carries the
usage cap, revocation and expiry, spent before anything is minted or
written, on auto-approve, approve and re-mint. Approve re-validates the
token (410 if revoked/expired/exhausted since queued, 403 if the owner is
banned), writes the node record before the request flips to APPROVED,
and resolves through ResolveEnrollmentRequest, which only touches a
PENDING row: a second approve or a reject after approve is 409, not a
second biscuit or an approved request with no node behind it.
M1 - The issuer's "roles" claim was minted as role(), the same predicate
mesh policy bindings grant and RequireRole/relay rights key on: an IdP
emitting roles: ["sam:role:router"] made a router. The claim now maps to
idp_role(); bindings and allowed_targets use idp_role:<name>, and role:
is no longer a valid member prefix (BindingMemberPrefixes drives the
control plane's validation and the node's rule compiler, which also
closes L15: agent: and role: members no longer compile into role rules).
M2 - A mesh with no policy roles minted every non-router an unrestricted
token (granted_service_all_types + target_unrestricted). Removed: no
policy, no grants, as the docs already claimed for enrollment.
M9 - An email the issuer marks email_verified: false no longer resolves
bindings, is not minted and is not stored; absent email_verified is kept
(several issuers never emit it). A subject already registered under
another issuer is refused rather than sharing the account.
Smaller: non-admin bootstrap tokens capped at 7 days / 10 usages (L9);
/user/status no longer returns live biscuits, public keys or claims, and
shows policy and routers to admins only (L11); /readyz and user-auth
failures no longer echo internal errors (L10); GET /policies tries the
node biscuit before OIDC so a biscuit no longer produces a verification
error log or a users row (I13); SetNodeBanned reports ErrNotFound.
Tests (.gemini/styleguide.md §3.3, §3.6): identity_lifecycle_test.go
drives each finding from the attacker's side - banned identity on
/user/status, /user/bootstrap-tokens, /user/revoke (403), its pre-ban
token at /enroll (REJECTED) and a queued approval (403), then unban
restores both halves; 12 concurrent enrollments on a 1-use token approve
exactly 1; approve after revoke (410), after the cap (410), second
approve and reject-after-approve (409) with a node record behind the
approved request; user token ceilings; /user/status shape for user and
admin; verified vs unverified email resolving an email: binding;
cross-issuer subject collision (401); empty policy mints role only.
Storage: atomic consume under 20 goroutines, expired/revoked/unknown,
pending-only resolve. resolveRoles pins that a role: member never
resolves from the claim; TestPolicyPermutations gains an unbound-claim
case and drops the roles: [sam:role:node] hack that relied on M1. With
the identity-ban check removed, the M17 test reproduces the audit (201
with a fresh token for the banned identity).
Stacked on audit-pr1-proof-of-possession: the /register tests need its
challenge helpers.
…(audit PR 4)
Findings H2, M18, L30, L32, L33, I15 from audit.md, all on the node's
ingress and egress data path.
H2 - Dispatch ignored the type the policy was evaluated on. The registry
was keyed on name alone and both dispatch paths looked a target up by
name, so a peer granted a2a://reports reached mcp://reports when two
services shared a name (which Register silently allowed), and the stream
path had a fallback that looked the raw target string up as a name.
Now: Register refuses a same-named service of another type and validates
the type://name URI (so a name carrying its own scheme, "plugin://x", is
refused rather than dispatched under a type nobody evaluated); dispatch
uses GetTyped(type, name) on both /libp2p-http and /sam/mcp, and the
stream path additionally requires the granted type to be MCP, the only
thing it can carry. The name-only fallback is gone.
M18 - Command-backed MCP services on HTTP ingress share one stdio
process, and the bridge in front of it routed replies by the caller's
JSON-RPC id and broadcast every stdout line to every SSE (GET) reader:
peer B holding GET received peer A's tool output; A and B posting the
same id got each other's replies. The bridge now owns the id space: each
request's id is replaced with a bridge-assigned one before it reaches the
backend and restored on the way out, a reply is delivered only to the
request it answers, unowned lines (notifications) are delivered to nobody,
and GET is 405. Residual, documented on the type: the backend process is
still one per service, so backend-side session state is shared across
authorized callers; the mesh-stream path already gives each session its
own process.
L32 - "..": both proxies pass the path after /{type}/{name} to the
backend verbatim, so a caller granted service a could send
/mcp/a/../b/tools to a backend that resolves dot segments. Ingress and
egress refuse any "." or ".." segment.
L33 - Host / X-Forwarded-For: the URL-backend reverse proxy set the
upstream Host to whatever the remote peer sent; it is now the backend's
configured host (the original stays in X-Forwarded-Host). The inference
Director proxy does not manage X-Forwarded-*, and RemoteAddr there is a
peer id, so an inbound X-Forwarded-For reached the backend as-is; the
transport drops the X-Forwarded-* trio.
L30 - An OpenAI SDK configured with api_key=<sidecar token> plus
X-Sam-Authentication as a default header sends the token twice; the gate
stripped only the header it consumed and forwarded the other copy to the
remote inference provider. withAuth now also drops an Authorization header
whose bearer value is the sidecar token, on the TCP and socket paths.
I15 - get_mesh_info over the remote catalog stream disclosed the local
Unix socket path, the connected-peer list and the router id; the remote
variant returns peer id and DHT size. The sidecar's local tool is unchanged.
Tests: registry refuses a second type under a name and GetTyped refuses
the other type; bridge tests answer with the bridge id, GET is 405, and
two callers sharing id 1 each get their own reply while a notification
reaches neither; hasDotSegment table; withAuth strips a duplicated token
but keeps a distinct provider credential; reverse proxy asserts the
backend Host; gate tests target mcp://name as clients do; the mesh-wide
fan-out test uses valid names and pins that scheme-carrying names are
refused. Integration: the stdio datapath compares the echo as JSON.
Deferred from this group: L13 (gating /debug/ to socket or mTLS) - twelve
integration tests reach /debug/connect-peer over TCP as plumbing and need
to move to sockets first.
…t PR 5) M22 gossip flood: register a GossipSub topic validator for the control-plane events topic on both node and router. Unsigned, forged or undecodable events are rejected (dropped and not re-forwarded) at the first hop; stale events are ignored. The node's per-peer rate limit moves after validation and is keyed on the author (msg.GetFrom) instead of the forwarding peer, so a junk flood through the router can no longer exhaust the router's budget and drop real BANNED / KEY_ROTATION events. M7 relay ACL: AllowConnect on both the router and the node relay now requires the *source* to be authenticated as well as the destination. Previously any host that could reach the router port could open circuits to every admitted peer. H3-a callee verification: a provider must always present a control-plane signed biscuit bound to its peer ID and carrying role(node), not only when the caller requires labels or the operator set an egress floor. A router's or admin's biscuit is a valid identity but not a service provider. Applied on the three consumer paths: ConnectMCPSession, the OpenAI facade (nil verifier now fails closed with 503) and the raw /sam/<peer>/... egress proxy. Discovery names candidates; only this says the peer is enrolled. L34 discovery table: cap entries per signer (16) so one peer announcing many service names cannot evict everyone else's entries; the signer's own oldest entry is displaced first. Remove the mesh_pubsub_broadcast / poll_messages / subscribe_topic MCP tools (M8, L31). A2A is the agent-to-agent channel; the raw gossip tools had an unbounded per-topic buffer, no topic cap, and could not be held to an egress floor. Their integration test and the playground snippet that only exercised them go with them. Tests: TestValidateMeshEvent (node, router), TestGossipJunkFloodIsRejected- NotForwarded (verified failing with the validator neutralised: 50 junk messages forwarded and the signed ban dropped), TestRelayACLAllowConnect- RequiresAuthenticatedSource, TestNodeRelayACL_AllowConnect updated, TestProviderTableCapsEntriesPerSigner, TestVerifyPeerLabelsDoesNotShortCircuit, "router biscuit is not a provider", facade "no requirement still verifies the provider"; TestFailoverUpdatesRelay now authenticates its client to the router before dialling the circuit and asserts an anonymous host is refused. Test fixtures give provider nodes real identities via enrollUnderRoot. GossipSub is created with StrictSign pinned explicitly (the library default): the validator and the per-author rate limit key on msg.GetFrom(), which only the envelope signature makes trustworthy. Deferred with reasons: H3-b/c (pubsub WithPeerFilter / DHT peer filters) — non-handshaked DHT peers legitimately gossip discovery announcements, so a filter there breaks discovery; revisit with a handshake-before-gossip design.
Whoever answers the control-plane URL is the trust root: /keys, enrollment and router addresses all come from it. Four changes close the paths by which an on-path party could become that root or kill the process. Plaintext transport: a plain http:// control-plane URL to a non-loopback host is refused by both sam-node and sam-router unless --insecure-control-plane is passed. The check runs in the HTTP transport used for every control-plane request (redirects included), so a stored or FFI-supplied URL is held to the same policy as a flag; sam-node also fails fast on a flag-supplied URL. Loopback stays allowed (sam-one, local development). Charts, the k8s templates, the e2e harness and the docs opt in explicitly for the in-cluster Service URL; the sam-node chart does so only when controlPlaneUrl is http://. Signed /keys: KeysResponse gains timestamp and one signature per listed key, by that key, over the deterministic encoding (api.SignKeysResponse / VerifyKeysResponse). Node and router accept the set only when a listed key they already trust signed it and the timestamp is within five minutes, so the first key always comes from enrollment and a rotation is learned through the retiring key. A node with no stored keys skips the sync. KEY_ROTATION signed by the retiring key: the control plane signed the event with the new key, which no node trusted yet, so every node dropped it as a spoofing attempt. It is now signed by the key just retired into its grace period, the one receivers can verify. Refresh hardening: the refreshed biscuit is verified like the enrolled one (trusted signer, bound to this peer, configured role) before it replaces the identity, on node and router. A 403 from /refresh is an error, not os.Exit: only a verified MeshEvent_BANNED is the control plane's word, and the node or router keeps serving on its current biscuit until it expires. Tests: api TestValidateControlPlaneTransport and TestKeysResponseSignatureChain (retiring-key chain, stranger set, listed-but-not-signing trusted key, tampering, replay, unsigned legacy); node TestSyncMeshConfigRefusesUntrustedKeySet, TestControlPlaneClientRefusesPlaintextToNonLoopback, TestRefreshEnrollment ForbiddenDoesNotExit, TestRefreshEnrollmentRejectsUntrustworthyToken; router TestOptionsValidateControlPlaneTransport, TestControlPlaneClientRefusesPlaintextHop, TestSyncKeysRequiresTrustedSignature, TestRouterRefreshEnrollmentHardening; controlplane TestKeyRotationEventIsSignedByTheRetiringKey and /keys self-verification; chart tests for the opt-in flag.
L37 demo.cast: the asciinema recording on the docs site carried a real
daemon API token (46 of 64 hex chars) in some sixty frames. Redacted in place
with a same-width placeholder so the frames stay aligned. hack/verify-secrets.sh
(run by `make verify` in CI) now fails on `Bearer [0-9a-f]{32,}` and on any
PEM private-key block in tracked files; both rules fire against the previous
HEAD. Operator action still needed: rotate the token on the machine that
recorded the cast (delete ~/.config/sam-mesh/api-token and re-run
`sam-node run --daemonize`).
L38 mock OIDC signing key: the RSA private key of the mock issuer was
committed three times (docs manifest, e2e image, inline in policy.bats) and
the docs recommended that issuer for local testing. All three now generate
the key at start-up and derive the JWKS from it; the guide warns that a
control plane trusting the mock admits anyone who can reach it.
L19 chart secrets: the sam-node chart put `apiToken: devtoken` in the pod's
env; it is now a generated Secret (<release>-api-token, stable across
upgrades, pin with --set) mounted as a file and read via --api-token-path,
matching the sam-mesh chart's admin-token pattern. The router bootstrap
token minted by the sam-mesh bootstrap job had max_usages 999999; it is now
one use per router replica.
L22 / I22 security contexts: the sam-node Deployment and the internal
postgres StatefulSet had none. Both get runAsNonRoot with the numeric uid
(65532 distroless, 70 postgres with fsGroup for the PVC), seccomp
RuntimeDefault, allowPrivilegeEscalation false, all capabilities dropped,
and automountServiceAccountToken false (the node uses the projected,
audienced token; postgres needs none).
I25 skill: the sam-mesh skill and the node's MCP instructions told the agent
to read ~/.gemini/config/mcp_config.json or ~/.claude.json to recover the
node token, which puts every other MCP server's headers into the model
context; L37 is what that produces. They now point at the daemon token file
with a curl form that reads it without putting the value in argv, and say to
ask the user otherwise.
I5: delete internal/sambox/gateway.go, the dead TLS-MITM forward proxy (no
non-test caller).
Chart tests cover the mounted Secret, the security contexts, and the router
token cap; the verify-secrets rules were checked against HEAD before the fix.
…se location (audit PR 8)
H5: the mobile app's sidecar token was the fixed string "secret-token", and
Android loopback is shared by every installed app, so any of them could drive
the phone's mesh identity. The token is now generated on first launch
(32 random bytes, base64url) and kept in the app-private data dir next to
the node key and biscuit it protects; the Config tab shows it (hidden by
default) with copy and regenerate. There is no default value anywhere.
M10: the phone-sensors MCP backend listened on 127.0.0.1:9090 with no
authentication. It now binds a random loopback port, mints a token per start
and refuses any request without it (constant-time compare). The node reaches
it through a general backend-credential mechanism added here. A service may
name a file in target_auth_path ("TOKEN" is sent as "Authorization: Bearer
TOKEN", "user:pass" as Basic); the node reads it once at start and composes
the credential into the in-memory target as URL userinfo. A credential
written into target_url inside sam-node.yaml is refused at load: that file
is copied, committed and rendered into ConfigMaps. Userinfo is still honoured
on an in-memory config, which is how the mobile app hands over its per-launch
token without writing it. On every backend path (reverse proxy, MCP client
transport, inference proxy and engine, A2A card probe) the node presents the
credential, overrides any caller-supplied Authorization there, strips it from
the URL it dials, and never echoes a URL in an error (url.Error would print
the userinfo).
L39: the app requested ACCESS_FINE_LOCATION and returned metre-level GPS
coordinates under a switch labelled "coarse". It now requests only coarse
permission, reads the network provider, rounds to two decimals (about a
kilometre) and says so in the tool description, the switch and the README.
ACCESS_FINE_LOCATION is dropped from the manifest.
M12 (data half): android:allowBackup="false", so the node key, biscuit and
API token are excluded from cloud and adb backups and a restore cannot clone
the identity onto another device. The release-signing half still needs a
keystore in CI secrets and is left for the operator.
Tests: TestParseBackendTarget, TestReverseProxySendsBackendCredential,
TestBackendCredentialComesFromAFileNotTheConfig,
TestBackendCredentialRefusedInConfigFileOnly (node);
Dart: random loopback port with token as userinfo, request without or with a
wrong token refused (401), request with the launch token served, new token
per start, constant-time comparison. flutter analyze clean.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces extensive security hardening across the Sovereign Agent Mesh (SAM) codebase. Key changes include implementing proof-of-possession challenges for registration and router leasing, enforcing secure control plane transport validation, restricting database file permissions, and preventing directory traversal and credential leakage in proxies. Additionally, GossipSub mesh events are now validated at the first hop to prevent DoS attacks, and the StdioBridge has been refactored to prevent cross-wire reply leakage under concurrency. Feedback on these changes suggests further optimizing StdioBridge by separating the state and write mutexes to avoid blocking concurrent requests, and refactoring the global allowInsecureControlPlane variable into an encapsulated client struct to improve testability.
Comment on lines
+180
to
+187
| b.mu.Lock() | ||
| if b.closed { | ||
| b.mu.Unlock() | ||
| http.Error(w, "Backend process is no longer running", http.StatusServiceUnavailable) | ||
| return | ||
| } | ||
| _, err = b.stdin.Write(append(toBackend, '\n')) | ||
| b.mu.Unlock() |
Contributor
There was a problem hiding this comment.
5. Review output (Performance)
Writing to b.stdin is a blocking I/O operation. Holding the main state mutex b.mu during b.stdin.Write can cause head-of-line blocking for all concurrent requests and prevent deliver from routing completed responses if the backend is slow or hung.
Recommendation:
Release b.mu before writing to b.stdin. Use the newly added b.writeMu to serialize writes to b.stdin safely.
b.mu.Lock()
isClosed := b.closed
b.mu.Unlock()
if isClosed {
http.Error(w, "Backend process is no longer running", http.StatusServiceUnavailable)
return
}
b.writeMu.Lock()
_, err = b.stdin.Write(append(toBackend, '\n'))
b.writeMu.Unlock()The sam-mesh internal postgres was moved to runAsUser 70 with the PVC mounted at /var/lib/postgresql/data. initdb chmods its data directory and the PVC root belongs to root, so it failed with "could not change permissions of directory", the db crash-looped, the control plane never answered /info and the bootstrap post-install hook hit its deadline — the kind-mesh and bats e2e jobs both failed on `helm install`. PGDATA now points at the pgdata/ subdirectory, which postgres creates and owns (fsGroup 70 gives it group access to the PVC root). Same shape as the .github/k8s control-plane template. Reproduced with docker and on a kind cluster: without PGDATA the pod crash-loops, with it the database is ready in ~10s as uid 70 with pgdata/ at 0700. Upgrade note in values.yaml: a release whose postgres ran as root with data at the PVC root must move it into pgdata/ first.
Review finding on google#415. The bridge held its state lock across the write to the backend's stdin. A subprocess reads stdin and writes stdout on one thread: while it is blocked writing a large reply it is not reading, so a caller's write blocks once the pipe is full — holding the lock that deliver() needs to drain stdout. Neither side could proceed. Writes now serialize on writeMu; mu is never held across I/O. The closed check moves out from under the write; if the backend exits in between, the reader's shutdown has already closed the caller's reply channel and the request answers 503 as before. TestStdioBridge_BlockedStdinWriteDoesNotStallDelivery: caller B blocked in a stdin write nobody reads, caller A's reply must still be delivered. Fails in 2s on the previous code (and without the signalling writer it would hang, which is the bug). Test fixture's stdin buffer is now mutex-guarded since writes no longer happen under b.mu.
Collaborator
Author
|
Follow-ups pushed:
|
run-local-node.sh derives the control-plane URL at runtime from the Gateway's LoadBalancer address, so the sweep that added --insecure-control-plane to every hardcoded in-cluster http:// launch site missed it, and the kind-mesh e2e job failed at "plaintext http:// control plane URL to a non-loopback host". This is the flag's intended case: the docker bridge on the developer's machine is the trust boundary, and the script already fetched the admin token over the same hop. TLS was considered and left for its own change: the chart's Gateway listener is HTTP-only, the control plane has no TLS serving mode, and sam-node has no CA-pinning flag for a self-signed dev certificate.
Collaborator
Author
|
kind-mesh e2e: |
Verifying the callee's biscuit on every egress call (H3-a) made the label gate the first thing that dials a peer, ahead of the egress proxy that used to do the address resolution and relay reservation for it. The gate dialled with a plain context, so libp2p demanded a direct connection and peers behind a relay or not yet in the peerstore failed with "no addresses" -> egress 403. The relay and sam-one flows carry no labels or floor, so the gate had never been exercised on them before. Run preparePeerAddrs first and open the auth stream with WithAllowLimitedConn, mirroring what the guarded call does. Unit test pins the limited-connection opt-in.
auth_flows: the two `sam-node join` invocations take the control plane URL positionally and were missed when the other http:// call sites opted into --insecure-control-plane; join now refuses plaintext to a non-loopback host by default. datapath: the stdio assertions encoded the old broadcast behaviour (POST reply delivered on a shared SSE stream to whoever was listening). The bridge now correlates replies to the requesting POST, so the test issues two concurrent POSTs with the same id and checks each caller gets its own reply, and asserts GET returns 405 as the MCP Streamable HTTP transport permits for servers that do not offer a server-push stream.
Collaborator
Author
|
Bats e2e triage (run 35217706060, 5 failures) — each failure was classified as breaking change, test relying on an assumption this PR removes, or regression, before touching anything:
Verified locally with |
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.
Remediation of the 2026-09-16 security review, stacked as one commit per area so each can be read on its own. Everything below is alpha-breaking where noted.
What changes
Enrollment proves key possession (
fe9a110).POST /register,POST /enrollandPOST /routers/leasenow carry a timestamp and a signature over an endpoint-specific challenge made with the key being enrolled. Previously anyone could register an arbitrary peer id, and any enrolled node could rewrite a router's addresses with the router's own (widely shared) biscuit. Router leases are verified against the router's stored key and require the router role.Pre-authentication work is bounded (
0f2ceba). Inbound biscuits with appended blocks are refused before any Datalog runs; the authorizer has fact and iteration caps; auth-handshake streams get a per-peer rate limit and a deadline on both node and router.Control plane: identity bans and token spending (
cbd1e49). A banned OIDC identity can no longer mint bootstrap tokens or re-enroll a new device via/user/*. Bootstrap-token usage is consumed atomically (UPDATE … WHERE usages < max), the node record is written before an enrollment request is resolved, approve re-checks the token, and there is an unban path. IdProlesclaims becomeidp_rolefacts. User tokens are clamped to 7 days / 10 uses;/user/statusno longer returns the mesh policy to every user; the CLI ban path canonicalises peer ids and errors on zero rows.Node dispatch is bound to the service type (
3bccddc). A stream authorized formcp://foocannot reachinference://foo; the registry refuses one name under two types; ingress and egress reject..segments. Command-backed MCP services get a per-request id space (two callers with the same JSON-RPC id no longer receive each other's replies) and the SSE broadcast side is removed. The sidecar strips its own token fromAuthorizationbefore forwarding (only when it equals the sidecar token; upstream credentials pass through). The reverse proxy addresses the backend by its configured host and drops inboundX-Forwarded-*. Remoteget_mesh_inforeturns only peer id and DHT size.Gossip, relay and callee verification (
b0c5c3c). A GossipSub validator on the control-plane events topic rejects unsigned or forged events at the first hop, so a junk flood via the router can no longer exhaust the rate budget that real ban and rotation events share; the limit is keyed on the author. The relay ACL requires the source to be authenticated too. Every provider a node talks to must present a control-plane-signed biscuit bound to its peer id and carryingrole(node)— on MCP streams, the OpenAI facade and the raw egress proxy — not only when labels or an egress floor apply. The discovery table caps entries per signer. Themesh_pubsub_broadcast/poll_messages/subscribe_topicMCP tools are removed; A2A is the agent-to-agent channel.Control-plane transport trust (
4d92e21). Whoever answers the control-plane URL is the trust root, so a plaintexthttp://URL to a non-loopback host is refused bysam-nodeandsam-routerunless--insecure-control-planeis passed (charts, k8s templates, e2e and docs opt in for the in-cluster Service URL).GET /keysis signed by every valid key and accepted only when a key the receiver already trusts signed it.KEY_ROTATIONevents are signed by the retiring key — they were signed by the new key, which no node trusted yet, so rotation announcements were being dropped. A refreshed biscuit is verified (trusted signer, own peer id, role) before it replaces the identity, and a403on/refreshno longeros.Exits the process.Secrets out of the tree and out of env (
0244e42). The docs-site demo recording carried a real daemon token (redacted;hack/verify-secrets.shnow fails CI on bearer tokens and PEM keys in tracked files). The mock OIDC issuer's RSA private key was committed three times; all three generate it at start-up. The sam-node chart'sapiToken: devtokenenv var becomes a generated Secret mounted as a file; the router bootstrap token is one use per replica instead of 999999; the sam-node Deployment and the internal postgres get security contexts andautomountServiceAccountToken: false. The agent skill no longer tells the model to read MCP client config files to recover the node token. The dead TLS-MITM gateway ininternal/samboxis deleted.Mobile (
3b6dc32). The app's sidecar token was the fixed stringsecret-token, and Android loopback is shared by every installed app; it is now generated per device and shown on the Config tab. The phone-sensors MCP backend was unauthenticated on127.0.0.1:9090; it now binds a random port with a per-launch bearer token. Location is coarse-only (network provider, ~1 km) to match what the switch promises;ACCESS_FINE_LOCATIONis dropped;allowBackup=false. Supporting this, a service may name a credential file intarget_auth_path; a credential written intotarget_urlinsam-node.yamlis refused.Hygiene (
024de4c). Admin token masked in the deploy workflow and minted over a port-forward; SQLite files 0600; secrets read from files unset their env var so subprocesses don't inherit them; response bodies from the control plane are size-capped; log lines are bounded;.dockerignore; release workflow limited tov*tags withoutpackages: write; dependabot covers the other ecosystems.Breaking
/register,/enroll,/routers/lease./keysresponses are signed; unsigned ones are refused. Nodes/routers older than this cannot follow a rotation.http://to a non-loopback control plane needs--insecure-control-plane.role(node).apiTokendefaults to a generated Secret;--api-token-pathreplaces the env var.Deferred (reasons in the commit messages)
Gossip/DHT peer admission filters (would break discovery for peers that have not handshaken), the
/debug/connect-peerdial oracle, a per-route control-plane rate limiter, cosign signing of releases (needs a new Action), a release-signing keystore for the APK, and the hub canaries'SAM_API_TOKENenv vars and floating images.Operator action
Rotate the daemon token on the machine that recorded
site/static/demo.cast(rm ~/.config/sam-mesh/api-token && sam-node run --daemonize).Verification
make build; golangci-lint (root andcmd/nano-init) 0 issues; deadcode;hack/verify-generated.sh;hack/verify-secrets.sh; helm unittest 46/46;flutter analyze; fullgo test -race ./...includingtests/integration. Every security branch has a negative test; the gossip-flood and provider-role tests were checked to fail with the fix reverted. Four independent review passes over the final diff; their findings (provider role, explicit StrictSign, loopback edge cases) are folded in.