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

NIP-47 Optional Secret #792

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TonyGiorgio
Copy link

I think secret can be optional here.

Reasoning / Use case:

This can be used for more than just traditional nostr clients. A service provider may have their own secured private key or HSM to sign events with that they prefer to use when requesting money from a user's lightning wallet. This would be more secure than having the user copy paste private keys that they are allowing to withdraw money.

On the wallet side, the UX would be whenever a NWC profile is being created, the wallet should allow the user to paste the public key of the service provider or auto generate one.

Existing nostr clients may not want to change anything but it would add some benefits with more lightning wallet integrations.

CC other NWC peeps: @benthecarman @bumi @rolznz

@benthecarman
Copy link
Contributor

benthecarman commented Sep 23, 2023

Concept ACK

I would like to see a protocol to negotiate secrets for NWC instead of relying on just copying pasting between both. This would be a good first step.

Maybe the protocol can just be if the service receives the NWC with no secret, it generates its keys and it send its public key as an encrypted dm to the public key in the nwc string.

@rolznz
Copy link

rolznz commented Sep 24, 2023

On the wallet side, the UX would be whenever a NWC profile is being created, the wallet should allow the user to paste the public key of the service provider or auto generate one.

We have also added a flow to our NWC app allowing the nostr client to generate the secret, and only passing the public key to the wallet service. This way there is no transfer of the secret and is more secure. I believe this PR covers this usecase, but it would be nice to add it to the NIP explicitly, what do you think?

The client MAY generate a 32-byte random secret and store it and uses this secret when it wants to perform an action. The corresponding public key must be passed to the wallet service to allow it to authorize the client. The client MUST only use this secret if the wallet service does not return a different secret to the client.

@TonyGiorgio
Copy link
Author

Wording sounds good to me.

only passing the public key to the wallet service

Do you do this in any specific way? There's query param stuff im sure we can both do but is there any other way you're thinking about?

@bumi
Copy link

bumi commented Sep 24, 2023

+1 for this change. And as @rolznz says I also like the flow where the client is generating the secret and only the public key is exchanged with the NWC application. This makes the flow quite flexible. It can be easily initiated from the client and does not require the NWC app to communicate back a secret.

Passing the pubkey is done through query params to the URL where the user has to approve the connection: https://github.com/getAlby/nostr-wallet-connect/#client-created-secret

Because the client would need to know the anyway URL upfront am not sure if this can/should be part of a spec?
but most relevant query params would be:
pubkey - public key of the client's secret for the user to authorize
return_to - (optional) if a return_to URL is provided the user will be redirected to that URL after authorization. The lud16, relay and pubkey query parameters will be added to the URL.
c - name of the client app

With a spec for these query params it would be probably possible to simply ask the user for their NWC domain in the client and then initiate the connection flow. 🤔
I think activitypub/mastodon is doing something like this for their follow-flow? 🤔

@TonyGiorgio
Copy link
Author

We're doing a few query params ourselves, looks similar but using different names. I couldn't think of a better way to do it that is still easy but isn't dependent upon hardcoded links / buttons. I think if there was a standard way to pass parameters to wallets like you have here, then it could support a user pasting in a domain.

I'll change out the text with what was mentioned above when I get to my computer.

But what would be the best way to document the params? Is that NIP worthy? Or on the wallet side with a BLIP? Yours seem like a good starting off point.

@TonyGiorgio
Copy link
Author

On the wallet side, the UX would be whenever a NWC profile is being created, the wallet should allow the user to paste the public key of the service provider or auto generate one.

We have also added a flow to our NWC app allowing the nostr client to generate the secret, and only passing the public key to the wallet service. This way there is no transfer of the secret and is more secure. I believe this PR covers this usecase, but it would be nice to add it to the NIP explicitly, what do you think?

The client MAY generate a 32-byte random secret and store it and uses this secret when it wants to perform an action. The corresponding public key must be passed to the wallet service to allow it to authorize the client. The client MUST only use this secret if the wallet service does not return a different secret to the client.

Added your text to this PR.

@rolznz
Copy link

rolznz commented Sep 26, 2023

We're doing a few query params ourselves, looks similar but using different names. I couldn't think of a better way to do it that is still easy but isn't dependent upon hardcoded links / buttons. I think if there was a standard way to pass parameters to wallets like you have here, then it could support a user pasting in a domain.

I'll change out the text with what was mentioned above when I get to my computer.

But what would be the best way to document the params? Is that NIP worthy? Or on the wallet side with a BLIP? Yours seem like a good starting off point.

In my opinion this would be simpler for apps to implement and more convenient for more users who use different wallets/NWC apps. Unfortunately it is not just the params in our case (it's also a specific path /apps/new) to setup the connection, however if whatever is decided upon is different it should be fine to setup a simple redirect (for example something like a .well-known endpoint like in https://github.com/lnurl/luds/blob/luds/16.md eg. https://example.com/.well-known/nip47 ?).

What about an optional appendix to the bottom of this NIP (similar to NIP-57) that covers interaction with the wallet service through a browser and defines a suggested URL and query parameters that MAY be supported?

@ok300
Copy link
Contributor

ok300 commented Oct 1, 2023

If NWC treats secret as optional then apps who want to integrate NWC are faced with this choice:

  • generate / negociate a new secret, then store it per NWC connection
  • "keep it simple" and re-use a single secret
  • "keep it simple ++" and re-use the client's private key (it's secret as well, so what can go wrong)
    • this will also look appealing because most NWC events are almost identical to NIP-04 DMs, which already use the client sk

IMO the path of least resistence will be the last one.

For the users of such apps:

  • all their NWC interactions can now be linked to their main pubkey
  • if they ever lose their private key:
    • all their past NWC interactions can be decrypted (surely some relays will silently store the "ephemeral" events)
    • same for all their current and future NWC activity
    • whoever gets that secret has full access to spend from all NWC wallets that user configured in that app

So IMO one big downside of this proposal is, it will make it easy for app developers to make such mistakes.

@TonyGiorgio
Copy link
Author

We can add text here saying that nostr clients must not use the user's private keys for the secret.

@ok300
Copy link
Contributor

ok300 commented Oct 1, 2023

IMO this is weakening the security guarantees of all NWC wallets and endpoints.

As to what the change would enable: service providers can already be limited in how much access they have to someone's LN wallet: NWC has provisions for budget, rate limiting, max amount, expiration date, etc. So a user could create a customer tailored NWC connection string per service provider, each with its own limits.

@TonyGiorgio
Copy link
Author

That can be your opinion but you're wrong if you think copy and pasting private keys is better security.

@ok300
Copy link
Contributor

ok300 commented Oct 1, 2023

By this logic, a password manager is less secure because it needs you to copy-paste passwords.

But what use is a password manager, when we can add some text on each website to tell the user they must never reuse a password. That'll work.

@ok300
Copy link
Contributor

ok300 commented Oct 1, 2023

IMO the scenario you're talking about is better solved by event delegation.

Just like NIP-26 allows someone to delegate the signing of certain events to another key, why wouldn't a a similar solution work here?

For example, the delegation could "link" a new secret, known only by the service provider (delegatee), to the original NWC secret by an event signed by account owner (delegator).

Just lke now relays and clients who support NIP-26 treat an event signed by the delegatee as if it came from the delegator, a similar NIP could make it such that NWC endpoints treat NWC events signed by the delegatee key (HSM, etc) as if it was signed by the original NWC secret.

What do you think? Would that work?

@TonyGiorgio
Copy link
Author

And how does that solve your hypothetical nostr dev using user private in ways they shouldn't? They'd just turn around and have the delegated key be the user key anyways.

And this hypitical dev that doesn't read that they should not use user private keys is going to be someone capable of implementing event delegation?

These are financial tools we are talking about here. If a dev isn't competent enough to read a NIP dealing with money and security then they shouldn't be implementing it.

But either way it's not rocket science. Nothing changes for nostr clients, this is a service provider use case.

@ok300
Copy link
Contributor

ok300 commented Oct 1, 2023

Well, with NIP-47 as it is now, an incompetent / malicious nostr dev cannot get NWC to work by re-using secrets, even if he wanted to. The NIP doesn't allow it by design.

With this change, an incompetent / malicious dev finds it easier to re-use secrets, than to implement it properly. The NIP will have weakened its security guarantees.

This is btw not possible with the delegation approach, because the service provider's sk is never exposed. And the nostr dev has to sign the delegation event properly, or it won't work.

@rolznz
Copy link

rolznz commented Oct 2, 2023

If an incompetent / malicious dev has access to your nostr private key, this is all irrelevant. So I don't think it's a good argument.

@ok300
Copy link
Contributor

ok300 commented Oct 2, 2023

You're missing the point.

A) Do step 1, 2, 3, then it works. Any mistake and it fails.

vs

B) Do step 1, 2, and optionally do 3, but please make sure to do it this way. NWC will work even if you make mistakes, but be careful!

You don't have to be malicious or incompetent to make mistakes in B), of which the easiest is to reuse secrets.

Its the weakened design that allows for mistakes.

@TonyGiorgio
Copy link
Author

I'm not sure how you read "generate random secret" and interpret it as "use your user's private key" unless you're someone that shouldn't be a developer dealing with private keys of ANY kind in the first place. You're also still expecting this hypothetically moronic dev to store financial private keys properly too.

@ok300
Copy link
Contributor

ok300 commented Oct 2, 2023

Just a couple mistakes that are now possible, and NWC will still work:

  • using a constant instead of a random secret (saves time, easier to impl)
  • re-using the NWC secret across NWC connections
  • building the NIP-04-like NWC messages the wrong way (calling nip04_encrypt(sk, pk, msg) without remembering to use the different, separate sk per NWC connection), which inadvertendly uses the user's sk

The question is, what for? To allow some sevice provider to maybe use an HSM? Well, key delegation sounds better for that. It would not weaken the existing NIP and even keep secrets out of the new interaction by design.

Looks like a bad tradeoff.

@rolznz
Copy link

rolznz commented Oct 26, 2023

@TonyGiorgio @bumi @benthecarman I see one inconsistency so far in https://www.zapplepay.com/autozap/ (we use c for the client/app name, mutiny uses name. I think name is better. Should Alby's nostr-wallet-connect be updated to support both, but recommend using name)?

@TonyGiorgio
Copy link
Author

@TonyGiorgio @bumi @benthecarman I see one inconsistency so far in https://www.zapplepay.com/autozap/ (we use c for the client/app name, mutiny uses name. I think name is better. Should Alby's nostr-wallet-connect be updated to support both, but recommend using name)?

Ah, we tried to use most of the params you were also using, I think that one slipped because we were already using name.

Though the whole query parameter / callback stuff might be worthy of a different PR or something trying to standardize that? This one is mostly around the NWC secret.

Though on that note, we've had some ideas around callbacks that you might be interested in as well. Something more nostr native that should work across any device/app/web/extension compatibility matrix. Basically an inverse NWC flow for receiving the NWC string from the wallet. We've been running into weird callback hacks we've had to do, even going from native android app back to the requesting website, which might be a more common interaction in the future.

@rolznz
Copy link

rolznz commented Oct 28, 2023

@TonyGiorgio that sounds good to me. Please let us know if you have any updates regarding the other ideas.

@bumi
Copy link

bumi commented Oct 28, 2023

+1 for the name parameter. I don't even know why I have chosen that short c param back then.

I am interested in discussing an inverse flow which potentially could also remove the need to standardize some of these query params and could also provide an answer how the app knows it is "connected". I've also been thinking about other options there. (though I think the wallet should be able to decide on the relay)

@benthecarman
Copy link
Contributor

benthecarman commented Oct 29, 2023

Instead of custom urls for every wallet I think this would be a better approach and solves the underlying problem with added functionality

#851

Copy link
Collaborator

@Semisol Semisol left a comment

Choose a reason for hiding this comment

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

#851 would fix this. NWC URIs are supposed to not require any interaction except creating the connection string and pasting it into the app.

@TonyGiorgio
Copy link
Author

Definitely in favor of #851 in general, but does that define a way to allow the NIP-47 string to be optional or allow coordination on who sets it? I do agree having a seamless flow for it without additional interactivity would be preferred.

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

Successfully merging this pull request may close these issues.

None yet

6 participants