fix(attest): require Ed25519 signature on /attest/submit

[rebel117]

Problem

The /attest/submit endpoint accepts attestation submissions without Ed25519 signature verification. When both signature and public_key fields are omitted, the entire verification block is skipped — execution falls through to nonce validation and ticket issuance.

This means anyone can submit forged attestations impersonating any wallet, since /attest/challenge issues nonces without authentication either.

Reported in #8016.

Fix

Added a guard right after the existing field normalization that rejects unsigned attestations with HTTP 400 unless explicitly allowed:

allow_unsigned_attest = os.environ.get(
    RTC_ALLOW_UNSIGNED_ATTEST, 
).lower() in (1, true, yes)
if (not sig_hex or not pubkey_hex) and not allow_unsigned_attest:
    return jsonify({
        "ok": False,
        "error": "missing_signature",
        "message": "Ed25519 signature and public_key are required",
        "code": "MISSING_SIGNATURE"
    }), 400

The RTC_ALLOW_UNSIGNED_ATTEST env var provides a backward-compat escape hatch for operators still running older miners that have not been updated to sign attestations.

Test changes

• test_missing_signature_allowed → renamed to test_missing_signature_rejected — now asserts HTTP 400 with MISSING_SIGNATURE
• Added test_unsigned_allowed_via_compat_flag — verifies the compat path still works when RTC_ALLOW_UNSIGNED_ATTEST=true
• Updated docstring on test_signature_rejected_when_pynacl_missing to reflect the new default behavior

Both new tests pass individually. The full test file has pre-existing prometheus registry conflicts when run together (unrelated to this change).

[github-actions]

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

• Non-doc PRs have a BCOS-L1 or BCOS-L2 label
• Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
• 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!

[FlintLeng]

PR Review: #8052 - fix(attest): require Ed25519 signature on /attest/submit

Summary

This PR addresses a critical security vulnerability where the /attest/submit endpoint accepts attestation submissions without Ed25519 signature verification, allowing potential forgery attacks.

Review

Security Impact: High

• Fixes a critical authentication bypass in attestation submission
• Prevents wallet impersonation through forged attestations
• Related to Issue CRITICAL: Unsigned Attestation Signature Bypass - Wallet Impersonation #8016 (CRITICAL: Unsigned Attestation Signature Bypass)

Code Quality:

• ✅ Clear problem statement and fix explanation
• ✅ Proper environment variable fallback for backward compatibility (RTC_ALLOW_UNSIGNED_ATTEST)
• ✅ Defensive coding with explicit error messages
• ✅ Maintains flexibility for development/testing scenarios

Testing Considerations:

• Should verify that signed attestations still work correctly
• Should verify that unsigned attestations are properly rejected
• Should test the RTC_ALLOW_UNSIGNED_ATTEST override behavior
• Edge case: malformed signatures should also be rejected

Suggested Follow-up:

• Consider adding integration tests for this security fix
• Consider logging rejected unsigned attempts for security monitoring
• Documentation update for the new environment variable

Recommendation

This is a critical security fix that should be prioritized for merge. The implementation is sound and maintains backward compatibility through an opt-in environment variable.

Approval: ✅ Recommend merge after CI passes

---

Reviewer: FlintLeng
RTC Wallet: RTC019e78d600fb3131c29d7ba80aba8fe644be426e

[mumuzhong3]

Substantive review on the compat-flag duplication and a couple of follow-ups worth considering before merge:

1. monkeypatch.setenv("RTC_ALLOW_UNSIGNED_ATTEST", "true") is now repeated in 5 separate fixtures (test_attest_init_schema.py, test_attestation_fuzz.py, test_rip201_fleet_bypass.py, test_wallet_review_holds.py, plus the new test_unsigned_allowed_via_compat_flag). Every existing attestation test had to be touched because unsigned attestations were the implicit baseline. Suggest hoisting this into a single autouse fixture in tests/conftest.py (or a node-level tests/node/conftest.py) so future attestation tests inherit the compat flag and individual files stop carrying the same boilerplate. Also means toggling the default in future is a one-line change.

2. Manual env save/restore in test_missing_signature_rejected — prev = os.environ.pop(...) then later if prev is not None: os.environ[...] = prev. If the test raises between the pop and the restore (assertion in _submit, an unexpected exception), the env var leaks into other tests in the same process. monkeypatch.setenv / monkeypatch.delenv would auto-clean on test teardown regardless of outcome. Tiny safety win for pytest-xdist runs.

3. Document RTC_ALLOW_UNSIGNED_ATTEST somewhere operators will see it. Operators still on the unsigned miner path need to know the env var exists and that it's only meant as a migration bridge. Even a one-liner in .env.example or the operator runbook would do — currently nothing in the repo (I grepped RTC_ALLOW) points new operators at it.

4. Error shape consistency. The new 400 returns {ok, error, message, code} which matches what test_tampered_commitment_rejected already returns (code: "INVALID_SIGNATURE"). Good — worth noting in the PR description that this matches existing error-envelope conventions so client error handling doesn't need to branch.

5. The (not sig_hex or not pubkey_hex) check — does it correctly handle the case where one is empty string and the other is non-empty (e.g., sig_hex="", pubkey_hex="abc...")? The OR short-circuits correctly here, but it's worth a parametrized test for that mixed case (currently only test_missing_signature_rejected covers both-blank, and test_tampered_commitment_rejected covers both-present).

None of these block merge — fix is sound and the rename of test_missing_signature_allowed → _rejected plus the compat test pair are exactly the right shape. Happy to open a small follow-up PR for items 1 + 2 (conftest hoist + monkeypatch) if useful.