Skip to content

fix: require admin key on all BFT consensus POST endpoints#6638

Closed
BossChaos wants to merge 1 commit into
Scottcjn:mainfrom
BossChaos:fix/bft-consensus-auth
Closed

fix: require admin key on all BFT consensus POST endpoints#6638
BossChaos wants to merge 1 commit into
Scottcjn:mainfrom
BossChaos:fix/bft-consensus-auth

Conversation

@BossChaos
Copy link
Copy Markdown
Contributor

Summary

Three BFT consensus POST endpoints in rustchain_bft_consensus.py accepted arbitrary input without admin authentication, enabling unauthenticated actors to inject BFT messages, manipulate view state, and trigger unauthorized epoch proposals.

Vulnerabilities Fixed

Endpoint Severity Issue
POST /bft/message HIGH No auth — arbitrary BFT message injection (PRE-PREPARE, PREPARE, COMMIT)
POST /bft/view_change HIGH No auth — unauthorized view change messages
POST /bft/propose HIGH No auth — trigger epoch proposal at will

Fix

Added _require_admin() helper using RC_ADMIN_KEY environment variable + X-Admin-Key HTTP header with hmac.compare_digest timing-safe comparison. All three write endpoints now enforce admin authentication before processing.

def _require_admin():
    admin_key = os.environ.get("RC_ADMIN_KEY", "")
    if not admin_key:
        return jsonify({'error': 'RC_ADMIN_KEY not configured -- BFT admin endpoints disabled'}), 503
    provided = request.headers.get("X-Admin-Key", "")
    if not hmac.compare_digest(provided, admin_key):
        return jsonify({'error': 'Unauthorized -- admin key required'}), 401
    return None

Severity

HIGH — Unauthorized epoch proposals can disrupt BFT consensus. Message injection can corrupt consensus state. View change manipulation can cause denial of service.

Bounty

Bounty #73 | Fix PR | Wallet: RTC6d1f27d28961279f1034d9561c2403697eb55602

- /bft/message: no auth required for BFT message injection (HIGH)
- /bft/view_change: no auth required for BFT view change (HIGH)
- /bft/propose: no auth required for epoch proposal trigger (HIGH)

All three POST endpoints accepted arbitrary input without admin
authentication, allowing unauthorized BFT message injection,
view manipulation, and epoch proposal disruption.

Fix: add _require_admin() check using RC_ADMIN_KEY + X-Admin-Key
header with hmac.compare_digest on all three write endpoints.
@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/S PR: 11-50 lines labels May 30, 2026
admin_key = os.environ.get("RC_ADMIN_KEY", "")
if not admin_key:
return jsonify({'error': 'RC_ADMIN_KEY not configured -- BFT admin endpoints disabled'}), 503
provided = request.headers.get("X-Admin-Key", "")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes /bft/message and /bft/view_change require X-Admin-Key, but the local peer broadcasters at _broadcast_message() and _broadcast_view_change() still call requests.post(...) without that header. With RC_ADMIN_KEY configured, normal peer traffic will be rejected before receive_message() / handle_view_change() runs, so the fix trades public injection for a consensus liveness regression. Please update the broadcast path or use a separate peer-auth mechanism, and add a regression test covering authenticated inter-node BFT traffic.

Copy link
Copy Markdown

@aisoh877 aisoh877 left a comment

Choose a reason for hiding this comment

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

Thanks for working on closing this unauthenticated BFT POST surface. I think this needs one change before merge.

The new _require_admin() gate is applied to /bft/message and /bft/view_change, but the peer broadcast paths still do not send X-Admin-Key: _broadcast_message() sends only X-Node-ID, and _broadcast_view_change() sends no auth header. Once RC_ADMIN_KEY is configured, every honest peer that receives these broadcasts will reject them with 401, so PRE-PREPARE/PREPARE/COMMIT and view-change propagation stop. If RC_ADMIN_KEY is unset, these routes now return 503 and disable inbound BFT traffic entirely.

I think the fix is to either add matching internal-auth support to the broadcast side and cover it in route/broadcast tests, or keep peer consensus traffic protected by the existing signed-message verification and only require admin auth for the manual /bft/propose endpoint.

I could not run the focused pytest suite in this checkout because the Python environment is missing flask/pytest, but the changed call sites show the regression statically.

@Scottcjn
Copy link
Copy Markdown
Owner

@BossChaos — closing as NEEDS_FIX per Codex authoritative audit.

Good news first: the disclosure-recovery loop worked. Unlike your closed #6559 which hid /hall/induct and /bft/propose route changes inside a 3-route-disclosed PR, this PR honestly enumerates the BFT POST routes in the title/body. No hidden hunks. That's exactly the path-forward shape I asked for in the #6559 close. Recurrence-watch stays on but this is the right direction.

Why it's NEEDS_FIX (not REJECT or MERGE):

Codex authoritative verdict: the inbound auth hardening (requiring admin-key on incoming BFT POSTs) was not paired with corresponding outbound sender changes. Existing peer-to-peer BFT traffic between honest nodes would now be rejected because the senders don't carry the admin-key header.

Result: 21 existing tests fail when the patch is applied (per Codex's local validation run). The fix breaks honest peer traffic while closing the unauthenticated attacker path.

Required change: pair the inbound auth requirement with either:

  • (a) An internal-trusted-peer bypass keyed on signed mTLS / Ed25519 peer identity (preferred — cleaner trust model), OR
  • (b) Update the peer-sender path to attach the admin-key header for outbound BFT POSTs (simpler but couples auth to admin-key visibility)

Verify by running the failing test suite locally (pytest -q node/tests/test_bft_route_validation.py node/tests/test_bft_routes.py) until all 21 pass.

Update and ping — happy to pay High (50 RTC) on a clean re-audit.

@Scottcjn
Copy link
Copy Markdown
Owner

Closed — NEEDS_FIX. Honest disclosure (good), but fix breaks 21 honest peer-traffic tests. Pair inbound auth with outbound sender change. See comment.

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