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+lnwire: invalid onion payload #3470

Merged
merged 8 commits into from Nov 2, 2019

Conversation

cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Sep 5, 2019

Builds on #3465

Modifies the link to return an InvalidOnionPayload failure when it is unable to process a TLV onion payload. In the process, we unify the hop.ErrInvalidPayload failure to include required type failures, and propagate the type back to the caller in the InvalidOnionPayload.

@cfromknecht cfromknecht requested a review from Roasbeef as a code owner Sep 5, 2019
@cfromknecht cfromknecht changed the title Invalid onion payload htlcswitch+lnwire: invalid onion payload Sep 5, 2019
@joostjager
Copy link
Collaborator

@joostjager joostjager commented Sep 10, 2019

Do you want to make the change to result_interpretation.go too?

@Roasbeef Roasbeef added error messages htlcswitch spec P2 labels Oct 3, 2019
@Roasbeef Roasbeef added this to the 0.9 milestone Oct 8, 2019
@cfromknecht cfromknecht requested a review from joostjager as a code owner Oct 9, 2019
@cfromknecht
Copy link
Collaborator Author

@cfromknecht cfromknecht commented Oct 9, 2019

Do you want to make the change to result_interpretation.go too?

Done

Copy link
Collaborator

@joostjager joostjager left a comment

Figuring out how to map to the failure message isn't very simple. I see a cost now and in the future for keeping that working as good as possible. Why don't we just return no info in the failure message and only add it when this type of debugging becomes essential to have. So many things can't go wrong that you can't find out during dev.

htlcswitch/hop/payload.go Show resolved Hide resolved
htlcswitch/hop/payload.go Outdated Show resolved Hide resolved
lnwire/onion_error.go Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
routing/result_interpretation.go Outdated Show resolved Hide resolved
routing/result_interpretation_test.go Outdated Show resolved Hide resolved
routing/result_interpretation_test.go Show resolved Hide resolved
tlv/stream.go Outdated Show resolved Hide resolved
htlcswitch/hop/payload.go Show resolved Hide resolved

// RequiredViolation indicates that an unknown even type was found in
// the payload that we could not process.
RequiredViolation
Copy link
Collaborator

@joostjager joostjager Oct 15, 2019

Choose a reason for hiding this comment

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

Required and Omitted sound like the same thing. Is the real difference that one happens during tlv decode and the other one on the higher level? If that is indeed the case, maybe the names should reflect that. Or just have one of the two.

Copy link
Collaborator Author

@cfromknecht cfromknecht Oct 15, 2019

Choose a reason for hiding this comment

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

no they are different, required means that the sender required us to understand a record, but we don't. omitted means that the receiver expected a field that the sender didn't include.

routing/result_interpretation_test.go Show resolved Hide resolved
nextHop := lnwire.NewShortChanIDFromInt(cid)
err = ValidateParsedPayloadTypes(parsedTypes, nextHop)
if err != nil {
return nil, err
Copy link
Collaborator

@joostjager joostjager Oct 15, 2019

Choose a reason for hiding this comment

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

Isn't this a bit too much? Always returning RequiredViolation here doesn't seem to be bad. Especially because it also requires changes to the tlv parser.

Copy link
Collaborator Author

@cfromknecht cfromknecht Oct 29, 2019

Choose a reason for hiding this comment

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

it is in some sense overkill, but i think an accurate log message will help debug if things go wrong. perhaps another solution is to not log the final/intermediate info when encountering a required violation? however, many of the new records being added are specific to the hop type, which would make that information more useful.

@joostjager joostjager added this to WIP in v0.9.0-beta via automation Oct 23, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Oct 23, 2019
@joostjager joostjager requested a review from guggero Oct 24, 2019
Copy link
Collaborator

@guggero guggero left a comment

I did a high-level pass of the PR, mainly focusing on code quality aspects. Only found a few minor nits.
Nice to see that there are multiple test cases around the changes and the coverage actually increase slightly.
The changes look solid to me though I cannot speak to the possible implications this has in routing.

htlcswitch/link.go Outdated Show resolved Hide resolved
htlcswitch/link.go Outdated Show resolved Hide resolved
htlcswitch/hop/payload.go Outdated Show resolved Hide resolved
htlcswitch/hop/payload.go Show resolved Hide resolved
@cfromknecht
Copy link
Collaborator Author

@cfromknecht cfromknecht commented Oct 29, 2019

thanks for the review @guggero!

cfromknecht added 4 commits Oct 31, 2019
Previously we would not mark a success for the first hop if the fail
source index was 2. We also add a test to assert this behavior.
This commit adds a hop.PayloadViolation enum which encompasses the cases
where the sender omits, includes, or requires a type that causes an
ErrInvalidPayload faiulre.

The existing Omitted bool is converted to this PayloadViolation, and
NewPayloadFromReader is updated to return such a failure with a
RequiredViolation when an unknown required type is detected.

The unit tests are updated to cover the three possible cases of
RequiredViolations, as well as included valid intermediate and final hop
tests.
This commit modifies the link return an InvalidOnionPayload failure when
it cannot parse a TLV payload. The offset is left at zero, since its
unclear how useful it will be in practice and would require some
significant reworkings of the abstractions in the tlv package.

TODO: add unit tests. currently none of the test unit infrastructure is
setup to handle TLV payloads, so this would require implementing a
separate mock iterator for TLV payloads that also supports injecting
invalid payloads. Deferring this non-trival effor till a later date
cfromknecht added 4 commits Oct 31, 2019
An InvalidOnionPayload implies that the onion was successfully received
by the reporting node, but that they were unable to extract the
contents. Since we assume our own behavior is correct, this mostly
likely poins to an error in the reporter's implementation or that we
sent an unknown required type. Therefore we only penalize that single
hop, and consider the failure terminal if the receiver reported it.
This commit modifies the NewPayloadFromReader to apply known
presence/omission contraints in the event that the tlv parser returns an
unknown required type failure.

Now that the parser has been modified to finished parsing the stream to
obtain a proper parsed type set, we can accurately apply these higher
level validation checks. This overrides required type failures, such
that they are only returned if the sender properly abided by the
constraints on fields for which we know.

The unit tests are updated to create otherwise valid payloads that then
return unknown required type failures. In one case, a test which
previously returned an unknown required type failure is made to return
an included failure for the sid, indicating the unknown required type 0
is being overruled.
v0.9.0-beta automation moved this from Needs Review to Approved Nov 2, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 📡

@Roasbeef Roasbeef merged commit acd8a6e into lightningnetwork:master Nov 2, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Nov 2, 2019
@cfromknecht cfromknecht deleted the invalid-onion-payload branch Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants