fix: require admin key for machine passport events#4801
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
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
Bot
added
BCOS-L1
saim256
left a comment
saim256
left a comment
There was a problem hiding this comment.
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.
godd-ctrl
left a comment
godd-ctrl
left a comment
There was a problem hiding this comment.
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.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
✅ Reviewed for RustChain bounty. Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
✅ Reviewed for RustChain bounty. Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for contributing. Approved.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
Great contribution! LGTM. 🚀
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
Great contribution! LGTM. 🚀
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
Great contribution! LGTM. 🚀
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
Great contribution! LGTM. 🚀
loganoe
left a comment
loganoe
left a comment
There was a problem hiding this comment.
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.
|
Addressed this in commit 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:
|
github-actions
Bot
added
size/L
saim256
left a comment
saim256
left a comment
There was a problem hiding this comment.
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-> passedpython -m py_compile node\machine_passport_api.py tests\test_machine_passport_event_json_validation.py node\tests\test_machine_passport.py-> passedpython -m pytest tests\test_machine_passport_event_json_validation.py node\tests\test_machine_passport.py -q-> 51 passedpython tools\bcos_spdx_check.py --base-ref origin/main-> OK- Manual Flask test-client smoke with
ADMIN_KEY=expected-admin-keyandX-Admin-Key: �against/api/machine-passport/m1/repair-log-> 401unauthorized, not 500
No production machine-passport API or live node testing was performed.
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
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.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
7 participants
Summary
ADMIN_KEYis unsetX-Admin-Keyor legacyX-API-Keybefore passport lookup, JSON parsing, or mutationCloses #4562
Bounty: Scottcjn/rustchain-bounties#71
Miner/wallet ID:
lampten-codex-earnerwallet: lampten-codex-earner
Validation
uv run --no-project --with pytest --with flask python -m pytest tests/test_machine_passport_event_json_validation.py -quv run --no-project --with pytest --with flask python -m pytest node/tests/test_machine_passport.py tests/test_machine_passport_event_json_validation.py -qpython -m py_compile node/machine_passport_api.py tests/test_machine_passport_event_json_validation.pygit diff --check -- node/machine_passport_api.py tests/test_machine_passport_event_json_validation.pypython tools/bcos_spdx_check.py --base-ref origin/mainNo production testing or destructive actions performed.