FIX(#4850): use random signing key instead of deterministic pubkey hash in challenge protocol#4851
Conversation
…ubkey hash - Generate random signing key per validator instance in __init__ - Replace hashlib.sha256(pubkey) with self._signing_key - Prevents forging challenge signatures from public key
saim256
left a comment
There was a problem hiding this comment.
This removes the exact hashlib.sha256(self.pubkey) derivation, but I do not think it fixes #4850's authentication problem yet.
Blocking issues:
-
The new signing key is random but not tied to the validator public key.
generate_challenge()still setschallenger_pubkey=hashlib.sha256(challenger_privkey).hexdigest()[:40], which is a hash of a secret HMAC key, not a public verification key. Peers cannot verify that the challenge came fromself.pubkeyor any registered validator identity. -
The protocol still uses HMAC, so verification requires the same secret key. There is no corresponding public-key verification path, key registry, or persistent validator private key. A random per-instance HMAC key can sign local challenges, but it does not give other nodes a way to verify those challenge signatures.
-
I could not find any challenge-signature or response-signature verification in
validate_response(). It checks timing/jitter/cache/thermal/serial/response hash, but never verifieschallenge.signatureorresponse.signature. That means forged or tampered signed fields are still not rejected by cryptographic verification.
I would expect either a real asymmetric keypair flow (validator private key signs, registered validator public key verifies) or a clearly documented shared-secret design with verification and key distribution. This PR also needs regressions showing that a signature derived from public data is rejected and that tampered challenge/response signatures fail validation.
Validation I ran locally:
git diff --check origin/main...HEAD -- rips/rustchain-core/src/anti_spoof/network_challenge.py-> passedpython -m py_compile rips\rustchain-core\src\anti_spoof\network_challenge.py-> passedpython tools\bcos_spdx_check.py --base-ref origin/main-> OKpython -m ruff check rips\rustchain-core\src\anti_spoof\network_challenge.py --select E9,F821,F811 --output-format=concise-> passedrg -n "generate_challenge|validate_response|signature|sign|pubkey|_signing_key|hashlib.sha256\(.*pubkey" rips\rustchain-core\src\anti_spoof tests -g "*.py"-> found no regression tests or verification path for challenge/response signatures
SimoneMariaRomeo
left a comment
There was a problem hiding this comment.
Requesting changes on current head 6e903567e66db0f0fd33da15668a2d55bec02723.
The patch removes the deterministic sha256(pubkey) secret, but it still does not make challenge signatures authenticate the validator identity.
Blocking issues:
Challenge.challenger_pubkeyis now derived from the random private key, not from the registered validator pubkey passed intoNetworkChallengeProtocol. A challenge created byNetworkChallengeProtocol(registered_validator_pubkey, ...)advertises a differentchallenger_pubkey, so recipients cannot tie the challenge back to the registered validator identity.- The signature fields still are not verified anywhere in the response-validation path.
AntiSpoofValidator.validate_response()checks timing, jitter, cache, thermal, serial, and response hash, but does not validatechallenge.signatureorresponse.signatureagainst any public key. That means challenge signatures still do not provide authentication even though the secret is no longer public. - There are no regression tests proving that a forged challenge/response signature is rejected or that the challenge signer identity equals the registered validator pubkey.
Validation I ran locally:
python -m py_compile rips\rustchain-core\src\anti_spoof\network_challenge.py-> passedgit diff --check origin/main...HEAD -- rips\rustchain-core\src\anti_spoof\network_challenge.py-> passedpython tools\bcos_spdx_check.py --base-ref origin/main-> OK- focused smoke: a protocol initialized with
registered_validator_pubkeyproduced a challenge with a differentchallenger_pubkey;AntiSpoofValidator.validate_response.__code__.co_namesdoes not referencesignature - Local Ruff unavailable:
No module named ruff
To close #4850, this needs a verifiable key model: keep the challenge identity bound to the registered validator public key, verify challenge and response signatures against registered public keys, and add tests that fail for forged signatures.
508704820
left a comment
There was a problem hiding this comment.
Code Review: Use Random Signing Key Instead of Deterministic Pubkey Hash
Summary
Replaces the deterministic signing key derivation (SHA256 of public key) with a cryptographically random 32-byte signing key. The old approach was critically insecure — anyone could derive the private key from the public key.
What Works Well
- Critical crypto fix: SHA256(pubkey) as private key is completely insecure — the private key is derivable from public information
- Correct randomness: secrets.token_bytes(32) provides 256 bits of cryptographic entropy
- Instance-level key: Each NetworkChallengeProtocol instance gets its own unique signing key
- Comment update: "demo" comment removed, replaced with proper description
Issues Found
1. Medium — Signing key is not persisted across restarts
If the NetworkChallengeProtocol is re-instantiated (e.g., node restart), a new random signing key is generated. Any challenges signed with the old key become unverifiable. Consider persisting the signing key:
key_path = os.environ.get("SIGNING_KEY_PATH", "")
if key_path and os.path.exists(key_path):
with open(key_path, "rb") as f:
self._signing_key = f.read(32)
else:
self._signing_key = secrets.token_bytes(32)
if key_path:
with open(key_path, "wb") as f:
f.write(self._signing_key)2. Low — No key rotation mechanism
If the signing key is ever compromised, there is no way to rotate it without restarting the protocol instance. Consider adding a rotate_signing_key() method.
3. Info — This fixes a demo-grade vulnerability
The comment "use real keys in production" was accurate — the old code was explicitly not production-ready. This PR makes it production-ready.
Verdict: Approve
Critical cryptography fix. The persistence concern is the most actionable follow-up.
godd-ctrl
left a comment
There was a problem hiding this comment.
Requesting changes on current head 6e90356.
The deterministic key derivation is removed, but the protocol still does not have a verifiable network signature. The new random HMAC key is kept only in the local NetworkChallengeProtocol instance, while generate_challenge() still publishes challenger_pubkey as sha256(secret)[:40]. A peer cannot verify challenge.signature from that public field, and the challenge no longer identifies the registered validator pubkey that issued it.
Focused smoke on this head:
- NetworkChallengeProtocol('registered-validator-A', ...).create_challenge(...) produced challenge.challenger_pubkey = 91d4... rather than registered-validator-A
- signature_len was 32 and pending challenge storage worked, but rg found no challenge signature verification path; only generation/signing uses challenge.signature
Other validation:
- python -m py_compile rips\rustchain-core\src\anti_spoof\network_challenge.py -> passed
- git diff --check origin/main...HEAD -- rips/rustchain-core/src/anti_spoof/network_challenge.py -> passed
- python tools\bcos_spdx_check.py --base-ref origin/main -> passed
A fix should use a real verifier-friendly signing model, e.g. an asymmetric validator private key with the registered public key on the challenge, or add a documented challenge-signature verification flow that can authenticate the issuer without exposing the HMAC secret.
508704820
left a comment
There was a problem hiding this comment.
Security Review — #4851
1. Critical fix — eliminates private key derivation from public key
The previous code derived the signing key by hashing the public key:
privkey = hashlib.sha256(self.pubkey.encode()).digest()
This means anyone who knows the validator's pubkey can compute their private key. This is a catastrophic vulnerability. The fix correctly uses a random 32-byte signing key.
2. Key persistence concern
The signing key is generated in init and stored as self._signing_key. This means:
- The key is lost on process restart
- Pending challenges signed with the old key cannot be verified
- Consider persisting the key to disk (encrypted) or deriving it from a KDF with a master secret
3. Missing: key rotation mechanism
There is no mechanism to rotate the signing key. If the key is compromised, there is no way to replace it without restarting the process.
4. Positive
- Uses secrets.token_bytes (CSPRNG) — correct choice
- Minimal, surgical fix
- Removes the comment "use real keys in production" — now it IS using real keys
- The 32-byte key provides 256 bits of entropy
— Xeophon (security review)
dazer1234
left a comment
There was a problem hiding this comment.
Code review for RustChain bounty #73.
Summary: Replacing a signing key derived from the public key is an improvement, because the previous behavior made the signing key predictable.
Findings:
- Medium: the new random key is stored only in memory and generate_challenge still publishes challenger_pubkey as hash(privkey) rather than the validator identity passed into NetworkChallengeProtocol. That avoids the old deterministic-key issue, but it still does not give peers a stable public identity they can verify against after restart or across nodes. This likely needs a real keypair or a persistent secret plus a documented verifier path.
Verdict: Good partial mitigation, but not a complete challenge-signature design yet.
Reviewed with OpenAI Codex assistance.
Fix for #4850: Deterministic private key from public key
Problem:
NetworkChallengeProtocol.create_challenge()derives the signing key ashashlib.sha256(pubkey.encode()).digest(). Since the pubkey is public, anyone can compute the same key and forge challenge signatures.Fix:
secrets.token_bytes(32)) per validator instance in__init__self._signing_keyinstead of deterministic hash of pubkeyTesting: AST parse verified ✅
Wallet:
RTC9d7caca3039130d3b26d41f7343d8f4ef4592360