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 use of offer blinded paths #3139

Merged
merged 29 commits into from
Jul 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5278d31
Change Nonce visibility to pub
jkczyz Jun 19, 2024
0a5918e
Reorder imports
jkczyz Jun 19, 2024
d7aeaa0
Move Nonce to a separate offers sub-module
jkczyz Jun 19, 2024
219691f
Pass Nonce directly to OfferBuilder
jkczyz Jun 19, 2024
e156415
Add InvoiceRequest::verify_using_recipient_data
jkczyz Jun 20, 2024
c0cae08
Assert and document valid Metadata states
jkczyz Jul 11, 2024
c58a1bb
Clean up MessageContext docs
jkczyz Jul 9, 2024
7904e3c
Wrap docs at 100 characters
jkczyz Jul 9, 2024
f546aad
Expand OffersContext::OutboundPayment docs
jkczyz Jul 9, 2024
1ff8c8d
Fix grammar
jkczyz Jul 18, 2024
a5145e4
Fix OffersContext::Unknown docs
jkczyz Jul 18, 2024
6a54618
Add OffersContext::InvoiceRequest
jkczyz Jul 3, 2024
35b75fd
Authenticate InvoiceRequest using OfferContext
jkczyz Jul 2, 2024
bf42847
Elide metadata from Offer with derived keys
jkczyz Jun 20, 2024
9d46340
Rename InvoiceRequest::verify
jkczyz Jul 19, 2024
f537abd
Add docs to Metadata::without_keys
jkczyz Jul 18, 2024
c2a120e
Authenticate Bolt12Invoice using OfferContext
jkczyz Jul 2, 2024
559daeb
Don't send InvoiceError on failed authentication
jkczyz Jul 3, 2024
bdf3330
Add failure tests for offer message authentication
jkczyz Jul 3, 2024
fd596c3
Pass Nonce directly to InvoiceRequestBuilder
jkczyz Jul 12, 2024
114954c
Pass Nonce directly to RefundBuilder
jkczyz Jul 12, 2024
868fee7
Add Bolt12Invoice::verify_using_payer_data
jkczyz Jul 12, 2024
14634c6
Add nonce to OffersContext::OutboundPayment
jkczyz Jul 12, 2024
2c2f3fe
Authenticate Bolt12Invoice using BlindedPath data
jkczyz Jul 12, 2024
e6ee194
Include OffersContext in Event::InvoiceReceived
jkczyz Jul 15, 2024
4ed37d8
Correct docs
jkczyz Jul 19, 2024
df5d7ea
Elide nonce from payer metadata
jkczyz Jul 15, 2024
718bc47
Rename Bolt12Invoice::verify
jkczyz Jul 19, 2024
825bda0
Add pending changelog for BOLT12 authentication
jkczyz Jul 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions lightning/src/offers/invoice_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -774,9 +774,11 @@ macro_rules! invoice_request_respond_with_explicit_signing_pubkey_methods { (
} }

macro_rules! invoice_request_verify_method { ($self: ident, $self_type: ty) => {
/// Verifies that the request was for an offer created using the given key. Returns the verified
/// request which contains the derived keys needed to sign a [`Bolt12Invoice`] for the request
/// if they could be extracted from the metadata.
/// Verifies that the request was for an offer created using the given key by checking the
/// metadata from the offer.
///
/// Returns the verified request which contains the derived keys needed to sign a
/// [`Bolt12Invoice`] for the request if they could be extracted from the metadata.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to update docs here to say that this is deprecated and users really need to use the using_recipient_data variant, this is just here for legacy offers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still needed for offers without blinded paths.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we update the method name to clarify the contexts in which it makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Updated now and did the same for invoice verification methods. Also updated some missed tests and docs.

///
/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice
pub fn verify<
Expand All @@ -800,6 +802,35 @@ macro_rules! invoice_request_verify_method { ($self: ident, $self_type: ty) => {
})
}

/// Verifies that the request was for an offer created using the given key by checking a nonce
/// included with the [`BlindedPath`] for which the request was sent through.
///
/// Returns the verified request which contains the derived keys needed to sign a
/// [`Bolt12Invoice`] for the request if they could be extracted from the metadata.
///
/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice
pub fn verify_using_recipient_data<
#[cfg(not(c_bindings))]
T: secp256k1::Signing
>(
$self: $self_type, nonce: Nonce, key: &ExpandedKey,
#[cfg(not(c_bindings))]
secp_ctx: &Secp256k1<T>,
#[cfg(c_bindings)]
secp_ctx: &Secp256k1<secp256k1::All>,
) -> Result<VerifiedInvoiceRequest, ()> {
let (offer_id, keys) = $self.contents.inner.offer.verify_using_recipient_data(
&$self.bytes, nonce, key, secp_ctx
)?;
Ok(VerifiedInvoiceRequest {
offer_id,
#[cfg(not(c_bindings))]
inner: $self,
#[cfg(c_bindings)]
inner: $self.clone(),
keys,
})
}
} }

#[cfg(not(c_bindings))]
Expand Down
38 changes: 32 additions & 6 deletions lightning/src/offers/offer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -913,18 +913,28 @@ impl OfferContents {
self.signing_pubkey
}

/// Verifies that the offer metadata was produced from the offer in the TLV stream.
pub(super) fn verify<T: secp256k1::Signing>(
&self, bytes: &[u8], key: &ExpandedKey, secp_ctx: &Secp256k1<T>
) -> Result<(OfferId, Option<Keypair>), ()> {
match self.metadata() {
self.verify_using_metadata(bytes, self.metadata.as_ref(), key, secp_ctx)
}

pub(super) fn verify_using_recipient_data<T: secp256k1::Signing>(
&self, bytes: &[u8], nonce: Nonce, key: &ExpandedKey, secp_ctx: &Secp256k1<T>
) -> Result<(OfferId, Option<Keypair>), ()> {
self.verify_using_metadata(bytes, Some(&Metadata::RecipientData(nonce)), key, secp_ctx)
}

/// Verifies that the offer metadata was produced from the offer in the TLV stream.
fn verify_using_metadata<T: secp256k1::Signing>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd feel much better if the two verification methods used different IVs. I know it shouldn't matter cause the offer can't be changed, but I'd feel better about it.

&self, bytes: &[u8], metadata: Option<&Metadata>, key: &ExpandedKey, secp_ctx: &Secp256k1<T>
) -> Result<(OfferId, Option<Keypair>), ()> {
match metadata {
Some(metadata) => {
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_recipient_keys()
},
OFFER_NODE_ID_TYPE => !metadata.derives_recipient_keys(),
_ => true,
}
});
Expand All @@ -933,7 +943,7 @@ impl OfferContents {
None => return Err(()),
};
let keys = signer::verify_recipient_metadata(
metadata, key, IV_BYTES, signing_pubkey, tlv_stream, secp_ctx
metadata.as_ref(), key, IV_BYTES, signing_pubkey, tlv_stream, secp_ctx
)?;

let offer_id = OfferId::from_valid_invreq_tlv_stream(bytes);
Expand Down Expand Up @@ -1296,6 +1306,14 @@ mod tests {
Err(_) => panic!("unexpected error"),
}

// Fails verification when using the wrong method
let invoice_request = offer.request_invoice(vec![1; 32], payer_pubkey()).unwrap()
.build().unwrap()
.sign(payer_sign).unwrap();
assert!(
invoice_request.verify_using_recipient_data(nonce, &expanded_key, &secp_ctx).is_err()
);

// Fails verification with altered offer field
let mut tlv_stream = offer.as_tlv_stream();
tlv_stream.amount = Some(100);
Expand Down Expand Up @@ -1357,6 +1375,14 @@ mod tests {
Err(_) => panic!("unexpected error"),
}

let invoice_request = offer.request_invoice(vec![1; 32], payer_pubkey()).unwrap()
.build().unwrap()
.sign(payer_sign).unwrap();
match invoice_request.verify_using_recipient_data(nonce, &expanded_key, &secp_ctx) {
Ok(invoice_request) => assert_eq!(invoice_request.offer_id, offer.id()),
Err(_) => panic!("unexpected error"),
}

// Fails verification with altered offer field
let mut tlv_stream = offer.as_tlv_stream();
tlv_stream.amount = Some(100);
Expand Down
22 changes: 22 additions & 0 deletions lightning/src/offers/signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ pub(super) enum Metadata {
/// Metadata as parsed, supplied by the user, or derived from the message contents.
Bytes(Vec<u8>),

/// Metadata for deriving keys included as recipient data in a blinded path.
RecipientData(Nonce),

/// Metadata to be derived from message contents and given material.
Derived(MetadataMaterial),

Expand All @@ -54,6 +57,7 @@ impl Metadata {
pub fn as_bytes(&self) -> Option<&Vec<u8>> {
match self {
Metadata::Bytes(bytes) => Some(bytes),
Metadata::RecipientData(_) => None,
Metadata::Derived(_) => None,
Metadata::DerivedSigningPubkey(_) => None,
}
Expand All @@ -62,6 +66,7 @@ impl Metadata {
pub fn has_derivation_material(&self) -> bool {
match self {
Metadata::Bytes(_) => false,
Metadata::RecipientData(_) => false,
Metadata::Derived(_) => true,
Metadata::DerivedSigningPubkey(_) => true,
}
Expand All @@ -75,6 +80,7 @@ impl Metadata {
// derived, as wouldn't be the case if a Metadata::Bytes with length PaymentId::LENGTH +
// Nonce::LENGTH had been set explicitly.
Metadata::Bytes(bytes) => bytes.len() == PaymentId::LENGTH + Nonce::LENGTH,
Metadata::RecipientData(_) => false,
Metadata::Derived(_) => false,
Metadata::DerivedSigningPubkey(_) => true,
}
Expand All @@ -88,6 +94,7 @@ impl Metadata {
// derived, as wouldn't be the case if a Metadata::Bytes with length Nonce::LENGTH had
// been set explicitly.
Metadata::Bytes(bytes) => bytes.len() == Nonce::LENGTH,
Metadata::RecipientData(_) => true,
Metadata::Derived(_) => false,
Metadata::DerivedSigningPubkey(_) => true,
}
Expand All @@ -96,6 +103,7 @@ impl Metadata {
pub fn without_keys(self) -> Self {
match self {
Metadata::Bytes(_) => self,
Metadata::RecipientData(_) => self,
Metadata::Derived(_) => self,
Metadata::DerivedSigningPubkey(material) => Metadata::Derived(material),
}
Expand All @@ -106,6 +114,7 @@ impl Metadata {
) -> (Self, Option<Keypair>) {
match self {
Metadata::Bytes(_) => (self, None),
Metadata::RecipientData(_) => (self, None),
Metadata::Derived(mut metadata_material) => {
tlv_stream.write(&mut metadata_material.hmac).unwrap();
(Metadata::Bytes(metadata_material.derive_metadata()), None)
Expand All @@ -126,10 +135,22 @@ impl Default for Metadata {
}
}

impl AsRef<[u8]> for Metadata {
fn as_ref(&self) -> &[u8] {
match self {
Metadata::Bytes(bytes) => &bytes,
Metadata::RecipientData(nonce) => &nonce.0,
Metadata::Derived(_) => &[],
Metadata::DerivedSigningPubkey(_) => &[],
}
}
}

impl fmt::Debug for Metadata {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Metadata::Bytes(bytes) => bytes.fmt(f),
Metadata::RecipientData(Nonce(bytes)) => bytes.fmt(f),
Metadata::Derived(_) => f.write_str("Derived"),
Metadata::DerivedSigningPubkey(_) => f.write_str("DerivedSigningPubkey"),
}
Expand All @@ -145,6 +166,7 @@ impl PartialEq for Metadata {
} else {
false
},
Metadata::RecipientData(_) => false,
Metadata::Derived(_) => false,
Metadata::DerivedSigningPubkey(_) => false,
}
Expand Down