Add replay protection timestamp validation to sign requests#42
Add replay protection timestamp validation to sign requests#42kwsantiago merged 1 commit intomainfrom
Conversation
WalkthroughAdds replay-window protection: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
keep-frost-net/src/node.rs (2)
108-110: Document the timing constraint for calling this setter.The setter requires
&mut self, which means it cannot be called whilerun()is executing (sincerun()holds an immutable borrow). Consider adding documentation to clarify that this must be called before starting the node.📝 Suggested documentation
+ /// Sets the replay window in seconds. + /// + /// Must be called before `run()` is invoked, as it requires exclusive access. pub fn set_replay_window(&mut self, secs: u64) { self.replay_window_secs = secs; }
371-381: Consider improving timestamp logging for better observability.The
created_atfield is logged as a raw Unix timestamp (u64), which is difficult to interpret in logs. Consider adding a human-readable datetime representation or at least labeling it as a Unix timestamp.🔎 Enhanced logging example
if !request.is_within_replay_window(self.replay_window_secs) { warn!( session_id = %hex::encode(request.session_id), - created_at = request.created_at, + created_at_unix = request.created_at, + created_at_utc = %chrono::DateTime::from_timestamp(request.created_at as i64, 0) + .map(|dt| dt.to_rfc3339()) + .unwrap_or_else(|| "invalid".to_string()), "Rejecting sign request: outside replay window" );keep-frost-net/src/protocol.rs (1)
135-140: Document the symmetric window behavior for clock skew tolerance.The method accepts timestamps within
window_secsin both the past and future (creating a symmetric window). This is intentional for handling clock skew between nodes, but the behavior should be documented to clarify that requests with future timestamps (up to the window limit) are accepted.📝 Suggested documentation
+ /// Validates that the request timestamp is within the replay window. + /// + /// Returns `true` if `created_at` falls within `[now - window_secs, now + window_secs]`. + /// The symmetric window accommodates clock skew between nodes in distributed environments. + /// + /// # Arguments + /// * `window_secs` - Maximum acceptable time delta (in seconds) from current time pub fn is_within_replay_window(&self, window_secs: u64) -> bool { let now = chrono::Utc::now().timestamp() as u64; let min_valid = now.saturating_sub(window_secs); let max_valid = now.saturating_add(window_secs); self.created_at >= min_valid && self.created_at <= max_valid }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keep-frost-net/src/lib.rskeep-frost-net/src/node.rskeep-frost-net/src/protocol.rs
🧰 Additional context used
🧬 Code graph analysis (2)
keep-frost-net/src/protocol.rs (1)
keep-frost-net/src/node.rs (1)
new(57-94)
keep-frost-net/src/node.rs (3)
keep-frost-net/src/protocol.rs (1)
session_id(36-45)keep-frost-net/src/session.rs (1)
session_id(67-69)keep-agent/src/session.rs (1)
created_at(155-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
keep-frost-net/src/lib.rs (1)
69-70: LGTM! Public export enhances configurability.The export of
DEFAULT_REPLAY_WINDOW_SECSallows library consumers to reference the default value when configuring custom replay windows, improving API usability.keep-frost-net/src/node.rs (2)
53-53: LGTM! Field initialization follows best practices.The
replay_window_secsfield is properly initialized with the default constant, providing sensible out-of-the-box replay protection.Also applies to: 92-92
377-380: TheReplayDetectederror variant is already defined and correctly used.The
ReplayDetectedvariant is defined inkeep-frost-net/src/error.rs(line 35) asReplayDetected(String). The code at lines 377-380 correctly uses this variant by passing a formatted string message. No changes are needed.Likely an incorrect or invalid review comment.
keep-frost-net/src/protocol.rs (3)
7-7: LGTM! Reasonable default for replay protection.A 5-minute window (300 seconds) provides adequate protection against replay attacks while accommodating reasonable clock skew between distributed nodes.
106-106: Field rename improves semantic clarity.Renaming
timestamptocreated_atbetter conveys the intent that this captures when the request was created, not just any arbitrary timestamp.Also applies to: 125-125
402-420: Comprehensive test coverage for replay window validation.The tests properly validate:
- Current timestamps are accepted
- Past timestamps outside the window are rejected
- Future timestamps outside the window are rejected
- Configurable windows work correctly
wksantiago
force-pushed
the
Add-replay-protection-timestamp
branch
from
8eb3ccc to
b50a173
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keep-frost-net/src/node.rs (1)
401-411: Improve timestamp logging and error message clarity.The replay window validation logic is correct, but consider these improvements:
Human-readable timestamp: Line 404 logs
created_atas a raw u64, which is difficult to interpret. Format it as an ISO 8601 datetime string for better observability.Direction indicator: The error message doesn't indicate whether the timestamp is too old or too new, which would help diagnose clock skew issues.
🔎 Suggested improvements
if !request.is_within_replay_window(self.replay_window_secs) { + let now = chrono::Utc::now().timestamp() as u64; + let age = if request.created_at > now { + format!("+{} seconds in future", request.created_at - now) + } else { + format!("-{} seconds old", now - request.created_at) + }; warn!( session_id = %hex::encode(request.session_id), - created_at = request.created_at, + created_at = %chrono::DateTime::<chrono::Utc>::from_timestamp(request.created_at as i64, 0) + .map(|dt| dt.to_rfc3339()) + .unwrap_or_else(|| format!("invalid({})", request.created_at)), + age = %age, - "Rejecting sign request: outside replay window" + "Rejecting sign request: timestamp outside replay window" ); return Err(FrostNetError::ReplayDetected(format!( - "Request created_at {} outside {} second window", - request.created_at, self.replay_window_secs + "Request timestamp {} outside {} second window", + age, self.replay_window_secs ))); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keep-frost-net/src/lib.rskeep-frost-net/src/node.rskeep-frost-net/src/protocol.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- keep-frost-net/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (1)
keep-frost-net/src/node.rs (4)
keep-frost-net/src/protocol.rs (1)
session_id(47-56)keep-core/src/frost/signing.rs (1)
session_id(91-93)keep-frost-net/src/session.rs (1)
session_id(97-99)keep-agent/src/session.rs (1)
created_at(155-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
keep-frost-net/src/node.rs (1)
138-140: LGTM: Replay window is now configurable at runtime.The setter allows applications to adjust the replay protection window based on their security requirements and network conditions.
keep-frost-net/src/protocol.rs (3)
7-7: LGTM: Reasonable default replay window.The 300-second (5-minute) window provides a good balance between security and tolerance for clock skew and network delays.
588-606: LGTM: Comprehensive test coverage for replay window validation.The tests appropriately verify:
- Current timestamps are accepted
- Old timestamps beyond the window are rejected
- Future timestamps beyond the window are rejected
- Wider windows accept the edge cases
200-205: Field rename verification complete; bidirectional replay window is intentional.No references to the old
timestampfield remain in the codebase—the rename tocreated_atis consistent throughout.The bidirectional replay window that accepts future timestamps is intentional and explicitly tested (see
test_replay_window_validation()which verifies both past and future timestamps are accepted within the configured window). This provides tolerance for clock skew in both directions, which is a deliberate design choice.
2 participants
Adds timestamp-based replay protection for FROST signing requests with configurable window.
Summary by CodeRabbit
New Features
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.