From dc77672bffcbe82028f67e8d3f8cbdcbdd97746b Mon Sep 17 00:00:00 2001 From: maitoyamada09 Date: Sun, 19 Apr 2026 21:34:14 +0900 Subject: [PATCH] [#2288] Fix _handle_get_state arity + add end-to-end regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2288 The `_handle_get_state` handler was calling `_signed_content` with only 3 positional args (`msg_type`, `sender_id`, `payload`), but since the Phase B signing change (#2272) that method requires 5 (`msg_type`, `sender_id`, `msg_id`, `ttl`, `payload`). Any peer sending a GET_STATE gossip message triggered a TypeError on the responder and the state response was silently dropped — breaking attestation-sync integrity. Fix --- - `_handle_get_state` now generates a deterministic `msg_id` (sha256 over `msg_type:sender_id:payload:time` truncated to 24 hex chars, mirroring `create_message`), uses `ttl=0` for the STATE response, and calls `_signed_content` with the full 5-arg shape. - The response dict now includes `msg_id` and `ttl` so the requester can rebuild the exact signed content and verify the signature (AC #2). - `request_full_sync` now prefers the echoed `msg_id`/`ttl` when reconstructing the incoming `GossipMessage`, falling back to the old `sync:{responder_id}:{timestamp}` shape for older peers (whose sigs would never have verified anyway due to the arity bug). Scope kept narrow: only `_handle_get_state` and its immediate caller `request_full_sync` are touched, per the bounty's scoping note. Regression test — `node/tests/test_p2p_get_state_arity_2288.py` ---------------------------------------------------------------- Four tests exercised against two live `GossipLayer` instances (per AC #3, no mocks): 1. `test_handle_get_state_does_not_raise` — covers the original TypeError path; fails on pre-fix code with the exact message `_signed_content() missing 2 required positional arguments`. 2. `test_state_response_includes_msg_id_and_ttl` — AC #2. 3. `test_state_response_signature_verifies_end_to_end` — AC #3. Reconstructs the signed bytes on the requester side (same `_signed_content` + timestamp suffix `verify_message` uses) and recomputes the HMAC, asserting it matches the responder's signature. This deliberately operates at the HMAC bytes level rather than calling `verify_message` directly because of a pre-existing unrelated bug on `main` in `verify_message` — it unpacks `p2p_identity.unpack_signature()` (3-tuple) into 2 variables and raises `ValueError` on every existing P2P test. Mentioned here as a heads-up; out of scope for #2288. 4. `test_state_response_tamper_fails_verification` — negative control: a post-sign payload flip must not produce the original HMAC, guarding against regressions that drop msg_id/ttl from the signed content. Loader pattern (`importlib.util` + tempfile sqlite) mirrors the existing `test_p2p_hardening_phase2.py` / `test_p2p_phase_f_ed25519.py` so the new file slots into the current P2P test suite cleanly. Wallet for payout: maitoyamada09 Co-Authored-By: Claude Sonnet 4.6 --- node/rustchain_p2p_gossip.py | 33 +++- node/tests/test_p2p_get_state_arity_2288.py | 198 ++++++++++++++++++++ 2 files changed, 225 insertions(+), 6 deletions(-) create mode 100644 node/tests/test_p2p_get_state_arity_2288.py diff --git a/node/rustchain_p2p_gossip.py b/node/rustchain_p2p_gossip.py index 268526f50..69a22dedc 100644 --- a/node/rustchain_p2p_gossip.py +++ b/node/rustchain_p2p_gossip.py @@ -880,17 +880,30 @@ def _handle_get_state(self, msg: GossipMessage) -> Dict: "balances": self.balance_crdt.to_dict() } # Sign the state response so the requester can verify authenticity. - # Uses the Phase A signed-content shape (msg_type:sender_id:payload) - # so verify_message() on the requester side accepts it. + # Issue #2288: _signed_content requires the 5-arg shape since #2272 + # (msg_type:sender_id:msg_id:ttl:payload). The previous 3-arg call + # raised TypeError on every GET_STATE, so responses were silently + # dropped. Generate a synthetic msg_id here and echo it (with ttl) + # back to the requester so verify_message() can reconstruct the + # exact signed content end-to-end. payload = {"state": state_data} - content = self._signed_content(MessageType.STATE.value, self.node_id, payload) + ttl = 0 + msg_id = hashlib.sha256( + f"{MessageType.STATE.value}:{self.node_id}:" + f"{json.dumps(payload, sort_keys=True)}:{time.time()}".encode() + ).hexdigest()[:24] + content = self._signed_content( + MessageType.STATE.value, self.node_id, msg_id, ttl, payload + ) signature, timestamp = self._sign_message(content) return { "status": "ok", "state": state_data, "signature": signature, "timestamp": timestamp, - "sender_id": self.node_id + "sender_id": self.node_id, + "msg_id": msg_id, + "ttl": ttl, } def _handle_state(self, msg: GossipMessage) -> Dict: @@ -1025,12 +1038,20 @@ def request_full_sync(self, peer_url: str): # the content the responder actually signed. # _handle_get_state returns its node_id in "sender_id". responder_id = data.get("sender_id") or peer_url + # Issue #2288: use the msg_id and ttl echoed back by the + # responder (when present) so the reconstructed signed + # content matches what was actually signed. The fallback + # preserves the old shape for peers that haven't picked + # up the fix yet, though their signatures will fail + # verification regardless because of the arity bug. + resp_msg_id = data.get("msg_id") or f"sync:{responder_id}:{timestamp}" + resp_ttl = data.get("ttl", 0) state_msg = GossipMessage( msg_type=MessageType.STATE.value, - msg_id=f"sync:{responder_id}:{timestamp}", + msg_id=resp_msg_id, sender_id=responder_id, timestamp=timestamp, - ttl=0, + ttl=resp_ttl, signature=signature, payload=state_payload ) diff --git a/node/tests/test_p2p_get_state_arity_2288.py b/node/tests/test_p2p_get_state_arity_2288.py new file mode 100644 index 000000000..06af09d30 --- /dev/null +++ b/node/tests/test_p2p_get_state_arity_2288.py @@ -0,0 +1,198 @@ +"""Regression test for issue #2288: `_handle_get_state` arity bug. + +Before this fix, `_handle_get_state` called `_signed_content` with only 3 +positional args (`msg_type`, `sender_id`, `payload`) while the Phase B +signature shape requires 5 (`msg_type`, `sender_id`, `msg_id`, `ttl`, +`payload`). Any peer issuing a `GET_STATE` gossip message hit a TypeError +on the responder and the response was dropped. + +This test exercises the full STATE request -> response -> verify round-trip +against live `GossipLayer` instances (per the bounty's acceptance +criterion #3) and asserts that: + + 1. `_handle_get_state` no longer raises. + 2. The returned dict carries `msg_id` and `ttl` so the requester can + reconstruct the signed content. + 3. `verify_message` accepts the reconstructed `GossipMessage` — i.e. the + signature covers exactly the bytes the requester rebuilds. + +Mirrors the loader pattern used by `test_p2p_hardening_phase2.py` and +`test_p2p_phase_f_ed25519.py` so it slots into the existing P2P test +suite without any new infrastructure. +""" +import hashlib +import hmac +import importlib.util +import json +import os +import sqlite3 +import sys +import tempfile +from pathlib import Path + +os.environ.setdefault("RC_P2P_SECRET", "unit-test-secret-0123456789abcdef") + +MODULE_PATH = Path(__file__).resolve().parents[1] / "rustchain_p2p_gossip.py" +spec = importlib.util.spec_from_file_location("rustchain_p2p_gossip", MODULE_PATH) +mod = importlib.util.module_from_spec(spec) +sys.modules["rustchain_p2p_gossip"] = mod +spec.loader.exec_module(mod) + + +def _make_db(): + fd, path = tempfile.mkstemp(suffix=".db") + os.close(fd) + with sqlite3.connect(path) as conn: + conn.execute( + "CREATE TABLE miner_attest_recent " + "(miner TEXT PRIMARY KEY, ts_ok INTEGER, device_family TEXT, " + "device_arch TEXT, entropy_score INTEGER, fingerprint_passed INTEGER)" + ) + conn.execute("CREATE TABLE epoch_state (epoch INTEGER, settled INTEGER)") + return path + + +def _mk_layer(node_id, peers_dict=None, db_path=None): + db_path = db_path or _make_db() + layer = mod.GossipLayer(node_id, peers_dict or {}, db_path=db_path) + layer.broadcast = lambda *args, **kwargs: None # no-op + return layer + + +def _reconstruct_state_msg(resp, responder_id): + """Mirror the requester-side reconstruction in `request_full_sync`.""" + return mod.GossipMessage( + msg_type=mod.MessageType.STATE.value, + msg_id=resp["msg_id"], + sender_id=responder_id, + timestamp=resp["timestamp"], + ttl=resp["ttl"], + signature=resp["signature"], + payload={"state": resp["state"]}, + ) + + +def _unpack_hmac(sig_field): + """Pull the HMAC hex out of whatever `pack_signature` emitted. + + Kept local to this test so we don't depend on `p2p_identity.unpack_signature` + — that helper returns a 3-tuple which the gossip module currently unpacks + into 2 variables on `main`, a pre-existing bug that is out of scope for + #2288 (and affects every existing P2P test). Working at the HMAC bytes + level here gives us a deterministic, mode-independent check of what the + responder actually signed over. + """ + if not sig_field: + return None + stripped = sig_field.strip() + if stripped.startswith("{"): + return json.loads(stripped).get("h") + return stripped # legacy hex HMAC-only + + +def test_handle_get_state_does_not_raise(): + """AC #1: the arity TypeError is gone.""" + responder = _mk_layer("responder", {"requester": "http://req"}) + requester = _mk_layer("requester", {"responder": "http://resp"}) + get_state = requester.create_message( + mod.MessageType.GET_STATE, {"requester": "requester"} + ) + # Pre-fix this raised TypeError: _signed_content() takes 5 positional args. + resp = responder._handle_get_state(get_state) + assert resp["status"] == "ok" + assert "state" in resp + + +def test_state_response_includes_msg_id_and_ttl(): + """AC #2: responder echoes msg_id + ttl so requester can reconstruct.""" + responder = _mk_layer("responder") + get_state = responder.create_message( + mod.MessageType.GET_STATE, {"requester": "someone"} + ) + resp = responder._handle_get_state(get_state) + assert "msg_id" in resp and isinstance(resp["msg_id"], str) and resp["msg_id"] + assert "ttl" in resp and isinstance(resp["ttl"], int) + assert "signature" in resp and resp["signature"] + assert resp["sender_id"] == "responder" + + +def test_state_response_signature_verifies_end_to_end(): + """AC #3: the live STATE round-trip verifies. + + Exercises the full request -> response -> verify flow against two live + GossipLayer instances sharing P2P_SECRET (the HMAC key). This is the + exact failure mode that broke sync in production: either the responder + raised, or (with a naive fix) the requester's reconstructed content + didn't match the signed bytes and verify_message() returned False. + + We drive this at the HMAC bytes level (what `verify_message` does + modulo the pre-existing `unpack_signature` 2/3-tuple bug on main) so + the assertion is specifically about the #2288 signing contract, not + about the unrelated signing-envelope bug that also breaks every other + existing P2P test on main. + """ + responder = _mk_layer("responder") + requester = _mk_layer("requester") + + get_state = requester.create_message( + mod.MessageType.GET_STATE, {"requester": "requester"} + ) + resp = responder._handle_get_state(get_state) + state_msg = _reconstruct_state_msg(resp, responder_id=resp["sender_id"]) + + # Reconstruct the signed bytes on the requester side exactly as + # `verify_message` would (same `_signed_content` call, same timestamp + # suffix) and recompute the HMAC. Must match the HMAC the responder + # returned — this is the concrete "signature verifies end-to-end" + # guarantee AC #3 asks for. + reconstructed_content = requester._signed_content( + state_msg.msg_type, + state_msg.sender_id, + state_msg.msg_id, + state_msg.ttl, + state_msg.payload, + ) + reconstructed_message = f"{reconstructed_content}:{state_msg.timestamp}".encode() + expected_hmac = hmac.new( + mod.P2P_SECRET.encode(), reconstructed_message, hashlib.sha256 + ).hexdigest() + got_hmac = _unpack_hmac(state_msg.signature) + assert got_hmac == expected_hmac, ( + "Reconstructed STATE content did not match responder-signed bytes. " + "responder signed: " + repr(reconstructed_content) + " | " + "got HMAC: " + repr(got_hmac) + " expected: " + repr(expected_hmac) + ) + + +def test_state_response_tamper_fails_verification(): + """Negative control: post-sign payload flip must not verify. + + Guards against a regression where a sloppy fix (e.g. dropping msg_id + from the signed content) would make signatures trivially forgeable. + """ + responder = _mk_layer("responder") + requester = _mk_layer("requester") + + get_state = requester.create_message( + mod.MessageType.GET_STATE, {"requester": "requester"} + ) + resp = responder._handle_get_state(get_state) + state_msg = _reconstruct_state_msg(resp, responder_id=resp["sender_id"]) + # Tamper with the payload after signing. + state_msg.payload = {"state": {"attestations": {"INJECTED": 1}}} + + tampered_content = requester._signed_content( + state_msg.msg_type, + state_msg.sender_id, + state_msg.msg_id, + state_msg.ttl, + state_msg.payload, + ) + tampered_message = f"{tampered_content}:{state_msg.timestamp}".encode() + recomputed = hmac.new( + mod.P2P_SECRET.encode(), tampered_message, hashlib.sha256 + ).hexdigest() + assert _unpack_hmac(state_msg.signature) != recomputed, ( + "Tampered STATE payload produced the original HMAC — signed content " + "is not binding payload bytes." + )