Skip to content

fix: use secret network challenge signing key#5102

Merged
Scottcjn merged 1 commit into
Scottcjn:mainfrom
galpetame:codex-fix-network-challenge-signing-key-4850
May 14, 2026
Merged

fix: use secret network challenge signing key#5102
Scottcjn merged 1 commit into
Scottcjn:mainfrom
galpetame:codex-fix-network-challenge-signing-key-4850

Conversation

@galpetame
Copy link
Copy Markdown

Fixes #4850.

What changed

  • Added explicit per-validator challenge signing key loading instead of deriving an HMAC key from the public validator key.
  • Supports a provided validator_signing_key, RC_NETWORK_CHALLENGE_SIGNING_KEY, or a per-instance random secret when no key is configured.
  • Rejects empty configured signing material.
  • Added regressions proving a public-key-derived forged signature no longer matches and that the generated/configured secret is used for challenge HMACs.

Validation

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests\test_network_challenge_signing_key.py -q -> 3 passed
  • python -m py_compile rips\rustchain-core\src\anti_spoof\network_challenge.py tests\test_network_challenge_signing_key.py -> passed
  • git diff --check origin/main...HEAD -- rips\rustchain-core\src\anti_spoof\network_challenge.py tests\test_network_challenge_signing_key.py -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> BCOS SPDX check: OK

No live validator, wallet, private key, token transfer, or destructive operation was used.

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes size/M PR: 51-200 lines and removed BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes labels May 13, 2026
@508704820
Copy link
Copy Markdown
Contributor

Code Review: Use secret network challenge signing key

Summary

Replaces a potentially predictable or hardcoded signing key for network challenges with a CSPRNG-derived key.

Consistent with the PRNG security fixes (#5099, #5080).

LGTM -- good security fix.

**Review quality: Security-focused review (CWE-321)

Copy link
Copy Markdown
Contributor

@loganoe loganoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I verified NetworkChallengeProtocol now loads explicit secret signing material from the constructor, RC_NETWORK_CHALLENGE_SIGNING_KEY, or a per-instance random secret instead of deriving the HMAC key from public validator data. Empty explicit signing material is rejected, and the regression confirms a signature forged from the public-key-derived path no longer matches.

Validation run:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 timeout 180 uv run --no-project --with pytest --with flask python -m pytest tests/test_network_challenge_signing_key.py -q -> 3 passed
  • python3 -m py_compile rips/rustchain-core/src/anti_spoof/network_challenge.py tests/test_network_challenge_signing_key.py -> passed
  • python3 tools/bcos_spdx_check.py --base-ref origin/main -> BCOS SPDX check OK
  • git diff --check origin/main...HEAD -- rips/rustchain-core/src/anti_spoof/network_challenge.py tests/test_network_challenge_signing_key.py -> clean

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bounty #73 review — APPROVED

I reviewed this focused RustChain PR and validated the changed files before approving.

Scope inspected:

  • PR: #5102 — fix: use secret network challenge signing key
  • Changed files: rips/rustchain-core/src/anti_spoof/network_challenge.py, tests/test_network_challenge_signing_key.py
  • Diff size: 2 files changed, 117 insertions(+), 6 deletions(-)

Validation evidence:

  • git diff --check origin/main...HEAD — passed
  • python3 tools/bcos_spdx_check.py --base-ref origin/main — passed
  • Added-line secret-pattern scan — passed
  • python3 -B -m py_compile rips/rustchain-core/src/anti_spoof/network_challenge.py tests/test_network_challenge_signing_key.py — passed
  • python3 -B -m pytest -q tests/test_network_challenge_signing_key.py — passed

Review reasoning:

  • The diff is scoped to rips/rustchain-core/src/anti_spoof/network_challenge.py, tests/test_network_challenge_signing_key.py and matches the PR intent.
  • Whitespace/conflict checks are clean, no new unlicensed code files were introduced, and no added secret-like literals were detected.
  • The applicable syntax/test gates passed locally, so I did not find a merge-blocking issue in this review.

Copy link
Copy Markdown

@8npyvz5bd8-lang 8npyvz5bd8-lang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the network challenge signing-key change.

This removes the previous public-key-derived challenge signing material and makes the signature depend on explicit secret material, an environment-provided secret, or a freshly generated per-instance secret. I checked the updated call path through NetworkChallengeProtocol.create_challenge() and the regression coverage: the tests verify that a signature forged from sha256(pubkey) no longer matches, that the configured/instance secret is used for HMAC, and that an explicitly empty key is rejected.

Verification I ran locally:

  • git diff --check origin/main...HEAD
  • python -m pytest tests/test_network_challenge_signing_key.py -q -> 3 passed

No blocking findings. One operational note for maintainers: production deployments should prefer RC_NETWORK_CHALLENGE_SIGNING_KEY or injected validator_signing_key so restarts do not silently rotate the signing secret; the random fallback is fine as a safe default for unconfigured/local instances.

Copy link
Copy Markdown
Contributor

@508704820 508704820 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review — #5102

1. CRITICAL — adds proper signing key management

Previously, the NetworkChallengeProtocol may have used a hardcoded or insecure signing key. This PR adds:

  • _load_signing_key method for proper key initialization
  • Optional validator_signing_key parameter (bytes or str)
  • Support for loading from environment or configuration

2. Key type flexibility

Union[bytes, str] allows the key to be provided as raw bytes or as a hex-encoded string. This supports different deployment scenarios.

3. Consider: key validation

The _load_signing_key method should validate:

  • Minimum key length (e.g., 32 bytes for Ed25519)
  • Key format (valid hex string if str type)
  • Key is not a known placeholder or empty

4. Environment variable for key storage

If the key is loaded from an environment variable, ensure:

  • The env var is not logged or exposed in error messages
  • The key is not stored in process memory longer than necessary
  • Key rotation is supported

— Xeophon (security review)

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for contributing. Approved.

Copy link
Copy Markdown
Contributor

@himanalot himanalot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I verified the signing-key behavior from 395c5fbfa392db4c6e42b9564a86fba6f2a6e965 in a temporary directory with a stdlib script because pytest is not installed here.

The check covered the important trust boundary: challenge signatures are produced with the configured/instance secret, not sha256(pubkey), so a verifier cannot forge challenges from public data. I also verified the random-secret fallback path and that an explicitly empty signing key is rejected.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review - Standard Quality ✓

Reviewed: Review submitted via GitHub API
Bounty: #73 - PR Reviews Bounty
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

This PR has been reviewed as part of the RustChain bounty program. All standard review criteria met.


🤖 Automated review via RustChain RTC bounty bot

@Scottcjn Scottcjn merged commit 60d57fb into Scottcjn:main May 14, 2026
3 checks passed
Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: NetworkChallengeProtocol derives private key deterministically from public key

9 participants