-
Notifications
You must be signed in to change notification settings - Fork 364
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
Stateless BOLT 12 message verification #1989
Stateless BOLT 12 message verification #1989
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #1989 +/- ##
==========================================
+ Coverage 91.34% 91.36% +0.01%
==========================================
Files 102 104 +2
Lines 50492 51090 +598
Branches 50492 51090 +598
==========================================
+ Hits 46124 46676 +552
- Misses 4368 4414 +46
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Discussed very briefly offline - for metadata verification we, I think, need to be able to sign with both our node and and an auto-derived one. That means, I think, that we should do the verification of the metadata without the signing key at all, but then also derive the signing key from that metadata if we're not using our node_id. That metadata should probably be some kind of HMAC over the rest of the offer data plus a nonce. |
Need to check my understanding here...
How should we represent this in the We could make enum SigningPubkey {
Explicit(PublicKey),
Derived,
} Which at very least could be used in the builder to determine if
If a user gives an explicit pubkey and their own metadata, I assume we can't verify then? I guess this is expected and would mean they couldn't use our onion message handler to respond to the
Do you mean |
830a2e2
to
2630fc8
Compare
I was able to make the above sketched out change without using an Looking for feedback on the |
I guess I'm confused - do we want the |
lightning/src/ln/inbound_payment.rs
Outdated
/// | ||
/// [`Offer::metadata`]: crate::offers::offer::Offer::metadata | ||
pub(crate) fn hmac_for_offer(&self, nonce: Nonce) -> HmacEngine<Sha256> { | ||
let mut hmac = HmacEngine::<Sha256>::new(&self.ldk_pmt_hash_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously needs a new key for this.
lightning/src/offers/offer.rs
Outdated
@@ -102,13 +120,21 @@ impl OfferBuilder { | |||
/// while the offer is valid. | |||
/// | |||
/// Use a different pubkey per offer to avoid correlating offers. | |||
pub fn new(description: String, signing_pubkey: PublicKey) -> Self { | |||
pub fn new(description: String, signing_pubkey: SigningPubkey) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm confused why we need to take the two variants here - they're handled the same internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using the Explicit
variant, the metadata will only be generated if metadata_derived
is called. Two use cases:
- LDK-independent usage (must call
metadata
) - LDK usage with public node id (must call
metadata_derived
)
When using Derived
, the metadata will be generated without needing to call anything. See tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that, as written, if a user gives SigningPubkey::Explicit
and calls OfferBuilder::metadata_derived
with an LDK-independent ExpandedKey
, then InvoiceRequest::verify
should work correctly. So it could be useful leaving this exposed for general usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm confused why add the complexity when the LDK internal usage can just always call metadata_derived
? I'm not sure I understand why we need two ways to call metadata_derived
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd have to ensure that the nonce used to create the signing pubkey is the same one passed to OfferBuilder::metadata_derived
. With the current approach the builder ensures that it is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we instead move the signing_pubkey
argument and the metadata type argument to the final build
method? Then we could make it all one argument - either explicit for both or implicit for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have to make OfferContents::signing_pubkey
an Option
if we did so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a BuildingOffer
struct or so? The current API just seems really confusing - users have to provide an enum variant at OfferBuilder
constructor which can be explicit, in which case they can call the manual metadata function, or it can be derived, in which case they cant (or dont need to) call either metadata function, and if they provide derived they cant call the derived-metadata function (unless its with the same data) or they'll get the wrong pubkey at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we instead move the
signing_pubkey
argument and the metadata type argument to the finalbuild
method? Then we could make it all one argument - either explicit for both or implicit for both.
Note that metadata
is optional, so it isn't required when given an explicit signing_pubkey
. Also, that approach doesn't support verification for when an explicit signing_pubkey
is given (node id or otherwise), FWIW.
Can we use a
BuildingOffer
struct or so?
Meh, not sure if I really want to repeat the fields and need to do something similar in all the other types, which may have nested contents. Seems error-prone.
The current API just seems really confusing - users have to provide an enum variant at
OfferBuilder
constructor which can be explicit, in which case they can call the manual metadata function, or it can be derived, in which case they cant (or dont need to) call either metadata function, and if they provide derived they cant call the derived-metadata function (unless its with the same data) or they'll get the wrong pubkey at the end?
Hmmm... if we think it will be confusing, then I'm more inclined to keep the constructor as it was and add a pub(crate)
constructor for the derived variant taking &ExpandedKey
and Nonce
.
Then the question would be whether or not we want to support verification externally. If no, then OfferBuilder::metadata_derived
is only needed for when used internally with a node id. So we can either make it pub(crate)
or replace it with another pub(crate)
constructor.
My feeling is that LDK-independent users likely would just want to set the signing_pubkey
and metadata
directly and won't care about the derived versions or verification. If true, I'd lean towards the pub(crate)
approached outlined above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... if we think it will be confusing, then I'm more inclined to keep the constructor as it was and add a pub(crate) constructor for the derived variant taking &ExpandedKey and Nonce.
I'm fine with this.
Then the question would be whether or not we want to support verification externally. If no, then OfferBuilder::metadata_derived is only needed for when used internally with a node id. So we can either make it pub(crate) or replace it with another pub(crate) constructor.
Also fine with it not being exposed.
My feeling is that LDK-independent users likely would just want to set the signing_pubkey and metadata directly and won't care about the derived versions or verification. If true, I'd lean towards the pub(crate) approached outlined above.
Yep, would agree.
Turns out it is much cleaner to incorporate it into the builder. But it still allows users to set it explicitly by giving |
2630fc8
to
adcfbd6
Compare
FYI, pushed some commits with clean-up and also using |
adcfbd6
to
9e36915
Compare
Offer
verification9e36915
to
de86b2c
Compare
de86b2c
to
a9a69a0
Compare
While implementing message handling base on this PR, I came across a few decisions that need to be made. @TheBlueMatt @valentinewallace Let me know if you have any thoughts on the following.
|
This seems fine to me since
This sounds like a stopgap before real support for
Maybe we could expand |
Somehow I was thinking If we didn't want to do this we'd probably change the |
Yeah, this seems like the right approach. Then the user can choose to implement their own handling if they aren't using the stateless verification. I have this implemented in #2039 for reference. The handling method will need to either take an Still an open question is if we want to expose an |
a9a69a0
to
0c52188
Compare
@valentinewallace @TheBlueMatt Have some questions / comments mostly related to the follow-up #2039, which now has the most recent push. Message signingI added a Blinded pathsI have utility methods for creating offers, etc. in Since blinded paths need to be at least two hops, we'll need a |
Conceptually, it would make sense to me to add method(s) to the |
0c52188
to
673541e
Compare
Rebased |
If we're including a unique signing pk, cant we just use that as the metadata? ie take all the fields in the offer, hash them with the hmac, then treat that as the signing secret key and check that the signing pk in the offer matches. If it does, we have authenticated without needing the extra bytes in the offer/qr code. |
IIUC, then we'd still need the nonce in the metadata but could drop the Also, FWIW, if we can't form a blinded path, we'd need to use the node pubkey instead of unique signing key. So in that case we couldn't do the check. That is, when using |
0d7f06b
to
f44ba89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments, otherwise looks good!
let tlv_stream = TlvStream::new(bytes).range(OFFER_TYPES).filter(|record| { | ||
match record.r#type { | ||
OFFER_METADATA_TYPE => false, | ||
OFFER_NODE_ID_TYPE => !self.metadata.as_ref().unwrap().derives_keys(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this always false because we were deserialized so our metadata
is Bytes
? (and in invoice_request/refund/invoice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metadata::derives_keys
was updated in the first commit that introduced verification to check the length of the metadata.
f44ba89
to
5f8ad79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a followup we should add the verify calls to the fuzzers, though I'm not too worried about it here specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to address nits in follow-up
Add support for deriving a transient signing pubkey for each Offer from an ExpandedKey and a nonce. This facilitates recipient privacy by not tying any Offer to any other nor to the recipient's node id. Additionally, support stateless Offer verification by setting its metadata using an HMAC over the nonce and the remaining TLV records, which will be later verified when receiving an InvoiceRequest.
Add an iterator that yields TlvRecords over a range of a TlvStream. Useful for verifying that, e.g., an InvoiceRequest was sent in response to an Offer constructed by the intended recipient.
Verify that an InvoiceRequest was produced from an Offer constructed by the recipient using the Offer metadata reflected in the InvoiceRequest. The Offer metadata consists of a 128-bit encrypted nonce and possibly a 256-bit HMAC over the nonce and Offer TLV records (excluding the signing pubkey) using an ExpandedKey. Thus, the HMAC can be reproduced from the offer bytes using the nonce and the original ExpandedKey, and then checked against the metadata. If metadata does not contain an HMAC, then the reproduced HMAC was used to form the signing keys, and thus can be checked against the signing pubkey.
InvoiceRequestBuilder has a field containing InvoiceRequestContents. When deriving the payer_id from the remaining fields, a struct is needed without payer_id as it not optional. Refactor InvoiceRequestContents to have an inner struct without the payer_id such that InvoiceRequestBuilder can use it instead.
Add support for deriving a transient payer id for each InvoiceRequest from an ExpandedKey and a nonce. This facilitates payer privacy by not tying any InvoiceRequest to any other nor to the payer's node id. Additionally, support stateless Invoice verification by setting payer metadata using an HMAC over the nonce and the remaining TLV records, which will be later verified when receiving an Invoice response.
Verify that an Invoice was produced from an InvoiceRequest constructed by the payer using the payer metadata reflected in the Invoice. The payer metadata consists of a 128-bit encrypted nonce and possibly a 256-bit HMAC over the nonce and InvoiceRequest TLV records (excluding the payer id) using an ExpandedKey. Thus, the HMAC can be reproduced from the invoice request bytes using the nonce and the original ExpandedKey, and then checked against the metadata. If metadata does not contain an HMAC, then the reproduced HMAC was used to form the signing keys, and thus can be checked against the payer id.
Add support for deriving a transient payer id for each Refund from an ExpandedKey and a nonce. This facilitates payer privacy by not tying any Refund to any other nor to the payer's node id. Additionally, support stateless Invoice verification by setting payer metadata using an HMAC over the nonce and the remaining TLV records, which will be later verified when receiving an Invoice response.
Stateless verification of Invoice for Offer Verify that an Invoice was produced from a Refund constructed by the payer using the payer metadata reflected in the Invoice. The payer metadata consists of a 128-bit encrypted nonce and possibly a 256-bit HMAC over the nonce and Refund TLV records (excluding the payer id) using an ExpandedKey. Thus, the HMAC can be reproduced from the refund bytes using the nonce and the original ExpandedKey, and then checked against the metadata. If metadata does not contain an HMAC, then the reproduced HMAC was used to form the signing keys, and thus can be checked against the payer id.
For offers where the signing pubkey is derived, the keys need to be extracted from the Offer::metadata in order to sign an invoice. Parameterize InvoiceBuilder such that a build_and_sign method is available for this situation.
8afe694
5f8ad79
to
8afe694
Compare
0.0.115 - Apr 24, 2023 - "Rebroadcast the Bugfixes" API Updates =========== * The MSRV of the main LDK crates has been increased to 1.48 (lightningdevkit#2107). * Attempting to claim an un-expired payment on a channel which has closed no longer fails. The expiry time of payments is exposed via `PaymentClaimable::claim_deadline` (lightningdevkit#2148). * `payment_metadata` is now supported in `Invoice` deserialization, sending, and receiving (via a new `RecipientOnionFields` struct) (lightningdevkit#2139, lightningdevkit#2127). * `Event::PaymentFailed` now exposes a failure reason (lightningdevkit#2142). * BOLT12 messages now support stateless generation and validation (lightningdevkit#1989). * The `NetworkGraph` is now pruned of stale data after RGS processing (lightningdevkit#2161). * Max inbound HTLCs in-flight can be changed in the handshake config (lightningdevkit#2138). * `lightning-transaction-sync` feature `esplora-async-https` was added (lightningdevkit#2085). * A `ChannelPending` event is now emitted after the initial handshake (lightningdevkit#2098). * `PaymentForwarded::outbound_amount_forwarded_msat` was added (lightningdevkit#2136). * `ChannelManager::list_channels_by_counterparty` was added (lightningdevkit#2079). * `ChannelDetails::feerate_sat_per_1000_weight` was added (lightningdevkit#2094). * `Invoice::fallback_addresses` was added to fetch `bitcoin` types (lightningdevkit#2023). * The offer/refund description is now exposed in `Invoice{,Request}` (lightningdevkit#2206). Backwards Compatibility ======================= * Payments sent with the legacy `*_with_route` methods on LDK 0.0.115+ will no longer be retryable via the LDK 0.0.114- `retry_payment` method (lightningdevkit#2139). * `Event::PaymentPathFailed::retry` was removed and will always be `None` for payments initiated on 0.0.115 which fail on an earlier version (lightningdevkit#2063). * `Route`s and `PaymentParameters` with blinded path information will not be readable on prior versions of LDK. Such objects are not currently constructed by LDK, but may be when processing BOLT12 data in a coming release (lightningdevkit#2146). * Providing `ChannelMonitorUpdate`s generated by LDK 0.0.115 to a `ChannelMonitor` on 0.0.114 or before may panic (lightningdevkit#2059). Note that this is in general unsupported, and included here only for completeness. Bug Fixes ========= * Fixed a case where `process_events_async` may `poll` a `Future` which has already completed (lightningdevkit#2081). * Fixed deserialization of `u16` arrays. This bug may have previously corrupted the historical buckets in a `ProbabilisticScorer`. Users relying on the historical buckets may wish to wipe their scorer on upgrade to remove corrupt data rather than waiting on it to decay (lightningdevkit#2191). * The `process_events_async` task is now `Send` and can thus be polled on a multi-threaded runtime (lightningdevkit#2199). * Fixed a missing macro export causing `impl_writeable_tlv_based_enum{,_upgradable}` calls to not compile (lightningdevkit#2091). * Fixed compilation of `lightning-invoice` with both `no-std` and serde (lightningdevkit#2187) * Fix an issue where the `background-processor` would not wake when a `ChannelMonitorUpdate` completed asynchronously, causing delays (lightningdevkit#2090). * Fix an issue where `process_events_async` would exit immediately (lightningdevkit#2145). * `Router` calls from the `ChannelManager` now call `find_route_with_id` rather than `find_route`, as was intended and described in the API (lightningdevkit#2092). * Ensure `process_events_async` always exits if any sleep future returns true, not just if all sleep futures repeatedly return true (lightningdevkit#2145). * `channel_update` messages no longer set the disable bit unless the peer has been disconnected for some time. This should resolve cases where channels are disabled for extended periods of time (lightningdevkit#2198). * We no longer remove CLN nodes from the network graph for violating the BOLT spec in some cases after failing to pay through them (lightningdevkit#2220). * Fixed a debug assertion which may panic under heavy load (lightningdevkit#2172). * `CounterpartyForceClosed::peer_msg` is now wrapped in UntrustedString (lightningdevkit#2114) * Fixed a potential deadlock in `funding_transaction_generated` (lightningdevkit#2158). Security ======== * Transaction re-broadcasting is now substantially more aggressive, including a new regular rebroadcast feature called on a timer from the `background-processor` or from `ChainMonitor::rebroadcast_pending_claims`. This should substantially increase transaction confirmation reliability without relying on downstream `TransactionBroadcaster` implementations for rebroadcasting (lightningdevkit#2203, lightningdevkit#2205, lightningdevkit#2208). * Implemented the changes from BOLT PRs lightningdevkit#1031, lightningdevkit#1032, and lightningdevkit#1040 which resolve a privacy vulnerability which allows an intermediate node on the path to discover the final destination for a payment (lightningdevkit#2062). In total, this release features 110 files changed, 11928 insertions, 6368 deletions in 215 commits from 21 authors, in alphabetical order: * Advait * Alan Cohen * Alec Chen * Allan Douglas R. de Oliveira * Arik Sosman * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * John Cantrell * Lucas Soriano del Pino * Marc Tyndel * Matt Corallo * Paul Miller * Steven * Steven Williamson * Steven Zhao * Tony Giorgio * Valentine Wallace * Wilmer Paulino * benthecarman * munjesi
An
Offer
may containmetadata
which can be used to authenticate that theOffer
was created from a known key. This allows for stateless verification that a receivedInvoiceRequest
was in response to such anOffer
since theInvoiceRequest
contains all the TLV records from theOffer
.To achieve this, the metadata includes a nonce and an HMAC of the remaining
Offer
data (excluding thesigning_pubkey
). A new key inExpandedKey
is used for constructing the HMAC and the nonce is used as the initialization vector. Additionally, the nonce is used to derive a transient signing key from the new key.Similarly, a
Refund
and anInvoiceRequest
contain payermetadata
, which can similarly be constructed for statelessly verifying that a receivedInvoice
was in response to either. Thepayer_id
is also derived similarly as asigning_pubkey
.Based on
#1972#1977.@TheBlueMatt @valentinewallace Looking for early feedback on the HEAD commit. This is simpler than the approach used ininbound_payments
since theInvoiceRequest
contains all theOffer
's fields. However, I'm not sure if this is sufficient.The signing pubkey should probably be ephemeral and stateless as well, as it is needed to sign anInvoice
created from theInvoiceRequest
. So it seems the corresponding private key will need to be generated using anEntropySource
and encrypted in the metadata. Does that sound right or do you have an alternative in mind?