Repository navigation
Conversation
…y-id upgrade
An XMesh review of lib/config.js 0.14.2 returned DO-NOT-SHIP with 13 findings. Each was reproduced in a
sandbox state dir before anything was changed; the data-loss one was also reported from a developer's machine.
What lost the key: migrateNodeDir refused to move a 0.13 node when nodes/by-id/<nodeId>/ existed, even empty or
holding only lock.pid, and left the name unindexed. loadIdentity then read "refused" as "no such agent", made a
new identity, renamed the 0.13 directory onto the new node's path and wrote the new identity.json over the old
one. A pinned start was bricked instead. The guard asked whether a directory existed; it must ask whether an
identity does.
Root causes, changed rather than patched:
- A second claim on a nodeId is a different identity (or content that cannot merge) already filed under it. An
empty directory or a stale lock claims nothing: the node is merged into it. The same identity on both sides is
an interrupted move and is finished. A real duplicate is still refused and nothing is overwritten.
- loadIdentity treated every answer it did not recognise as "mint". It now mints only when nothing on the host is
known under the name: a 0.13 identity still under the name, or a name indexing a node whose identity is missing,
halts (IdentityHaltError).
- The name is indexed before anything moves and the identity moves last, so a second starter finds the agent and
an interrupted move is re-entered by the identity's absence. A step that fails because another process just did
it is re-run; a path that became an alias of its destination is never merged onto itself.
- Key files: rename keeps the mode 0.13 wrote (0644). identity.json and e2e-keypair.json now end at 0600 and their
directories at 0700 after a migration; loading narrows an existing key file, which heals nodes already migrated by
0.14.0-0.14.2. A rewrite of identity.json is atomic.
- A move across file systems copies, verifies byte for byte, then removes the source.
- Smaller: an identity with a public key and no private key halts; the live-lock fallback checks the tombstone;
a symlink that is not a by-id link no longer reads as already indexed; SYM_IDENTITY_DIR switched on for a node
whose identity is in its node dir halts with instructions instead of making a new identity.
Not fixed, listed in the CHANGELOG: migrateIdentities still discards per-node errors; loadIdentity({nodeId, name})
re-points a name that indexes another node; SYM_IDENTITY_DIR first-time does not move the key for the operator;
an explicit nodeId unknown to the index is ignored when a new identity is made.
tests/identity-migration-by-id.test.js: 36 tests, 30 of which fail on 0.14.2. Four real processes starting one
0.13 agent at once, 320 starters: every one loaded the original agent. Seventeen mutants of lib/config.js, each
caught.
A name whose identity file is missing halts and names what to remove to start the name again as a different agent. It named only nodes/by-name/<name>.json, but nodes/<name> is a link to the old by-id directory and re-indexes the name from it on the next start, so removing the index alone halts again. The message now names both, and a test follows the message to the end (the first removal is shown not to be enough). The test count in the CHANGELOG is now 37 (31 fail on 0.14.2).
…n lost the key) The merge into an existing by-id directory was safe only if no other process was doing the same. It assumed each step was idempotent, so several processes could interleave them. They cannot: when one finishes it replaces nodes/<name> with the compat link, and another, still holding the old path, then sees that path lead to the destination, finds the "duplicate" file identical to itself and deletes it - the only copy of identity.json. Plain concurrent runs passed 320 starters. A fuzzer that stalls every file call for a few ms in four real processes lost the identity file in 2 of 300 rounds (everything else filed, identity.json gone). A Linux run under a throttled CPU (node 22, 0.3 cpu) failed the new race test twice in 40 with the same symptom: identity.json missing from by-id after every starter had finished. The design assumption was "a retry makes concurrent migration safe". It does not, and a guard on the alias only narrows the window. The migrator now holds a lock (nodes/.migrating/lock.pid, the identity lock) while it moves something; others wait, read the state again on their turn, and find it done. It is taken only when a 0.13 directory is there to move. The retry loop and its ENOENT tolerance are gone; the alias check stays as a last guard on a key file. With the lock the fuzzer ran 1,200 starters (300 rounds, husk) and 1,600 (400 rounds, none) with every starter loading the original agent and no errors. Also fixed in this commit: the F6 test passed for the wrong reason once the lock existed (its injected write failure hit the lock file's write); it now fails only the identity temp file. A starter that read the index just before another process wrote it, and then migrates nothing, re-reads the index instead of making a new identity; that line had no test and now has one. Tests: 41 (33 fail on 0.14.2); 17 mutants, each caught.
…g it waited The test asserted the second start waited at least 250 ms of a 400 ms hold. Under a throttled cpu (node 20 in a container at 0.3 cpu) the holder's timer had run for 150-170 ms before the test process was told it held the lock, so 4 of 30 runs measured 225-243 ms and failed although the start had waited. It now has the holder report the time it let go, and asserts the start returned at or after that time (a start that moved the node alongside the holder returns well before it). Mutant "no migration lock" still fails it.
loadIdentity ran the migration only for a start by name alone (`!nodeId && name`). mesh-channel starts every
node as { name, nodeId: <the pin in node.json>, create: false }, so a pinned node whose files were still the
0.13 nodes/<name>/ could not start (EIDENTITYABSENT) until something started it by name; with the default
create: true, 0.14.2 made a new identity and lost the key, and the first 0.14.3 draft halted. Reproduced in a
sandbox state dir on fe285a3 and on the branch before this change (plain, empty by-id/<pin>, create true/false).
When the pin's identity is not in place and a 0.13 directory under the name is still there, loadIdentity now
runs the same locked, idempotent migrateNodeDir(name) and goes on to load the pinned agent. It is one path:
the unpinned and pinned starts share the block; migrateNodeDir takes the pin as an optional second argument.
It is not run when there is no 0.13 directory to move, so a pinned start writes nothing it did not before.
Decision, a pin that is a DIFFERENT nodeId than the directory's: that directory is another agent's, so it is
left where it is, unindexed (migrateNodeDir answers 'skipped-duplicate' before it moves or indexes anything).
The pinned identity stays absent and the start fails as before: IdentityAbsentError with create: false, and
with create: true the halt that names the stranded 0.13 identity and the pin. Nothing is lost and nothing is
made. That agent moves when it is started by name or by its own pin.
Tests (tests/identity-migration-by-id.test.js, 51 now): plain and empty/locked by-id/<pin> with create false;
create true loads the original and does not mint; SYM_IDENTITY_DIR; a pin that is another agent's; a link to
another agent does not get indexed by a pinned start; the name indexed to another agent; a live 0.13 holder and
the tombstone; four real processes with injected stalls, with and without a by-id dir already in place. Eight
of them fail on 0.14.3 as pushed (051f44b) and on fe285a3; two guard against over-migrating. Four new mutants
(no pinned migrate, no pin guard, no legacy-dir gate, adopting the index) are each caught.
…view never refuses a proving session and is never promoted to proven A delegating grant of any scope (an issuer seating one world) could bind any unbound nodeId in the mesh to its holder's key: the victim's own session was then refused as an identity conflict, and once the squatter connected the binding was stored as proven and outlived the revoke. - AuthorityStore.grantKey takes only grants that are unscoped and whose whole chain is admin grants rooted in the anchor (bindsKey). - The settle offers only those grants to the key registry. - The grant view binds only a nodeId nothing else binds: a session, pin or legacy claim naming another key binds on its own and the grant's disagreement is recorded; a session proving the key the view names stores nothing, so a revoke removes what the grant bound. - A live session's proven key outranks the view in _identityKey.
…opped and counted, never charged as a forgery The cmb-encrypted AAD binds only key, assertionId, createdByNodeId, room and to; the signature, createdBy, createdTimestamp, lineage and the application descriptor travel in clear. A relay or on-path writer could edit one and get an honest peer closed and banned 60 s as a forger. - The AAD-bound assertionId is recomputed and checked before the signature (the 8.8.5 order): a mismatch is clear metadata altered, dropped and counted (cmb-clear-metadata-altered). - A bad record signature is never a penalty: the signature itself is outside the AEAD. A record in the session's own name is dropped and counted (cmb-signature-unattributable); one relayed in another's name keeps the relayed path. The forgery penalty stays for attestations, checkpoints and witnesses, which travel wholly sealed. - tests: security-0.14.3 (R2-1, R2-2); core-secure-rereview N1 encoded the penalty for an own-name record and now expects drop-and-count.
…e, actionable de-dup by assertion, pruned ledger R2-3: one admitted peer could saturate CPU and grow memory and disk without bound with large, distinct, validly signed records (24 records of 2 x 200 KB: 15 s CPU, 1.6 s loop block). - The verification and record lanes are also charged by size (record texts and application data): 1 MiB/s burst 4 MiB before verification, 128 KiB/s burst 1.5 MiB before SVAF. Small records count as before. - The SVAF kernel reads at most 16 KiB of one text. - The local-state context reads at most 2 KiB of an entry, 16 KiB in all. - The re-encode after an admit runs at most once a second (leading run, one trailing run). - The store's entry cache is an LRU: 2,048 entries, 32 M characters. - A record relayed in another's name whose assertionId does not match counts toward the pair's failure window (the mute), as a bad signature does. R2-6 (8.8.2): an actionable room record (a signed application section) is de-duplicated for delivery by its assertion identity, and one that memory de-duplicates (redundant, or its cognition key held) is still delivered, remixed:false. An actionable room record signed longer ago than the ledger keeps its mark (30 days) is refused as stale. R2-22: delivery-ledger marks carry an expiry (directed: the replay window past the signed time; others 30 days), expired marks drop at load, the file is rewritten when it doubles, and past 200,000 marks the author holding the most loses its oldest.
… registry One record (a directed one is delivered whatever SVAF decides, and an empty memory admits anything) earned a binding that never expired, so a flood of fresh identities filled the registry for good and no honest newcomer could earn one. - A proven binding is NEW until it has verified records on occasions at least a day apart: it expires after 30 days unseen, and new bindings hold at most a quarter of the registry (past it a newcomer is held for its session only, counted as refusedShareCount). - An ESTABLISHED binding expires after 180 days unseen. - Pinned, vouched, anchored and legacy-claim bindings never expire. A 0.14.2 binding that verified something counts as established. - tests: core-secure-binding-lifetime encoded 'verified never expires' and an unshared registry; updated to the new rule.
…r text in logs, bounded Bonjour cache R2-5: an unauthenticated LAN client-hello costs the server a key agreement and a signature and its sender nothing. The listener now takes a token per remote host (8/s, burst 32) and one over all hosts (64/s, burst 256) when the first frame is a client-hello, before any crypto; past either the connection closes unanswered, counted and said at most once a minute. R2-11: a sealed frame from a relay `from` went only to an ADMITTED session, so a gated room's grant-holder, whose room-join is sealed and sent right after it confirms, never reached the door and a gated room admitted nobody over a relay. It now goes to any confirmed session carrying its sessionId; the door still decides (only room-join is taken before admission). R2-18: node._log writes every C0/C1 control, DEL, line separator and bidi override as an escape, so a peer-chosen name (hello, relay, TXT) cannot forge a log line or drive a terminal. mesh-agent and the sym CLI escape peer text too. R2-20: the Bonjour reconnect cache holds only what the node dials (a canonical nodeId, mmp=2.0), at most 512, the least recently announced going first.
…6, R2-17
R2-4 The signature checks a node made grew with its sessions: each lane was
spent before verification and the global ceiling only after. Every new
gossiped statement now also takes a token of one global budget before
its signature is checked, through the peer's fair share of it (rate and
burst divided by the peers that sent gossip in the last 10 s), so the
checks are bounded whatever the number of sessions and a flooding peer
spends only its own share. The post-verification ceiling is gone (it
could no longer bind).
R2-7 An attester holding equivocation evidence is never evicted from the
checkpoint index; when every place holds evidence a newcomer is refused
(counted). Own-witness memory is never forgotten: a set leaving the LRU
(or a position past the newest 256) folds into a per-attester watermark,
persisted on the own-witnessed line; past 65,536 watermarks a new
attester is not witnessed (counted).
R2-8 aggregateAttestations weighs only attestations naming the held record's
assertionId (or opts.assertionId); the rest are excluded.
R2-15 A checkpoint that starts right after a held one must name its root as
prev, and one ending right before a held one must have the root that
one names: otherwise malformed-link, never stored or witnessed. A §5.2
conflicting copy is still kept as evidence.
R2-16 A second attestation at a held (attester, seq) is refused as a fork and
kept once as evidence (persisted, re-derived at restart); the attester
counts as equivocated.
R2-17 A witness contradicting a checkpoint of an attester known to have
equivocated is refused, not muted; a witness muted for contradicting an
attester is unmuted once that attester's equivocation is known.
Tests that encoded the old behaviour are updated: two attestation-store
cases (same-seq replacement), the gossip-lanes ceiling cases, a gossip-budget
fixture with unlinked consecutive checkpoints, and the aggregate tests (which
now name their assertion).
…token (R2-21) parseInvite echoed the first 120 characters of a URL it could not read, so a mistyped team invite put its relay token into every log and error shown.
…one signature, one spelling (R2-12, R2-13, R2-14)
R2-12. @sym-bot/sym/core verifyCMB is now core's verifyRecord: for a v2.0
record, MMP 8.8.5 in order on the signed projection (closed schema and
8.8.6 bounds, so any member the signature does not cover is unsigned-member;
the assertionId recomputed before the signature; the strict Ed25519 rule;
the content address). A valid one says coreSecure: true and carries its
assertionId and the frozen projection. A top-level payload is allowed only
when it is the JSON of the signed application section (the copy sym keeps).
A record under the 0.13 suite (mmp-sig-v2) reports suite and coreSecure:
false; one whose sections carry what 0.13 never minted there (an
application, a node id, an assertionId, an eighth category) is invalid, and
a genuine one stays valid unless the caller passes { coreSecure: true }.
Core's own verifyCMB, the signature step on an already projected record,
is unchanged for the node's ingress.
R2-13. A lone surrogate encodes as U+FFFD, so one signature verified two
strings. The projection refuses a signed string that is not well formed,
the signing preimages refuse one, core verifyCMB refuses a category text
that is not, and createCMB mints a lone surrogate as U+FFFD.
R2-14. Keys and signatures are taken only in canonical unpadded base64url
(exact length, unused bits zero): verifyStrict's string input, the record
projection, isIdentityKey, the room-join grant, the handshake proof and the
attestation frames.
… penalty for record signatures, size budgets, binding lifetimes
…nds only what nothing else binds, R2-1); the conflict is said
…ms make room, an unread one never does It was bounded by count only (500, up to 2,000 unread) while an item carries its record's text three times, up to 720 KiB a record, and the whole inbox is rewritten on every change: a peer's large records could make it gigabytes. A delivery that does not fit is refused like one past the unread limit (inbox-full-refused, reason size).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e94ccf1e3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+367
to
+369
| this._attConflicts.set(att.by, { seq: att.seq, kept, other: att }); | ||
| this._attEvidence.add(key); | ||
| this._keep(ATT_FILE, att); |
There was a problem hiding this comment.
When _attConflicts fills, every entry retains the conflicting attestation and may also retain its already-evicted kept copy through _heldRecords, but these additional lines are absent from the ATT_FILE startup budget and the kept copy is subtracted from _held during normal eviction. Consequently, a valid live log can exceed _readBudget[ATT_FILE] without containing discardable records; on restart _replay reads only the tail, forgets older fork evidence, and may then rotate the omitted evidence away. For example, with max: 1, maxAttesters: 2, and two conflicting attesters, a restart retained only one of the two conflicts, allowing the forgotten equivocator to be witnessed again. Account for both retained fork copies in the attestation held-byte and read-budget calculations.
Useful? React with 👍 / 👎.
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
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.
Security fixes for 0.14.3 from the independent R2 review. Each finding has a test in
tests/security-0.14.3*.test.js. Each test fails on agit archive 632b95dcopy and passes on this branch. The strict verifier, the spec vectors (projection, size, authority, sym-attest) and record ingress still pass.Based on 632b95d. It merges separately from the identity branch (
fix/0.14.3-identity-migration) and the transport branch (fix/0.14.3-transport). This branch does not touchconfig.js,relocation.js,relay.jsorframe-parser.js.High
admingrants (AuthorityStore.bindsKey).proven.Medium and low
verifyCMBrejects unsigned members and a wrong assertionId. It reportssuiteandcoreSecure, and takes a{ coreSecure: true }option.R2-10 and R2-19 are not in this PR; they are on the transport branch.
For XMesh (R2-12)
server/stored-verify.tsshould callverifyCMB(cmb, pub, { coreSecure: true })and accept a record only whenv.valid && v.coreSecure === true. It should then read categories and parents fromv.record, not from the raw file. See the CHANGELOG anddocs/reference.md.Spec
§6.6.9 needs an erratum narrowing grants as a key-binding source. The text is not in the spec repo yet.