Skip to content

Secure Rent-a-Relic completion route#4896

Open
saim256 wants to merge 1 commit into
Scottcjn:mainfrom
saim256:fix-rent-relic-complete-admin-auth
Open

Secure Rent-a-Relic completion route#4896
saim256 wants to merge 1 commit into
Scottcjn:mainfrom
saim256:fix-rent-relic-complete-admin-auth

Conversation

@saim256
Copy link
Copy Markdown
Contributor

@saim256 saim256 commented May 12, 2026

Fixes #4762.\n\n## Summary\n- require RC_ADMIN_KEY before POST /relic/complete parses the request body or mutates reservation/escrow state\n- compare provided admin credentials as UTF-8 bytes with hmac.compare_digest, so malformed/non-ASCII header values return controlled 401 responses instead of 500s\n- fail closed with 503 when RC_ADMIN_KEY is unset and preserve locked escrow on unauthorized attempts\n\n## Validation\n- python -m pytest tests\test_rent_a_relic.py -q -> 37 passed\n- python -m py_compile tools\rent_a_relic\server.py tests\test_rent_a_relic.py\n- git diff --check\n- python tools\bcos_spdx_check.py --base-ref origin/main -> BCOS SPDX check: OK\n\nNo production testing or live-target probing was performed.

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

saim256 commented May 12, 2026

CI follow-up: all GitHub checks are now green, including est and BCOS v2 Engine Scan; merge state is CLEAN.

@508704820
Copy link
Copy Markdown
Contributor

Code Review: Secure Rent-a-Relic completion route

Summary

Adds admin key requirement to the /relic/complete endpoint via RC_ADMIN_KEY env var. Previously, anyone could complete a relic session.

Positive ✅

  1. Uses monkeypatch.setenv for test admin key — proper test isolation
  2. Adds admin_headers() helper for clean test code
  3. RC_ADMIN_KEY naming is consistent with other RustChain admin key patterns

Minor note 🔍

The complete endpoint now requires admin auth, but the create endpoint (/relic/rent) doesn't seem to require auth. Is it intentional that anyone can create a session but only admins can complete it? If so, this seems like a deliberate trust model (rental is open, completion requires verification).

LGTM ✅ Good security addition.

Review quality: Standard review

@galpetame
Copy link
Copy Markdown

galpetame commented May 12, 2026

Review for #4896

I validated the current PR head locally:

  • git diff --check origin/main...HEAD -> passed
  • python -m py_compile tools/rent_a_relic/server.py tests/test_rent_a_relic.py -> passed
  • python -m pytest tests/test_rent_a_relic.py -q -> 37 passed

I also checked the important ordering property from #4762: auth happens before completion-body parsing.

Extra local probe with a non-object JSON body:

  • missing admin key + json=["not", "object"] -> 401
  • wrong admin key + json=["not", "object"] -> 401
  • valid admin key + json=["not", "object"] -> 400

So the route rejects unauthenticated completion attempts before it reaches body-shape validation, and only authenticated callers reach the existing JSON-object check.

No blocker from me. Small test-hardening suggestion: add an explicit regression for the missing/wrong-admin + non-object-body case so the "auth before parse" invariant stays locked in. The current tests cover missing/wrong/unconfigured keys preserving escrow, and valid-admin non-object JSON returning 400, but they do not directly combine malformed body with missing/wrong admin.

Bounty #73 payout wallet if this review is eligible: RTCe4fbe4c9085b8b2ed3f1228504de66799025f6ce

GitHub handle for tagging: @galpetame

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.

Reviewed the Rent-a-Relic completion auth change. The admin check happens before request-body parsing and before reservation/escrow mutation, fails closed when RC_ADMIN_KEY is unset, and supports both configured-key and wrong-key paths without leaving escrow in a released state.

Validation run locally:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. /tmp/rustchain-flask-venv/bin/python -m pytest -q tests/test_rent_a_relic.py
  • /tmp/rustchain-flask-venv/bin/python -m py_compile tools/rent_a_relic/server.py tests/test_rent_a_relic.py

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: Solid Fix

Approve. Security pattern is correct.

**Verdict: Approve.

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

@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. Addresses the issue correctly.

**Verdict: Approve.

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. Addresses the issue correctly.

**Verdict: Approve.

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

@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

@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

@himanalot himanalot 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 active tools/rent_a_relic/server.py path and the new tests. The admin-key check now runs before JSON/body handling and before any reservation or escrow mutation, fails closed when RC_ADMIN_KEY is unset, and the tests cover missing/wrong/non-ASCII keys plus escrow preservation.

I do not see a blocking issue in this patch. Approved.

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.

Validated the admin-gated completion path and the escrow boundary. This looks good to merge from my review pass.

Validation run against head 8d2d6223d2cea3a2f6cef08ab2cee378a056aa30:

  • git diff --check origin/main...HEAD -- tools/rent_a_relic/server.py tests/test_rent_a_relic.py passed.
  • python3 -B -m py_compile tools/rent_a_relic/server.py tests/test_rent_a_relic.py passed.
  • python3 -B -m pytest -q tests/test_rent_a_relic.py passed with 37 passed, 1 warning in 0.43s.
  • Focused Flask runtime probe reserved g5-dual with the minimum 8.0 RTC, then checked the privileged completion route:
    • X-Admin-Key: é returned 401 {'code': 401, 'error': 'invalid admin key'} and left the reservation active with escrow locked.
    • the valid X-Admin-Key: expected-admin-key path returned 200 completed and moved escrow to released.

The patch fails closed when RC_ADMIN_KEY is missing, rejects absent/wrong/non-ASCII keys without releasing escrow, and only releases escrow after a valid admin key.

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) size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SECURITY] Rent-a-Relic completion releases escrow without admin authorization

7 participants