Repository navigation
Security hardening: fix authz, client verification, and protocol issues from full-codebase review - #10
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An expired cached session key caused sessionKeyMu.RUnlock() to be called twice, a fatal unrecoverable error that let any authenticated user crash the metadata leader. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The client signed a raw server-chosen 32-byte challenge with its identity key. A malicious server could set the challenge to an inode ManifestHash and obtain a valid UserSig for forged metadata. Login signatures are now over "DistFS-Login-v1\x00" || challenge, and the client rejects challenges that are not exactly 32 bytes. This is a wire protocol change: SERVER-API.md and DISTFS-RAFT.md updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IssueToken copied the client's mode verbatim and only required write permission when the mode was exactly "W", and never checked requested chunks against the inode manifest. Any user could mint delete tokens for any chunk in the cluster, read arbitrary chunk ciphertext, and skip quota reservation with "RW". - Clients may only request R, W or D. - R requires every chunk to be in the inode's manifest. - W and D require write access; D needs explicit chunks that are not in the committed manifest, and a session. - Data nodes record which session created each chunk and only honor a session-bound delete for chunks that session created. Cluster-issued (unbound) tokens used by GC and replication are unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
executeCreateGroup never checked whether the group already existed, and self-owned groups skip management authorization. Any user could resend CreateGroup with a victim group's ID and GID to replace its owner, keys and lockbox, making themselves a member and evicting real members. Also removes a test step that silently relied on recreating a group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Chunk pages are generated by the FSM as <inodeID>:p<i>, but create, update and lease placeholders accepted client-supplied page IDs as-is. A user could reference another inode's pages, then update to [] to delete them (wiping the victim's manifest) or delete the inode to have GC remove the victim's chunks. New inodes and placeholders may not carry pages; updates may only reference pages the inode already owns. Also adds TODO S6 for the remaining manifest/GC ownership issue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Chunk manifests are client-supplied. A user could list a victim's chunk IDs (visible via GetInode) in their own file and delete it, making GC delete the victim's data; listing extra node IDs made the replication monitor prune real replicas. The FSM now keeps a chunk_owners index (chunk ID -> inode ID). Create and update may only add chunks that are unowned or already owned by the same inode, and GC only enqueues chunks owned by the inode being finalized. Replication only acts on manifests, which can no longer contain foreign chunks. The index is rebuilt from existing inodes on open and after snapshot restore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The S3 restriction (R tokens only for manifest chunks) broke the FUSE read-modify-write path and direct chunk reads, which legitimately read chunks uploaded in the current session before they are committed. An R request for chunks outside the manifest now requires write access and a session, and yields a creator_only token that data nodes honor only for chunks the bound session created. Creator records now last for the session lifetime (1h). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Inode, group and registry-attestation signatures were checked against keys fetched unverified from the server, while deferred registry verification only recorded the signer ID: it was skipped when the ID was already in the verified cache and otherwise re-fetched the user. A malicious server could return substitute keys for the signature check and the genuine record for the confirmation, forging any metadata. - Signer, owner and verifier keys now come from getUser: pinned or verified keys from cache, or keys recorded for deferred verification. - The verification queue records the exact user record used and confirms that record; it fails if it differs from the verified cache or if different keys were used for the same user. - verifyInode/verifyUser/verifyGroup process their own queue when called outside an existing one. - Registry attestations must name the user/group being verified. - verifyGroup only promotes a group to the verified cache after the ID commitment check. - getUserRaw/getGroupRaw reject responses for a different ID. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
provisionRecipient used recipient keys fetched unverified from the server, and AddUserToGroup ignored the out-of-band verified ContactInfo it was given. A malicious server could substitute its own key and learn file keys (chmod/chown/setfacl) or group epoch seeds (group add). - provisionRecipient resolves users and groups through the verified getters; recipients without verified keys are not provisioned. - AddUserToGroup encrypts to the ContactInfo keys when provided, and rejects the operation if the server's keys for that user differ. - Tests that added unverified users now pass contact info. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
registry-add showed a verification code computed from one server fetch and then signed an attestation for keys from a second fetch, so a malicious server could pass the out-of-band check and still get its own keys attested. The code was also only 24 bits, cheap to grind. - AnchorUserInRegistryWithCode refuses to sign unless the attested keys match the code the admin confirmed; registry-add uses it. - The code is now 128 bits (8 groups of 4 hex digits). - 'distfs whoami' prints the user's own security code, so the user can actually read it to the admin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An inode's signature binds its content to its own ID, but the client never checked that the server answered with the inode it asked for. A malicious server could serve any validly signed inode (e.g. a file written by a colluding user) at any path. getInodeInternal now rejects a response whose ID differs from the requested ID, and getInodes rejects unrequested or duplicate inodes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The client returned unsealed response bodies as trusted data even when it expected a sealed reply, and only checked the cluster binding signature when the server chose to include one. A network attacker could forge metadata responses and a forking server could skip the timeline binding. The server also fell back to plaintext when sealing failed, and several sealed actions (world private key, ValidateMetadata, admin audit, empty Raft results) were answered in plaintext. Client: - Successful responses to sealed requests must be sealed. - Once the cluster signing key is known, sealed responses must carry a valid binding signature. - The client verifies its own identity against the keys it holds rather than the registry. - AnchorGroupInRegistry verifies the group's signature and ID commitment before attesting it. Server: - Never falls back to plaintext for sealed requests. - Seals the world private key, ValidateMetadata, audit stream and empty Raft command responses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Size bounds reads of the zero-padded chunk data but was not covered by ManifestHash, so a malicious server could truncate files (e.g. cut the tail of a script or authorized_keys) or extend them with NULs without invalidating the owner's signature. This changes the signed format: inodes signed before this change no longer verify and must be rewritten. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
Chunk IDs are SHA-256 of the ciphertext, but the client never checked downloaded data against the ID named in the signed manifest. Since file keys persist across versions, a malicious storage node could serve an older ciphertext for a chunk and it would decrypt and be cached. Each replica's response is now checked against the chunk ID (a mismatch counts as a failed replica), and cached chunks are re-checked. Tests that served arbitrary bytes under fake IDs now use real hashes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
Inodes returned by the server after a write were trusted, cached in the path cache and used to advance the root anchor without verification. A malicious server could inject forged directory or file inodes that later path resolutions would use, and an unverified root fetch could poison the root anchor (e.g. with a huge version). - Create/update results must have the written ID and version and pass verifyInode before use. - The root anchor (owner, version, TOFU keys) only advances from a verified root inode, and the version never moves backwards. - The conflict mock now echoes the written inode like the server does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
verifyJWT only checked the signature against the IdP's JWKS. An ID token the IdP issued to any other application for the same user (e.g. on a shared Google/Okta tenant) was accepted, letting that application fetch the user's KeySync blob or register the identity with its own keys. Tokens must now carry iss = the discovered issuer, aud = the configured client ID (new --oidc-client-id flag, default "distfs", matching all DistFS clients) and an exp, and use an asymmetric algorithm. The test OIDC server now sets aud. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
…ssion - Any user could take an exclusive lease on any inode (e.g. the root directory or another user's home) and renew it, blocking every mutation by other sessions. Exclusive leases now require write access and lease placeholders must be owned by the requester (and only name groups they belong to). Shared leases stay unchecked because the server cannot see anonymous group membership. - Batch sub-commands could set their own sid/ts/uid. A writer could use another session's ID to bypass its exclusive lease, or a far-future timestamp to expire leases. For client batches the authenticated user, session and timestamp now always override sub-command values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
GetInode/GetInodes returned every lease's session ID and nonce to any user, which (before the batch session binding fix) let a writer impersonate another session to bypass its lease. Leases held by other sessions are now returned without session identifiers; the requester's own leases are unchanged. Signed inode fields cannot be withheld without breaking client-side verification, and chunk IDs are no longer exploitable after the capability and chunk ownership fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
DISTFS-RAFT §2.3 claimed lockbox recipient IDs hide membership without the ClusterSecret, but they are keyed by the public GroupID, so anyone who can read the group can test a known UserID for membership (as §4 already acknowledged). This cannot be fixed in code: the server needs membership to enforce group permissions, and a member cannot hold a group secret before decrypting their own entry. Document the HMAC as pseudonymization and point to the AnonymousLockbox for anonymity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
- Usage is charged by an inode's declared Size, which could be 0 for a manifest of any length. A manifest may no longer hold more chunks than the declared size needs. - Any user could create a quota-enabled group (quota 0 = unlimited) and charge all their files to it. Only administrators can now create quota-enabled groups; otherwise usage falls back to the user. - CreateGroup/UpdateGroup persisted client-supplied Usage, Quota and GID, letting a manager zero a group's usage or limits or change its GID without a uniqueness check. These are now server-managed. Test fixtures that created files with chunks but no size, or quota groups as a non-admin, are updated. Also fixes gofmt alignment in server.go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
authenticate cached the *User for the session token's whole lifetime (1h), so an admin locking a compromised account had no effect on its existing sessions. The session cache now only skips re-verifying the token signature; the user record is re-read from the FSM, whose cache is evicted when the lock changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
The replay cache key used the first 16 bytes of the sealed request, which in symmetric (session) mode is the unused, unauthenticated KEM ciphertext slot. Anyone who observed a sealed request could flip a byte there and replay it within the 2-minute window, repeating e.g. quota reservations or lease and group operations. The key is now a hash of the request's ML-DSA signature, which is authenticated and unique per signed message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
During AdminClusterJoin the leader probed the new node over an unverified TLS connection, and the node's HMAC proof covered only the nonce. An on-path attacker could relay the probe, keep the valid proof header and substitute its own TLS, signing and encryption keys, then receive the bootstrap payload (ClusterSecret, FSM keyring) and the raft secret header, and be added as a voter. - The node's proof now covers a hash of the exact response body, so the reported keys are authenticated by the raft secret. - The bootstrap push uses a connection pinned to the TLS key that was authenticated during discovery. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
The service worker streamed decrypted files from /distfs-media/ on the application's origin with a Content-Type derived from the file extension or content sniffing (text/html, image/svg+xml, ...), no CSP and no nosniff, and answered navigations by borrowing any open tab. A shared HTML or SVG file opened as a page could run script that reads and exfiltrates every file the victim can access through the worker. - Media is only served to subresource requests from our pages; top-level and framed navigations are refused. - Only inert image/video/audio types are served inline; everything else is application/octet-stream. - All worker responses carry CSP sandbox and nosniff, and download filenames are percent-encoded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
verifyPeer accepted any mTLS peer whose 8-byte node ID prefix matched an active node, never comparing the stored public key. A key with a colliding 64-bit prefix (about 2^64 work) would be accepted as that node on the Raft and cluster APIs. When a node's TLS key is on record it must now match exactly, and nodes added via AdminClusterJoin now record the TLS and signing keys that were authenticated during discovery. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
/v1/auth/challenge decoded an unbounded body and kept the caller's user_id in a cache with no size limit for two minutes, letting unauthenticated clients make the server retain arbitrary amounts of memory. /v1/login and /v1/user/register also read unbounded bodies. The challenge body is capped at 4KB, user IDs at 256 bytes, pending challenges at 100k, and login/registration bodies at 1MB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
Most commands were proposed without a timestamp, so each node applied them with its own time.Now(), and lease checks always used time.Now(). Lease expiry, finalize-vs-unlink decisions and similar outcomes could differ between nodes and on log replay, silently diverging FSM state (usage, GC). The timeline hash covers log bytes, not state, so this was not detected. LogCommand.Marshal now stamps the proposer's time, all proposal sites use it, and lease checks use the command timestamp. Legacy entries without a timestamp keep the previous fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
They use the native-only test helpers, which broke the WASM unit test build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
Printing the security code from 'whoami' broke scripts that capture its output as the user ID. The code is now shown with 'distfs whoami --security-code'. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
getInodes rejected a response containing the same inode twice, but the client legitimately requests repeated IDs (e.g. hard links) and the server answers each. Responses are still rejected if they contain an inode that was not requested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
All items are resolved. M10 (pinning the rotating server KEM key) is deferred pending a protocol change: sign epoch keys with the cluster signing key and pin that key in CLI and web configs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
Truncation updated Size but kept the full chunk manifest. Since the quota fix the server rejects manifests with more chunks than the size needs, so truncating a chunked file (e.g. O_TRUNC racing another writer in TestFUSE_POSIXCompliance/ConcurrentWrites) failed with EIO. setAttr now trims the manifest to the chunks the new size covers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5185JhuUeHz5tmgWAFGeA
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
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.
Summary
Fixes the findings from a full-codebase security review. Every fix has a regression test, and nearly all were confirmed to fail on the old code.
Threat model: unauthenticated network attackers, authenticated users attacking other users, and a malicious or compromised metadata server, which clients must detect.
"DistFS-Login-v1\x00" || challenge).creator_only.Sizeis now part ofManifestHash. Inodes signed before this change no longer verify and must be rewritten.--oidc-client-id(defaultdistfs). ID tokens must have a matchingaud, the discoverediss, and anexp.registry-adduses a 128-bit verification code (previously 24 bits). The user displays it withdistfs whoami --security-code.Fixes
Critical: server authorization
RUnlockon an expired session key let any user crash the leader.IssueTokenminted delete/read tokens for any chunk after only a read check. Tokens are now limited to the inode's manifest. Delete tokens, and reads of uncommitted chunks, only work for chunks the requesting session created on that data node.CreateGroupcould overwrite an existing group (full group takeover).High: client must not trust the server
ContactInfois honored.Sizewasn't signed, allowing truncation or extension.Medium
GetInode.Low
Known limitations / follow-ups
ls -lon unreadable files).Testing
go test ./pkg/... ./cmd/...(metadata, data, crypto, client, CLI), plus the WASM build and WASM unit tests.scripts/run-tests.sh --fast, all 18 Docker e2e tests.pkg/metadata/security_test.go,pkg/data/security_test.goandpkg/client/security_test.go. Existing fixtures that relied on the insecure behavior were updated.🤖 Generated with Claude Code