Drop ChannelManager::pending_inbound_payment and replace payment_secret with HMAC(payment data)

[TheBlueMatt]

Currently we require the user register all inbound payments before we receive them so that we can authenticate them using the payment secret. This is kinda annoying, we have to store a bunch of payment state and time that out appropriately based on user-provided timeout data (via block timestamps...).

This is maybe all fine except its a good bit of complexity and we'd like to avoid it if possible. In lightning/bolts#912, Joost suggested lightning nodes just take their local payment metadata and encrypt it to themselves (with an HMAC), then have the sender send us that data in the HTLC onion. This way, we make the sender (via the invoice) deal with tracking our payment metadata and we don't actually have to store it at all.

For most lightning clients this is probably annoying cause they have material payment metadata, but, we don't - we (currently, see PendingInboundPayment) only care about the payment hash/preimage, payment secret, amount, expiry, and user_payment_id. If we drop the last one, I believe we can trivially just start doing this today, except via the payment secret and not a new field.

There is a bit of complexity here, however, because we have effectively two different payment types - those from create_inbound_payment where we constructed the payment preimage, and create_inbound_payment_for_hash where the user gave us the payment hash. This is rectifiable, however.

To shove all the needed info in the payment secret, we'd replace the payment secret with an encrypted blob. That encrypted blob would be different based on the type of payment:

For create_inbound_payment payments, we'd set it to contain:

 * the minimum amount field - 8 bytes
 * the high bit of the minimum amount field is 0
 * the payment expiry timestamp - 8 bytes
 * 16 bytes of random data

Then, we'd take an HMAC of the above and set the payment preimage to the HMAC of the above. We'd (of course) hash the payment preimage as the payment hash. Thus, when we receive a payment, we take the payment secret, we HMAC it, we hash the HMAC and we check that the payment hash matches - if it does, we've effectively HMAC'd the data and know its ours and process using the encrypted metadata, otherwise we fail the payment.

For create_inbound_payment_for_hash we do something similar, but:

• instead of the high bit of the minimum amount field being 0, we set it to 1, which allows us to identify the type of payment
• instead of 16 bytes of random data we set it to HMAC(payment_hash || minimum amount field || expiry field)
  Thus, upon receipt, we re-generate the now-explicit HMAC and check that it matches the metadata and payment hash.

All of this requires that we drop the user_payment_id, but @valentinewallace's understanding from asking our users is none of them use it, and they can always store local payment metadata against the payment hash itself.

[jkczyz]

the user_payment_id field - 8 bytes

Should this be the expiry?

[TheBlueMatt]

Should this be the expiry?

Oops, yes, edit'd.

[TheBlueMatt]

Spent a bit of time whiteboarding this and I think we can avoid introducing AES/a block cipher, the construction is

METADATA = one bit for the method used || payment amount || expiry
For create_inbound_payment,
    IV=16 random bytes,
    payment_preimage = HMAC(K1, IV || METADATA)
For create_inbound_payment_for_hash,
    IV=HMAC(K2, METADATA || payment_hash)[0..16]
payment_secret = IV || CHACHA(K3, IV) xor METADATA

Then we always decrypt it by using standard CHACHA with a 16 byte IV/nonce, getting us the METADATA. From there, we look at the top bit of the metadata and either re-derive the payment_preimage or treat the IV as a 16-byte HMAC and check it.

[joostjager]

Interesting that you are implementing this right away. Will it be a hard requirement for LDK payments that the sender supplies the metadata or is the regular invoice with receiver-side state still possible?

[TheBlueMatt]

LDK is a bit unique here in that the answer is "both". LDK is going to hard-require that the metadata is in the payment secret with this scheme, but we expect users to likely store additional metadata locally. This change doesn't change our public api at all - like all lightning implementations we already require payment secrets to authenticate the sender, we're just changing the format of them so that we don't do our own tracking of inbound payments to authenticate the amount.

[ariard]

• the high bit of the minimum amount field is 0

Maybe we want a 3 or 4-width bitmask if we introduce new payment types (PTLCs, ...) in the future ? And ensure old-clients recognize newer payments as $UNKNOWN ?

Then we always decrypt it by using standard CHACHA with a 16 byte IV/nonce

Have you considered other PRF schemes such as SipHash ? I think we're introducing a new HMAC operation in our inbound payment validation pipeline, which could turn as a DoS vector (though ultimately we're bounded by MAX_ACCEPTED_HTLC * number of channels, at least without option_dusty_htlc_uncounted) ? I don't think HMAC performance is that much a real concern so good to know if we're fine on this side.

[devrandom]

what is "the minimum amount field"? is it just the BOLT-11 invoice amount?

[TheBlueMatt]

Maybe we want a 3 or 4-width bitmask if we introduce new payment types (PTLCs, ...) in the future ? And ensure old-clients recognize newer payments as $UNKNOWN ?

Yep, good point.

Have you considered other PRF schemes such as SipHash ?

IIRC, SipHash isn't intended/marketed as a cryptographic hash function, I don't think that applies here?

I think we're introducing a new HMAC operation in our inbound payment validation pipeline, which could turn as a DoS vector (though ultimately we're bounded by MAX_ACCEPTED_HTLC * number of channels, at least without option_dusty_htlc_uncounted) ?

Yea, I thought about that somewhat, that's ultimately why AES got replaced with ChaCha here, even though it feels like we should want a block cipher. That said, receiving payments always requires at least two fsync's, so I'm not sure there's almost anything we could do that is slower than that.

what is "the minimum amount field"? is it just the BOLT-11 invoice amount?

basically, yes.

[devrandom]

for MPP, don't we need to store state anyway so we can figure out when the invoice is paid?

[TheBlueMatt]

for MPP, don't we need to store state anyway so we can figure out when the invoice is paid?

Yes, but conveniently we already track the set of MPP parts via ChannelHolder::claimable_htlcs separately from the set of payments we expect to receive in the future (pending_inbound_payments).

[joostjager]

I've been looking through this issue and related PRs, but what I couldn't find documented is the rationale for encrypting the metadata. There seems to be nothing truly sensitive in the metadata and the hmac already takes care of authentication.

What is the reason for adding the additional encryption step?

[TheBlueMatt]

I don't think it's particularly sensitive, no, but it's almost free to do, and feels better than not? If a user tweaks the amount or CLTV setting and for some reason changes them without changing the invoice it's nice to protect it? It's also nice to hide internal implementation details of the user stored in the first bit of the metadata, though IIRC there's some timing attacks which could reveal it.

[joostjager]

If a user tweaks the amount or CLTV setting and for some reason changes them without changing the invoice it's nice to protect it?

I thought this is taken care of already via the hmac on the metadata? The htlc won't be accepted if the user tampered with any of the fields because the derived payment hash no longer checks out.

Other than that, I can sort of go along with the 'feel good' and 'nice' parts.

[TheBlueMatt]

I thought this is taken care of already via the hmac on the metadata? The htlc won't be accepted if the user tampered with any of the fields because the derived payment hash no longer checks out.

No I meant if someone changes the values and doesn't tell their counterparty with some intent to have their counterparty pay the amount in the invoice. It's a bit esoteric and the sender could always probe, but still.

It just seems like we should always encrypt data that contains implementation detail or policy information about our user, though your right it's not clear that it's important.