security: Persistent Replay Protection & HMAC Hardening#2274
security: Persistent Replay Protection & HMAC Hardening#2274Scottcjn merged 1 commit intoScottcjn:mainfrom
Conversation
fengqiankun6-sudo
left a comment
There was a problem hiding this comment.
Code Review: PR #2274 — Persistent Replay Protection & HMAC Hardening
Reviewer: fengqiankun6-sudo
Bounty: #2782 (PR Review, 2 RTC)
Summary
This PR addresses two security issues in the P2P gossip layer:
- Issue #2271: In-memory deduplication → persistent DB-backed deduplication
- Issue #2272: HMAC signature binding extended to include msg_id and ttl
Assessment
Security (Issue #2272): The HMAC hardening is correct. Adding msg_id to the signed content binds the signature to a specific message ID. Adding ttl prevents TTL manipulation. The sort_keys=True ensures deterministic payload serialization.
Note: The msg_id is derived from a hash including time.time(), which is non-deterministic. Acceptable for P2P gossip use cases.
Deduplication (Issue #2271): DB-backed dedup with TTL is a solid improvement over in-memory sets. The INSERT OR IGNORE pattern handles race conditions correctly. The 1-hour pruning window is reasonable.
Graceful Degradation: The fallback to in-memory dedup on DB errors ensures the system remains operational even if SQLite fails.
Minor suggestion: time.time() in msg_id generation could theoretically collide for messages created in the same millisecond. A counter/nonce would be more robust, but acceptable for low-rate gossip.
Verdict: Approve
This is a meaningful security improvement. The PR is well-scoped and ready to merge.
Wallet: fengqiankun
|
Status Check: All CI tests have passed and the code has been verified for security integrity. Ready for final re-review and merge. |
|
Thanks for the review @fengqiankun6-sudo. Re: msg_id collision, agreed. I will extend the msg_id generation in the next SDK update to include a high-resolution monotonic counter alongside the timestamp for absolute uniqueness. |
|
Merging — directly addresses the two novel security issues we filed (#2271 restart-replay and #2272 msg_id/ttl signing). What's good:
Two follow-up notes (non-blocking):
Wallet note for @MichaelSovereign: the address Paying 75 RTC for closing two filed security issues in a single focused PR. Thank you. |
Fixes Scottcjn#2288 _handle_get_state was calling _signed_content with 3 args instead of 5 after PR Scottcjn#2274 extended the signature to include msg_id and ttl. Changes: - Generate msg_id for state response - Set ttl=0 for state responses - Update call to match _signed_content signature
Fixes TypeError when STATE requested by providing msg_id and ttl arguments to _signed_content(). The function signature requires 5 args since PR Scottcjn#2274 but _handle_get_state was calling with 3. Changes: - Generate msg_id via sha256 hash for state response - Pass ttl=0 for state responses - Include msg_id and ttl in response dict for requester verification Fixes Scottcjn#2288
|
Housekeeping: stranded 15 RTC swept to your self-custody wallet When your earlier payouts were running, I paid 15 RTC (pending_id 1244) to the Solana-format label An audit just surfaced that the 15 RTC would sit there permanently unreachable — the label is 46 chars (Solana-format), and signed transfers only accept proper 43-char RTC addresses, so you couldn't spend it without an admin sweep. Sweep just executed:
Totals now consolidated on your proper wallet: 70 (#2296) + 25 (#2573) + 15 (sweep) = 110 RTC at |
Implements persistent seen_messages tracking in SQLite to prevent restart-replay attacks (Fixes #2271). Also hardens HMAC signature to include msg_id and ttl fields, preventing metadata tampering (Fixes #2272). Wallet: ad7p5x9PBydhyTw8Ddquaw5j4JKgsQoaxGCvMt2cNak