diff --git a/lib/frame-handler.js b/lib/frame-handler.js index 82822a8..1b56f9f 100644 --- a/lib/frame-handler.js +++ b/lib/frame-handler.js @@ -348,12 +348,23 @@ class FrameHandler { } /** - * Verify the Ed25519 signature on an inbound CMB against the sending peer's - * announced identity key (MMP §8.3). Returns true if the CMB must be REJECTED - * (a present-but-invalid signature — forged/tampered; or unsigned when the - * node is configured to require signatures). Sets msg._cmbVerified. Unsigned - * CMBs, or signed CMBs from a peer whose key we haven't seen yet, are allowed - * through (flagged unverified) for interop unless strict mode is on. + * Verify the Ed25519 signature on an inbound CMB (MMP §8.3). Returns true if + * the CMB must be REJECTED (a present-but-invalid signature — forged/tampered; + * or unsigned when the node is configured to require signatures). Sets + * msg._cmbVerified. + * + * The key is resolved from the DELIVERING peer, but a CMB is signed by its + * AUTHOR (`signingPayload` binds `cmb.createdBy`). Those coincide only when + * the author handed the block over directly. For a RELAYED CMB the resolved + * key is the wrong node's, and the verdict is meaningless — so a signed CMB + * whose named author is not the deliverer is passed through flagged + * unverified rather than reported as forged. Also passed through unverified: + * unsigned CMBs, and signed CMBs from a peer whose key we have not seen yet. + * Strict mode still rejects unsigned. + * + * What this does NOT do is authenticate a relayed CMB — it cannot, because + * the author's key is unresolvable from a name (see the comment at the + * rejection branch). It stops mislabelling unverifiable as forged. * @private */ _rejectOnBadSignature(peerId, peerName, msg) { @@ -387,6 +398,40 @@ class FrameHandler { if (!v.valid) { const keyShort = String(cmb.key || '').slice(0, 16); + + // The key resolved above belongs to the node that DELIVERED this CMB: + // `_identityKey` is keyed by nodeId and `peerId` is the transport peer. + // The signature is made by the AUTHOR — `signingPayload` binds + // `cmb.createdBy`. On a RELAYED CMB those are different nodes, so the + // verdict above was computed against the wrong node's public key and + // says nothing about authenticity: an untouched, genuinely-signed block + // fails it every time. Only relax the case we can PROVE is the wrong + // key — a named author that is not the deliverer. An absent author, or + // an author that IS the deliverer, still hard-rejects, so omitting + // `createdBy` cannot be used to dodge rejection. + // + // The author's own key is deliberately NOT resolved here: the roster is + // keyed by nodeId (`RosterKeyRegistry._byNode`) while `createdBy` is a + // NAME, so a name index would be required — and it would silently + // resolve to the wrong key the first time two nodes share a name. + // Carrying `createdByNodeId` on the CMB and binding it into + // `signingPayload` is the correct fix; that is an MMP schema change. + // + // Passing unverified is the posture line 381 already takes for a key we + // cannot resolve ("Cannot verify — do not reject"). This makes the two + // consistent: today an UNRESOLVABLE key passes while a resolvable-but- + // WRONG key is logged as forgery and dropped before SVAF ever sees it. + const author = cmb.createdBy || null; + if (author && author !== peerName) { + msg._cmbVerified = false; + this._node._log(`[sym-security] UNVERIFIABLE CMB ${keyShort} authored by ${author}, relayed via ${peerName} — this node holds no key for the author, so the signature cannot be checked; passing unverified${v.error ? ' (' + v.error + ')' : ''}`); + this._node.emit('metric', { + type: 'cmb-signature-unverifiable', from: peerName, author, + key: cmb.key, reason: v.error || 'author-key-unavailable', + }); + return false; + } + this._node._log(`[sym-security] BAD SIGNATURE on CMB ${keyShort} from ${peerName} — forged/tampered, rejected${v.error ? ' (' + v.error + ')' : ''}`); this._node.emit('metric', { type: 'cmb-signature-rejected', from: peerName, key: cmb.key, reason: 'invalid' }); if (typeof this._node._recordDecision === 'function') { @@ -507,11 +552,13 @@ class FrameHandler { } if (!msg.content) return; - // CMB authentication (MMP §8.3). A signed CMB MUST verify against the - // sending peer's Ed25519 identity key (announced in its handshake). A - // present-but-invalid signature means the CMB was forged or tampered in - // flight — reject it outright (audit-logged, never surfaced or stored). - // Unsigned CMBs (older peers, or peers without a known key yet) are allowed + // CMB authentication (MMP §8.3). A signed CMB verifies against its AUTHOR's + // Ed25519 key. When the author delivered it directly, the peer's announced + // handshake key IS that key and a present-but-invalid signature means forged + // or tampered in flight — reject outright (audit-logged, never surfaced or + // stored). When the block was RELAYED, the author's key is not resolvable + // here, so it is flagged unverified rather than rejected. Unsigned CMBs + // (older peers, or peers without a known key yet) are likewise allowed // through for interop but flagged unverified on msg._cmbVerified. if (this._rejectOnBadSignature(peerId, peerName, msg)) return; diff --git a/tests/frame-handler-author-key.test.js b/tests/frame-handler-author-key.test.js new file mode 100644 index 0000000..88a0a4e --- /dev/null +++ b/tests/frame-handler-author-key.test.js @@ -0,0 +1,152 @@ +'use strict'; + +require('./_isolate-home'); // redirect $HOME to a temp sandbox before lib/config loads + +/** + * Regression: a RELAYED CMB must not be reported as forged. + * + * `_rejectOnBadSignature` resolves the verifying key from the DELIVERING peer + * (`_identityKey(peerId)`, and the roster is keyed by nodeId), while + * `signingPayload` binds the AUTHOR (`cmb.createdBy`). Those coincide only when + * the author handed the block over directly. For every relayed CMB the check + * ran against the wrong node's public key, so an untouched, genuinely-signed + * block failed 100% of the time and was dropped before SVAF with the log text + * "forged/tampered" — invisible in every drift distribution and admission tally + * because the drop happens upstream of evaluation. + * + * The relaxation is deliberately narrow: only a NAMED author that is not the + * deliverer is treated as unverifiable. An absent `createdBy`, or an author that + * IS the deliverer, still hard-rejects — otherwise omitting the field would be a + * way to dodge signature rejection entirely. + * + * This does NOT authenticate relayed CMBs. The author's key is unresolvable from + * a name (roster is nodeId-keyed), so the honest outcome is "unverified", which + * is the posture the no-key branch already takes. Authenticating them needs + * `createdByNodeId` bound into `signingPayload` — an MMP schema change. + */ + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const crypto = require('node:crypto'); +const { FrameHandler } = require('../lib/frame-handler'); +const { createCMB, signCMB } = require('@sym-bot/core'); + +const GROUP = 'test-group'; +const RECEIVER_NODE_ID = 'receiver-node-id'; + +/** A fresh Ed25519 identity in the raw base64url form sym stores. */ +function identity() { + const { publicKey, privateKey } = crypto.generateKeyPairSync('ed25519'); + const rawPriv = privateKey.export({ type: 'pkcs8', format: 'der' }).subarray(16); + const rawPub = publicKey.export({ type: 'spki', format: 'der' }).subarray(12); + return { priv: rawPriv.toString('base64url'), pub: rawPub.toString('base64url') }; +} + +/** A signed CMB as it appears on the wire (emit.js: whole object, then signCMB). */ +function signedCMB({ author, signWith }) { + const cmb = createCMB({ fields: { focus: 'a relayed observation' }, createdBy: author }); + cmb.group = GROUP; + signCMB(cmb, signWith); + return cmb; +} + +/** Receiver node stub. `keyFor` maps the delivering peerId → the key we hold. */ +function harness(keyFor) { + const logs = []; + const metrics = []; + const decisions = []; + const node = { + nodeId: RECEIVER_NODE_ID, + _group: GROUP, + _requireSignedCmb: false, + _log: (m) => logs.push(m), + emit: (type, payload) => { if (type === 'metric') metrics.push(payload); }, + _identityKey: (peerId) => keyFor[peerId], + _recordDecision: (d) => decisions.push(d), + }; + return { fh: new FrameHandler(node, {}), logs, metrics, decisions }; +} + +describe('_rejectOnBadSignature — the verifying key must belong to the author', () => { + it('does NOT reject a CMB authored by A and relayed by B (the 100%-loss case)', () => { + const a = identity(); + const b = identity(); + const cmb = signedCMB({ author: 'node-a', signWith: a.priv }); + // C holds B's key for the delivering peer, and has never met A. + const { fh, logs, metrics, decisions } = harness({ 'peer-b': b.pub }); + const msg = { cmb, source: 'node-a' }; + + const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg); + + assert.strictEqual(rejected, false, 'a relayed CMB must reach SVAF, not be dropped as forged'); + assert.strictEqual(msg._cmbVerified, false, 'and must be flagged unverified — it is NOT authenticated'); + assert.ok( + !decisions.some((d) => d.decision === 'rejected-signature'), + `must never record rejected-signature, got: ${JSON.stringify(decisions)}`, + ); + assert.ok( + metrics.some((m) => m.type === 'cmb-signature-unverifiable' && m.author === 'node-a'), + `the population must stay countable, got: ${JSON.stringify(metrics)}`, + ); + assert.ok( + !logs.some((m) => m.includes('forged/tampered')), + `must not be logged as forgery, got: ${logs.join(' | ')}`, + ); + }); + + it('still rejects a forgery from the peer that claims to have authored it', () => { + const a = identity(); + const b = identity(); + // B names itself the author but the bytes were signed by A: B's own key is + // the right key to check, and it fails. That is a real forgery. + const cmb = signedCMB({ author: 'node-b', signWith: a.priv }); + const { fh, logs, decisions } = harness({ 'peer-b': b.pub }); + const msg = { cmb, source: 'node-b' }; + + const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg); + + assert.strictEqual(rejected, true, 'author === deliverer means the verdict is meaningful'); + assert.ok(decisions.some((d) => d.decision === 'rejected-signature')); + assert.ok(logs.some((m) => m.includes('forged/tampered'))); + }); + + it('still rejects when createdBy is absent — omitting it cannot dodge rejection', () => { + const a = identity(); + const b = identity(); + const cmb = signedCMB({ author: 'node-a', signWith: a.priv }); + delete cmb.createdBy; + const { fh, decisions } = harness({ 'peer-b': b.pub }); + const msg = { cmb, source: 'node-a' }; + + const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg); + + assert.strictEqual(rejected, true, 'an unnamed author must not buy a pass'); + assert.ok(decisions.some((d) => d.decision === 'rejected-signature')); + }); + + it('verifies normally when the author delivers its own CMB', () => { + const a = identity(); + const cmb = signedCMB({ author: 'node-a', signWith: a.priv }); + const { fh, decisions } = harness({ 'peer-a': a.pub }); + const msg = { cmb, source: 'node-a' }; + + const rejected = fh._rejectOnBadSignature('peer-a', 'node-a', msg); + + assert.strictEqual(rejected, false); + assert.strictEqual(msg._cmbVerified, true, 'the direct path must still authenticate'); + assert.strictEqual(decisions.length, 0); + }); + + it('leaves the unresolvable-key path untouched (unverified, not rejected)', () => { + const a = identity(); + const cmb = signedCMB({ author: 'node-a', signWith: a.priv }); + const { fh, decisions } = harness({}); // no key for anyone + const msg = { cmb, source: 'node-a' }; + + const rejected = fh._rejectOnBadSignature('peer-b', 'node-b', msg); + + assert.strictEqual(rejected, false); + assert.strictEqual(msg._cmbVerified, false); + assert.strictEqual(decisions.length, 0); + }); +});