Skip to content

fix: use constant-time sync admin key check#4196

Merged
Scottcjn merged 1 commit intoScottcjn:mainfrom
0oAstro:codex/fix-sync-admin-key-timing
May 10, 2026
Merged

fix: use constant-time sync admin key check#4196
Scottcjn merged 1 commit intoScottcjn:mainfrom
0oAstro:codex/fix-sync-admin-key-timing

Conversation

@0oAstro
Copy link
Copy Markdown
Contributor

@0oAstro 0oAstro commented May 10, 2026

Summary

Fixes #3226 by replacing the sync admin decorator's timing-unsafe API key comparison with hmac.compare_digest().

This is intentionally scoped to one issue and two files, following the branch-contamination closure guidance on earlier broad timing-fix PRs.

Root cause

register_sync_endpoints(...).require_admin compared the provided X-Admin-Key/X-API-Key to the configured sync admin key with !=. The same module already uses hmac.compare_digest() for sync signatures, but the admin decorator did not.

Changes

  • Normalizes a missing admin header to an empty string.
  • Uses hmac.compare_digest(key, admin_key) for sync admin auth.
  • Adds focused regression coverage proving /api/sync/status calls hmac.compare_digest and still rejects/accepts invalid/valid keys correctly.

Validation

Passed:

python3 -m py_compile node/rustchain_sync_endpoints.py node/tests/test_rustchain_sync_endpoints.py
/tmp/rustchain-bounty-venv/bin/python -m pytest node/tests/test_rustchain_sync_endpoints.py -q
/tmp/rustchain-bounty-venv/bin/ruff check node/rustchain_sync_endpoints.py node/tests/test_rustchain_sync_endpoints.py --select E9,F63,F7,F82
git diff --check

Bounty

Claiming bug bounty for #3226.

RTC wallet: RTC5268f16391bcdff87c43cd8694fca3be9d995359

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

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

@cerredz cerredz left a comment

Choose a reason for hiding this comment

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

Sync admin timing fix review.

I verified this locally on the PR branch. The change is narrowly scoped to the require_admin decorator and preserves the existing reject/accept behavior while routing non-empty supplied keys through hmac.compare_digest().

Validation performed:

  • python -m pytest node\tests\test_rustchain_sync_endpoints.py -q => 1 passed
  • python -m py_compile node\rustchain_sync_endpoints.py node\tests\test_rustchain_sync_endpoints.py
  • git diff --check origin/main...HEAD

No blocking findings from my review.

@Scottcjn
Copy link
Copy Markdown
Owner

💰 PAID — 10 RTC pending, will confirm in 24h.

  • tx hash: bb1a6a7d4c20106a6996170431e5da53
  • Pending ID: 1404

What worked

Sync admin decorator hardened correctly + the new test hits real endpoint behavior, not just source text. Solid second contribution from @0oAstro — the test pattern (real endpoint, real auth path) is the gold standard.

— auto-triage 2026-05-10

@Scottcjn Scottcjn merged commit d46a0d9 into Scottcjn:main May 10, 2026
3 checks passed
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/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] rustchain_sync_endpoints.py: require_admin decorator uses != for admin key comparison — timing attack

3 participants