nostr-protocol / nips

Nostr Implementation Possibilities
2.19k stars 519 forks source link

nip-46 nsec bunker implementations deviate from the current NIP #1138

Open mikedilger opened 3 months ago

mikedilger commented 3 months ago

At first @fiatjaf noticed gossip-as-bunker didn't work with nak. Then @bu5hm4nn noticed Coracle didn't work with gossip-as-bunker. Then I noticed habla didn't work as gossip-with-bunker.

There are two points where gossip followed the NIP and everybody else didn't (or they followed a previous NIP). I think everybody else should change (of course!) because if left as is there are some problems I'm about to point out.

POINT 1: The first parameter of the "connect" command is the public key of the bunker's private key you want to sign with, not the ephemeral public key of the client. It makes sense to me, so that the bunker knows which private key you wanted to connect with. Seems everybody has put in the wrong public key. Gossip-as-bunker actually checks this param and fails (I could just ignore it, but I'm trying to be correct). Using the key you are trying to connect to is helpful for bunkers that hold many private keys, especially if they don't use a unique secret.

POINT 2: The second parameter is an optional secret. I don't think it should be optional, but I can punt on that issue. The current issue is that it is optional at the discretion of the bunker, not the client. The bunker creates the bunker:// url and if it has a secret, then the client MUST provide that secret or connection will not happen. Because otherwise someone can guess the connect URL and connect to bunkers they were not authorized to connect to.

Can we get @pablof7z @verbiricha (habla) and others involved to help hammer out these incompatibilities?

pablof7z commented 3 months ago

At first @fiatjaf noticed gossip-as-bunker didn't work with nak. Then @bu5hm4nn noticed Coracle didn't work with gossip-as-bunker. Then I noticed habla didn't work as gossip-with-bunker.

There are two points where gossip followed the NIP and everybody else didn't (or they followed a previous NIP). I think everybody else should change (of course!) because if left as is there are some problems I'm about to point out.

POINT 1: The first parameter of the "connect" command is the public key of the bunker's private key you want to sign with, not the ephemeral public key of the client. It makes sense to me, so that the bunker knows which private key you wanted to connect with. Seems everybody has put in the wrong public key. Gossip-as-bunker actually checks this param and fails (I could just ignore it, but I'm trying to be correct). Using the key you are trying to connect to is helpful for bunkers that hold many private keys, especially if they don't use a unique secret.

The first param should be the pubkey you want to connect as, so the remote key. I agree.

On NDK's backend I removed that check because there's a mix of implementations; some send the local key (per the old NIP iirc), some send the remote key (per the new NIP)

NDK as a signer sends the remote key, per what gossip and other signers expect.

NsecBunker, which uses NDK backend, listens for requests p-tagging the remote key, so the pubkey on the first param of connect is irrelevant: both remote and local pubkeys are known.

That's the long winded way of saying, I agree with @mikedilger ; there's no point in specifying the local key (is who signed the connect event!) but there could be a use in having the remote intended key for when the bunker runs with a different pubkey than the intended/remote pubkey.

(I wrote pubkey so many times even I am confused 😂)

POINT 2: The second parameter is an optional secret. I don't think it should be optional, but I can punt on that issue. The current issue is that it is optional at the discretion of the bunker, not the client. The bunker creates the bunker:// url and if it has a secret, then the client MUST provide that secret or connection will not happen. Because otherwise someone can guess the connect URL and connect to bunkers they were not authorized to connect to.

I don't see a need to make this a MUST, particularly within the context of the oauth-like flow the permissions are assigned without any need to previously contact the bunker.

Of course, if the bunker generated a secret and the user provides it to the client it would be dumb to not use it on the connect but not sure if something so obvious needs to be specified anywhere 😂

Can we get @pablof7z @verbiricha (habla) and others involved to help hammer out these incompatibilities?

Cc @v0l

bu5hm4nn commented 3 months ago

Regarding the spec and passing the pubkey as param[0]:

Since that parameter is not adding any new information to the handshake it is unnecessary and strictly speaking should be removed from the spec. Why? Because we already know the NIP46 client pubkey as it is the author of the request, and we also know the target pubkey on the NIP46 server since it is tagged in the 'p' tag, and the content has been encrypted with the target pubkey. If the client wanted to register for a different key, it would have tagged a different pubkey, right?

The terms "local" and "remote" are not specific, they can mean different things depending on which end of the software you are currently working on. We should use very clear terms like "NIP46 signer" and "NIP46 client" or something like that, and make sure the use is consistent with the glossary.

mikedilger commented 3 months ago

Since that parameter is not adding any new information to the handshake it is unnecessary and strictly speaking should be removed from the spec

I disagree. It is only redundant if the bunker's key and the key that the bunker is protecting are the same key, as is the case with gossip. For a bunker that manages multiple secret keys, you'll need to connect to the bunker via it's key and then separately specify which secret key it manages that you wanted to use.