Support signing BOLT 12 messages in `NodeSigner` #2432

jkczyz · 2023-07-19T17:16:14Z

BOLT 12 messages need to be signed in the following scenarios:

constructing an InvoiceRequest after scanning an Offer,
constructing an Invoice after scanning a Refund, and
constructing an Invoice when handling an InvoiceRequest.

Extend the NodeSigner trait to support signing BOLT 12 messages such that it can be used in these contexts. The method could be used then in an OffersMessageHandler when keys aren't derived from the offer or payer metadata.

Taken from #2371.

valentinewallace · 2023-07-21T17:49:48Z

This doesn't build for me on 1.58. Mind rebasing/fixing CI? Feel free to squash too IMO.

valentinewallace · 2023-07-21T17:55:10Z

lightning/src/offers/merkle.rs

+
+impl<F, E> SignFunction<E> for F where F: FnOnce(&TaggedBytes, &[u8]) -> Result<Signature, E> {}
+
+/// Bytes associated with a tag, which are used to produced a [`Message`] digest to sign.


Does the tag indicate what offers message is being signed? Would appreciate a docs clarification

also, would be good to have a short comment about the format of bytes (even if it can be deduced by the reader from the BOLT)

Renamed this to TaggedHash as that is more accurate and re-wrote the docs.

valentinewallace · 2023-07-21T19:22:34Z

lightning/src/util/test_utils.rs

@@ -795,6 +797,12 @@ impl NodeSigner for TestNodeSigner {
 		unreachable!()
 	}

+	fn sign_bolt12_message(
+		&self, _message: &TaggedBytes, _metadata: &[u8]


@devrandom @ksedgwic any feedback on this API?

isn't the metadata also tagged? would be good in general to have more specific comments about the format

BTW, the _metadata arg can stay if it provides performance advantages in some applications. validating signers can just ignore it.

This is what we use for CLN:

/// SignBolt12 #[derive(SerBolt, Debug, Serialize, Deserialize)] #[message_id(25)] pub struct SignBolt12 { pub message_name: WireString, pub field_name: WireString, pub merkle_root: Sha256, pub public_tweak: Octets, } /// #[derive(SerBolt, Debug, Serialize, Deserialize)] #[message_id(125)] pub struct SignBolt12Reply { pub signature: Signature, }

isn't the metadata also tagged? would be good in general to have more specific comments about the format

The metadata was included simply to provide a way to derive the signing keys from the metadata. However, after the discussion in #1989 (comment), it's unclear if this is actually needed.

As currently implemented, LDK's ChannelManager will either sign the invoice from derived keys for you or delegate to the NodeSigner. The latter is only reached when the originating offer did not include any blinded paths and thus included the actual node_id for the signing_pubkey in the offer. See:

rust-lightning/lightning/src/offers/offer.rs

Lines 158 to 159 in 685f266

/// recipient privacy by using a different signing pubkey for each offer. Otherwise, the

/// provided `node_id` is used for the signing pubkey.

This is what we use for CLN:

/// SignBolt12 #[derive(SerBolt, Debug, Serialize, Deserialize)] #[message_id(25)] pub struct SignBolt12 { pub message_name: WireString, pub field_name: WireString, pub merkle_root: Sha256, pub public_tweak: Octets, } /// #[derive(SerBolt, Debug, Serialize, Deserialize)] #[message_id(125)] pub struct SignBolt12Reply { pub signature: Signature, }

Does this mean you are blindly signing the tagged hash of the merkle root without computing the merkle root from the invoice or invoice_request TLV stream on your own?

Where does the tweak come from? Is it CLN-specific?

Apologies for the delay ...

I think we need the full invoice and invoice_request content. Our current interface is missing this.

Need to spend some time thinking about which things are important to enforce though, I'm not up to speed yet ...

The bytes are now exposed in TaggedHash.

jkczyz · 2023-07-25T21:51:06Z

This doesn't build for me on 1.58. Mind rebasing/fixing CI? Feel free to squash too IMO.

Build should be fixed now in the latest push. Appears newer versions of rustc did a better job at inferring the types of closure parameters.

lightning/src/offers/merkle.rs

jkczyz · 2023-07-27T22:20:57Z

FYI, I'll need to rebase this and dependent PRs soon as there are some merge conflicts.

jkczyz · 2023-07-28T22:29:29Z

Rebased now which included the Invoice rename to Bolt12Invoice and other related types.

valentinewallace

Basically LGTM if the VLS folks are happy

lightning/src/offers/merkle.rs

lightning/src/offers/invoice_request.rs

lightning/src/offers/merkle.rs

jkczyz · 2023-07-31T22:50:18Z

Basically LGTM if the VLS folks are happy

@devrandom IIUC, you don't need the metadata parameter as you'd parse the message if it's needed.

@TheBlueMatt Guessing we can drop it and add it later if needed as the NodeSigner isn't used when keys can be derived from the metadata.

devrandom

looks good from VLS point of view

devrandom · 2023-08-01T08:05:36Z

lightning/src/offers/merkle.rs


 	let pubkey = pubkey.into();
 	let secp_ctx = Secp256k1::verification_only();
-	secp_ctx.verify_schnorr(&signature, &digest, &pubkey).map_err(|e| SignError::Verification(e))?;
+	secp_ctx.verify_schnorr(&signature, &message.to_digest(), &pubkey)


why verify right after signing? isn't this infallible?

Only if the passed pubkey, which comes from the message`, corresponds to the key used by the signing function, which is user-supplied.

devrandom · 2023-08-01T10:37:17Z

lightning/src/offers/merkle.rs

+/// [BIP 340]: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki
+/// [BOLT 12]: https://github.com/rustyrussell/lightning-rfc/blob/guilt/offers/12-offer-encoding.md#signature-calculation
+/// Bytes associated with a tag, which are used to produced a [`Message`] digest to sign.
+pub struct TaggedHash<'a> {


nit: it's slightly unintuitive that this is called a "hash", but it actually contains the full stream. and in fact, the signer would look at the stream to validate. perhaps it makes sense to call it something like TaggedHashTlvStream or TlvStreamWithTaggedHash

Out of curiosity, how would the signer validate the stream? Presumably they may want to parse the message and do some checks. But when computing the hash, would calling TaggedHash::to_message be sufficient given this would be running on their process? Or would the signer not trust this code and opt to reimplement the hash computation?

I guess you mean to_digest. no need to reimplement anything, a code review is enough to trust code in dependencies, so there would be no problem calling that code.

if you are asking what further validation rules we could have on the actual data - we could have some sanity checks on the invoice fields (e.g. maximum amount). but we would also like to handle the payment preimages in the future, so that an attacker can't redirect incoming payments to an intermediate hop. so we would check that the payment hash in the invoice is for a preimage we control

I guess you mean to_digest. no need to reimplement anything, a code review is enough to trust code in dependencies, so there would be no problem calling that code.

Alright, so my argument for the name is that it's the input to the signing function, and the hash is what is being signed. The fact that the bytes that the hash were computed from is needed for some users is secondary.

if you are asking what further validation rules we could have on the actual data - we could have some sanity checks on the invoice fields (e.g. maximum amount). but we would also like to handle the payment preimages in the future, so that an attacker can't redirect incoming payments to an intermediate hop. so we would check that the payment hash in the invoice is for a preimage we control

Hmm... so to reiterate what I said earlier, NodeSigner::sign_bolt12_message isn't actually called in the normal, privacy-preserving case. There's essentially three ways we allow creating an Offer:

OfferBuilder::new which let's the user set any metadata,

OfferBuilder::deriving_signing_pubkey without calling OfferBuilder::path, and

OfferBuilder::deriving_signing_pubkey with calling OfferBuilder::path to add blinded paths.

Any InvoiceRequest messages created in the first case will be rejected by the ChannelManager.

The last two cases will set Offer::metadata such that the ChannelManager implementation of OffersMessageHandler can verify that any InvoiceRequest came from one of our Offers before replying with an Invoice.

Further, the last case will set Offer::signing_pubkey to an ephemeral pubkey and allow ChannelManager to derive the corresponding secret key from the Offer::metadata. Thus, for this way, NodeSigner::sign_bolt12_message is not even called. Instead, ChannelManager will sign the invoice by deriving the keys from the metadata.

It's only the second case where NodeSigner::sign_bolt12_message is invoked. See PR #2371 for details. So it seems you'll need some other way to validate this data. Either by implementing OffersMessageHandler yourself or having ChannelManager call some other method on NodeSigner specifically for validation.

Further, the last case will set Offer::signing_pubkey to an ephemeral pubkey and allow ChannelManager to derive the corresponding secret key from the Offer::metadata. Thus, for this way, NodeSigner::sign_bolt12_message is not even called. Instead, ChannelManager will sign the invoice by deriving the keys from the metadata.

Sorry for the slow reply and not fully understanding your first comment. But I must be still missing something, since this seems potentially problematic. How will a relying party know that this offer was securely issued by our node if the offer is not bound to our node key? or looking at it from a different direction, why would something outside of the signer be able to enter into an agreement that could in general cause financial loss?

It's a design goal of the signer abstraction that nothing outside the signer can lose money, either by moving assets or by signing bogus invoices and such.

why do we think this is a common case? if, for example, a user wants to pay some bill or buy some merchandise online, they would know what the node_id is (and hopefully allowlist the node_id to reduce having to approve things all the time).

Because its required for recipient privacy - while we could define some static "recipient payment key" which signs all BOLT12 structs, LDK seeks to encourage users to randomize so that offers aren't linkable across payments. For users on eg a mobile device seeking to receive regularly, this is a super nice privacy property. If a sender wants to cache a recipient to pay them regularly, we should consider a way for them to get an amount-less offer which they can pay regularly, possibly via the "do you know the secret key to the pubkey I'm thinking about" protocol Bitcoin Core folks have talked about using for P2P auth.

that said, I'm not sure how useful the offer is, given that it's unsigned and could be arbitrarily generated by anybody.

An invoice is signed by the pubkey which is in the offer, and the invoice bytes start with the full bytes of the offer, so you can validate that an invoice came from a given offer.

I see. in this case, we should have the approval UX pre-approve an offer for repeated payments. so we should use either the node_id or the offer_id as the "destination".

Note, the spec no longer has an offer_id.

I kinda assumed we'd just have some call on the signer to say "hey, here's an invoice, we're paying it, FYI" and be done with it. Presumably any VLS client will pass VLS any offers which the client has authorized to pay via some secure channel (presumably also how it works today with BOLT11) or has some policy enforcement. So all LDK really needs to do is give VLS a mapping from the offer to the invoice that we're about to pay, and VLS can validate that the invoice is built using the offer.

Can't VLS use the metadata to verify an invoice instead of needing the client to pass it authorized offers? Presumably, it has the same key material as the NodeSigner and thus can form an ExpandedKey to verify. Or am I misunderstanding the setup.

Can't VLS use the metadata to verify an invoice instead of needing the client to pass it authorized offers? Presumably, it has the same key material as the NodeSigner and thus can form an ExpandedKey to verify. Or am I misunderstanding the setup.

We could move all the metadata generation and signing into the signer, but I'm not sure that would fix this - ultimately the security of VLS depends on some kind of secure channel between VLS and the user (bypassing the untrusted "node") for the user to tell VLS they intend to pay something (BOLT11 invoice/BOLT12 offer/BOLT12 offer repeatedly up to some amount). Once we have that, there's not a lot of reason (IMO) to have VLS do that via metadata verification vs just storing the full BOLT12 offer.

valentinewallace · 2023-08-01T17:48:09Z

Basically LGTM if the VLS folks are happy

@devrandom IIUC, you don't need the metadata parameter as you'd parse the message if it's needed.

@TheBlueMatt Guessing we can drop it and add it later if needed as the NodeSigner isn't used when keys can be derived from the metadata.

Isn't the metadata also included in the TLV stream? Seems like another reason to exclude it as an explicit parameter for now, since presumably signers are already parsing the TLV stream.

codecov-commenter · 2023-08-01T19:24:54Z

Codecov Report

Patch coverage: 84.31% and project coverage change: +1.33% 🎉

Comparison is base (5cddf5e) 90.33% compared to head (6805acb) 91.66%.
Report is 67 commits behind head on main.

❗ Current head 6805acb differs from pull request most recent head 39012e3. Consider uploading reports for the commit 39012e3 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2432      +/-   ##
==========================================
+ Coverage   90.33%   91.66%   +1.33%     
==========================================
  Files         106      106              
  Lines       55732    68959   +13227     
  Branches    55732    68959   +13227     
==========================================
+ Hits        50347    63214   +12867     
- Misses       5385     5745     +360

Files Changed	Coverage Δ
lightning/src/offers/payer.rs	`66.66% <ø> (ø)`
lightning/src/routing/router.rs	`93.33% <0.00%> (-0.31%)`	⬇️
lightning/src/sign/mod.rs	`81.58% <0.00%> (-2.32%)`	⬇️
lightning/src/util/test_utils.rs	`71.47% <0.00%> (-0.99%)`	⬇️
lightning/src/offers/invoice.rs	`88.52% <83.70%> (-1.83%)`	⬇️
lightning/src/offers/refund.rs	`94.02% <88.00%> (-0.39%)`	⬇️
lightning/src/offers/invoice_request.rs	`93.16% <94.91%> (-0.22%)`	⬇️
lightning/src/offers/merkle.rs	`99.44% <100.00%> (+0.02%)`	⬆️
lightning/src/offers/offer.rs	`92.71% <100.00%> (+0.47%)`	⬆️
lightning/src/offers/test_utils.rs	`96.72% <100.00%> (ø)`

... and 25 files with indirect coverage changes

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

jkczyz · 2023-08-01T19:30:03Z

Isn't the metadata also included in the TLV stream? Seems like another reason to exclude it as an explicit parameter for now, since presumably signers are already parsing the TLV stream.

Yeah, went ahead and removed the parameter.

TheBlueMatt · 2023-08-08T23:30:14Z

lightning/src/sign/mod.rs

@@ -620,6 +621,15 @@ pub trait NodeSigner {
 	/// Errors if the [`Recipient`] variant is not supported by the implementation.
 	fn sign_invoice(&self, hrp_bytes: &[u8], invoice_data: &[u5], recipient: Recipient) -> Result<RecoverableSignature, ()>;

+	/// Signs a BOLT 12 message.
+	///
+	/// See [`SignFunction`] for details.


This isn't a super useful documentation, SignFunction also doesn't really say anything but "read TaggedHash" which only kinda says something about its contents.

Removed the SignFunction commit in the latest push now that merkle::sign_message only takes one parameter. Plus, older rustc were having a hard time inferring the parameter types in closures. Re-wrote the docs here and in merkle::sign_message.

TheBlueMatt · 2023-08-08T23:31:56Z

lightning/src/offers/merkle.rs

+	///
+	/// [`invoice::SIGNATURE_TAG`]: crate::offers::invoice::SIGNATURE_TAG
+	/// [`invoice_request::SIGNATURE_TAG`]: crate::offers::invoice_request::SIGNATURE_TAG
+	pub tag: &'a str,


Should this be an enum if there's only 2-3 valid tags?

This is now only a parameter to a pub(super) constructor, so leaving it as &str.

TheBlueMatt · 2023-08-08T23:32:44Z

lightning/src/offers/merkle.rs

+	pub tlv_stream: &'a [u8],
+
+	/// The cached digest to sign.
+	digest: RefCell<Option<Message>>,


This isn't safe since the tlv_stream and tag are pub, which would allow a user to change them then call to_digest and get a cached, now invalid, result.

Oh, duh. Good catch... should have used an accessor. The latest push removes the cache and instead makes TaggedHash simply a wrapper around Message.

TheBlueMatt · 2023-08-09T00:08:51Z

lightning/src/offers/merkle.rs

+/// [BIP 340]: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki
+/// [BOLT 12]: https://github.com/rustyrussell/lightning-rfc/blob/guilt/offers/12-offer-encoding.md#signature-calculation
+/// Bytes associated with a tag, which are used to produced a [`Message`] digest to sign.
+pub struct TaggedHash<'a> {


Talking more abstractly about security model for VLS in BOLT12:

For outbound payments, I feel like VLS should verify the HTLC we're sending by just receiving an invoice which it can detect matches up with a user-provided offer (and potentially amount) which it has listed as to-be-paid. This requires ~nothing from LDK aside from us giving them the invoice once we get it before sending the HTLC, and notably means VLS won't be involved in our metadata or invoice_request signing at all.

For inbound payments/invoice signing, VLS needs to be more involved as it wants to ensure we aren't signing invoices with bogus amounts for the expected payment, at least for those doing the non-private payments, as Jeff notes. Here too, I don't think VLS needs to be involved in the metadata (modern equivalent of payment_secret) generation/validation. However, in the non-private offer case, VLS/the signer wants to verify the BOLT12 offer/invoice data, at least the amount and description fields. In that case, I don't think it makes sense to make the API read like you're only blindly signing a hash, but rather make it clear that there's data there you may want to validate.

fuzz/src/chanmon_consistency.rs

valentinewallace · 2023-08-11T15:55:16Z

lightning/src/offers/merkle.rs

+///
+/// [BIP 340]: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki
+/// [BOLT 12]: https://github.com/rustyrussell/lightning-rfc/blob/guilt/offers/12-offer-encoding.md#signature-calculation
+pub struct TaggedHash(Message);


Do we still need this struct? It seems like a very thin wrapper

Yeah, as it ensures that Message is constructed properly.

How does it ensure that more than the underlying message_digest method ensures it? Maybe need to wait for a rebase of #2371 or #2039 to see more clearly.

No, it's exactly as you say. We only want to pass Messages to sign_message that have been constructed via message_digest. That's the typical use of a wrapper type. Having message_digest directly return a TaggedHash would be suitable, too, I suppose.

Oh, okay. I see that this guarantees that the Message was constructed properly in UnsignedInvoice{Request}::new. I may be missing something, this seems like a lot for an internal guarantee, but I see that reasoning.

valentinewallace · 2023-08-11T16:08:48Z

lightning/src/offers/invoice.rs

 	/// Signs the invoice using the given function.
 	///
 	/// This is not exported to bindings users as functions aren't currently mapped.
-	pub fn sign<F, E>(self, sign: F) -> Result<Bolt12Invoice, SignError<E>>
+	pub fn sign<F, E>(mut self, sign: F) -> Result<Bolt12Invoice, SignError<E>>


Why can't this take a NodeSigner? In #2371 the closure just calls the new NodeSigner method.

Hmmm.. I'd rather not limit people to using NodeSigner if they are using offers as a standalone module.

Hm, with a NodeSigner it could be a standalone module for non-Rust users as well, but 🤷‍♀️

Not sure I follow what you mean here.

Just that this method isn't exposed to bindings users, and people using offers as a standalone module are free to create a custom NodeSigner implementation.

valentinewallace · 2023-08-11T16:09:32Z

lightning/src/offers/invoice.rs

@@ -355,62 +360,82 @@ impl<'a> InvoiceBuilder<'a, DerivedSigningPubkey> {
 		}

 		let InvoiceBuilder { invreq_bytes, invoice, keys, .. } = self;
-		let unsigned_invoice = UnsignedBolt12Invoice { invreq_bytes, invoice };
+		let unsigned_invoice = UnsignedBolt12Invoice::new(invreq_bytes, invoice);

 		let keys = keys.unwrap();


~~Could you add a comment on this unwrap? Hard to tell at a glance how it's safe~~
I guess it's fine due to the concrete type DerivedSigningPubkey actually

Got rid of the unwrap by having DerivedSigningPubkey wrap KeyPair. It's a bit more difficult to do something similar for InvoiceRequestBuilder, unfortunately, since build_without_checks handles both parameterizations.

jkczyz · 2023-08-17T16:57:43Z

Unless @devrandom screams, I'd say we just add a "here's an invoice which we got in response to an invoice_request, we're gonna pay it, FYI" method and call it a day. Maybe let it return a Result and reject the payment if we really want...

Hmm... maybe, instead of us expanding NodeSigner's interface, VLS can just implement OfferMessageHandler by wrapping ChannelManager and delegating / intercepting responses as needed.

TheBlueMatt · 2023-08-17T19:50:05Z

well, for BOLT-11, we were assuming that the app developer would directly talk to us (not via LDK) to notify us of the invoice (and handle UX for approval / rejection).
is there a reason to do something different for BOLT-12? is there an invoice flow that is not controlled by the app code?

I kinda assumed the app developer would prefer a really simple API where they scan a QR code (BOLT12 offer) and then pass it to VLS "blessing it" and then pass it to LDK and be done with it. If we go the route where the user gets the invoice they'll have to deal with a bunch of onion message handling, as @jkczyz noted above.

lightning/src/offers/invoice.rs

lightning/src/offers/offer.rs

devrandom · 2023-08-17T21:21:31Z

well, for BOLT-11, we were assuming that the app developer would directly talk to us (not via LDK) to notify us of the invoice (and handle UX for approval / rejection).
is there a reason to do something different for BOLT-12? is there an invoice flow that is not controlled by the app code?

I kinda assumed the app developer would prefer a really simple API where they scan a QR code (BOLT12 offer) and then pass it to VLS "blessing it" and then pass it to LDK and be done with it. If we go the route where the user gets the invoice they'll have to deal with a bunch of onion message handling, as @jkczyz noted above.

OK, so in order to streamline the API, LDK should provide invoices to VLS. Just to be more precise about what VLS needs to validate:

an offer can result in multiple invoices, and there may be reasons that the user likes the offer, but VLS doesn't like the invoice (e.g. velocity exceeded)
offer, invoice request and invoices can each specify a different amount, which is another reason that accepting the offer can still result in an invoice being rejected

So we definitely need to see the invoice, even if there's a mechanism to bless an offer.

I'll follow up on @jkczyz's thoughts about how to do that.

devrandom · 2023-08-17T21:32:37Z

Hmm... maybe, instead of us expanding NodeSigner's interface, VLS can just implement OfferMessageHandler by wrapping ChannelManager and delegating / intercepting responses as needed.

That seems OK. I couldn't find where chanman implements this interface though in either this PR or in main?

jkczyz · 2023-08-17T22:04:35Z

Hmm... maybe, instead of us expanding NodeSigner's interface, VLS can just implement OfferMessageHandler by wrapping ChannelManager and delegating / intercepting responses as needed.

That seems OK. I couldn't find where chanman implements this interface though in either this PR or in main?

It's in #2371. Though @TheBlueMatt noted offline that this would require VLS users to use two parameterizations, which may not be desirable.

TheBlueMatt

LGTM, feel free to squash IMO.

TheBlueMatt · 2023-08-21T16:31:05Z

lightning/src/offers/invoice.rs

@@ -425,6 +425,7 @@ impl UnsignedBolt12Invoice {
 	}
 }

+// Allows passing a function that takes `UnsignedBolt12Invoice` to `merkle::sign_message`.


nit: In general, docs on trait impls don't really show up well on docs.rs. Its kinda nice to mention these things in the struct docs so that they're visible (or in this case the sign_message docs). Not a huge deal in this case tho.

Moved to sign_message.

TheBlueMatt · 2023-08-21T16:35:14Z

lightning/src/offers/merkle.rs

 	let pubkey = pubkey.into();
 	let secp_ctx = Secp256k1::verification_only();
-	secp_ctx.verify_schnorr(&signature, &digest, &pubkey).map_err(|e| SignError::Verification(e))?;
+	secp_ctx.verify_schnorr(&signature, digest, &pubkey).map_err(|e| SignError::Verification(e))?;


Can you remind me why we need to verify here? Its pretty nontrivial additional CPU cost in a method (presumably) reachable directly as a message handler that an attacker can hammer as much as they want.

It's for the case when the users signs but doesn't use the privkey corresponding to the given pubkey. We can make this a debug assertion if you prefer. Just need to rip out SignError since we wouldn't need two variants.

We can do it in a followup, but, yea, I don't think we need to be that careful - if users screw up then the user sees an error...somewhere else. As long as debug assertions hit it I don't think its worth it?

TheBlueMatt · 2023-08-21T16:52:47Z

lightning/src/offers/invoice.rs

@@ -730,6 +1024,25 @@ impl Writeable for InvoiceContents {
 	}
 }

+impl TryFrom<Vec<u8>> for UnsignedBolt12Invoice {


Hmm, what do we think about unknown fields here? Its kinda weird for an external signer to sign off on even an odd field that it doesn't understand, even if this will fail for even fields. For now we can document this on the struct definition/signer methods, I guess, but its something to consider....just not sure how.

Right, our deserialization code simply skips those records. If gave back some Vec of unknown TLV records, we could at least expose that in the interface. Added docs for now.

valentinewallace · 2023-08-21T18:35:55Z

LGTM after Matt's feedback is addressed.

TheBlueMatt · 2023-08-21T22:36:58Z

Feel free to squash fixups.

The function used to sign BOLT 12 messages only takes a message digest. This doesn't allow signers to independently verify the message before signing nor does it allow them to derive the necessary signing keys, if needed. Introduce a TaggedHash wrapper for a message digest, which each unsigned BOLT 12 message type constructs upon initialization. Change the signing function to take AsRef<TaggedHash>, which each unsigned type implements. This allows the signing function to take any unsigned message and obtain its tagged hash.

InvoiceBuilder is parameterized by a SigningPubkeyStrategy, either ExplicitSigningPubkey and DerivedSigningPubkey. It also holds an Option<KeyPair>, which may be None and Some for those strategies, respectively. This leads to methods for InvoiceBuilder parameterized by DerivedSigningPubkey needing to blindly unwrap the Option<KeyPair>. Instead, have DerivedSigningPubkey wrap KeyPair.

Using `contents` for the field name is more consistent with the signed messages.

InvoiceRequest wraps OfferContents, which shouldn't be exposed as it is an implementation detail. Define a macro for Offer accessor methods so that InvoiceRequest and UnsignedInvoiceRequest can also define them.

Various messages wrap InvoiceRequestContents, which shouldn't be exposed as it is an implementation detail. Define a macro for InvoiceRequest accessor methods so that these messages can also define them.

Also, expose both Offer and InvoiceRequest functions in UnsignedInvoiceRequest.

Bolt12Invoice can either be for an Offer (via an InvoiceRequest) or a Refund. It wraps those types, so expose their methods on both Bolt12Invoice and UnsignedBolt12Invoice. Since Refund does not have all the Offer/InvoiceRequest methods, use an Option return type such that None can returned for refund-based invoices. For methods that are duplicated between Offer/InvoiceRequest and Bolt12Invoice, prefer the (non-Option, if applicable) method from Bolt12Invoice (e.g., amount_msats, signing_pubkey).

An earlier commit introduced TaggedHash for use in sign_message. For consistency, use it in verify_signature, too.

BOLT 12 messages need to be signed in the following scenarios: - constructing an InvoiceRequest after scanning an Offer, - constructing an Invoice after scanning a Refund, and - constructing an Invoice when handling an InvoiceRequest. Extend the NodeSigner trait to support signing BOLT 12 invoices such that it can be used in the latter contexts. The method could be used in an OffersMessageHandler.

valentinewallace · 2023-08-22T17:16:33Z

lightning/src/offers/invoice.rs

+}
+
+impl Bolt12Invoice {
+	invoice_accessors!(self, self.contents);


If we added a parameter for Offer vs Refund to the {Unsigned}Bolt12Invoice type signatures, could we use {offer,refund}_accessors macros for each respective variant and avoid all the boilerplate-y enum-matching methods below? I suspect there might be issues but thought I'd ask.

Unfortunately not. Refund doesn't contain the same fields as Offer and includes InvoiceRequest fields. There are some Option mismatches, too.

Hmm... actually let me think a bit more on this... I suspect it would sorta be done but the real problem is overlapping methods from each. I guess a prefix may work, but bindings might be gross.

The bindings see through macros just fine, not sure what else would be gross.

I mean having a parameterized type where one set of methods is defined for one parameterization and another set is defined for another parameterization.

Ah, yea, that may be annoying. I guess there's not really a great way to do this :/

TheBlueMatt

Definitely agree it'd be nice to use the {offer,refund,invoice_request}_accessors in {Unsigned,}Invoice.

TheBlueMatt · 2023-08-22T18:01:18Z

lightning/src/offers/invoice.rs


 		// Append the signature TLV record to the bytes.
 		let signature_tlv_stream = SignatureTlvStreamRef {
 			signature: Some(&signature),
 		};
-		signature_tlv_stream.write(&mut bytes).unwrap();
+		signature_tlv_stream.write(&mut self.bytes).unwrap();


This is broken if the invoice_request or invoice or offer have any TLVs above 1000, right? Are those allowed/removed somehow? Its not a regression, so doesn't need fixing here, though.

Each TLV type will only read in its allowed range. And ParsedMessage ensures there are no more TLV records left.

Right, guess I just wasn't sure if we had any support for 1k+ TLVs anywhere.

Ah, no. I believe custom TLVs need to be in their respective message type ranges.

For offer:

if the offer contains any TLV fields greater or equal to 80:
- MUST NOT respond to the offer.

For invoice_request:

MUST fail the request if any non-signature TLV fields greater or equal to 160.

Nothing for invoice, but I'll leave a comment on the spec PR.

TheBlueMatt · 2023-08-22T19:16:29Z

lightning/src/offers/invoice.rs

+}
+
+impl Bolt12Invoice {
+	invoice_accessors!(self, self.contents);


The bindings see through macros just fine, not sure what else would be gross.

jkczyz requested a review from valentinewallace July 19, 2023 17:16

jkczyz mentioned this pull request Jul 19, 2023

BOLT 12 Invoice payments #2371

Merged

valentinewallace reviewed Jul 21, 2023

View reviewed changes

jkczyz force-pushed the 2023-07-bolt12-node-signer branch 2 times, most recently from 1aeee45 to f6aa143 Compare July 25, 2023 21:50

valentinewallace reviewed Jul 26, 2023

View reviewed changes

lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from f6aa143 to b7e1a0a Compare July 27, 2023 22:19

jkczyz added 2 commits July 28, 2023 14:21

Tidy up bitcoin::secp256k1 imports

9c708df

Make offers::merkle::SignError visible

4bb4a97

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from b7e1a0a to 08664c3 Compare July 28, 2023 21:06

valentinewallace reviewed Jul 31, 2023

View reviewed changes

lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved

lightning/src/offers/invoice_request.rs Outdated Show resolved Hide resolved

lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved

devrandom previously approved these changes Aug 1, 2023

View reviewed changes

jkczyz dismissed devrandom’s stale review via 9403d31 August 1, 2023 17:58

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from 08664c3 to 9403d31 Compare August 1, 2023 17:58

devrandom previously approved these changes Aug 1, 2023

View reviewed changes

jkczyz dismissed devrandom’s stale review via da70db6 August 1, 2023 19:29

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from 9403d31 to da70db6 Compare August 1, 2023 19:29

jkczyz mentioned this pull request Aug 2, 2023

BOLT 12 outbound PaymentId #2468

Merged

jkczyz added this to the 0.0.117 milestone Aug 7, 2023

TheBlueMatt reviewed Aug 9, 2023

View reviewed changes

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from da70db6 to ac6ca72 Compare August 11, 2023 01:14

valentinewallace reviewed Aug 11, 2023

View reviewed changes

valentinewallace reviewed Aug 17, 2023

View reviewed changes

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from cce0cc9 to 63e0174 Compare August 18, 2023 01:47

TheBlueMatt reviewed Aug 21, 2023

View reviewed changes

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from 63e0174 to 6805acb Compare August 21, 2023 21:24

jkczyz added 14 commits August 21, 2023 19:14

Rename field of unsigned BOLT message contents

889848d

Using `contents` for the field name is more consistent with the signed messages.

Unsigned BOLT 12 message parsing and serialization

230f081

Move BOLT 12 offer method implementations

270bc2e

Move BOLT 12 InvoiceRequest method implementations

85c471a

Move BOLT 12 invoice method implementations

68d4717

Macro-ize Offer accessors for reuse

66060ca

InvoiceRequest wraps OfferContents, which shouldn't be exposed as it is an implementation detail. Define a macro for Offer accessor methods so that InvoiceRequest and UnsignedInvoiceRequest can also define them.

Macro-ize InvoiceRequest accessors for reuse

9d02d06

Various messages wrap InvoiceRequestContents, which shouldn't be exposed as it is an implementation detail. Define a macro for InvoiceRequest accessor methods so that these messages can also define them.

Expose Offer accessor functions in InvoiceRequest

bde9823

Also, expose both Offer and InvoiceRequest functions in UnsignedInvoiceRequest.

Expose invoice accessors in UnsignedBolt12Invoice

57e62da

Use TaggedHash in merkle::verify_signature

63d0d55

An earlier commit introduced TaggedHash for use in sign_message. For consistency, use it in verify_signature, too.

jkczyz force-pushed the 2023-07-bolt12-node-signer branch from 6805acb to 39012e3 Compare August 22, 2023 00:14

valentinewallace approved these changes Aug 22, 2023

View reviewed changes

valentinewallace reviewed Aug 22, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 22, 2023

View reviewed changes

valentinewallace merged commit 0b196eb into lightningdevkit:main Aug 22, 2023
14 checks passed


		impl<F, E> SignFunction<E> for F where F: FnOnce(&TaggedBytes, &[u8]) -> Result<Signature, E> {}

		/// Bytes associated with a tag, which are used to produced a [`Message`] digest to sign.

	/// recipient privacy by using a different signing pubkey for each offer. Otherwise, the
	/// provided `node_id` is used for the signing pubkey.

Support signing BOLT 12 messages in NodeSigner #2432

Support signing BOLT 12 messages in NodeSigner #2432

Conversation

jkczyz commented Jul 19, 2023

valentinewallace commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jul 25, 2023

jkczyz commented Jul 27, 2023

jkczyz commented Jul 28, 2023

valentinewallace left a comment

Choose a reason for hiding this comment

jkczyz commented Jul 31, 2023

devrandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom Aug 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Aug 1, 2023 • edited

codecov-commenter commented Aug 1, 2023 • edited

Codecov Report

jkczyz commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Aug 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Aug 11, 2023 • edited

Choose a reason for hiding this comment

jkczyz Aug 11, 2023 • edited

Choose a reason for hiding this comment

jkczyz commented Aug 17, 2023

TheBlueMatt commented Aug 17, 2023

devrandom commented Aug 17, 2023

devrandom commented Aug 17, 2023 • edited

jkczyz commented Aug 17, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Aug 21, 2023

TheBlueMatt commented Aug 21, 2023

valentinewallace Aug 22, 2023 • edited

Choose a reason for hiding this comment

Support signing BOLT 12 messages in `NodeSigner` #2432

Support signing BOLT 12 messages in `NodeSigner` #2432

devrandom Aug 1, 2023 •

edited

valentinewallace commented Aug 1, 2023 •

edited

codecov-commenter commented Aug 1, 2023 •

edited

valentinewallace Aug 11, 2023 •

edited

valentinewallace Aug 11, 2023 •

edited

jkczyz Aug 11, 2023 •

edited

devrandom commented Aug 17, 2023 •

edited

valentinewallace Aug 22, 2023 •

edited