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

Onion messages v1 #1503

Merged
merged 11 commits into from
Aug 3, 2022

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented May 29, 2022

Implement v1 of onion messages per lightning/bolts#759. This will enable us to implement Offers messaging.

TODOs

  • our own integration tests
  • refuse to create packets that would cause their hop_data to exceed BIG_PACKET_HOP_DATA_LEN (because eclair will refuse to forward these)

Follow-ups documented in #1607

Based on #1518

@valentinewallace valentinewallace changed the title Onion messages Onion messages v1 May 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2022

Codecov Report

Merging #1503 (b8301da) into main (e403999) will decrease coverage by 0.07%.
The diff coverage is 88.84%.

❗ Current head b8301da differs from pull request most recent head 81e3c41. Consider uploading reports for the commit 81e3c41 to get more accurate results

@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
- Coverage   91.03%   90.95%   -0.08%     
==========================================
  Files          80       85       +5     
  Lines       44194    44679     +485     
  Branches    44194    44679     +485     
==========================================
+ Hits        40233    40639     +406     
- Misses       3961     4040      +79     
Impacted Files Coverage Δ
lightning/src/lib.rs 100.00% <ø> (ø)
lightning/src/ln/channel.rs 88.69% <0.00%> (-0.03%) ⬇️
lightning/src/ln/mod.rs 95.00% <ø> (ø)
lightning/src/util/ser.rs 89.24% <0.00%> (-1.54%) ⬇️
lightning/src/chain/keysinterface.rs 92.54% <50.00%> (-1.06%) ⬇️
lightning/src/ln/msgs.rs 86.18% <63.15%> (-0.32%) ⬇️
lightning/src/util/test_utils.rs 78.13% <75.00%> (-0.03%) ⬇️
lightning/src/onion_message/packet.rs 83.33% <83.33%> (ø)
lightning/src/onion_message/messenger.rs 89.15% <89.15%> (ø)
lightning/src/onion_message/blinded_route.rs 95.74% <95.74%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e403999...81e3c41. Read the comment docs.

@valentinewallace
Copy link
Contributor Author

According to @t-bast (and my experience), the spec test vectors are all broken and won't be fixed until lightning/bolts#765 has ACKs on the data model. Still gonna write our own tests, but removed the spec test vectors from the PR TODOs so they aren't blocking.

@valentinewallace valentinewallace force-pushed the 2022-05-onion-msgs branch 2 times, most recently from eaecccd to 050268a Compare June 1, 2022 01:23
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Browsed the new onion_message.rs API. Catching up with the spec itself.

pub fn new<Signer: Sign, K: Deref>(node_pks: Vec<PublicKey>, keys_manager: K) -> Result<Self, ()>
where K::Target: KeysInterface<Signer = Signer>,
{
if node_pks.len() <= 1 { return Err(()) }
Copy link

Choose a reason for hiding this comment

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

Would we like to enforce a max length ? From my understanding the specs isn't recommending one. I think we may be interested to check one at blinded route reception for memory bounds reasons, as it's a third-party input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a TODO to ensure that outbound onion packets can't exceed BIG_PACKET_HOP_DATA_LEN. Reasoning is that Eclair will drop onion messages that exceed this length

Copy link

Choose a reason for hiding this comment

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

If it's a value shared among implementation, nice to add it to the spec imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
/// If `path_id` is `Some`, it is used to identify the blinded route that this onion message is
/// sending to. This is useful for receivers to check that said blinded route is being used in
/// the right context.
path_id: Option<[u8; 32]>
Copy link

Choose a reason for hiding this comment

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

Suggestion : if path_id usage is to identify that an encrypted_payload is arriving from the expected, last-hop node id, we should call it something like blinded_route_id to avoid confusion with the notion of a payment path.

A part of that, it's unclear to me what kind of further checks could be realized to verify the "right context usage". I would be concerned it could introduce "naive" deanonymization attacks, where the encrypted path_id is tampered with to observe success/errors from timing processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Thankfully, the path_id cannot be tampered with -- it's always provided by the receiver in an encrypted and HMAC-d blob

Copy link

Choose a reason for hiding this comment

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

Right after reading the spec, if it's tampered it should break at the HMAC-level.

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
let node_secret = match self.keys_manager.get_node_secret(Recipient::Node) {
Ok(secret) => secret,
Err(e) => {
log_trace!(self.logger, "Failed to retrieve node secret: {:?}", e);
Copy link

Choose a reason for hiding this comment

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

Maybe "Onion Reception : Failed to retrieve node secret" ? get_node_secret is used few times across the codebase, nice to provide more info to identify the callsite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log should be prefixed by the onion_messages module name, do you think that helps? 🤔

Copy link

Choose a reason for hiding this comment

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

I would say yes, we start to have a lot of logs, being able to observe specific subsystem help to debug test from exp.

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
valentinewallace added a commit to valentinewallace/rust-lightning that referenced this pull request Jun 4, 2022
To get the next hop's packet's pubkey. This will be used to DRY onion message
forwarding in the upcoming Onion Messages PR lightningdevkit#1503
valentinewallace added a commit to valentinewallace/rust-lightning that referenced this pull request Jun 17, 2022
To get the next hop's packet's pubkey. This will be used to DRY onion message
forwarding in the upcoming Onion Messages PR lightningdevkit#1503
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Really quick initial first pass, but generally I think this is looking pretty good. The API seems sensible and the implementation is pretty straightforward.

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
Receive {
/// If `path_id` is `Some`, it is used to identify the blinded route that this onion message is
/// sending to. This is useful for receivers to check that said blinded route is being used in
/// the right context.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need a TODO to make it non-optional? Or are there reasons to set it to None once we're done with our full implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be None in the future if someone were sending to our unblinded public key and not to a blinded route we create

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved
}

impl Writeable for Packet {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is Packet never written/read in tests? At least according to codecov.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's bizarre, is there a link to that? Can't find it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, when I add logs to the read/write methods in the tests they def get hit 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ugh codecov sucks.

@valentinewallace
Copy link
Contributor Author

Will add two missing tests next but pushed up the latest changes, main difference being somewhat extensive module-level documentation being added and the new sending errors.

@valentinewallace valentinewallace force-pushed the 2022-05-onion-msgs branch 3 times, most recently from 0dfad2a to 15983c2 Compare June 24, 2022 04:20
@valentinewallace
Copy link
Contributor Author

This is good for review!

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Reviewed so far until 15983c2

/// Control TLVs for the final recipient of an onion message.
Receive {
/// If `path_id` is `Some`, it is used to identify the blinded route that this onion message is
/// sending to. This is useful for receivers to check that said blinded route is being used in
Copy link

Choose a reason for hiding this comment

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

nit: As I understand the path_id is useful for the receiver, so it could say "this onion message is coming from".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, in my mental model you send an onion message to a blinded route, which aligns with it being a Destination variant.

lightning/src/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Outdated Show resolved Hide resolved
pub fn new<Signer: Sign, K: Deref>(node_pks: Vec<PublicKey>, keys_manager: K) -> Result<Self, ()>
where K::Target: KeysInterface<Signer = Signer>,
{
if node_pks.len() <= 1 { return Err(()) }
Copy link

Choose a reason for hiding this comment

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

If it's a value shared among implementation, nice to add it to the spec imo.

/// If `path_id` is `Some`, it is used to identify the blinded route that this onion message is
/// sending to. This is useful for receivers to check that said blinded route is being used in
/// the right context.
path_id: Option<[u8; 32]>
Copy link

Choose a reason for hiding this comment

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

Right after reading the spec, if it's tampered it should break at the HMAC-level.

let node_secret = match self.keys_manager.get_node_secret(Recipient::Node) {
Ok(secret) => secret,
Err(e) => {
log_trace!(self.logger, "Failed to retrieve node secret: {:?}", e);
Copy link

Choose a reason for hiding this comment

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

I would say yes, we start to have a lot of logs, being able to observe specific subsystem help to debug test from exp.

@TheBlueMatt TheBlueMatt added the blocked on next release Should Wait Until Next Release To Land label Jun 27, 2022
@valentinewallace valentinewallace force-pushed the 2022-05-onion-msgs branch 4 times, most recently from 282ed9e to a063de6 Compare June 27, 2022 21:00
ariard
ariard previously approved these changes Aug 1, 2022
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Code Review ACK e4a50a3

Changes since last review:

  • minor comments change around construct_onion_packet_with_init_noise
  • buffered read for Packet
  • clarification of ecdh() comment
  • new construct_onion_message_packet to initialize ChaCha20
  • restructuring of packet_payloads_and_keys
  • spec rational comment in construct_onion_message_packet
  • new SendError::TooFewBlindedHops and corresponding test

fuzz/src/chanmon_consistency.rs Outdated Show resolved Hide resolved
lightning/src/ln/onion_utils.rs Show resolved Hide resolved
lightning/src/util/ser.rs Outdated Show resolved Hide resolved
Blinded routes can be provided as destinations for onion messages, when the
recipient prefers to remain anonymous.

We also add supporting utilities for constructing blinded path keys, and
control TLVs structs representing blinded payloads prior to being
encoded/encrypted. These utilities and struct will be re-used in upcoming
commits for sending and receiving/forwarding onion messages.

Finally, add utilities for reading the padding from an onion message's
encrypted TLVs without an intermediate Vec.
lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
};

let mut pending_per_peer_msgs = self.pending_messages.lock().unwrap();
let pending_msgs = pending_per_peer_msgs.entry(next_node_id).or_insert(Vec::new());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to keep track of if we're actually connected to the given peer here, and simply drop the message if we're not. We can do this in the followup that wires this up to PeerManager, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added a TODO on the follow-up PR to address soon

lightning/src/onion_message/packet.rs Outdated Show resolved Hide resolved
}
}

pub(crate) fn decode_next_hop<D: DecodeInput, R: ReadableArgs<D::Arg>, N: NextPacketBytes>(shared_secret: [u8; 32], hop_data: &[u8], hmac_bytes: [u8; 32], decode_input: D) -> Result<(R, Option<([u8; 32], N)>), OnionDecodeErr> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really not a fan of this being public with all the trait bounds that get auto-implemented based on what's passed in. Can we make this private and add a decode_next_message_hop like we have for decode_next_payment_hop, then we can make the DecodeInput and NextPacketBytes traits priv? Happy to see this in a followup, too, if you're feeling lazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Noted in the follow-up issue, will aim to be a quick follow-up

@TheBlueMatt
Copy link
Collaborator

LGTM, feel free to squash when other reviewers are happy.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM modulo one comment. Feel free to squash.

lightning/src/util/ser.rs Outdated Show resolved Hide resolved
valentinewallace and others added 10 commits August 2, 2022 19:17
…noise for it

We need to add a new Packet struct because onion message packet hop_data fields
can be of variable length, whereas regular payment packets are always 1366
bytes.

Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
This method will help us avoid retrieving our node secret, something we want to
get rid of entirely.  It will be used in upcoming commits when decoding the
onion message packet, and in future PRs to help us get rid of
KeysInterface::get_node_secret usages across the codebase
OnionMessenger will be hooked up to the PeerManager to send and receive OMs in
a follow-up PR.
This adds several utilities in service of then adding
OnionMessenger::send_onion_message, which can send to either an unblinded
pubkey or a blinded route. Sending custom TLVs and sending an onion message
containing a reply path are not yet supported.

We also need to split the construct_keys_callback macro into two macros to
avoid an unused assignment warning.
This required adapting `onion_utils::decode_next_hop` to work for both payments
and onion messages.

Currently we just print out the path_id of any onion messages we receive. In
the future, these received onion messages will be redirected to their
respective handlers: i.e. an invoice_request will go to an InvoiceHandler,
custom onion messages will go to a custom handler, etc.
Pre-existing to this PR, we were reading next packet bytes with io::Read::read,
which is not guaranteed to read all the bytes we need, only guaranteed to read
*some* bytes.

We fix this to be read_exact, which is guaranteed to read all the next hop
packet bytes.
@TheBlueMatt TheBlueMatt merged commit 28c9b56 into lightningdevkit:main Aug 3, 2022
@jkczyz jkczyz mentioned this pull request May 10, 2023
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants