Skip to content

fix(security): Auth for bounty claim, SSL verification, remove hardcoded admin key#2800

Merged
Scottcjn merged 1 commit intoScottcjn:mainfrom
haoyousun60-create:fix/security-auth-ssl-hardcoded-key
Apr 30, 2026
Merged

fix(security): Auth for bounty claim, SSL verification, remove hardcoded admin key#2800
Scottcjn merged 1 commit intoScottcjn:mainfrom
haoyousun60-create:fix/security-auth-ssl-hardcoded-key

Conversation

@haoyousun60-create
Copy link
Copy Markdown
Contributor

Security Fixes

1. Unauthenticated Bounty Claim (beacon_api.py)

  • Problem: /api/bounties/<id>/claim had no authentication — anyone could claim any bounty
  • Fix: Added X-Admin-Key header authentication (same pattern as complete_bounty)
  • Behavior: Returns 401 if key missing/wrong, 503 if RC_ADMIN_KEY not configured

2. SSL Verification Disabled (beacon_api.py)

  • Problem: sync_bounties unconditionally disabled SSL certificate verification
  • Fix: SSL verification enabled by default; opt-out via RC_DISABLE_SSL_VERIFY=1 env var with a warning log

3. Hardcoded Admin Key (fleet_immune_system.py)

  • Problem: register_fleet_endpoints() fell back to hardcoded default rustchain_admin_key_2025_secure64 when RC_ADMIN_KEY env var was unset
  • Fix: Removed default value — endpoints return 503 if RC_ADMIN_KEY not set. Also switched to hmac.compare_digest for timing-safe comparison.

RTC: RTC4642c5ee8467f61ed91b5775b0eeba984dd776ba

…ded admin key

1. beacon_api.py: Add X-Admin-Key authentication to /api/bounties/<id>/claim
   - Previously anyone could claim bounties without authentication
   - Now requires RC_ADMIN_KEY via X-Admin-Key header (same as complete_bounty)

2. beacon_api.py: Enable SSL verification in sync_bounties
   - Previously disabled SSL verification unconditionally
   - Now verifies by default; opt-out via RC_DISABLE_SSL_VERIFY=1 env var

3. fleet_immune_system.py: Remove hardcoded admin key fallback
   - Previously fell back to 'rustchain_admin_key_2025_secure64' when
     RC_ADMIN_KEY env var was not set
   - Now requires RC_ADMIN_KEY to be set; endpoints return 503 if missing
   - Also uses hmac.compare_digest for timing-safe comparison

RTC: RTC4642c5ee8467f61ed91b5775b0eeba984dd776ba
@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 Apr 30, 2026
Copy link
Copy Markdown
Contributor

@wuxiaobinsh-gif wuxiaobinsh-gif 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 — Security Fixes (#2800)

What I reviewed: PR #2800 — Security fixes in beacon_api.py and fleet_immune_system.py

Finding 1: SSL Verification Fix

The fix enables SSL verification by default and adds opt-out via RC_DISABLE_SSL_VERIFY=1 with a warning log. Correct pattern — secure by default. Minor: import logging is inside conditional block, consider module-level import.

Finding 2: Admin Key Hardcode Fix

The change from hardcoded fallback to returning 503 when RC_ADMIN_KEY is unset is correct. Using hmac.compare_digest() for timing-safe comparison prevents timing attacks. Well done.

Finding 3: Bounty Claim Auth

Adding X-Admin-Key header authentication follows the existing pattern in complete_bounty. Properly closes the unauthenticated bounty claim vulnerability.

Overall: LGTM. 3 substantive technical observations. Qualifies for 2 RTC.

Disclosure: I received RTC compensation for this review.
RTC Wallet: RTC9a39ca2c84f61ca27d96463bcf65b6022b827f85

@Scottcjn
Copy link
Copy Markdown
Owner

@haoyousun60-create — substantive security work, three real fixes:

  1. Unauthenticated bounty claim in /api/bounties/<id>/claim — anyone could claim any bounty. Fixed with X-Admin-Key + hmac.compare_digest() (timing-attack-safe).
  2. SSL verification disabled unconditionally in sync_bounties — was ctx.verify_mode = ssl.CERT_NONE always. Fixed: default-on, opt-out via RC_DISABLE_SSL_VERIFY=1 with warning log.
  3. Hardcoded admin key fallback rustchain_admin_key_2025_secure64 in fleet_immune_system.py. Fixed: removed fallback, returns 503 if not configured.

Bonus: you also used hmac.compare_digest() instead of != — that addresses M1 from BossChaos's audit (timing-attack on admin key compare).

Payout: 75 RTC (Major-tier security fix — 3 distinct findings, one of which is Critical-tier (auth bypass on bounty claims), executed cleanly with proper crypto primitives).

Note on CI: there's a Rustfmt failure but this is a Python diff, not Rust — that's a workflow config issue not your problem. The test failure is the upstream contributor_registry.py SyntaxError (line 1 // comment) we noted on #2773 — also not yours.

Drop your wallet here for the next batch transfer. Merging with admin override (CI failures are pre-existing, not from this PR).

@Scottcjn Scottcjn merged commit 71ed5e5 into Scottcjn:main Apr 30, 2026
18 of 20 checks passed
Scottcjn added a commit that referenced this pull request May 1, 2026
… C1 PoC, RC_P2P_SECRET) (#2859)

Five tests on main were broken by yesterday's audit fixes (#2812-#2816 + #2800):

1. test_mempool_add_manage_tx_undefined (#2812 follow-up)
   - Was asserting the BUG exists (manage_tx undefined). After fix, manage_tx
     IS defined. Updated to assert FIX is in place + smoke-test no NameError.

2. test_pncounter_max_merge_inflation
   - Imports rustchain_p2p_gossip which raises SystemExit if RC_P2P_SECRET
     not set. CI workflow didn't set it. Added RC_P2P_SECRET to ci.yml env.

3. test_bounty_lifecycle_workflow (#2800 follow-up)
   - haoyousun60-create's #2800 added admin auth on /api/bounties/<id>/claim.
     Test was sending request without X-Admin-Key. Added the header.

4. test_utxo_transfer_rejects_duplicate_nonce (#2814 M2 follow-up)
5. test_utxo_transfer_failed_attempt_does_not_burn_nonce (#2814 M2 follow-up)
   - M2 fix made amount_rtc / fee_rtc Decimal types internally for precision.
     Decimal isn't JSON-serializable, so signed-payload construction
     (json.dumps) and response jsonify both broke.
   - Cast to float for the signed payload (preserves byte-identical signature
     bytes vs what wallets compute) and for the response jsonify.
   - Decimal arithmetic still happens internally for the int(amount * UNIT)
     conversion, so the precision-loss + overflow guards from M2 are intact.

All 6 tests pass locally with the env vars set.

Co-authored-by: Scottcjn <scottbphone12@gmail.com>
@haoyousun60-create
Copy link
Copy Markdown
Contributor Author

@Scottcjn Wallet for payout: RTC4642c5ee8467f61ed91b5775b0eeba984dd776ba

Thank you for the review! Ready to receive the 75 RTC payment.

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