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

payRequest multi-step overcomplicated #133

Open
TheBlueMatt opened this issue Nov 26, 2021 · 10 comments
Open

payRequest multi-step overcomplicated #133

TheBlueMatt opened this issue Nov 26, 2021 · 10 comments

Comments

@TheBlueMatt
Copy link

I went to implement LNURL-16 figuring it would be quite simple (ie request-invoice - get-invoice) but instead it requires a few hoops without any motivation for why. Specifically, the multiple requests to get an invoice may be fine, but multiple steps with server-side state required is a bunch of complication (not to mention DoS risk). Worse, using description_hash instead of description makes LNURL-payRequest/LNURL-16 harder to integrate with some standard tooling, and there doesn't seem to be any motivation listed for why the description_hash needs to match the first request (there doesn't appear to be any security motivation for it?) and without the metadata being passed back to the server.

Can we define an LNURL-21 that is just GET /.well-known/lnurlq/ that just returns an invoice. No JSON, no complexity, no server-side state, just an invoice? Or maybe remove the description_hash-matching requirement from the LNURL-payRequest requirements?

@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 26, 2021

I don't think this is overcomplicated at all and the server doesn't have to store any state. Look at this pseudocode for a request handler at /.well-known/lnurlp/:username:

handle (request) {
  metadata = json([["text/plain", "Pay {request.path.username}"], ["text/identifier", "{request.path.username}@{request.hostname}"]])

  if (request.query.amount) {
    invoice = makeInvoice(amount=amount, description_hash=sha256(metadata))
    return json({
      pr: invoice,
      routes: [],
    })
  } else {
    return json({
      tag: 'payRequest',
      callback: request.url,
      minSendable: 1000,
      maxSendable: 10000,
      metadata: metadata,
    })
  }
}

@TheBlueMatt
Copy link
Author

This isn't an explanation for why it works the way it does, only one possible way to implement it (assuming the local invoice-generation API supports description_hash).

@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 26, 2021

It's a response to your claims that the server must store state between requests.

The hash checking is required so the user doesn't see a description that says they are buying a car and then on the next step their wallet blindly pays a signed invoice from the service with a description that says "a bicicle".

@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 26, 2021

The choice of description_hash comes from reading BOLT11. BOLT11 defines a special field for encoding a hash of a description bigger than what would fit in an invoice, and it says the transport method for the data before hashing was not defined in the BOLTs.

Since lnurl-pay was the perfect match for that because it defined the transport already, we decided to use it. But if you think that was a ridiculously stupid idea then I would just forward that criticism to the BOLT11 authors. Why has it defined such a field? I don't know either.

To me it just feels safer to use a hash on a place dedicated to hashes instead of what?, a hex-encoded hash in a place dedicated to human-readable descriptions?

@TheBlueMatt
Copy link
Author

TheBlueMatt commented Nov 26, 2021

It's a response to your claims that the server must store state between requests.

Fair, I suppose if you put the state back in the URL for the second request you get it back and the server can verify it and re-use it. Its still a level of indirection that isn't explained in the docs at all, and requires the server to verify whatever state came back from the client in the second request.

I'm still confused as to why this multi-request design is required.

The hash checking is required so the user doesn't see a description that says they are buying a car and then on the next step their wallet blindly pays a signed invoice from the service with a description that says "a bicicle".

This doesn't track. The point of descriptions in BOLT 11 is because the payment request is authenticated by the node signature. In LNURL there isn't (really) an authenticated payment request at all There is no requirement in LNURL that the invoice be signed by any given node, and an LNURL endpoint can return an invoice signed by a random one-off pubkey (i.e. an attacker can provide an invoice from their node) and services today generate invoices against many different nodes. The standard BOLT 11 proof-of-payment design no longer applies, making the description somewhat redundant.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 26, 2021

I agree the proof-of-payment scheme is not working as designed, but I don't know if we have all collectively agreed to throw it into the trash already. In many cases it's still possible with some manual work it's still possible to prove after the fact that a specific service was responsible for signing a given bolt11 invoice. And there are ways for making that process more seamless and automatic without having to change anything in the architecture of current services, although these improvements would be independent from LNURL.

In the case of an attacker they would have to be in control of the domain of the service the user is interacting with and that already breaks the basic trust assumption of LNURL and they can probably do more harmful things.

But the hash checking still prevents a service from sending the wrong invoice due to a bug, for example. And the preimage verification has worked in the real world many times to clarify misunderstandings between customer and service provider. I've seen it happen a dozen of times, so it's in the interest of service providers to issue valid invoices from stable pubkeys (although I realize this is not very relevant in the specific case of LNURL, or is it?).

Is there any other alternative for payment proofs?

@TheBlueMatt
Copy link
Author

TheBlueMatt commented Nov 27, 2021

I agree the proof-of-payment scheme is not working as designed, but I don't know if we have all collectively agreed to throw it into the trash already

Oh, don't get me wrong, I'm not claiming at all that proof-of-payment is something we should give up on, only that in an HTTPS-authenticated environment its not possible. That's fine, plenty of users will use HTTPS-authenticated environments, and invoice authentication isn't possible in that environment, there's just not anything we can do about that.

We can chat about ways to make the node public key signing the invoice committed to by a URL (ie have the domain commit to one by putting it in a DNS TXT record authenticated by DNSSEC - a functioning client can get a full authenticity proof from the DNS), but today it doesn't exist.

In the case of an attacker they would have to be in control of the domain of the service the user is interacting with and that already breaks the basic trust assumption of LNURL and they can probably do more harmful things.

Yes, my point is that the trust model of LNURL is HTTPS CAs (personally I'd prefer if DANE were required, but that ship may have sailed). There is nothing tying the BOLT11 invoice to HTTPS, and I don't think there's much hope of changing that. Relying on the BOLT11 invoice to contain authenticated data doesn't add anything given the trust model. In general, LNURL docs seem to be lacking on motivation sections - ie where is the specified trust model that implementors need to consider and that provides guarantees for users?

But the hash checking still prevents a service from sending the wrong invoice due to a bug, for example.

You could also avoid that by making it one request instead of two :p.

And the preimage verification has worked in the real world many times to clarify misunderstandings between customer and service provider. I've seen it happen a dozen of times, so it's in the interest of service providers to issue valid invoices from stable pubkeys (although I realize this is not very relevant in the specific case of LNURL, or is it?).

This seems unrelated to the invoice description? Payment preimage exchange indeed can be useful and service providers hopefully know the set of public keys from nodes they use, though some service providers (eg strike) use many nodes today already and seem to have no issues with it.

All of this said, I'm not sure how any of this is connected to payment description in invoices in LNURL. If we move to requiring DNSSEC-authenticated node ids to sign the BOLT11 invoices we can still just put the description in the invoice and sign it - same amount of data as the current first response, just avoids the second response (and JSON in between). There's no particular size limits to BOLT11 invoices - the only restrictions come from putting them in QR codes.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 27, 2021

Ok, I agree with you on everything. My only remaining points are:

  1. The fact that the hash checking is useless today (except for preventing bugs, and preventing bugs is important, specially because all lightning-powered services are so crappy) doesn't mean we should abandon it. It is there already, it is simple enough, and I have hope we will have some standard for tying domain names to node pubkeys in the near future, and then the hash checking will become powerful (it will also fix the non-LNURL invoice passing non-verifiability craze that exists today, so we would have to do it even if LNURL didn't exist).
  2. Most of your suggestions seems sensible, but I don't think any of them is worth breaking all the compatibility between all wallets and services. I also have many things I wish were different, but decisions were made in the past in different circumstances and that's what we have. If there is any improvement that can be made in a backwards-compatible way, then we should probably do it, otherwise it's probably better to make a new protocol from scratch (hopefully with some lessons learned from this).
  3. Adding DANE support is probably implementable in a backwards-compatible way. Some people suggested it on Consider DANE as an alternative to Certificate Authority andrerfneves/lightning-address#15 and I'm very interested, although I know nothing about it and didn't have time to research yet.
  4. Having huge descriptions would be cool, since the invoices are sent over HTTPS anyway, but then we would run into the same tooling issues you have with description_hash (although I think the description_hash support is better than you think). Node APIs and wallets will often reject big descriptions, refuse to create invoices with large descriptions etc (even though the protocol says there is no limit). Probably ideally we would encode the full invoice in JSON directly instead of in bech32 then JSON, as the bech32 part is unnecessarily wasteful -- but then, again, that would be even worse on the tooling side.
  5. We debated having two steps and one step when coming up with the lnurl-pay protocol. There were pros and cons on both sides. Not sure if the discussion is on a GitHub issue or buried in the Telegram group, but two pros on the two-step side I can see now are: (a) flexibility on the initial params (for example, the initial amount ranges can change as time passes, instead of being hardcoded in the printed QR); (b) potentially side-effect-free first step, allowing an LNURL endpoint to be queried for free instead of having to generate an invoice each time (relevant because most services use nodes that keep a database of issued invoices, instead of a stateless invoice scheme that would be ideal, so adding a confirmation step helps with that).

@TheBlueMatt
Copy link
Author

Most of your suggestions seems sensible, but I don't think any of them is worth breaking all the compatibility between all wallets and services. I also have many things I wish were different, but decisions were made in the past in different circumstances and that's what we have. If there is any improvement that can be made in a backwards-compatible way, then we should probably do it, otherwise it's probably better to make a new protocol from scratch (hopefully with some lessons learned from this).

That's fair, though given how early things are, I wonder how much effort (on the sending side) it would really be to have a lnaddress-minified option at least for lightning-address. Simple request, gets an invoice, user can pay or not. Adding support on the sending side is significantly more trivial than any other existing implementation, and it would avoid new receivers having to be as complicated (given the relatively weak support for lnaddress-receive, providing authenticated invoices instead of HTTPS-secure-connections may enable better uptake, though that may be better off putting the node id in the QR code than relying on DNSSEC or similar systems).

@hsjoberg
Copy link
Collaborator

hsjoberg commented Nov 27, 2021

That's fair, though given how early things are, I wonder how much effort (on the sending side) it would really be to have a lnaddress-minified option at least for lightning-address

@TheBlueMatt Are we talking about LNURL-pay or Lightning Address?

Lightning Address is not a payment protocol, it 100% reliant on LNURL-pay (LUD-06), which is the actual payment method.
Doing some kind of simplified LNURL-pay makes very little sense to me, it's already an established standard.

However, breaking out Lightning Address to be able to support multiple payment methods (raw bolt11, bolt12/bolt12address, amp invoice, whatever) is something I've been positive about in the past and could be a good idea to continue pursuing.
It has its own slew of issues however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants