Uh oh!
There was an error while loading. Please reload this page.
Simplify LSPS5/validator: drop time checks & custom signature storage - #3961
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
martinsaposnic
commented
Jul 25, 2025
thoughts? @tnull@TheBlueMatt |
f4dc5a0 to
e397c77CompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3961 +/- ##
=======================================
Coverage 88.93% 88.94% =======================================
Files 174 174 Lines 123880 123835 -45 Branches 123880 123835 -45 =======================================
- Hits 110176 110147 -29 + Misses 11251 11240 -11 + Partials 2453 2448 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a882fed to
2122b24CompareTheBlueMatt
commented
Jul 25, 2025
I'm a fan of this, and the code seems Obviously Correct (tm), but I'll let @tnull chime in in case he thinks there is some attack the more full-featured replay protection fixes. |
ldk-reviews-bot
commented
Jul 28, 2025
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
As discussed elsewhere, concept ACK from my side. I'll actually also see to update the bLIP-55 PR to drop the mention of the timestamp header there.
That said, I'm a bit confused why we have a signature cache at all now? If we think TLS sufficiently protects against replaying packets, what additional protection does the cache give us? Especially, since the cache is of course easily gamed, as any adversary able to intercept and replay messages could now simply intercept 6 different messages and replay them in batches, ensuring that the cache lookup always misses?
martinsaposnic
commented
Jul 28, 2025
from what I remember from our convo a few weeks ago, the cache is mainly there to handle "accidental" replays from the LSP, e.g. the connection drops and the LSP doesn’t get the ACK, so it retries even though the notification actually landed. but yeah, I agree we could just drop those extra lines and keep things simpler. @TheBlueMatt what are your thoughts? |
tnull
left a comment
There was a problem hiding this comment.
Independently from the discussion on whether we need to store signatures at all, this needs a rebase.
Remove timestamp validation, TimeProvider, SignatureStore trait and its InMemory implementation in favor of a fixed size signature cache. HTTPS already secures delivery, so a small cache is sufficient for basic replay protection.
2122b24 to
dc97b9cComparemartinsaposnic
commented
Jul 29, 2025
done ✅ |
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMatt
commented
Jul 29, 2025
Yea, this was my thinking. IMO this is gonna happen occasionally, and it might be nice to protect against. But at the same time maybe it doesn't matter - if its two servers talking to each other, re-transmitting a notification will happen occasionally, but it should be quick-ish and maybe we wake the phone twice in a minute but its fine? Its currently < 20 LoC, though, so it kinda seems worth keeping to me 🤷♂️ |
| } | ||
| fn check_for_replay_attack(&self, signature: &str) -> Result<(), LSPS5ClientError> { | ||
| let mut signatures = self.recent_signatures.lock().unwrap(); | ||
| if signatures.contains(&signature.to_string()) { |
There was a problem hiding this comment.
| if signatures.contains(&signature.to_string()){ | |
| if signatures.iter().any(|sig| &sig == &signature){ |
tnull
left a comment
There was a problem hiding this comment.
Alright, I'm not convinced that we really need the signature store (esp. given that it's yet another thing that will need to be persisted), but apart from that this PR LGTM.
Uh oh!
There was an error while loading. Please reload this page.
Remove timestamp validation, TimeProvider, SignatureStore trait and its InMemory implementation in favor of a fixed size signature cache. HTTPS already secures delivery, so a small cache is sufficient for basic replay protection.
pub const MAX_RECENT_SIGNATURES: usize = 5;-> I came up with the 5 number limit, not sure if we need more/less. Please comment your opinion on thisThis is a planned follow up for the recently merged PR #3662, here is link with the discussion and motivation for this change #3662 (comment)