Skip to content

fix: require admin key for machine passport events#4801

Closed
lampten wants to merge 2 commits into
Scottcjn:mainfrom
lampten:fix-machine-passport-event-auth
Closed

fix: require admin key for machine passport events#4801
lampten wants to merge 2 commits into
Scottcjn:mainfrom
lampten:fix-machine-passport-event-auth

Conversation

@lampten
Copy link
Copy Markdown
Contributor

@lampten lampten commented May 12, 2026

Summary

  • fail closed on machine passport repair, attestation, benchmark, and lineage writes when ADMIN_KEY is unset
  • require X-Admin-Key or legacy X-API-Key before passport lookup, JSON parsing, or mutation
  • add regression coverage for unauthorized writes plus valid repair, attestation, benchmark, and lineage updates

Closes #4562
Bounty: Scottcjn/rustchain-bounties#71
Miner/wallet ID: lampten-codex-earner
wallet: lampten-codex-earner

Validation

  • uv run --no-project --with pytest --with flask python -m pytest tests/test_machine_passport_event_json_validation.py -q
  • uv run --no-project --with pytest --with flask python -m pytest node/tests/test_machine_passport.py tests/test_machine_passport_event_json_validation.py -q
  • python -m py_compile node/machine_passport_api.py tests/test_machine_passport_event_json_validation.py
  • git diff --check -- node/machine_passport_api.py tests/test_machine_passport_event_json_validation.py
  • python tools/bcos_spdx_check.py --base-ref origin/main

No production testing or destructive actions performed.

@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 12, 2026
Copy link
Copy Markdown
Contributor

@saim256 saim256 left a comment

Choose a reason for hiding this comment

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

Approved current head 2f7e701 after reviewing the machine-passport event write auth fix for #4562.

The four vulnerable subresource write routes now fail closed when ADMIN_KEY is unset and require either X-Admin-Key or the legacy X-API-Key before passport lookup, JSON parsing, or ledger mutation. That covers repair-log, attestations, benchmarks, and lineage, including the sensitive lineage owner update path. Public read routes remain unchanged, and valid admin-key flows still preserve the existing empty-body defaults for attestation/benchmark writes.

Validation run locally:

  • git diff --check origin/main...HEAD -- node/machine_passport_api.py tests/test_machine_passport_event_json_validation.py -> passed
  • python -m py_compile node\machine_passport_api.py tests\test_machine_passport_event_json_validation.py -> passed
  • python -m pytest tests\test_machine_passport_event_json_validation.py -q -> 24 passed
  • python -m pytest node\tests\test_machine_passport.py tests\test_machine_passport_event_json_validation.py -q -> 50 passed
  • python -m ruff check node\machine_passport_api.py tests\test_machine_passport_event_json_validation.py --select E9,F821,F811 --output-format=concise -> All checks passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> OK

No production machine-passport API or live node testing was performed.

Copy link
Copy Markdown
Contributor

@godd-ctrl godd-ctrl left a comment

Choose a reason for hiding this comment

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

Approved current head 2f7e701 after reviewing the machine-passport event write auth fix for #4562. The four subresource write routes now fail closed when ADMIN_KEY is unset and require X-Admin-Key or the legacy X-API-Key before passport lookup, JSON parsing, or ledger mutation. That covers repair-log, attestations, benchmarks, and lineage, including the sensitive lineage owner update path, while public reads remain unchanged and valid admin-key writes preserve the existing empty-body defaults for attestation and benchmark events. Local validation: tests/test_machine_passport_event_json_validation.py passed 24 tests; node/tests/test_machine_passport.py plus the event validation file passed 50 tests; py_compile passed; Ruff E9/F821/F811 passed; git diff --check passed; BCOS SPDX check passed. No production machine-passport API or live node testing was performed.

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.

✅ Reviewed for RustChain bounty. Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

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.

✅ Reviewed for RustChain bounty. Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

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

@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.

Great contribution! LGTM. 🚀

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.

Great contribution! LGTM. 🚀

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.

Great contribution! LGTM. 🚀

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.

Great contribution! LGTM. 🚀

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.

Requesting changes because malformed admin-key input can crash the protected routes.

require_configured_admin_key() calls hmac.compare_digest(admin_key, expected_admin_key) on Python strings. If a request sends a non-ASCII header value, compare_digest() raises TypeError: comparing strings with non-ASCII characters is not supported, so the route returns a 500 instead of a clean 401. I reproduced this against /api/machine-passport/m1/repair-log with ADMIN_KEY=expected-admin-key and X-Admin-Key: é.

The new guard should normalize to bytes or catch this malformed-input case before calling compare_digest(). The same issue also applies to the other existing string-based admin-key comparisons in this module.

Validation performed: git diff --check origin/main...HEAD, PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /tmp/rustchain-flask-venv/bin/python -m pytest -q tests/test_machine_passport_event_json_validation.py node/tests/test_machine_passport.py, python3 -m py_compile node/machine_passport_api.py tests/test_machine_passport_event_json_validation.py, and python3 tools/bcos_spdx_check.py --base-ref origin/main all pass with 50 tests. Manual Flask test-client repro with a non-ASCII admin header returns 500.

@lampten
Copy link
Copy Markdown
Contributor Author

lampten commented May 12, 2026

Addressed this in commit e8905e0. I kept the scope narrow: the admin-key comparison now encodes both sides to bytes before hmac.compare_digest(), so malformed/non-ASCII header text is treated as an auth miss instead of raising TypeError. Normal valid and invalid ASCII admin-key behavior is unchanged.

I also routed the two existing admin-key comparisons in this module through the same helper to avoid leaving the same 500 path next to the newly protected routes.

Validation rerun locally:

  • uv run --no-project --with pytest --with flask python -m pytest tests/test_machine_passport_event_json_validation.py node/tests/test_machine_passport.py -q -> 51 passed
  • python -m py_compile node/machine_passport_api.py tests/test_machine_passport_event_json_validation.py node/tests/test_machine_passport.py -> passed
  • git diff --check -- node/machine_passport_api.py tests/test_machine_passport_event_json_validation.py node/tests/test_machine_passport.py -> passed
  • python tools/bcos_spdx_check.py --base-ref origin/main -> OK

@github-actions github-actions Bot added size/L PR: 201-500 lines and removed size/M PR: 51-200 lines labels May 12, 2026
Copy link
Copy Markdown
Contributor

@saim256 saim256 left a comment

Choose a reason for hiding this comment

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

Approved current head e8905e0ea29fe0ef5b1b56400f2a2ef23794fd7e.

I re-reviewed the branch after the malformed admin-key follow-up. The protected machine-passport write routes still fail closed when ADMIN_KEY is unset, require X-Admin-Key or legacy X-API-Key before lookup/parsing/mutation, and the latest helper now compares UTF-8 encoded bytes so non-ASCII header values are treated as unauthorized instead of raising TypeError.

Validation run locally:

  • git diff --check origin/main...pr-4801 -> passed
  • python -m py_compile node\machine_passport_api.py tests\test_machine_passport_event_json_validation.py node\tests\test_machine_passport.py -> passed
  • python -m pytest tests\test_machine_passport_event_json_validation.py node\tests\test_machine_passport.py -q -> 51 passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> OK
  • Manual Flask test-client smoke with ADMIN_KEY=expected-admin-key and X-Admin-Key: � against /api/machine-passport/m1/repair-log -> 401 unauthorized, not 500

No production machine-passport API or live node testing was performed.

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.

Code Review: Require Admin Key for Machine Passport Events

Summary

Adds fail-closed admin key authentication for machine passport event endpoints. When ADMIN_KEY is not configured, returns 401 (not allow-all). UTF-8 encoding with UnicodeError handling.

Verdict: Approve

Correct fail-closed implementation, consistent with #4882/#4883/#4884/#4894/#4895.

Copy link
Copy Markdown

@shuibui shuibui left a comment

Choose a reason for hiding this comment

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

Code Review: Approve

Good fix.

**Verdict: Approve.

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.

Reviewed the machine-passport event auth hardening. This looks good to merge.

Validation I ran:

  • git diff --check origin/main...HEAD -- node/machine_passport_api.py node/tests/test_machine_passport.py tests/test_machine_passport_event_json_validation.py — passed.
  • python3 -B -m py_compile node/machine_passport_api.py node/tests/test_machine_passport.py tests/test_machine_passport_event_json_validation.py — passed.
  • python3 -B -m pytest tests/test_machine_passport_event_json_validation.py node/tests/test_machine_passport.py -q51 passed, 1 warning in 0.64s.
  • Focused Flask stub probe covered all four protected event-write routes (repair-log, attestations, benchmarks, lineage): unset ADMIN_KEY returned 503 with no writes, wrong X-Admin-Key returned 401 with no writes, and a valid X-Admin-Key returned 200 and reached exactly the expected write methods (repair, attestation, benchmark, lineage).

The guard is now before ledger writes on the mutable event endpoints, fails closed when the admin secret is not configured, accepts only the existing header paths (X-Admin-Key / X-API-Key), and keeps the compare in constant-time byte form with malformed text handled as unauthorized. I do not see a blocker here.

@lampten
Copy link
Copy Markdown
Contributor Author

lampten commented May 14, 2026

Closing this from my side because upstream main already includes the machine-passport admin fail-closed/guard fixes in 6b689d3, 2ed75ee, and 09cd06f. No need to keep a conflicting duplicate PR open.

@lampten lampten closed this May 14, 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/L PR: 201-500 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SECURITY] Machine passport subresource writes allow unauthenticated history and owner tampering

8 participants