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

htlcswitch+routerrpc: support unencrypted failure reasons #7067

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/release-notes/release-notes-0.16.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,11 @@ all unconfirmed transactions are rebroadcast on start up.
`ForwardingHistory`rpcs](https://github.com/lightningnetwork/lnd/pull/7471) to
the rpc response.

* HTLC interceptor clients can now [supply unencrypted failure
reasons](https://github.com/lightningnetwork/lnd/pull/7067) when failing
HTLCs. This allows returning any type of failure to the sender as if it
originates from the LND node.

## Wallet

* [Allows Taproot public keys and tap scripts to be imported as watch-only
Expand Down
21 changes: 21 additions & 0 deletions htlcswitch/hop/error_encryptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ type ErrorEncrypter interface {
// until the error arrives at the source of the payment.
IntermediateEncrypt(lnwire.OpaqueReason) lnwire.OpaqueReason

// EncryptEncodedFirstHop encrypts an already encoded failure message.
// This method will be used at the source that the error occurs. It
// differs from IntermediateEncrypt slightly, in that it computes a
// proper MAC over the error.
EncryptEncodedFirstHop(
encodedFailureMessage []byte) (lnwire.OpaqueReason, error)

// Type returns an enum indicating the underlying concrete instance
// backing this interface.
Type() EncrypterType
Expand Down Expand Up @@ -117,6 +124,20 @@ func (s *SphinxErrorEncrypter) EncryptFirstHop(
return s.EncryptError(true, b.Bytes()), nil
}

// EncryptEncodedFirstHop encrypts an already encoded failure message. This
// method will be used at the source that the error occurs.
func (s *SphinxErrorEncrypter) EncryptEncodedFirstHop(
encodedFailureMessage []byte) (lnwire.OpaqueReason, error) {

var w bytes.Buffer
err := lnwire.EncodeFailureHeader(&w, encodedFailureMessage, 0)
if err != nil {
return nil, err
}

return s.EncryptError(true, w.Bytes()), nil
}

// EncryptMalformedError is similar to EncryptFirstHop (it adds the MAC), but
// it accepts an opaque failure reason rather than a failure message. This
// method is used when we receive an UpdateFailMalformedHTLC from the remote
Expand Down
153 changes: 142 additions & 11 deletions htlcswitch/interceptable_switch.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package htlcswitch

import (
"bytes"
"crypto/sha256"
"fmt"
"sync"

"github.com/btcsuite/btcd/btcec/v2/ecdsa"
"github.com/go-errors/errors"
"github.com/lightningnetwork/lnd/chainntnfs"
"github.com/lightningnetwork/lnd/channeldb/models"
Expand Down Expand Up @@ -83,6 +85,11 @@ type InterceptableSwitch struct {
// currentHeight is the currently best known height.
currentHeight int32

// signChannelUpdate is used when an intercepting application includes
// an unsigned channel update to be signed by us.
signChannelUpdate func(u *lnwire.ChannelUpdate) (*ecdsa.Signature,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, shouldn't we assume that they want to instead send the latest update? Otherwise i can see this causing an error where: user sends custom channel update, the one on disk conflicts, so routing errors occur.

Copy link
Member

Choose a reason for hiding this comment

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

We can prevent this in the forwarding level during the validation of the returned message. If it's an encoded channel update, then it needs to match what we'd send out anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context in which this is to be used is to update the routing policy of a virtual channel. The channel isn't broadcasted through gossip and doesn't exist in the graph on disk. There is no 'latest update'.

The existence of the virtual channel is solely communicated to the payer through invoice route hints. If the receiver (the virtual hop) changes its policy, the channel update contained in the failure message updates the sender with the new values.

Copy link
Member

Choose a reason for hiding this comment

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

The context in which this is to be used is to update the routing policy of a virtual channel. The channel isn't broadcasted through gossip and doesn't exist in the graph on disk.

Assuming the virtual channel has a distinct pubkey (and also shared secret in the onion), then how would this work in practice? lnd would use diff logic for if it was the final hop vs the penultimate hop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If node N is the virtual node, then it is the job of node N-1 to return a channel update. The example is again fee_insufficient.

Real node N-1 doesn't know about this channel at all (it is virtual), so what happens is that the virtual node N returns a channel update for the virtual channel to N-1, and N-1 just signs the update.

error)

wg sync.WaitGroup
quit chan struct{}
}
Expand Down Expand Up @@ -119,9 +126,15 @@ type FwdResolution struct {
// FwdActionSettle.
Preimage lntypes.Preimage

// FailureMessage is the encrypted failure message that is to be passed
// back to the sender if action is FwdActionFail.
FailureMessage []byte
// EncryptedFailureMessage is the encrypted failure message that is to
// be passed back to the sender if action is FwdActionFail. This field
// is mutually exclusive with FailureMessage.
EncryptedFailureMessage []byte

// FailureMessage is the decoded failure message that is to be encrypted
// for the first hop. This field is mutually exclusive with
// EncryptedFailureMessage.
FailureMessage lnwire.FailureMessage

// FailureCode is the failure code that is to be passed back to the
// sender if action is FwdActionFail.
Expand Down Expand Up @@ -158,6 +171,11 @@ type InterceptableSwitchConfig struct {
// RequireInterceptor indicates whether processing should block if no
// interceptor is connected.
RequireInterceptor bool

// SignChannelUpdate is used when an intercepting application includes
// an unsigned channel update to be signed by us.
SignChannelUpdate func(u *lnwire.ChannelUpdate) (*ecdsa.Signature,
joostjager marked this conversation as resolved.
Show resolved Hide resolved
error)
}

// NewInterceptableSwitch returns an instance of InterceptableSwitch.
Expand All @@ -181,6 +199,7 @@ func NewInterceptableSwitch(cfg *InterceptableSwitchConfig) (
cltvRejectDelta: cfg.CltvRejectDelta,
cltvInterceptDelta: cfg.CltvInterceptDelta,
notifier: cfg.Notifier,
signChannelUpdate: cfg.SignChannelUpdate,

quit: make(chan struct{}),
}, nil
Expand Down Expand Up @@ -377,17 +396,115 @@ func (s *InterceptableSwitch) resolve(res *FwdResolution) error {
return intercepted.Settle(res.Preimage)

case FwdActionFail:
if len(res.FailureMessage) > 0 {
return intercepted.Fail(res.FailureMessage)
}
switch {
// Fail with encrypted failure message.
case len(res.EncryptedFailureMessage) > 0:
return intercepted.Fail(
res.EncryptedFailureMessage, false,
)

// Fail with known failure message that is to be encoded and
// encrypted.
case res.FailureMessage != nil:
msg := res.FailureMessage

// Re-sign the channel update if present. Note that this
// changes the passed in FwdResolution.
update := getChannelUpdateRef(msg)
if update != nil {
err := s.validateChannelUpdate(update)
if err != nil {
return err
}

err = s.resignChannelUpdate(update)
Copy link
Member

Choose a reason for hiding this comment

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

Should we also commit this update to disk as well? Apologies if this was already an older review comment...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because the real node isn't aware of this virtual channel at all. It's all up to the external interceptor logic to keep track of the virtual channel properties. As mentioned in #7067 (comment), the external interceptor is taking over part of the responsibilities of the switch.

if err != nil {
return err
}
}

return intercepted.FailWithCode(res.FailureCode)
var encodedMsg bytes.Buffer
err := lnwire.EncodeFailureMessage(
&encodedMsg, msg, 0,
)
if err != nil {
return err
}

return intercepted.Fail(
encodedMsg.Bytes(), true,
)

// Fail with failure code.
default:
return intercepted.FailWithCode(res.FailureCode)
}

default:
return fmt.Errorf("unrecognized action %v", res.Action)
}
}

func (s *InterceptableSwitch) validateChannelUpdate(
update *lnwire.ChannelUpdate) error {

// The maxHTLC flag is mandatory.
if !update.MessageFlags.HasMaxHtlc() {
Copy link
Member

Choose a reason for hiding this comment

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

More validation is needed here, eg: max HTLC shouldn't be greater than the channel capacity, etc, etc. I think we should just call the normal validation routine.

Copy link
Contributor Author

@joostjager joostjager Aug 8, 2023

Choose a reason for hiding this comment

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

Wanted to call

func ValidateChannelUpdateFields(capacity btcutil.Amount,

But that would create a circular reference. For now, I just copied over the additional max_htlc >= min_htlc check. Capacity checking is not possible, because the real lnd doesn't know what the capacity is. It's probably infinite because the channel is typically virtual.

If desired I can do a bigger refactor and move the validation logic into htlcswitch. But not sure if worth it.

return errors.Errorf("max htlc flag not set for channel")
}

// Check that max htlc is at least min htlc.
maxHtlc := update.HtlcMaximumMsat
if maxHtlc == 0 || maxHtlc < update.HtlcMinimumMsat {
return errors.Errorf("invalid max htlc for channel update ")
}

return nil
}

// resignChannelUpdate signs the provided channel update with our node key.
func (s *InterceptableSwitch) resignChannelUpdate(
update *lnwire.ChannelUpdate) error {

sig, err := s.signChannelUpdate(update)
if err != nil {
return err
}

update.Signature, err = lnwire.NewSigFromSignature(sig)
if err != nil {
return nil
}

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't commit it. Even if we do decide to allow this, we also need to make sure that the update would actually validate in the first place (has all the needed fields, bounds respected, etc).

Copy link
Contributor Author

@joostjager joostjager May 23, 2023

Choose a reason for hiding this comment

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

You mean commit as in write to disk? As explained in #7067 (comment), it is the intention to just pass through without persisting.

Added a validation function that carries out the checks from routing.ValidateChannelUpdateFields that apply here, which is just the max htlc mandatory check. Open to suggestions what else to add.

return nil
}

// getChannelUpdateRef returns a reference to an embedded channel update if
// present in the failure message.
func getChannelUpdateRef(msg lnwire.FailureMessage) *lnwire.ChannelUpdate {
switch m := msg.(type) {
case *lnwire.FailFeeInsufficient:
return &m.Update

case *lnwire.FailIncorrectCltvExpiry:
return &m.Update

case *lnwire.FailTemporaryChannelFailure:
return m.Update

case *lnwire.FailAmountBelowMinimum:
return &m.Update

case *lnwire.FailExpiryTooSoon:
return &m.Update

case *lnwire.FailChannelDisabled:
return &m.Update
}

return nil
}

// Resolve resolves an intercepted packet.
func (s *InterceptableSwitch) Resolve(res *FwdResolution) error {
internalRes := &fwdResolution{
Expand Down Expand Up @@ -615,10 +732,24 @@ func (f *interceptedForward) Resume() error {
return f.htlcSwitch.ForwardPackets(nil, f.packet)
}

// Fail notifies the intention to Fail an existing hold forward with an
// encrypted failure reason.
func (f *interceptedForward) Fail(reason []byte) error {
obfuscatedReason := f.packet.obfuscator.IntermediateEncrypt(reason)
// Fail notifies the intention to Fail an existing hold forward.
func (f *interceptedForward) Fail(reason []byte, encryptFirstHop bool) error {
var (
obfuscatedReason []byte
obfuscator = f.packet.obfuscator
)

if encryptFirstHop {
var err error
obfuscatedReason, err = obfuscator.EncryptEncodedFirstHop(
reason,
)
if err != nil {
return err
}
} else {
obfuscatedReason = obfuscator.IntermediateEncrypt(reason)
}

return f.resolve(&lnwire.UpdateFailHTLC{
Reason: obfuscatedReason,
Expand Down
7 changes: 4 additions & 3 deletions htlcswitch/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,10 @@ type InterceptedForward interface {
// forward with a given preimage.
Settle(lntypes.Preimage) error

// Fail notifies the intention to fail an existing hold forward with an
// encrypted failure reason.
Fail(reason []byte) error
// Fail notifies the intention to fail an existing hold forward with a
// failure reason. The encryptFirstHop bool indicates whether the
// failure reason still needs to be encrypted for the first hop.
Fail(reason []byte, encryptFirstHop bool) error
joostjager marked this conversation as resolved.
Show resolved Hide resolved

// FailWithCode notifies the intention to fail an existing hold forward
// with the specified failure code.
Expand Down
15 changes: 15 additions & 0 deletions htlcswitch/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,21 @@ func (o *mockObfuscator) EncryptFirstHop(failure lnwire.FailureMessage) (
return b.Bytes(), nil
}

func (o *mockObfuscator) EncryptEncodedFirstHop(
reason []byte) (lnwire.OpaqueReason, error) {

var b bytes.Buffer
if _, err := b.Write(fakeHmac); err != nil {
return nil, err
}

if err := lnwire.EncodeFailureHeader(&b, reason, 0); err != nil {
return nil, err
}

return b.Bytes(), nil
}

func (o *mockObfuscator) IntermediateEncrypt(reason lnwire.OpaqueReason) lnwire.OpaqueReason {
return reason
}
Expand Down
Loading
Loading