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

Authenticate blinded payment paths #3435

Merged
merged 10 commits into from
Dec 13, 2024

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Dec 2, 2024

When receiving a PaymentContext from a blinded payment, the context must be authenticated. Otherwise, the context can be forged and would appear within a PaymentPurpose. In order to authenticate a PaymentContext, an Hmac and a Nonce must be included along with it in payment::ReceiveTlvs. Upon receiving an HTLC over a blinded path, ChannelManager can then authenticate it.

This is also useful for preventing de-anonymization attacks where payment::ReceiveTlvs is forged to use a PaymentSecret from a BOLT11 invoice.

Fixes #3427

@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from 49486e6 to c371f95 Compare December 2, 2024 23:46
@jkczyz jkczyz added this to the 0.1 milestone Dec 3, 2024
@@ -260,6 +264,8 @@ pub struct ReceiveTlvs {
pub payment_constraints: PaymentConstraints,
/// Context for the receiver of this payment.
pub payment_context: PaymentContext,
/// An HMAC of `payment_context` along with a nonce used to construct it.
pub authentication: (Hmac<Sha256>, Nonce),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be simpler to just authenticate the whole ReceiveTlvs? We probably care that the payment_secret and payment_context line up, and I think the current logic is fine, but its kinda awkward that its fine "because of hose its used" (because its in a blinded path, which we always build so the key to encrypt+MAC it is a key we picked, thus no one can take the pieces apart and use them separately).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So still write as we are for BlindedPaymentTlvs but read it as a separate component of BlindedPaymentTlvs::Receive? We'd need to pass it through to Router::create_blinded_payment_paths, BlindedPaymentPath::new, etc. Though, I think that would mean we wouldn't need to modify Bolt12RefundContext since the secret would be included.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking more that we cheat and just fill in dummy values, HMAC the whole struct, then replace the dummies with the calculated values...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, yeah that would reduce the amount of code we'd need to touch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use an Option, which is taken at verification time.

if let Some(payment_context) = payment_context.as_ref() {
if let Some((hmac, nonce)) = authentication {
if payment_context.verify_for_offer_payment(hmac, nonce, &self.inbound_payment_key).is_err() {
fail_htlc!(claimable_htlc, payment_hash);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just do this when we decode the onion? Then we don't have to store the value and can just fail right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... we could, but it would require having access to the ExpandedKey. And that would leak into the public onion_payment::peel_payment_onion function's interface, too, IIUC. Maybe that is ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea, that seems worth it to avoid storing the stuff just to check it late, no?

I mean technically we do have the NodeSigner in peel_payment_onion so we could re-derive the ExpandedKey, though I'm not sure its worth doing.cause its not free (even if its much cheaper than the ECDH we do later with the signer). We could also consider changing get_inbound_payment_key_material to return an ExpandedKey directly so that the signer can cache it..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm inclined to update NodeSigner as you mentioned. Also, instead of touching peel_payment_onion, we could do authenticate a little earlier when parsing the InboundOnionPayload. I guess this is more what you meant? It also has a NodeSigner and the entire ReceiveTlvs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated accordingly.

@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from c371f95 to 5b204f7 Compare December 4, 2024 22:40
@@ -46,6 +47,9 @@ const ASYNC_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[6; 16];
// HMAC input for a `PaymentHash`. The HMAC is used in `OffersContext::InboundPayment`.
const PAYMENT_HASH_HMAC_INPUT: &[u8; 16] = &[7; 16];

// HMAC input for `ReceiveTlvs`. The HMAC is used in `blinded_path::payment::PaymentContext`.
const PAYMENT_TLVS_HMAC_INPUT: &[u8; 16] = &[8; 16];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this needs to be updated as it conflicts with #3408.

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.

Basically LGTM

lightning/src/sign/mod.rs Show resolved Hide resolved
return Err(DecodeError::InvalidValue);
}
} else {
return Err(DecodeError::InvalidValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably this needs a pending changelog as any payments for bolt12 invoices which we haven't received yet will be failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll also fail backwards without blinding the error. That might give a hint that we're the recipient if combined with other information, not sure if also worth noting. I guess this would also mean that someone can create a blinded path terminating at us and force us to reveal that we are the last node in their inauthentic path 🤔 weird case 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.

I think we'll also fail backwards without blinding the error. That might give a hint that we're the recipient if combined with other information, not sure if also worth noting.

Would we need to use the original approach to prevent this?

I guess this would also mean that someone can create a blinded path terminating at us and force us to reveal that we are the last node in their inauthentic path 🤔 weird case though

Not sure I understand this scenario. Don't they know we are the recipient? Or is it being revealed to someone else along the path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we need to use the original approach to prevent this?

The original approach should prevent this, or alternately add a new DecodeError or somehow bubble up from the Read method that the error needs to be blinded.

Not sure I understand this scenario. Don't they know we are the recipient? Or is it being revealed to someone else along the path?

Ah, it would be potentially revealed to the hop preceding us. I'm not sure this is worth addressing since it's a fairly narrow pre-0.1 case, but yeah just wanted to point it out.

@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from 5b204f7 to 6b85985 Compare December 5, 2024 22:59
Comment on lines 10492 to 10494
let nonce = Nonce::from_entropy_source(entropy);
let hmac = payee_tlvs.hmac_for_offer_payment(nonce, expanded_key);
payee_tlvs.authentication = Some((hmac, nonce));
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'll need to document how this should be set. But I'm wondering if we should pass the NodeSigner to BlindedPaymentPath::new, which already has an EntropySource. Then anyone creating a BlindedPaymentPath wouldn't need to do this explicitly. Drawback is we'd either need to pass both of those to Router::create_blinded_payment_paths or initialize DefaultRouter with a NodeSigner -- it already has an EntropySource.

@TheBlueMatt Not sure if that is really preferable. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if we could consider passing expanded_key as an input parameter to Router::create_blinded_payment_paths. This could potentially enable DefaultRouter to handle both HMAC and nonce creation directly within create_blinded_payment_paths, setting them appropriately in the process.

However, I noticed that the codebase doesn't seem to have any pub functions that take expanded_key as an input parameter, so this approach might not be feasible.

Curious to hear your thoughts on this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, it does seem a bit award to pass key material through to a "router". Another option would be to have a constructor on ReceiveTlvs (even if the fields are still public), allowing it to be easily done without actually doing it when building the blinded path itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I took some inspiration from this and a discussion based on @shaavan's comment to make an UnauthenticatedReceiveTlvs with a method to convert to ReceiveTlvs. This way we can never have ReceiveTlvs in an invalid state since the Option is no longer used.

@@ -1416,6 +1435,60 @@ fn custom_tlvs_to_blinded_path() {
);
}

#[test]
fn fails_receive_tlvs_authentication() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valentinewallace I cribbed this off some of the other tests here. Let me know if something looks off or if it could be done in a simpler manner.

@jkczyz jkczyz marked this pull request as ready for review December 6, 2024 00:59
@@ -437,7 +424,7 @@ impl Readable for BlindedPaymentTlvs {
(12, payment_constraints, required),
(14, features, (option, encoding: (BlindedHopFeatures, WithoutLength))),
(65536, payment_secret, option),
(65537, payment_context, (default_value, PaymentContext::unknown())),
(65537, payment_context, option),
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a silly question about the code here, but what happens if the payment_context is already set to UnknownPaymentContext in a running LDK node?

Is it possible for this event to occur?

Copy link
Member

Choose a reason for hiding this comment

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

Let me give this a shot!

Since we are removing the variant in this PR, an upgraded LDK node won't "understand" the payment_context it encounters during a read. As a result, parsing the BlindedPaymentTlvs and, by extension, that Blinded Payment Path will fail. However, this should be fine because the introduction of authentication in this PR inherently breaks legacy compatibility, meaning the path wouldn't be usable regardless.

@jkczyz, hope I got this right! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, when receiving a payment we'd fail to parse any TLVs using PaymentContext::unknown(). I believe this should be rare as it would only occur for BlindedPaymentPaths that preexisted PaymentContext. Additionally, the PaymentContext::Unknown variant cannot be created outside of the crate, so there's no way for a user to construct one. Any BlindedPaymentPath containing one could be parsed, however, since that data is encrypted in the path. But we never created those. It's only used when parsing the TLVs from incoming payments.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Concept ACK, I left a question for learning purposes :)

Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Looks great on the first pass! 🚀

One minor suggestion:

Since we shouldn't create or accept a BlindedPaymentPath without ReceiveTlvs authentication, how about adding a fail-safe in Router::create_blinded_payment_path? For example, a debug_assert!() or an Err!() that triggers if receive_tlvs.authentication.is_none(). This could help catch any incorrect usage early on.

@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from 4ce56d4 to 26d27a2 Compare December 11, 2024 15:31
@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 11, 2024

Squashed the fixups as per our offline conversation.

@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from 87359a3 to 1c11e70 Compare December 11, 2024 20:14
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Basically looks good on first pass, will take another look tomorrow!

Comment on lines 97 to 104
fn hmac_payee_tlvs(
payee_tlvs: &ReceiveTlvs, keys_manager: &test_utils::TestKeysInterface,
) -> (Hmac<Sha256>, Nonce) {
let nonce = Nonce([42u8; 16]);
let expanded_key = keys_manager.get_inbound_payment_key();
let hmac = payee_tlvs.hmac_for_offer_payment(nonce, &expanded_key);
(hmac, nonce)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice as a reviewer if this commit were squashed into 5350bfa to avoid rewriting some of that commit's code and 059f9bf, though I get it's somewhat late in the game..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Managed to move parts of that commit across the relevant commits. Required moving some commits around, though.

return Err(DecodeError::InvalidValue);
}
} else {
return Err(DecodeError::InvalidValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll also fail backwards without blinding the error. That might give a hint that we're the recipient if combined with other information, not sure if also worth noting. I guess this would also mean that someone can create a blinded path terminating at us and force us to reveal that we are the last node in their inauthentic path 🤔 weird case though

@valentinewallace
Copy link
Contributor

Needs rebase

@valentinewallace
Copy link
Contributor

LGTM after rebase!

Comment on lines +1468 to +1502
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
check_added_monitors(&nodes[0], 1);

let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
let ev = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events);
let mut payment_event = SendEvent::from_event(ev);

nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
check_added_monitors!(nodes[1], 0);
do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, true, true);
nodes[1].node.process_pending_htlc_forwards();
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, btw I think you might be able to replace this with something like:

	nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(),
	PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
	check_added_monitors(&nodes[0], 1);
	pass_along_route(&nodes[0], &[&[&nodes[1]]], amt_msat, payment_hash, payment_secret);

Taken from do_one_hop_blinded_path. But not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... I see the following failure using that since the HTLC should fail, IIUC.

thread 'ln::blinded_payment_tests::fails_receive_tlvs_authentication' panicked at 'assertion failed: node_a.node.get_and_clear_pending_msg_events().is_empty()', lightning/src/ln/functional_test_utils.rs:2141:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:48:5
   3: lightning::ln::functional_test_utils::do_commitment_signed_dance
             at ./src/ln/functional_test_utils.rs:2141:3
   4: lightning::ln::functional_test_utils::do_pass_along_path
             at ./src/ln/functional_test_utils.rs:2710:4
   5: lightning::ln::functional_test_utils::pass_along_path
             at ./src/ln/functional_test_utils.rs:2789:2
   6: lightning::ln::functional_test_utils::pass_along_route
             at ./src/ln/functional_test_utils.rs:2820:3
   7: lightning::ln::blinded_payment_tests::fails_receive_tlvs_authentication
             at ./src/ln/blinded_payment_tests.rs:1470:2
   8: lightning::ln::blinded_payment_tests::fails_receive_tlvs_authentication::{{closure}}
             at ./src/ln/blinded_payment_tests.rs:1434:1
   9: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
  10: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, the mentioned test method does include a failure but it actively fails the payment. Not gonna bother looking into it further lol.

@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from 1c11e70 to f37ba47 Compare December 12, 2024 19:06
When receiving a PaymentContext from a blinded payment, the context must
be authenticated. Otherwise, the context can be forged and would appear
within a PaymentPurpose. Add functions for constructing and verifying an
HMAC for the ReceiveTlvs, which contains the PaymentContext.
@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from f37ba47 to 27135cb Compare December 12, 2024 19:30
@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 12, 2024

Rebased

TheBlueMatt
TheBlueMatt previously approved these changes Dec 12, 2024
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.

Some nits but LGTM.

Comment on lines 132 to 133
/// `keys` is generated by calling [`NodeSigner::get_inbound_payment_key`] and then
/// calling [`ExpandedKey::new`] with its result. It is recommended to cache this value and not
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// `keys` is generated by calling [`NodeSigner::get_inbound_payment_key`] and then
/// calling [`ExpandedKey::new`] with its result. It is recommended to cache this value and not
/// `keys` is generated by calling [`NodeSigner::get_inbound_payment_key`]. It is recommended to cache this value and not

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not fix the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that was an oversight on my part. Completely missed the comment somehow.

lightning/src/blinded_path/payment.rs Show resolved Hide resolved
lightning/src/ln/blinded_payment_tests.rs Show resolved Hide resolved
@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 12, 2024

Addressed the nits.

diff --git a/lightning/src/blinded_path/payment.rs b/lightning/src/blinded_path/payment.rs
index 3f3c3c768..e3a819271 100644
--- a/lightning/src/blinded_path/payment.rs
+++ b/lightning/src/blinded_path/payment.rs
@@ -624,6 +624,7 @@ impl Readable for PaymentConstraints {
 
 impl_writeable_tlv_based_enum_legacy!(PaymentContext,
        ;
+       // 0 for Unknown removed in version 0.1.
        (1, Bolt12Offer),
        (2, Bolt12Refund),
 );
diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs
index 7d25c472b..c1fad65c1 100644
--- a/lightning/src/ln/blinded_payment_tests.rs
+++ b/lightning/src/ln/blinded_payment_tests.rs
@@ -1433,7 +1433,7 @@ fn fails_receive_tlvs_authentication() {
        let chan_upd = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0).0.contents;
 
        let amt_msat = 5000;
-       let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
+       let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
        let payee_tlvs = UnauthenticatedReceiveTlvs {
                payment_secret,
                payment_constraints: PaymentConstraints {
@@ -1444,8 +1444,37 @@ fn fails_receive_tlvs_authentication() {
        };
        let nonce = Nonce([42u8; 16]);
        let expanded_key = chanmon_cfgs[1].keys_manager.get_inbound_payment_key();
-       let mut payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key);
+       let payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key);
+
+       let mut secp_ctx = Secp256k1::new();
+       let blinded_path = BlindedPaymentPath::new(
+               &[], nodes[1].node.get_our_node_id(), payee_tlvs, u64::MAX, TEST_FINAL_CLTV as u16,
+               &chanmon_cfgs[1].keys_manager, &secp_ctx
+       ).unwrap();
+
+       let route_params = RouteParameters::from_payment_params_and_value(
+               PaymentParameters::blinded(vec![blinded_path]),
+               amt_msat,
+       );
+
+       // Test authentication works normally.
+       nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
+       check_added_monitors(&nodes[0], 1);
+       pass_along_route(&nodes[0], &[&[&nodes[1]]], amt_msat, payment_hash, payment_secret);
+       claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
+
        // Swap in a different nonce to force authentication to fail.
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
+       let payee_tlvs = UnauthenticatedReceiveTlvs {
+               payment_secret,
+               payment_constraints: PaymentConstraints {
+                       max_cltv_expiry: u32::max_value(),
+                       htlc_minimum_msat: chan_upd.htlc_minimum_msat,
+               },
+               payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}),
+       };
+       let nonce = Nonce([43u8; 16]);
+       let mut payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key);
        payee_tlvs.authentication.1 = Nonce([0u8; 16]);
 
        let mut secp_ctx = Secp256k1::new();

NodeSinger::get_inbound_payment_key_material returns KeyMaterial, which
is used for constructing an ExpandedKey. Change the trait to return an
ExpandedKey directly instead. This allows for direct access to the
ExpandedKey when a NodeSigner referenced is available. Otherwise, it
would either need to be reconstructed or passed in separately.
Now that NodeSigner::get_inbound_payment_key returns an ExpandedKey
instead of KeyMaterial, the latter is no longer needed. Remove
KeyMaterial and replace its uses with [u8; 32].
In order to authenticate a PaymentContext, an HMAC and Nonce must be
included along with it in payment::ReceiveTlvs. Compute the HMAC when
constructing a BlindedPaymentPath and include it in the recipient's
BlindedPaymentTlvs. Authentication will be added in an upcoming commit.
When receiving a payment over a BlindedPaymentPath, a PaymentContext is
included but was not authenticated. The previous commit adds an HMAC of
the payment::ReceiveTlvs (which contains the PaymentContext) and the
nonce used to create the HMAC. This commit verifies the authenticity
when parsing the InboundOnionPayload. This prevents a malicious actor
from for forging it.
UnknownPaymentContext is used when payment::ReceiveTlvs doesn't contain
a PaymentContext. This is only needed for a legacy BlindedPaymentPath.
Since these paths a short-lived, UnknownPaymentContext is no longer
needed. Remove it and require that payment::ReceiveTlvs always contains
a PaymentContext.

Any such path would fail authentication since the payment::ReceiveTlvs
would be missing an HMAC and Nonce, so this is a good time to remove
UnknownPaymentContext.
@jkczyz jkczyz force-pushed the 2024-12-hmac-payment-context branch from e25757c to d287bf0 Compare December 13, 2024 15:26
@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 13, 2024

Fixed docs as per missed comment.

diff --git a/lightning/src/ln/inbound_payment.rs b/lightning/src/ln/inbound_payment.rs
index cffb692d8..72f877978 100644
--- a/lightning/src/ln/inbound_payment.rs
+++ b/lightning/src/ln/inbound_payment.rs
@@ -129,9 +129,8 @@ fn min_final_cltv_expiry_delta_from_metadata(bytes: [u8; METADATA_LEN]) -> u16 {
 /// `ChannelManager` is required. Useful for generating invoices for [phantom node payments] without
 /// a `ChannelManager`.
 ///
-/// `keys` is generated by calling [`NodeSigner::get_inbound_payment_key`] and then
-/// calling [`ExpandedKey::new`] with its result. It is recommended to cache this value and not
-/// regenerate it for each new inbound payment.
+/// `keys` is generated by calling [`NodeSigner::get_inbound_payment_key`]. It is recommended to
+/// cache this value and not regenerate it for each new inbound payment.
 ///
 /// `current_time` is a Unix timestamp representing the current time.
 ///

@TheBlueMatt TheBlueMatt merged commit 1a8bf62 into lightningdevkit:main Dec 13, 2024
15 of 17 checks passed
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 97.80220% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (6e85a0d) to head (d287bf0).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/blinded_path/payment.rs 89.18% 0 Missing and 4 partials ⚠️
lightning/src/ln/blinded_payment_tests.rs 99.09% 1 Missing ⚠️
lightning/src/util/test_utils.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3435      +/-   ##
==========================================
+ Coverage   89.71%   89.73%   +0.02%     
==========================================
  Files         130      130              
  Lines      107625   107793     +168     
  Branches   107625   107793     +168     
==========================================
+ Hits        96553    96727     +174     
+ Misses       8672     8664       -8     
- Partials     2400     2402       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Shouldn't Bolt12Offer/RefundContext be HMAC'd?
5 participants