Encrypt payment_metadata when we build the payment secret#4628
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
I've thoroughly re-reviewed the entire PR diff, reading all key files. Let me check the memory for any prior issues that might now be resolved or still pending. The diff is clean. The prior review already captured the meaningful issues, and I verified:
No new issues found beyond the prior review. Review SummaryNo new issues found beyond those flagged in the prior review pass. Prior comment status
Verification notes
|
| } | ||
|
|
||
| if let Some(metadata) = payment_metadata { | ||
| ChaCha20::new_from_block( |
There was a problem hiding this comment.
How about following the PaymentMetadata / EncryptedPaymentMetadata state pattern we introduced in lightningdevkit/ldk-node#899?
We intentionally did that to improve readability and to use the type system to ensure we can't ever leak an unencrypted raw Vec<u8> into the metadata field.
There was a problem hiding this comment.
Hmm, I'm not seeing much opportunity to do this. lightning-invoice can't switch types as it has to handle counterparty data, so we have to make it a Vec<u8> again almost immediately. We could do it in PendingHTLCRouting::Receive/ReceiveKeysend but the structure in process_receive_htlcs is a bit annoying and I'm not entirely clear its worth it just for the inbound edge.
There was a problem hiding this comment.
Hmm, okay, I think I would still prefer a bit more structured/typed approach but at the very least it would be good to make this some dedicated utility methods, if only to isolate all the unwraps in a single place rather than sprinkling them everywhere (and reviewers getting used to reading "unwrap").
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
CI is very failing right now.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4628 +/- ##
==========================================
+ Coverage 86.58% 86.69% +0.10%
==========================================
Files 159 159
Lines 110498 110604 +106
Branches 110498 110604 +106
==========================================
+ Hits 95678 95888 +210
+ Misses 12281 12198 -83
+ Partials 2539 2518 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Ack, aside from rustfmt failing
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Feel free to squash. rustfmt and bench are unhappy, but otherwise fine by me!
In 657ac8f we started committing to the `payment_metadata` in the `payment_secret`. We'd largely assumed that downstream code could simply encrypt the `payment_metadata` itself before passing it to `lightning` and decrypt before reading it from `lightning`. However, this presents a challenge - we'd very much love for that downstream code to avoid adding any extra bytes to its `payment_metadata` if at all possible, but it doesn't have a great way to get a decent IV without simply shoving it in the encrypted `payment_metadata`. Instead, here, we encrypt and decrypt the `payment_metadata` internally in `lightning`. This allows us to reuse the IV that is used for `lightning`-generated `payment_hash`es as the IV for the encrypted `payment_metadata` as well. Sadly, we don't have any similar IV for user-provided `payment_hash`es. In that case, we simply accept the limitations and document that users must avoid encrypting multiple `payment_metadata`s for payments with the same `payment_hash`. This avoids padding the size of the `payment_metadata` and should generally not be a material concern - `payment_hash` reuse should generally not exist anyway, and if it does it should only be in cases where its "the same payment" being retried after failure, at which point `payment_metadata` should hopefully be the same.
Most of our `chacha20` calls don't actually care about the concept of ChaCha20's "seek" vs "nonce" - we just want to use the full 128 bits of nonce space as nonce. Here we unify those calls to keep a consistent API and consolidate the `unwrap`s to one place.
TheBlueMatt
force-pushed
the
2026-05-encrypt-metadata-internally
branch
from
cfbf274 to
4fac0fe
Compare
|
Fixed and squashed: $ git diff-tree -U1 cfbf274d56 4fac0fe1c1
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index a0da238311..2adb0a1ca5 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -22134,3 +22134,4 @@ pub mod bench {
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
- let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200, None, None).unwrap();
+ let (payment_secret, _no_payment_metadata) =
+ $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200, None, None).unwrap();
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index 8e682baa43..3adc638029 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -40,3 +40,3 @@ use lightning_invoice::RawBolt11Invoice;
use crate::chain::transaction::OutPoint;
-use crate::crypto::utils::{apply_chacha20 ,hkdf_extract_expand_twice, sign, sign_with_aux_rand};
+use crate::crypto::utils::{apply_chacha20, hkdf_extract_expand_twice, sign, sign_with_aux_rand};
use crate::ln::chan_utils; |
|
No material changes since @tnull said "otherwise fine by me", so just gonna land. |
5 participants
In 657ac8f we started committing to the
payment_metadatain thepayment_secret. We'd largely assumed that downstream code could simply encrypt thepayment_metadataitself before passing it tolightningand decrypt before reading it fromlightning. However, this presents a challenge - we'd very much love for that downstream code to avoid adding any extra bytes to itspayment_metadataif at all possible, but it doesn't have a great way to get a decent IV without simply shoving it in the encryptedpayment_metadata.Instead, here, we encrypt and decrypt the
payment_metadatainternally inlightning. This allows us to reuse the IV that is used forlightning-generatedpayment_hashes as the IV for the encryptedpayment_metadataas well. Sadly, we don't have any similar IV for user-providedpayment_hashes. In that case, we simply accept the limitations and document that users must avoid encrypting multiplepayment_metadatas for payments with the samepayment_hash. This avoids padding the size of thepayment_metadataand should generally not be a material concern -payment_hashreuse should generally not exist anyway, and if it does it should only be in cases where its "the same payment" being retried after failure, at which pointpayment_metadatashould hopefully be the same.