lnurl / luds

lnurl specifications
584 stars 138 forks source link

LNURL-pay: Drop metadata description hash validation #234

Open callebtc opened 11 months ago

callebtc commented 11 months ago

This PR removes the description_hashrequirement in LUD06 and LUD18.

Background

LNURL-pay requires the SERVER to provide an invoice with a specific description_hash (metadata in LUD06, and metadata+payerData in LUD18). WALLET is then supposed to verify that the invoice it receives indeed has this description_hash.

Rationale

If I understand correctly, the original intention of this is to make the server commit to the receiver (or receiver+payer) identities. Practically, this doesn't really improve anything: If there was a MITM changing the invoice, they could do so since neither of the data committed to is secret. If the server itself is malicious, they could fake everything as they wish anyway.

Implementing this (possibly redundant) feature is also the biggest challenge when implementing LNURL-pay.

This will also once and for all solve all the issues we have had for years dealing with CLN's requirement to provide the entire description at invoice creation.

I propose to drop this requirement, starting with removing the check WALLET performs on the invoice received in the second LNURL-p response. SERVER's who don't want to upgrade, can remain as is.

Progress

Supported

Unclear

(please report PRs or test results for more wallets)

Kukks commented 11 months ago

ACK from my end. Less hassle dealing with LN implementation support and much easier to build some more complex flows.

Egge21M commented 11 months ago

ACK. Current Wallet, in foresight of this PR, never implemented this check.

kaloudis commented 11 months ago

ACK. Will support this change in Zeus.

bumi commented 11 months ago

ACK this makes it easier for wallet developers and it makes it also easier for services to offer LNURL/lightning address with different node backends.

Alby will support this in the next release.

arcbtc commented 11 months ago

Awesome, description_hash got in the way a bunch

callebtc commented 10 months ago

Looks like there is broad support for this PR! Anything that should be considered before it can be merged?

dni commented 10 months ago

:+1:

fiatjaf commented 10 months ago

My only worry is that services will drop support for this too soon and start breaking wallets that haven't yet.

dpad85 commented 9 months ago

ACK, committing the lnurl-pay metadata to the invoice does not bring additional security AFAICT. The service is trusted anyway. Next Phoenix release will remove this check.

callebtc commented 8 months ago

Can we merge this or are there any blocking issues @fiatjaf?

hsjoberg commented 8 months ago

Is there are any value to commit LUD-18 data to the invoice? This proposal removes desc hashing altogether.

NCrashed commented 5 months ago

I would like to offer some critical feedback on the proposed change, particularly focusing on a crucial aspect of the lud06: 9. LN WALLET pays the invoice, no additional user confirmation is required at this point. The absence of a description hash eliminates the connection between the invoice and the user's explicit agreement, leaving the user without any evidence of discrepancies or potential misconduct by the server. A practical application of metadata commitment, from my experience, involves embedding exchange order numbers within this field, underscoring its significance. This issue becomes even more critical with payerData in lud18, as this data originates from the client, not the server.

In summary:

As an alternative, I suggest making the description hash optional and introducing an additional step for user confirmation when the commitment is absent.

evd0kim commented 5 months ago

NACK. I do not see a single valid argument here.

Joining to @NCrashed.

I don't care about imaginary services and wallets that would benefit from doing less work. The developer's job is to ensure that the digest corresponds to the received data.

The current protocol ensures that the previous round of communication corresponds to the next one, and the wallet checks for the user in the background that THIS invoice is for THAT menu inputs + information.

The thesis about trusted/non-trusted service is entirely irrelevant here. If meta contained some important info there is no basis for building trust for random user-service as well as no hash to prove misbehaving or indicating for the user that the service showed one info and asked to pay for another.

MITM changing the invoice, they could do so since neither of the data committed to is secret.

The check is not related to invoices but rather to any data put into Metadata.

If the server itself is malicious, they could fake everything as they wish anyway.

Yes. If the service wants to receive payment and doesn't provide any services or goods, the protocol is irrelevant, but this change enables a whole spectrum of situations when the service may attempt to cheat just a little bit. For example, let's consider the original purpose of the LNURL pay protocol. Selling channel liquidity. The service may provide in Metadata specific parameters about channel fees, channel size and active period, and node pubkey. It receives the payment for the service and opens the channel of size not 1.5M but 1.499M, fees not 1000 ppm but 1001 ppm. Now good luck catching such dishonest service in every UI of every wallet you meant while saying "it will make everything much easier". Instead, Metadata could be shown once, and description_hash could be checked in the background.

callebtc commented 3 months ago

The absence of a description hash eliminates the connection between the invoice and the user's explicit agreement

Could you be specific? What does this prevent in your view? If you could make an explicit example of how this improves a payment flow or how it could be abused, it might be easier to understand what you mean.

In my view, if I offer you an invoice, you can choose to pay it or not, that is the only necessary expression of your agreement. The proof of payment is the preimage you'll receive.

What else other than authenticated transport (https) do you need to be sure that this particular invoice is from the server you've actually asked the invoice from? And what misconduct does the payment hash prevent?

callebtc commented 3 months ago

The service may provide in Metadata specific parameters about channel fees, channel size and active period, and node pubkey.

This is a good example of a case that almost nobody cares about but it could be absolutely implemented even if the requirement would be dropped. If you run such a service, put the terms of the contract into the description hash and make your user read the contract and explicitly agree to it. That's the only way to make sure that both parties explicitly agree.

I'm guessing that 99.99% of lnurl payments are without any explicit commitments that the user cares about, they just want to pay. I haven't observed a single case where a dispute was resolved by comparing descriptions and hashes. For those cases where it's relevant: just do it.

evd0kim commented 3 months ago

Could you be specific? What does this prevent in your view? If you could make an explicit example of how this improves a payment flow or how it could be abused, it might be easier to understand what you mean.

I provided an example.

In my view, if I offer you an invoice, you can choose to pay it or not, that is the only necessary expression of your agreement. The proof of payment is the preimage you'll receive.

Checking a hash against a description that is stored or provided off-band may be more relevant for automated payments. It may be important for the client side, too.

What else other than authenticated transport (https) do you need to be sure that this particular invoice is from the server you've actually asked the invoice from? And what misconduct does the payment hash prevent?

The point being made is not about faking the server. The point being made is about the server that provides some imprecise/non-deterministic data.

If you run such a service, put the terms of the contract into the description hash and make your user read the contract and explicitly agree to it. That's the only way to make sure that both parties explicitly agree.

Not relevant at all for machine-machine interfaces.

I haven't observed a single case where a dispute was resolved by comparing descriptions and hashes.

It wouldn't happen with the former rules, even if it were the case. There would not be any dispute at all.