Add LSPS5 DOS protections. #3993
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3993 +/- ##
=======================================
Coverage 88.91% 88.91%
=======================================
Files 174 174
Lines 125066 125159 +93
Branches 125066 125159 +93
=======================================
+ Hits 111197 111291 +94
- Misses 11358 11359 +1
+ Partials 2511 2509 -2
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:
|
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
| use lightning::sign::NodeSigner; | ||
|
|
||
| /// A trait for implementing Denial-of-Service (DoS) protection mechanisms for LSP services. | ||
| pub trait DosProtectionEnforcer { |
There was a problem hiding this comment.
IMO this is over-engineering. We can just have a single method on LSPS5ServiceHandler that checks the necessary bools. I also don't think this warrants to be in a separate module right now.
| peer_state | ||
| .outbound_channels_by_intercept_scid | ||
| .values() | ||
| .any(|c| c.is_pending_channel_open()) |
There was a problem hiding this comment.
Hmm, we'll also want to allow LSPS5 for any activity beyond this point, no? I think rather than adding helpers for each individual state, maybe we finally should expose the list of OutboundJITChannelStates at least pub(crate) and also implement Ord for OutboundJITChannelState? This would allow us to use comparison operators on the channel states.
Also, can we somehow enforce that for a pending payment, we'd always first process it and advance the LSPS2 state before evaluating whether we can notifiy the LSPS5 client?
There was a problem hiding this comment.
what I thought here (and I didn't clarify, sorry) is that it should only check for the pending_channel_open. after that state, the channel will be opened and the LSPS5 service has_active_channels check will kick in, so there is no point on checking the other LSPS2 states. does that make sense?
lightning-liquidity/src/manager.rs
Outdated
| if !self.peer_is_engaged(sender_node_id) { | ||
| return Err(LightningError { | ||
| err: format!( | ||
| "Rejecting LSPS5 request from {:?} without existing engagement", |
There was a problem hiding this comment.
"existing engagement" is pretty unclear terminology, would be good to find more intuitive wording.
lightning-liquidity/src/manager.rs
Outdated
| "Rejecting LSPS5 request from {:?} without existing engagement", | ||
| sender_node_id | ||
| ), | ||
| action: ErrorAction::IgnoreAndLog(Level::Info), |
There was a problem hiding this comment.
Please don't log on on Info here.
| @@ -64,6 +64,8 @@ struct StoredWebhook { | |||
| pub struct LSPS5ServiceConfig { | |||
| /// Maximum number of webhooks allowed per client. | |||
| pub max_webhooks_per_client: u32, | |||
| /// Require an existing channel or active LSPS1/LSPS2 flow before accepting requests. | |||
| pub enforce_dos_protections: bool, | |||
There was a problem hiding this comment.
Let's at least start out with enabling this by default, i.e., don't make it configurable.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
When handling an incoming LSPS5 request, the manager will check if the counterparty is 'engaged' in some way before responding. `Engaged` meaning = active channel | LSPS2 active operation | LSPS1 active operation. Logic: `If not engaged then reject request;` A single test is added only checking for the active channel condition, because it's not super easy to get LSPS1-2 on the correct state to check this (yet). Other tangential work is happening that will make this easier and more tests will come in the near future
martinsaposnic
force-pushed
the
lsps5-dos
branch
2 times, most recently
from
38c8436 to
ffe25e6
Compare
|
@tnull thanks for the review! I think this is a much better I pushed a fixup commit addressing all the comments ffe25e6 I still have my doubts regarding this:
I think that maybe that's not necessary and I posted a question related to that: #3993 (comment) thanks! |
- get rid of dos_protection_enforcer. logic will go inside lsps5/service - improve naming conventions so it's clearer - do not log on level info for possible spam - make the dos protections enabled by default
martinsaposnic
force-pushed
the
lsps5-dos
branch
from
ffe25e6 to
aab6332
Compare
When handling an incoming LSPS5 request, the manager will check
if the counterparty is 'engaged' in some way before responding.
Engagedmeaning = active channel | LSPS2 active operation | LSPS1 active operation.Logic:
If not engaged then reject request;A single test is added only checking for the active channel condition,
because it's not super easy to get LSPS1-2 on the correct state to check this (yet).
Other tangential work is happening that will make this easier and more tests will come in the near future.
A few decisions that could be changed:- the DOS protections are optional (default true). maybe this should not be configurable?- I made thedos_protection_enforcergeneric enough so it would be possible to add more behavior in the future. also some logic could be moved here like theignored_peerslogic from the manager, which could make sense to move to the dos enforcerthoughts @tnull ?