Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intercept forward htlc #4018

Merged
merged 3 commits into from Jun 22, 2020
Merged

Conversation

roeierez
Copy link
Contributor

@roeierez roeierez commented Feb 19, 2020

This PR introduces the ability to intercept htlc forward events that are handled at the htlcswitch package and to override the switch default behavior. Basically, similar to HODL invoices, this PR enables routing nodes to HODL HTLCs and to run custom validations or processes before forwarding (or cancelling) the HTLC.
More specifically for every HTLC that is intended to be forwarded, the implementor can choose to hold the forward and signal the switch to wait for a later resolution. Such resolution can be one of the following:

  1. Resume - Resume execution to the default behavior (probably just forward).
  2. Fail - Fail the htlc backward, the implementor is not interested in forwarding it.
  3. Settle - For a given preimage that was revealed in a way not known to the switch the implementor can settle the htlc.
    Having more control over the forward behavior enables new important use cases to be implemented.
    We are specifically looking to support the following use cases:

Pay to Open
This is about being able to pay someone with no channel or with a low capacity channel by opening a channel during the payment.
Let's say Alice (that has a channel) wants to pay Bob (that doesn't have a channel).

  1. Bob is connected to a node and issues an invoice with a routing hint that points to a fake channel between Bob and the node.
  2. When Alice pays Bob's invoice, the node intercepts the HTLC and holds it.
  3. At this stage one of the two options can follow:
    • The node opens a channel to Bob and upon confirmation (or not) resumes the forward.
    • The node opens a channel to Bob, pays Bob the original Alices amount minus service fee (in a different invoice), gets the preimage from Bob (in exchange to the payment) and settles the hold forward.

Notify to Pay
Let's say Alice wants to pay Bob, a mobile user, which is connected to a node via private channel.

  1. Alice pays, the node intercept the forward, checks Bob's availability and notices Bob is offline (inactive).
  2. The node notifies Bob using an agreed-upon notification mechanism.
  3. The node waits for Bob to be online and once the channel is active, it resumes the forward.

Implementation flow & Usage

  1. Link forward a packet, packet intercepted and sent to the RPC layer.
  2. For each packet the RPC layer do the following:
  • Hold the packet immediately and signal hold backwards to the switch.
  • Send the packet for the client
  1. In parallel RPC listens to client resolutions responses on the same stream and resolve packets accordingly.
  2. On disconnection all remaining held packets are resolved using the default behavior (currently Resume)

I am attaching a sample file to show a sample usage that filter packets according to amount
cmd_intercept_forward.txt

@halseth
Copy link
Collaborator

@halseth halseth commented Feb 20, 2020

Can be integrated with the HTLC notifier? #3781
cc @carlaKC

@carlaKC
Copy link
Collaborator

@carlaKC carlaKC commented Feb 20, 2020

Can be integrated with the HTLC notifier? #3781

I think they're different sides of the same coin? HTLCNotifier notifies forwarding decisions post-fact, once we have made the forwarding/failing decision.

My understanding of this is that it's preemptive: receive forward, callout to external forwarding predicate and then return, as specified in #3953.

@carlaKC carlaKC added htlcswitch P3 routing nodes labels Feb 20, 2020
@roeierez
Copy link
Contributor Author

@roeierez roeierez commented Feb 20, 2020

Can be integrated with the HTLC notifier? #3781

@halseth thanks for bringing this to the discussion.
Some things to note when thinking about integration:

  1. The notifications are dispatched after the forward has been processed while the interception should be hooked before that, this means the current htlcNotifier interface needs to be extended.
  2. Interception is bidirectional and need to handle responses as well, this affects both the middleware and the RPC layer.
  3. Controlling the forwards will probably need different RPC permissions than getting htlc notifications.
  4. One example I see in the code that seems similar to this case is the channel events vs the channel acceptor. Two different mechanisms that have similar path in the code.

I think it will be an attempt to mix different things together.
Hope that helps.

@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Feb 21, 2020

Interception should be done at the switch, not the link. Also extreme care must be taken to not block the entire forwarding fabric, as the switch manages its state synchronously atm.

@Roasbeef Roasbeef added this to the 0.11.0 milestone Feb 21, 2020
@champo
Copy link
Contributor

@champo champo commented Feb 21, 2020

@roeierez I really like the look of the API here. I've also been working on something similar. It allows you to register virtual channels and control HTLCs there through gRPC. It adds a new Link implementation and the intercept happens there. As far as the switch is concerned, nothing changes from a regular channel, which I think goes along the lines of what @Roasbeef suggests.

The highly-WIP code is at b3b2f7f and an usage example at https://gist.github.com/champo/8e90ce1ef38f2dd546814ff7898988c3 A knonw issue is that it doesn't close circuits so it kinda stops working after a success settle. It's easy to fix but I haven't gotten around to it yet.

@roeierez roeierez force-pushed the intercept-forward-htlc branch 8 times, most recently from df7f3de to a199479 Compare Feb 24, 2020
@roeierez
Copy link
Contributor Author

@roeierez roeierez commented Feb 25, 2020

Interception should be done at the switch, not the link. Also extreme care must be taken to not block the entire forwarding fabric, as the switch manages its state synchronously atm.

I have moved the interception to the switch, added unit tests and the Router RPC implementation.

uint64 htlc_id = 2;
}

message ForwardHtlcInterceptRequest {
Copy link
Contributor

@champo champo Mar 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to include the sphinx packet in this message. It would allow a receiver to check amount and expiry as it would if the HTLC had been forwarded through a link. This is useful for reverse swaps.

Copy link
Contributor Author

@roeierez roeierez Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@champo thanks for the feedback.
Currently the fields amount_sat expiry are what you are looking for as they are the actual values that would have been used in the forwarded packet in the default switch behavior.

Copy link
Contributor

@champo champo Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A third-party receiver (say a mobile app) shouldn't trust the fields. They would want to check the information against the sphinx. It would also enable usage of new fields like payment secret which is only known to the receiver.

Copy link
Contributor Author

@roeierez roeierez Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@champo I agree. At first I exposed only the fields that are required for the use cases I mentioned in the PR description. I do think it makes sense to expose all the forwarding info (such as outgoing channel, tlv fields and raw sphinx packet) as other use cases may need this info in order to take a decision at interception time.

Copy link
Member

@Roasbeef Roasbeef May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to include the sphinx packet in this message.

The raw packet itself can't be verified unless the receiver has the private key for the end node.

Copy link
Contributor

@champo champo Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Roasbeef in our case, the end node and the interceptor communicate over HTTP to negotiate the settling of the HTLC. The end node can then decode the sphinx packet to reduce trust.

@roeierez roeierez force-pushed the intercept-forward-htlc branch from a199479 to b1cdbc7 Compare Mar 4, 2020
@roeierez roeierez changed the title Intercept forward htlc [WIP] Intercept forward htlc Mar 4, 2020
htlcinterceptor/middleware.go Outdated Show resolved Hide resolved
htlcinterceptor/middleware.go Outdated Show resolved Hide resolved
htlcinterceptor/middleware.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
@roeierez roeierez force-pushed the intercept-forward-htlc branch 2 times, most recently from 5d72a87 to c416106 Compare Mar 29, 2020
@roeierez roeierez requested a review from joostjager as a code owner Mar 29, 2020
@roeierez roeierez force-pushed the intercept-forward-htlc branch from c416106 to ef5113a Compare Mar 29, 2020
@cfromknecht cfromknecht added this to In progress in v0.11.0-beta via automation Apr 21, 2020
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🔱

Don't think we need the REST annotation since this type of RPC isn't supported atm. Should be ready to land after a rebase!


switch htlc := packet.htlc.(type) {
case *lnwire.UpdateAddHTLC:
// We are not interested in intercepting initated payments.
Copy link
Member

@Roasbeef Roasbeef Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to reverse this in the future, as it would allow for the interceptor to be used as a firewall for all outgoing send attempts.

@@ -211,6 +211,8 @@ http:
# deprecated, no REST endpoint
- selector: routerrpc.Router.TrackPayment
# deprecated, no REST endpoint
- selector: routerrpc.HtlcInterceptor
Copy link
Member

@Roasbeef Roasbeef Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need anything here since this is a bi directional streaming RPC, which we don't yet support. The current web sockets implementation only supports server-side streaming requests.

Copy link
Contributor Author

@roeierez roeierez Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it because it is required by the make rpc-check

As part of the preparation to the switch interceptor feature, this
function is changed  to return error instead of error channel that
is closed automatically.
Returning an error channel has become complex to maintain and
implement when adding more asynchronous flows to the switch.
The change doesn't affect the current behavior which logs the
errors as before.
@roeierez roeierez force-pushed the intercept-forward-htlc branch from f2faff9 to cfa2ee8 Compare Jun 18, 2020
@roeierez
Copy link
Contributor Author

@roeierez roeierez commented Jun 18, 2020

Don't think we need the REST annotation since this type of RPC isn't supported atm

The build requires it atm: make rpc-check

ready to land after a rebase!

Rebased.

@roeierez roeierez force-pushed the intercept-forward-htlc branch from cfa2ee8 to 2b0bdab Compare Jun 18, 2020
v0.11.0-beta automation moved this from Review in progress to Reviewer approved Jun 18, 2020
Copy link
Collaborator

@joostjager joostjager left a comment

reviewed 2b0bdab

if err != nil {
return err
}
err = interceptedForward.Settle(preimage)
Copy link
Collaborator

@joostjager joostjager Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return interceptedForward.Settle(preimage)

Copy link
Contributor Author

@roeierez roeierez Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// forwards and find them when manual resolution is later needed.
func (s *Server) HtlcInterceptor(stream Router_HtlcInterceptorServer) error {
// We ensure there is only one interceptor at a time.
if !atomic.CompareAndSwapInt32(&s.forwardInterceptorActive, 0, 1) {
Copy link
Collaborator

@joostjager joostjager Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is still useful to make sure that any user of interceptable switch uses it correctly. But non-blocking

In this commit we implement a wrapper arround the switch, called
InterceptableSwitch. This kind of wrapper behaves like a proxy which
intercepts forwarded packets and allows an external interceptor to
signal if it is interested to hold this forward and resolve it
manually later or let the switch execute its default behavior.
This infrastructure allows the RPC layer to expose interceptor
registration API to the user and by that enable the implementation
of custom routing behavior.
@roeierez roeierez force-pushed the intercept-forward-htlc branch from 2b0bdab to 5501da3 Compare Jun 18, 2020
In this commit we add the ability to intercept forwarded htlc packets
straight from the RPC layer. The RPC layer handles a bidrectional stream
that comminucates to the client the intercepted packets and handles its
response by coordinating with the interceptable switch.
@roeierez roeierez force-pushed the intercept-forward-htlc branch from 5501da3 to 7b56268 Compare Jun 18, 2020
@joostjager joostjager merged commit 8f2a2fc into lightningnetwork:master Jun 22, 2020
14 checks passed
v0.11.0-beta automation moved this from Reviewer approved to Done Jun 22, 2020
Forwarded HTLC requests are sent to the client and the client responds with
a boolean that tells LND if this htlc should be intercepted.
In case of interception, the htlc can be either settled, cancelled or
resumed later by using the ResolveHoldForward endpoint.
Copy link
Contributor

@hsjoberg hsjoberg Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @roeierez. This description text looks wrong. I don't find any ResolveHoldForward endpoint.
AFAICT how to resume the incoming payment is by sending with action set to RESUME in this gRPC stream.
Perhaps I'm missing something?

Also by reading the protobuf, it doesn't look lke the client is supposed to respond with a boolean if the HTLC should be intercepted or not (rather it is decided by the incoming_circuit_key?)

Copy link
Contributor Author

@roeierez roeierez Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsjoberg You are right. Unfortunately this description was left from a previous iteration and is indeed wrong.
The interface was changes and the client should return a ForwardHtlcInterceptResponse with action that maps to one of the follows:

SETTLE: to settle the htlc (a preimage is expected as well).
FAIL: to fail the htlc
RESUME: to resume with default behavior.

Copy link
Contributor

@hsjoberg hsjoberg Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roeierez Thanks, that makes sense.

Cheers!

@hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Sep 14, 2020

@roeierez Correct me if I'm wrong about this, but AFAICT as it stands right now, this API is dangerous if an amountless invoice is used (and action is SETTLE).
Only the receiving node knows about amt_to_forward and payment_secret in their TLV payload, so isn't there a vulnerability here where the LSP node could give up the preimage for too little satoshi? (Basically the amountless invoice problem again.)

EDIT: Below is based on misunderstandings from my side.

@roeierez
Copy link
Contributor Author

@roeierez roeierez commented Sep 14, 2020

@roeierez Correct me if I'm wrong about this, but AFAICT as it stands right now, this API is dangerous if an amountless invoice is used (and action is SETTLE).
Only the receiving node knows about amt_to_forward and payment_secret in their TLV payload, so isn't there a vulnerability here where the LSP node could give up the preimage for too little satoshi? (Basically the amountless invoice problem again.)

@hsjoberg Not following. Settling an htlc backwards requires a preimage which is passed to the interceptor by the next node in the path either using regular lightning mechanism or some custom swap logic. In any case interceptor can make sure it has the required funds before settling. Don't see any risk.

@hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Sep 14, 2020

@roeierez In the case of an undetermined amount ("amountless invoice"), the interceptor node must somehow know how much it should reveal the preimage for to settle the payment. AFAICT basically what feature bits 14/15 is for.

@roeierez
Copy link
Contributor Author

@roeierez roeierez commented Sep 14, 2020

@roeierez In the case of an undetermined amount ("amountless invoice"), the interceptor node must somehow know how much it should reveal the preimage for to settle the payment. AFAICT basically what feature bits 14/15 is for.

@hsjoberg The interceptor doesn't know the total amount of a payment regardless of amount-less invoice or not. Invoice is between the payer and the payee. Also the way the interceptor gets the pre-image to settle backwards is totally implementation dependent so in that sense it can be risky or not.
The implementation in Breez won't change the way lightning works, means the interceptor will get the pre-image from the payee (once his invoice is settled) so I don't see how this adds risk to the interceptor node.

@hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Sep 14, 2020

The interceptor doesn't know the total amount of a payment regardless of amount-less invoice or not. Invoice is between the payer and the payee

But that could just be trivially communicated with the interceptor somehow in the case of a normal invoice with amount.


@roeierez Sorry, I will try to explain my concern better. I'll take it from the beginning:
There exists a vulnerability with amountless invoices where a malicious party Mallory could attempt to make the receiver reveal the preimage by forwarding another payment with same hash but with less amount.

Let's say A wants to pay C 1 BTC through M:
A -> M -> C

Instead of forwarding the real payment, M could make a new payment with 1 sat using the same hash in an attempt lure C to reveal the preimage.

This issue has been demonstrated in the past and fixed with the 14/15 payment secret and/or enforcing TLV/MPP/keysend. My high-level understanding is that a nonce is introduced that the payer is sending to the payee, which can be used to find out tampering.
The payment secret is encrypted in a TLV for the receiving node.

Now let's say we're using Pay to Open, and interceptor settling the payment and opening channel to C (either paying through push amount or another invoice, shouldn't matter AFAICT).
A wants to pay C 1 BTC through M and interceptor node I. C has no inbound channel:
A->M->I->C

The malicious party M could attempt to lure I and thus also C by forwarding 1 sat instead of the real payment.
Now, this is the gut of the whole thing, and this is more of a question rather than a conclusion, but even if the new payment scheme using TLV is enforced (or 14 payment secret), lnd and the code as it stands right now wouldn't check for tampering because it won't decrypt and check the actual receiving nodes (C) packet (as I is just settling it and not letting C see his packet), thus it looks vulnerable to the old amountless invoice problem.

@roeierez
Copy link
Contributor Author

@roeierez roeierez commented Sep 14, 2020

But that could just be trivially communicated with the interceptor somehow in the case of a normal invoice with amount.

Then what prevents the payee to communicate the amount he expects in the zero invoice case as well?

as I is just settling it and not letting C see his packet

If this is the case how the interceptor has the pre-image? It seems that you described a flow where the interceptor has the pre-image beforehand and this is not how we (at Breez) implementing Pay to Open. We do forward the packet to the destination and only once the destination reveals the pre-image the interceptor settles backwards using regular lightning mechanism.
The interceptor, in our implementation, also agrees with the end node on the amount to forward and the LSP fee (for opening a.channel) beforehand.
So in fact the interceptor has all the means to hold the forward until he receives enough htlcs amount to take his fees, before resuming the forwards.

@hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Sep 14, 2020

Then what prevents the payee to communicate the amount he expects in the zero invoice case as well?

The payee doesn't know what amount to expect, hence amountless invoice.
The final TLV payload has to be read by either the interceptor or payee somehow, as there lies the payment_secret and amt_to_forward that the payer sent to the payee.

If this is the case how the interceptor has the pre-image? It seems that you described a flow where the interceptor has the pre-image beforehand

The wallet simply hands the preimage to the LSP and the LSP uses the SETTLE action via the HtlcIntercept API.
The flow I am thinking about is not where the interceptor continues the forwarding (RESUME), rather it settles it themselves (SETTLE), and then afterwards opens a channel with the wallet.
In short:

  1. Wallet creates an invoice with route hint which goes to the LSP node with fake short chan id.
  2. Wallet hands preimage to LSP
  3. Payer pays invoice and uses the fake short chan id provided in route hint
  4. LSP intercepts the incoming HTLC and SETTLE's it
  5. LSP opens a new channel to Wallet (and pays using push amount, another invoice or something else)

I explained briefly in the OP that the concern is about the SETTLE action.

this is not how we (at Breez) implementing Pay to Open

I'm not talking about Breez's implementation per se, just resuming the forward should be completely fine.
But I think it should apply to the 3.2 scenario in Pay to Open described in your OP:
The node opens a channel to Bob, pays Bob the original Alices amount minus service fee (in a different invoice), gets the preimage from Bob (in exchange to the payment) and settles the hold forward.

The interceptor requests a new invoice from the wallet to pay through and then settles the hold forward in this scenario(?). Thus the original sender's TLV payload would not reach the receiver.

So in fact the interceptor has all the means to hold the forward until he receives enough htlcs amount to take his fees, before resuming the forwards.

Yes, if the interceptor just resumed the forwarding and uses a newly created channel to the payee, the problem I've described does not apply as 14 payment secret (or any other method) can be enforced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
htlcswitch P3 routing nodes v0.11
Projects
No open projects
v0.11.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants