Skip to content

fix(security): admin auth on block-slot, block-producers, and wallet endpoints#6715

Closed
BossChaos wants to merge 1 commit into
Scottcjn:mainfrom
BossChaos:fix/block-producer-admin-auth
Closed

fix(security): admin auth on block-slot, block-producers, and wallet endpoints#6715
BossChaos wants to merge 1 commit into
Scottcjn:mainfrom
BossChaos:fix/block-producer-admin-auth

Conversation

@BossChaos
Copy link
Copy Markdown
Contributor

Summary

Three unauthenticated endpoints exposed sensitive internal data:

Endpoint Exposure Severity
GET /block/slot Internal slot scheduling, balance summary, producer turn timing Medium
GET /block/producers Miner wallet addresses, selection weights, device arch Medium
GET /api/wallet/<wallet> Any wallet balance, enrollment weight, tier, network age Medium

Fix

Added _require_admin() guard (X-Admin-Key header vs RC_ADMIN_KEY env var) to:

  • list_producers()
  • get_current_slot()
  • api_wallet_lookup()

Consistent with the auth pattern used throughout the RustChain codebase (bcos_routes.py, sophia_governor.py, etc.).

Files

  • node/rustchain_block_producer.py: Added import hmac, _get_admin_key(), _require_admin(), auth on 2 endpoints
  • node/rustchain_dashboard.py: Added import hmac, _get_admin_key(), _require_admin(), auth on 1 endpoint

Vulnerabilities fixed:
- /block/slot: Exposed internal slot scheduling, balance summary, producer turn timing — attackers can infer mining schedule
- /block/producers: Exposed miner wallet addresses, selection weights, device arch — enables targeted attacks on producers
- /api/wallet/<wallet>: Exposed any wallet balance, enrollment weight, tier classification, network age — full account intelligence leak

All three endpoints now require X-Admin-Key header matching RC_ADMIN_KEY env var, consistent with the auth pattern used in other RustChain modules.

Fix: add _require_admin() guard to list_producers(), get_current_slot(), and api_wallet_lookup().
Copy link
Copy Markdown
Contributor

@zqleslie zqleslie 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 security hardening PR adding admin auth to three information-disclosure endpoints.

Scope reviewed:

  • node/rustchain_block_producer.py: _get_admin_key() + _require_admin() guard added to get_current_slot() and list_producers()
  • node/rustchain_dashboard.py: Duplicate _require_admin() guard added to api_wallet_lookup()

Assessment:

  1. Correctness: The _require_admin() pattern is identical to what's already deployed in rustchain_v2_integrated_v2.2.1_rip200.py for /withdraw/request, /governance/propose, and /governance/vote. This is consistent and low-risk.

  2. Fail-safe: When RC_ADMIN_KEY is unset, returns 503 rather than allowing unauthenticated access. Good.

  3. Endpoint impact:

    • GET /block/slot — exposes slot scheduling + balance summary → reasonable to protect
    • GET /block/producers — exposes miner addresses + weights → reasonable to protect
    • GET /api/wallet/<addr> — exposes any wallet balance → high value to protect, this is the most critical of the three
  4. Minor note: _require_admin() is duplicated across rustchain_block_producer.py and rustchain_dashboard.py. Not a blocker for this PR, but worth consolidating into a shared module in a follow-up.

No blockers. If accepted, routing via bounty #73.


This is a review claim for maintainer assessment only.

Copy link
Copy Markdown
Contributor

@zqleslie zqleslie 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 security hardening PR adding admin auth to 3 endpoints across 2 files.

Endpoints secured:

  • GET /block/slot → slot scheduling + balance summary (medium sensitivity)
  • GET /block/producers → miner addresses, weights, device arch (medium sensitivity)
  • GET /api/wallet/<addr> → any wallet balance, enrollment weight, tier (high sensitivity)

Assessment:

  1. _require_admin() pattern is consistent — matches the existing auth guard in rustchain_v2_integrated_v2.2.1_rip200.py for /withdraw/request and governance endpoints. Uses hmac.compare_digest() for timing-safe comparison. Returns 503 when RC_ADMIN_KEY is unset, which fails closed correctly.

  2. rustchain_block_producer.py: The _get_admin_key() and _require_admin() helpers are defined inside create_block_api_routes(), giving them access to Flask's request context via closure. This works correctly since Flask sets up request per-request in the same thread.

  3. rustchain_dashboard.py: Same pattern, standalone functions using Flask's request global. api_wallet_lookup is the most critical endpoint here — exposing arbitrary wallet balances without auth is a real information-leak risk. The 503 fallback when RC_ADMIN_KEY isn't set is correct (better than 401, signals misconfiguration not bad credentials).

  4. No blockers found — this is a clean, narrowly-scoped security fix. No runtime behavior changes beyond auth gating.


Review claim: bounty #73

Copy link
Copy Markdown
Contributor

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

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

I reviewed node/rustchain_block_producer.py and node/rustchain_dashboard.py in this PR.

Two technical observations:

  1. The fail-closed behavior is sound: _require_admin() returns 503 when RC_ADMIN_KEY is unset, so these newly protected endpoints do not accidentally remain public in a misconfigured deployment. Using hmac.compare_digest() for the provided X-Admin-Key also matches the existing constant-time comparison pattern used elsewhere in the project.

  2. The route coverage looks consistent with the stated data-exposure scope: both /block/slot and /block/producers are guarded inside create_block_api_routes(), and /api/wallet/<wallet_address> is guarded before opening the SQLite connection, so unauthenticated callers are rejected before wallet/producer details are read. One small cleanup I noticed: hashlib is imported in rustchain_block_producer.py but does not appear to be used by this patch.

I received RTC compensation for this review.

@Scottcjn
Copy link
Copy Markdown
Owner

Scottcjn commented Jun 1, 2026

Holding for a maintainer call — the auth helper is well-built (constant-time, fail-closed 503-on-unset), but this gates read-only GET endpoints /block/slot, /block/producers, and /api/wallet/<addr> behind admin auth. Those are public telemetry the explorer/dashboard depend on, and /api/wallet/<addr> was just added (#6736) + is used by tooling — admin-gating it would break public balance lookups. If the intent is to hide producer weights specifically, let's scope it to that endpoint rather than the public balance/slot reads.

Scottcjn added a commit that referenced this pull request Jun 1, 2026
…ucers (#6755)

Per Scott's call, /block/slot, /block/producers, and /api/wallet stay PUBLIC
(proof-of-antiquity consensus transparency the explorer/dashboard depend on;
no secrets/IPs/keys are exposed). The only hardening kept after tri-brain review:
expose device_info through an explicit field allowlist
(arch/family/model/year/enroll_weight) so a future column added to device_info
(e.g. an IP/hostname) can never leak through this unauthenticated endpoint.
Output is unchanged for current data; a non-dict/None row degrades to {} instead
of raising. is_my_turn and the balance summary are intentionally left as-is to
avoid breaking the public API contract.

Supersedes #6715 (which admin-gated these public endpoints and would have broken
the explorer). Tri-brain reviewed (Codex/Grok; GPT-OSS offline) — earlier
over-broad changes (removing is_my_turn, capping balance) were reverted as
regressions.

Co-authored-by: Scott Boudreaux <scottbphone12@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Scottcjn
Copy link
Copy Markdown
Owner

Scottcjn commented Jun 1, 2026

Closing in favor of keeping these endpoints PUBLIC. /block/slot, /block/producers, and /api/wallet are intentional proof-of-antiquity consensus transparency that the explorer/dashboard depend on — admin-gating them breaks public balance/slot/producer lookups, and they expose no secrets (wallet balances + producer weights are public-by-design on a transparent chain). The one legitimate concern (device_info field exposure) is addressed by an explicit allowlist in the merged PR above, which keeps the endpoint public. Thanks for the careful auth helper — the same fail-closed pattern is great for the genuinely-sensitive write endpoints (#6719 etc.).

@Scottcjn Scottcjn closed this Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/S PR: 11-50 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants