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).
… bounds, backoff and identity retries R3-5, R3-9, R3-11: start() and stop() are steps on one promise chain over four states (idle, starting, running, stopping), safe un-awaited. stop() during a start makes it abandon and undo what it set up; a failed start undoes everything and rejects (never an unhandled rejection); start() after stop() takes the identity lock and the inbox again. discovery.stop() lets go of its server before it waits. A relay URL is checked in the constructor and in invites (EBADRELAY). R3-4: records a relay pacer never wrote (the connection closed, or the session ended) are held for the peer's key, offered again to its next session, and listed in pendingDeliveries() until a socket writes them. Bounded; the residual (written and then lost) is documented. R3-1/R3-2: the ws ceiling is 8 MiB and the frame bound is per message; an oversized message is dropped and counted, the connection stays up, and the roster is read past the bound. R3-3: 4004/4006 are retried on a long backoff with a ping-pong detector (identityRetry: false keeps MMP 4.4.7). R3-10: the backoff resets on relay-peers, not on open. R3-15: the liveness timeout terminates. R3-13: sessions close before the relay, and its last flush sends the 1010 notices.
…ty without birth times; linear frame parsing
R3-7: lines are cut on bytes and decoded whole (lib/line-reader.js) in the interior and the daemon,
which refuse a line that is not UTF-8 and bound lines in bytes; the CLI, SymDaemonClient and the
child-process readers decode with setEncoding('utf8'). The interior no longer signs text damaged by
a read boundary.
R3-6: a Unix socket path longer than sun_path is mapped, the same in the daemon and every client, to
/tmp/sym-<uid>/<digest>.sock in a 0700 directory the clients check before dialling.
R3-14: a creation time of 0 is unknown; a socket's identity adds the inode change time and a token
the daemon writes into the socket's times.
R3-12: FrameParser keeps its chunks and copies a frame once, when it is whole.
… notes on the identity-close retry
…t, exclusive name claim The design assumption that failed twice: the state of an identity was inferred from whatever was at a path (a partial destination, an unreadable index, a node dir without identity.json, a stale lock's content). Replaced: - 0.13 -> by-id migration COPIES: staged beside its destination (nodes/by-id/.<id>.partial-*), identity.json last, synced, verified byte for byte, installed by ONE rename. The loser of two commits finds the same identity and discards its staging. The 0.13 directory is then renamed aside to .legacy-0.13/<name> (never deleted) and the compat link takes its place. A by-id dir without the commit record (.sym-commit) is a husk: emptied with rmdir, or its files merged into the staging and the husk kept at .<id>.husk. mergeMove, moveTree and the nodes/.migrating lock (Atomics.wait up to 30 s) are gone. - A name is claimed by an exclusive create (link) of its index entry after the commit; a losing first start discards its minted identity and loads the winner. The index reads as absent only on ENOENT; anything else halts. Index writes are fsynced. - A mint happens only when there is no trace of the name; a trace that provably is the name's identity is adopted, any other halts with what it is. - A live 0.13 process in the 0.13 directory refuses the start (EIDENTITYLOCK). - A stale lock is removed only by the holder of a claim named after that lock's file identity and content (compare-and-delete); locks are created whole via link. - relocation: intent, bundle, tombstone beside the key, scrub; resumable and abortable (abortExport); imports staged with 0700 dirs, 0600 keys, the key only in the identity dir. - validateName refuses names Windows cannot hold; compat links compared as resolved paths. tests/identity-commit.test.js: 25 tests, all failing on 632b95d.
…art installed A start copying the 0.13 directory while another start sets it aside can see it go mid-copy. That start's error is not the agent's state: the other start installed the identity and claimed the name, so loadIdentity reads it by the index instead of failing. Tests: four guard tests for the staged commit (a copy that does not read back is not installed; a linked subdirectory is copied as a directory; a husk with a live lock refuses; a live process's staging is left alone).
…tombstone, export intent, commit record) from a bundle
…identity (R4-9)
loadIdentity({ name, nodeId: pin }) re-pointed the name's index entry to the pin whenever it
named another identity. mesh-channel pins node_id from a repository's .sym/node.json, so a
folder pinning X under "reviewer" silently took the name from another folder's agent Y: Y's
next start by name ran as X and Y was left without its name, and a cloned repository could
choose which identity on the host a session signs as.
Now a pinned start may claim a name nobody has, and that is all. A name whose entry names
another identity halts (IdentityHaltError, saying both and how to resolve it) and the entry
is left as it is; with create: false and the pin not on this host it is IdentityAbsentError,
saying the name is another agent's. A pinned start is never answered with the name's agent.
…ose keeps its queue; the ping-pong holds An independent read of the diff found: - a re-offer stopped by the pacer's room with nothing of its own waiting never ran again: it now re-arms on a timer, and checks the pacer's room (half of each bound spare) before it offers, so a re-offer never makes the pacer refuse a sealed frame (which ends the session); - taking back the records queued before a sealed 1010 turned the notice into a sequence gap at the peer: a session that sent the notice keeps its queue, which its peer can still open; - the ping-pong detector's window equalled the longest backoff, so a real ping-pong fell out of it, and the host was told on every close: the ping-pong now holds until a connection outlasts 70 min, and the host is told at the first close in the window and when a ping-pong is found; - records came back to a stopping node and were held after the stop dropped the rest: anything handed back to a node that is not running is said undelivered at once; - re-offers no longer go to a relay client session still waiting to supersede (offered when it does); a failed start is 'stopping' while it undoes; start() on a running node with nothing queued queues nothing, so a stop() in the same turn still starts at once; the redelivery metrics never throw into the teardown.
…he ping-pong's hold
…nd by the multi-process fuzz)
Four starts at a 0.13 node over a lock-only by-id husk: one start judged the target a husk,
another emptied it (stale lock removed, rmdir) and committed its copy there, and the first then
read the directory's entries (the committed copy's), took them for a husk's files and renamed
the committed directory aside to .<id>.husk. Same identity, nothing lost, but an installed
directory moved: 9 of 40 rounds (6 of 40 pinned).
The husk is now examined again immediately before it is set aside; a commit found there is
the other start's ('exists'). A husk with files cannot be replaced by a commit (a rename onto
a non-empty directory fails), so after that look only another claim can move it, and then the
rename aside fails. Lock artifacts (a claim, a temp lock file) are not a husk's content: dead
ones (and this process's own, left by a failed attempt) are removed before the rmdir; one that
stays has the husk set aside with it.
The real cross-filesystem test is named for what it now checks: no rename crosses the volumes.
… aside (the fuzz's race, made deterministic)
…s and kill -9 fuzz, suite, gate, audit)
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.
Release candidate for sym 0.14.3: the three fix branches merged without conflicts, so they are tested and reviewed together.
Not to be merged until: the full suite, stock-nodes gate and npm audit pass on this head; Linux docker runs of the new test files pass; an independent review of the combined diff (fe285a3..head) finds nothing blocking; and the merge is approved.