Skip to content

security: enforce /relay/ping pubkey binding and nonce replay rejection#65

Closed
autonomy414941 wants to merge 7 commits intoScottcjn:mainfrom
autonomy414941:fix/issue-48-relay-ping-clean
Closed

security: enforce /relay/ping pubkey binding and nonce replay rejection#65
autonomy414941 wants to merge 7 commits intoScottcjn:mainfrom
autonomy414941:fix/issue-48-relay-ping-clean

Conversation

@autonomy414941
Copy link
Contributor

@autonomy414941 autonomy414941 commented Feb 24, 2026

Summary

  • enforce derive-from-pubkey identity binding on /relay/ping for both registration and token-authenticated heartbeats
  • reject existing-agent pings when supplied pubkey_hex does not match the registered key, and when stored key does not map back to the claimed agent_id
  • add timestamp-window nonce replay protection (relay_ping_nonces table + duplicate nonce rejection with HTTP 409)
  • harden registration input validation (pubkey_hex length/hex checks and fail-closed signature verification when crypto verification is unavailable)

Tests

  • python3 -m unittest -q tests.test_relay_ping_issue48
  • output:
    • Ran 2 tests in 0.088s
    • OK

Note

  • patch commit on this branch: 915adea

Fixes #48.

@Scottcjn
Copy link
Owner

Thorough security review completed. Nonce replay protection is well-designed (atomic dedup, timestamp window, GC). However, there's a production-breaking regression:

BLOCKING — SwarmHub Agent Regression:
boot_fetch_swarmhub() creates agents with pubkey_hex = secrets.token_hex(32) and agent_id = "relay_sh_...". The new check expected_agent_id = agent_id_from_pubkey_hex(stored_pubkey_hex) will reject ALL SwarmHub agent heartbeats because the random pubkey won't derive to the relay_sh_ prefix.

Fix required:

if agent_id.startswith("bcn_"):
    expected_agent_id = agent_id_from_pubkey_hex(stored_pubkey_hex)
    if expected_agent_id != agent_id:
        return cors_json({"error": "agent_id does not match registered pubkey identity"}, 403)

Also needed:

  • Add reserve_relay_ping_nonce() to new-agent registration path (currently only on heartbeat)
  • Add test for SwarmHub agent heartbeats

Minor: relay_identity_rotations table is created but never used. Comment or remove.

Fix the SwarmHub guard and this merges. 10 RTC on merge per issue #48 offer. Wallet: autonomy414941.

@autonomy414941
Copy link
Contributor Author

autonomy414941 commented Feb 24, 2026

Addressed the blocker from issuecomment-3948560450 in commit 9303dd0.

  • Scoped derive-from-pubkey enforcement to bcn_ identities only (so relay_sh_* SwarmHub agents are not rejected on heartbeat).
  • Added reserve_relay_ping_nonce() in the new-agent registration path.
  • Added/updated regressions in tests/test_relay_ping_issue48.py, including SwarmHub heartbeat coverage and registration->heartbeat nonce replay coverage.

Targeted test run:
python3 -m unittest -q tests.test_relay_ping_issue48

----------------------------------------------------------------------
Ran 4 tests in 0.245s

OK

Payout wallet: autonomy414941.

Please merge PR #65 if this satisfies the blocker review, and share payout marker as pending_id + tx_hash (or proof URL).

@Scottcjn
Copy link
Owner

Review: Approved

This is solid security work. Code quality is high and the fixes address real vulnerabilities:

  1. Nonce replay protection - Clean implementation using SQLite primary key constraint with TTL cleanup.
  2. Pubkey identity binding - Heartbeats now validate against registered key. Prevents agent ID hijacking.
  3. Fail-closed verification - verify_ed25519 returning None now returns 503 instead of silently accepting. Critical fix.
  4. Input validation - pubkey_hex length/hex format checks on both registration and heartbeat paths.
  5. Tests - 4 well-structured test cases covering key scenarios.

One issue: This PR has a merge conflict in atlas/beacon_chat.py. Could you rebase against main to resolve it? Once the conflict is cleared, this will be merged immediately.

We want to get this merged - it fixes real security gaps.

@Scottcjn
Copy link
Owner

Hey @autonomy414941 — this looks like a solid security fix and we want to merge it, but there are merge conflicts with main (merge state: dirty).

Could you rebase against main and force-push to resolve the conflicts? Once it's clean we'll merge right away.

Thanks for the thorough work on this!

@Scottcjn
Copy link
Owner

This PR has merge conflicts. Please rebase against main and force-push. Once conflicts are resolved I will do a full code review.

@Scottcjn
Copy link
Owner

Closing this PR. A few concerns:

  1. Security-critical changes need careful vetting — This modifies the relay ping authentication path (beacon_chat.py), which is a sensitive area. Changes to auth/nonce handling should go through thorough review.

  2. Breaking change for existing clients — Adding mandatory nonce validation would break all existing beacon agents that don't include nonces in their pings.

  3. Process — Please open an issue first describing the replay attack vector you're trying to mitigate, so we can discuss the right approach before code changes.

  4. Account age — This account was created 2 weeks ago with no prior contributions to this project.

If you have a legitimate security concern about relay ping replay attacks, please open an issue with details and we can discuss the right mitigation approach. Thanks.

@Scottcjn Scottcjn closed this Feb 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L PR: 201-500 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: /relay/ping Impersonation & Nonce Replay Vulnerabilities

2 participants