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

New version of NIP-46 #1047

Merged
merged 7 commits into from
Feb 20, 2024
Merged

New version of NIP-46 #1047

merged 7 commits into from
Feb 20, 2024

Conversation

erskingardner
Copy link
Contributor

@erskingardner erskingardner commented Feb 14, 2024

I recently implemented the current version of NIP-46 in nostr-ignition and found it very painful. After talking to several other folks that have also implemented this NIP and found it painful, I decided to try and improve the NIP to make it more likely that remote signing is adopted by more of Nostr.

My proposal changes NIP-46 significantly based on a few principles.

  • Use separate event kinds instead of JSON-RPC style payloads with methods.
  • Simplify payloads as much as possible, only using objects or arrays where absolutely necessary.
  • Use NIP-44 encryption for payloads instead of NIP-04

I've talked to quite a few folks over the last few weeks on this so thanks for all the input @fiatjaf, @pablof7z, @staab, @nostrband, and others.

I have a shed, it is ready for your bikes. 🚴‍♂️

@erskingardner
Copy link
Contributor Author

erskingardner commented Feb 14, 2024

Tagging folks I know of who have touched/implemented NIP-46 so that I can get your feedback.

@v0l
@staab
@monlovesmango
@hzrd149
@vitorpamplona
@alexgleason
@reyamir
@mikedilger

46.md Show resolved Hide resolved
@nostrband
Copy link
Collaborator

I don't like separate kinds for each method, relays are much more empowered to monitor/censor nip46 activity. I think making app request payload an array (many already are in your request kinds) and adding the method as the first element solves this and doesn't make this nip much more json-ish.

@erskingardner
Copy link
Contributor Author

erskingardner commented Feb 14, 2024

I don't like separate kinds for each method, relays are much more empowered to monitor/censor nip46 activity

I don't think censoring is an issue. It's trivial to have both parties on multiple relays, the chances of collusion across the entire set of relays is minimum. Also, for privacy sake, I imagine a lot of remote signers will want to run their own relays, further reducing this as an issue.

@vitorpamplona
Copy link
Collaborator

+1

I am feeling a convergence between DVMs and NostrConnect services.

JSON-RPC indeed sucks. It's like creating separate poorly designed little protocols everywhere. But adding stringified jsons in the .content doesn't seem that much better.

NIP-44 encryption is good.

Do we think that exposing kinds for each command could be a privacy issue? With these new kinds, a third-party entity connected to the relay can track how many signs, encrypts, and decrypts each key does.

A way to hide all of the actual info could be done using a single Ephemeral GiftWrap that nip44 wraps any of the other request/response kinds which can then be in plain text (No need for seals here). Then the proposed .content json stringified fields could just be regular tags. Trackers could see wraps going back and forth but they wouldn't necessarily know what they are. In fact, no one would know the keys are even talking to a signer service.

Now, this whole RPC ordeal makes me wonder if we shouldn't just specify a standard HTTP Services API for Nostr that simply takes an HTTP POST with a Nostr event and replies with another Nostr Event signed by the service. One entry point, one reply. Signers could just expose a Services Definition Event in Nostr pointing to their service URL.

Anyway. Forward we go.

Copy link
Member

@alexgleason alexgleason left a comment

Choose a reason for hiding this comment

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

This is so much better than the original. Thank you for doing this!

46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
@erskingardner
Copy link
Contributor Author

Do we think that exposing kinds for each command could be a privacy issue? With these new kinds, a third-party entity connected to the relay can track how many signs, encrypts, and decrypts each key does.

yeah, I thought about this a bunch and went back and forth. I agree there is a small amount of metadata leakage here but I think it's minimal to the point of not being an issue.

I actually initially wrote a version of this with gift wrapping and using tags like you'd suggested but the net effect was that it felt more bulky/clunky for the small number of a calls that require more than just a simple string for the content. In the end I went with the lighter, slightly leaky version.

@staab
Copy link
Member

staab commented Feb 14, 2024

+1 on minimizing metadata leakage, I hadn't thought of that. Everything we can do to keep people's stuff private is something worth doing.

a standard HTTP Services API for Nostr that simply takes an HTTP POST

I wouldn't mind such a thing, but I don't think it's the best solution in this case. Ideally, a signer would also be a relay, which means it could advertise itself and clients could send events to the signer directly, removing a hop and enabling cool things like relay proxying, which signers can do without escalating permissions.

@erskingardner
Copy link
Contributor Author

+1 on minimizing metadata leakage, I hadn't thought of that. Everything we can do to keep people's stuff private is something worth doing.

My worry with requiring the gift-wrap stuff is that it adds significant weight to implementing this for a very small privacy improvement. Think about it, even if someone watching can see that you were decrypting or encrypting events or were asking for signatures, what use is that? They can't see to who, or when those messages were actually published (sign doesn't exactly equate to publish).

I just think this is a over-optimization for now. If we have actual reason for concern in the future, it's a simple addition to add here that it's also possible to gift wrap these exact same events. Clients and remote signers would then only have to add one small change to listen for those event kinds and unwrap them on arrival.

Again doable now, but feels unnecessary.

@staab
Copy link
Member

staab commented Feb 14, 2024

Like Vitor said, I don't know if we need to gift wrap them, if we use the same kind for everything, just encrypt the content. I know that brings back the json rpc stuff, but it might actually be ideal. But gift wrap would probably be even better since it would even further reduce metadata leakage.

I don't know what particular attack vector there would be with people snooping, but what I learned working on NIP 44 is that a robust security model assumes all information leaked to an attacker can be used, if only through statistical analysis.

But maybe this is all irrelevant if signers are encouraged to run and recommend their own relays.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 14, 2024

This can only work if everybody agrees to stop using the previous flow on the same day and turn on the new flow. Otherwise it's better to not even try or we will forever have to implement two different protocols.

@nostrband
Copy link
Collaborator

A way to hide all of the actual info could be done using a single Ephemeral GiftWrap that nip44 wraps any of the other request/response kinds which can then be in plain text (No need for seals here). Then the proposed .content json stringified fields could just be regular tags. Trackers could see wraps going back and forth but they wouldn't necessarily know what they are. In fact, no one would know the keys are even talking to a signer service.

Nsec.app runs a server that needs to know who is trying to talk to whom to wake up the service worker in user's browser through web push. That means client will have to supply the info on how to track peers to my server and that defeats the purpose of gift wrapping to hide metadata.

That itself is a pity, but adding even more metadata, publicly available, just bcs we don't like jsons, makes no sense to me.

Now, this whole RPC ordeal makes me wonder if we shouldn't just specify a standard HTTP Services API for Nostr that simply takes an HTTP POST with a Nostr event and replies with another Nostr Event signed by the service. One entry point, one reply. Signers could just expose a Services Definition Event in Nostr pointing to their service URL.

There can't be a service URL with nsec.app, it can only talk through relays, same with Mutiny, so RPC over Nostr is good, but let's not make it worse.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 14, 2024

To be clear on the adds significant weight. The GiftWrap version uses a stack of 3 events, instead of a stack of 2. However, there is a separation of responsibilities between the 3 events that might lead to more interesting private developments.

This PR's version of sign_event

{
  "id": <id>,
  "kind": 24142 // sign event
  "pubkey": <local pubkey of the user>, // visible
  "tags": [["p", <remote pubkey>]], // visible (can't add any extra sensitive params to the call)
  "created_at": <unix timestamp in seconds>, // visible
  "content": nip44Encrypt(JSON.stringify(
    {
      "id": <id>,
      "kind": <kind>,
      "pubkey": <pubkey of the remote signer>,
      "content": "my content",
      "tags": [<any tags>],
      "created_at": <unix timestamp in seconds>,
    }
  )),
}

becomes

{
  "id": <id>,
  "kind": 21059 // new ephemeral gift wrap kind
  "pubkey": <random key>,
  "tags": [["p", <signer pubkey>]],
  "created_at": <random>,
  "content": nip44Encrypt(JSON.stringify(
    {
      "id": <id>,
      "kind": 24142 // sign event
      "pubkey": <local pubkey of the user>, // hidden sender key
      "tags": [], // additional parameters for the sign event command
      "created_at": <unix timestamp in seconds>, // date of the request
      "content": JSON.stringify(
        {
          "id": <id>,
          "kind": <kind>,
          "pubkey": <pubkey of the remote signer>,
          "content": "my content",
          "tags": [<any tags>],
          "created_at": <unix timestamp in seconds>,
        }
      ),
    }
  )),
}

Besides privacy, I do think that moving the RPC params from a JSON inside the .content to actual tags in the appropriate event, that are also encrypted, is beneficial to the "Nostr-nativeness" of this approach.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 14, 2024

Even though I like this new protocol more the coordination costs for migrating are too high and unless everybody who was tagged replies with gigantic enthusiasm I think it is not worth doing.

@alexgleason
Copy link
Member

This can only work if everybody agrees to stop using the previous flow on the same day and turn on the new flow.

That's exactly what would happen. NIP-46 is not precious enough yet. Now is exactly the right time to change it.

Copy link
Member

@monlovesmango monlovesmango left a comment

Choose a reason for hiding this comment

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

this is beautiful. huge improvement from previous nip46 and is actually consistent. thank you. also love the remote signer discovery via NIP-89. so cool.

as for leaking meta data, my preference would be to just communicate with the signer over a relay that enforces authentication for these event kinds. this way it is opt in and no overhead/complexity in data structure. I am working on a PR to spec out "authentication delegation tokens" which would work well for this, basically you could create delegation tokens that would be specifically for signing event kind 22242 and the query conditions would dictate what sensitive data the delegatee/local pubkey can access on behalf of the delegator/remote pubkey (for this nip would be the signer request/response kinds). hopefully that makes sense, but will try to get that PR out asap.

46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
46.md Outdated Show resolved Hide resolved
@pablof7z
Copy link
Member

pablof7z commented Feb 14, 2024

Now, this whole RPC ordeal makes me wonder if we shouldn't just specify a standard HTTP Services API for Nostr that simply takes an HTTP POST with a Nostr event and replies with another Nostr Event signed by the service. One entry point, one reply. Signers could just expose a Services Definition Event in Nostr pointing to their service URL.

No, we would lose behind-router signers that can easily expose an HTTP endpoint.

@pablof7z
Copy link
Member

This is a massive improvement. The JSONRPC-on-top-of-nostr is why this very valuable NIP has sat idle for basically a year. Pretty much all implementations that actually do NIP-46 just use NDK, to the point where a typo on the create_account (wrong kind) went undetected for a couple months with a various instances already out there.

I've spoken with a lot of the people that have asked questions on how to implement it and it's always a pretty painful process because of how unnecessarily complex it is due to the very unnostr nature of it.

If we want more adoption that aren't like farcaster/hypercore's "just use the SDK, bro" we need to reduce the complexity where we can.

Massive enthusiastic ACK

@staab
Copy link
Member

staab commented Feb 14, 2024

it's always a pretty painful process

I don't disagree with your main point, but I will say the main source of pain for me was not json rpc, but figuring out which key to use when, which nip05 to check and which key to pull from it, which kind to subscribe to, etc. Most of which are silent failures, making it very hard to debug. You've answered my questions about the general flow, but I can't help thinking there must be a better way. That would be the big thing to improve imo.

Currently it's something like this:

  • Fetch nip 89 handlers from some relays.
  • Infer the domain name from the nip05 of the handler.
  • Fetch the handler domain's nostr.json.
  • Get the signer's pubkey using the _ entry
  • Get the relays the signer listens on specifically for create_account requests (different relays may be used for all other requests). This comes from the relays field I believe, not the nip46 one.
  • Ask the user to select a domain
  • Check to see if the user's name + signer domain has a nip05 record
  • If not, use the signer's nostr.json relays to broker a create_account request addressed to the signer's pubkey.
  • If there is a record, use the user's nip46 relay selection from the signer's domain's nostr.json to broker a connect addressed to the user's pubkey.
  • In either case, make sure to start a REQ with the client's p-tag before sending the events or else you might miss them since they're ephemeral.

The main difficulty with the flow above is that it's not clear who you're asking for what. The mental model I would like would be to create a channel between the client and signer, and use the same model as DMs to p-tag the recipient (rather than the user).

But Pablo pointed out to me that you might have multiple signers listening for your events, in case you want redundancy, so you can't just p-tag the signer. You also might not know the signer's pubkey if you're not starting with a domain name. So there's a certain amount of irreducible asymmetry that is ok as long as we can explain it thoroughly (along with explanations as to why it's so weird).

Some improvements we can make to that process though:

  1. It's not clear that the signer's domain should be inferred from the 31990 handler's nip05 — for example, I used my personal nip05 for coracle's listing rather than _@coracle.social. This could confuse clients if I created a coracle signer. A "signer_domain" field or tag on kind 31990s would be clearer.

  2. Rather than mashing the signer's relays and pubkey into nostr.json alongside everyone else's nip05, I would do something like:

{
  "nip46": {
    "signer": "<signer pubkey>",
    "relays":  ["wss://relay.my-signer.com"],
  }
}

I don't think users should customize relays in the signer's nostr.json file, since they can already do that via nip 65 (maybe we could add a "signer" mark) if they want clients to send requests to all signers listening for the pubkey. But communicating with signers over user-selected relays doesn't really make sense to me because signers would have to listen on the superset of all user-selected relays, some of which the signer might not have permission to read from. So it seems like you're implicitly selecting a signer anyway based on the intersection of the user's and signer's choice of relays. What should probably be done is have users publish nip 65 signer relay selections reflecting the relays the signers they are registered with use (which signers can do automatically since they have the user's private key).

  1. I'd like to see create_account gracefully handle a record already existing for the username so that if a user has explicitly selected a signer domain, a client doesn't have to check the nip05 record to figure out whether they're already signed up and choose a login method based on that. This gives the bunker the opportunity to provide a better UX than a client might, and reduce latency.

TLDR; The one-user-to-many-concurrent-signers paradigm (enforced by the original bunker approach, which has to work passively) is the source of a lot of this complexity, and conflicts with the oauth-like flow in which a signer has been explicitly selected by the user. Maybe we need to split this protocol into two different ones?

@fiatjaf
Copy link
Member

fiatjaf commented Feb 14, 2024

It's not true that everybody is using NDK. There is an implementation on Snort, another standalone implementation from @jiftechnify, another on nostr-tools based on the code by @erskingardner, there is an implementation on Gossip from @mikedilger and one on go-nostr. And it's not true that the current protocol is as bad as you're making it sound. The JSON-RPC thing is actually quite simple and straightforward.

What has actually happened here was that there was no documentation for anything, no spec written, the old NIP-46 spec was weird and it was clear that nsecbunker was doing something different and the oauth-like flow even more, so everybody just took NDK because "the code is the spec" and we ended up in this situation.

With a comprehensive and well-written spec we can get away with the current way things are very easily going forward.

@mikedilger
Copy link
Contributor

mikedilger commented Feb 15, 2024

I feel tired. But if you need me to rescue nostr by dropping everything and redoing my nip46 implementation yet again to meet this new protocol, I can't say no. If my kid is stuck at school and missed the bus, I guess I have to go pick him up. But it is annoying.

I was a little annoyed at switching to "bunker://" but it was a quick fix and luckily I read about it. How many implementations didn't know about that change?

I didn't read through all of this, but it doesn't look like a simple change. Granted NIP-46 had some interpretation problems. But I don't think the thing that it was desperately needing was more kind numbers. Who knew?

IETF standards are a fucking pain in the ass to work out, hugely detailed, massively pedantic, battles rage between representatives from various corporations and interest groups about stupid details that nobody cares about and you will pluck your eyes out before it's over. But the reason they endure that pain is because once a standard is out there, it never changes. That's one thing I prefer with IETF RFCs. Nostr standards are a moving target that nobody can keep up with unless they work 28 hours per day just to keep up.

I guess my point is this: Take more time making sure that a standard will be maintainable and supported forever before merging it, and make new standards in new NIPs. This PR I think should be a new NIP that replaces NIP-46, but NIP-46 should remain behind for people who didn't upgrade to the replacement.

@erskingardner
Copy link
Contributor Author

So there's a certain amount of irreducible asymmetry that is ok as long as we can explain it thoroughly (along with explanations as to why it's so weird).

I'm planning on adding a few diagrams and some more detailed descriptions of the flow you outlined in your note for sure. Just didn't want to do that work until we hammered out the details here.

@staab I also think your point about making sure signers and users who use remote signing are signaling their relays of choice is a good one. We can add a section on relay best practices.

With a comprehensive and well-written spec we can get away with the current way things are very easily going forward.

I disagree with this. The current spec is just half-baked. Especially in responses; the response object is being used in different ways in different places. e.g., auth challenges coming through via the error field. We still have a small enough number of people who have implemented this that we can change things and vastly improve this without too heavy a lift in changing SDKs and client code.

All that said, There is a more minimal version of changes we could make that would reduce the blast radius here:

Instead of using kinds for each of the different commands, we keep a more JSON RPC structure for requests and responses (keeping the current kind there) and only add new kinds for errors and auth challenges (you could even drop the error kind and only have a new kind for auth challenges). Personally, I don't like this and would prefer to personally spend my time helping all implementations upgrade to the new version but, if we do end up with that simplified set of changes, I still think it would be a huge improvement over what we have.

@mikedilger
Copy link
Contributor

mikedilger commented Feb 15, 2024

I'm on the verge of leaving the nostr community. (I'm not gonna do that, I'm just impulsive).

If you can't get your shit together, I don't want to be walking through it. This is NOT NIP-46. This is something new. EITHER modify the wording of NIP-46 to make it more explicit and clear, or make a different NIP that doesn't require me to spend another week rewriting everything again. FOR FUCKS SAKE.

@mikedilger
Copy link
Contributor

I don't mind this new thing. I just don't think it should be an edit to NIP-46. That's all. Sorry I'm so angry... sometimes I get angry.

@nostrband
Copy link
Collaborator

With a comprehensive and well-written spec we can get away with the current way things are very easily going forward.

Exactly!

@fiatjaf
Copy link
Member

fiatjaf commented Feb 15, 2024

I don't think there should be a new NIP for doing the same thing this is already doing. I think since both @mikedilger and @nostrband are not enthusiastic about the change (and honestly I am not very keen to rewriting my two libraries either) we should just give up now and leave things as they are.

Would be nice to get a nice rewording and clarification of the current spec.

EDIT: If consensus is achieved I think we can still change this -- but creating two different standards living together side-by-side is a horrible thing that I want to totally want to avoid at all costs.

@erskingardner
Copy link
Contributor Author

I'm not ready to bail on this yet. I think this (or the simplified solution I proposed above) would both be significantly better for the tradeoff of reimplementing in a few libraries (2 of which I'll be doing myself) and clients (again, a few here too).

@v0l
Copy link
Member

v0l commented Feb 15, 2024

I don't think there is anything wrong with NIP-46 in its current form, its just a bit confusing because the spec is terrible.

I believe this all came about because of the oAuth flow, which i myself just read the doc on it today and it leaves out some important details like the url being in the error field... which seems kinda important.

I also dont like that oAuth flow is using the same id twice which already had a reply.

It would be better to just change oAuth flow since its new and leave everything else alone.

I would rather see a reply for auth like this:

{ 
  "id": "connect-id-or-create_account-id",
  "result": "auth_url",
  "error": "oAuth flow requested",
  "url": "https://popup.com",
  "next": "next-id"
}

next-id being the reply after external auth flow, next-id could also simply be connect-id + auth or some other random value to make sure its unique enough for the client (i use UUID so its fine for me, but if clients use a counter then it might be a problem)

@erskingardner
Copy link
Contributor Author

nip44 encrypt/decrypt calls should take arrays and getCoversationKey should not be exposed (see novel above)

I still think getConversationKey is very valid and useful. Sounds like it's not a settled discussion on the nip-07 thread either?

auth url should be new kind or have different payloads so it is explicit that this response should be handled differently. for example, for nip04 decryption if the plaintext just happens to be a URL how is the client supposed to know whether that is an auth URL response or a real response?

That was my original idea that was roundly rejected. In your example, clients would know because the result field is auth_url and the error field is the URL for the auth challenge. With decryption, the results are the results.

why not upgrade encryption of content to nip44?

Again, I wanted to do that but sounds like people are more interested in not changing that yet.

create_account should also be for login of existing bunker users who don't have nip05

Why? That doesn't make any sense to me.

mention that request can also be authored by signer under request details for only the connect method , and response can be authored by client only in response to signer connect request

I'm not sure what you mean here. Can you elaborate?

@fiatjaf
Copy link
Member

fiatjaf commented Feb 19, 2024

We could just merge this because it is much better than what we currently have and then get PRs improving it later. Any objections?

@fiatjaf
Copy link
Member

fiatjaf commented Feb 19, 2024

About the NIP-44 calls, I think we should have them like NIP-07, and services who want to provide the conversation key can do it and reject all encrypt/decrypt requests -- while others can not expose the conversation key and just handle arrays with tons of encrypt/decrypt requests.

Clients should first try to get the conversation key, and if that fails try sending arrays of messages to be decrypted.

NIP-07 should expose the exact same methods.

@fiatjaf fiatjaf mentioned this pull request Feb 19, 2024
46.md Outdated

## Event payloads
1. Clients need several things to start: a local keypair and the details of the pubkey and remote signer the user would like to use.
Copy link
Member

Choose a reason for hiding this comment

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

No, the user just needs to know the remote user pubkey and the relays in which to talk to, both things can come from the bunker:// or if using a nip-05 from the nip46 entry in the nostr.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And they need a disposable set of keys to communicate with the bunker.

I'll make the source of these details more clear.

46.md Outdated
}
```

Clients should display (in a popup or new tab) the URL from the `error` field and then subscribe/listen for another response from the remote signer (reusing the same request ID). This event will be sent once the user authenticates in the other window (or will never arrive if the user doesn't authenticate).
Copy link
Member

Choose a reason for hiding this comment

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

Clients can add an extra redirect_uri to get the bunker to redirect the user to that URI after account creation. This is useful in popup/new-tab-blocking contexts where all the client is able to do is redirect the user to a different URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a note.

@pablof7z
Copy link
Member

ok, trying something new to make this clearer on this gist:

https://gist.github.com/pablof7z/dc5b08c3e39cb73512473a53f5f83b48

if we move the create_account command to a different section then this becomes way clearer because now the whole document talks about only two pubkeys. Who to p-tag, who to encrypt-to is now obvious because there is no third key involved.

  • local pubkey (client)
  • remote pubkey (whom I want to sign as)

then the "bunker-commands" that can simply live in a new section that introduces the concept of, instead of talking to a pubkey, you are talking to the bunker and asking it to do things (create_accsount)

that also means that the section on using NIP-89 to discovery a bunker and stuff like that gets tied to that part of the flow. I think this approach makes it way simpler and less confusing.

@arthurfranca
Copy link
Contributor

@monlovesmango if we expose conversation key [...] clients can forever read your private messagesg. getCoversationKey should not be exposed
@erskingardner I still think getConversationKey is very valid and useful
@fiatjaf services who want to provide the conversation key can do it and reject all encrypt/decrypt requests

From nip-07 discussions, getConversationKey can be made secure when NIPs using NIP-44 encryption define a salt to use when generating the conversation key (and pass it as argument to getConversationKey).

A good default is using the involved event's created_at first 4 digits from the left as salt, which would make the getConversationKey different every 12 days. So a conversation key wouldn't be able to decrypt events outside this time window.

First step would be merging #1020, then we should add the salt argument to NIP-46 and NIP-07 getConversationKey calls.

@arthurfranca
Copy link
Contributor

Pardon me. @staab is right. If the salt is based on created_at, the malicious client can just ask the nip07/46 thing to generate a bunch of conversation keys related to many timestamps.

The salt is still useful for other use case but that's another subject.

@arthurfranca
Copy link
Contributor

@staab the salt instead could be a random value on a public event tag though like ["salt", "37981873721398"]. This way the malicious client currently using nip07/46 would not be able to ask to generate conversation keys to decrypt future events.

Just like not revealing conversation key is useful to protect just future events aswell isn't it? Cause clients using nip07/46 can ask to decrypt any previous ciphertext/event.

Am i right?

@staab
Copy link
Member

staab commented Feb 19, 2024

Sure, but now you have a linked list, which introduces new failure modes. I haven't yet seen a use of salt that solves a real problem that doesn't also have the same problems explicit key exchange does. Maybe one exists, I mean, this is not my wheelhouse, but I think the only real solution to forward secrecy is a real ratchet. Maybe salt is part of that, but I don't know.

@pablof7z
Copy link
Member

Guys, let's not bring the salt and nip-44 discussion to this PR; whatever is decided wrt NIP-07's NIP-44 interface this NIP should do the same.

@monlovesmango
Copy link
Member

monlovesmango commented Feb 20, 2024

I still think getConversationKey is very valid and useful. Sounds like it's not a settled discussion on the nip-07 thread either?

yep, I just added it back to the nip07 pr.

That was my original idea that was roundly rejected. In your example, clients would know because the result field is auth_url and the error field is the URL for the auth challenge. With decryption, the results are the results.

hmm where was it roundly rejected? maybe I missed it. I think most were against creating new kinds for existing functionality, but the auth URL would be net new functionality and so shouldn't duplicate anything that already exists. at the very least, even if we don't want a new kind we can put auth url in a new field like auth_url rather than put it in result. offers a lot more clarity for little/no cost.

Again, I wanted to do that but sounds like people are more interested in not changing that yet.

oh I hadn't seen that.

create_account should also be for login of existing bunker users who don't have nip05

Why? That doesn't make any sense to me.

what is the current easy login flow for an existing bunker user that has no nip05 identifier? I haven't seen that accounted for yet. basically #3 from @staab 's #1047 (comment).

mention that request can also be authored by signer under request details for only the connect method , and response can be authored by client only in response to signer connect request

I'm not sure what you mean here. Can you elaborate?

right now request events are explicitly defined as:

{
    "id": <id>,
    "kind": 24133,
    "pubkey": <local_keypair_pubkey>,
    "content": <nip04(<request>)>,
    "tags": [["p", <remote_user_pubkey>]], // NB: in the `create_account` event, the remote signer pubkey should be `p` tagged.
    "created_at": <unix timestamp in seconds>,
}

however when a signer sends a connect request to the client, the pubkey will actually be remote_user_pubkey and the tags will actually be [["p", <local_keypair_pubkey>]].

the same for a response event for a signer sent connect request, the pubkey and tags will have local_keypair_pubkey and remote_pubkey respectively.

@monlovesmango
Copy link
Member

monlovesmango commented Feb 20, 2024

@pablof7z really like how the gist is arranged, agree its clearer. for the "The flow" section under item 2 I would add that pubkey can also be obtained by signer sent connect request

@fiatjaf no objections to merging as is

@erskingardner
Copy link
Contributor Author

I just moved things around in line with @pablof7z's gist to give the Signer commands it's own section and to move OAuth-like stuff to it's own section as well.

I have also purposefully updated the NIP44 commands to be "potential future addition" so that we can merge this without committing to the form of those commands/responses yet.

AFAICT, this is perfectly inline with what is out there in the wild now, just way better explained.

@vitorpamplona
Copy link
Collaborator

This is still so confusing. Let's say the user is doing the NIP-05 auth. Client gets the key from /.well-known/nostr.json and what? How does the client register the local keypair with the remote signer?

Is the OAuth-like Flow a requirement for this type of login? Otherwise, how does the user accept the new local key pair in their signer service? Do they have to have the signer open somewhere to approve the new local key pair?

@erskingardner
Copy link
Contributor Author

Is the OAuth-like Flow a requirement for this type of login? Otherwise, how does the user accept the new local key pair in their signer service? Do they have to have the signer open somewhere to approve the new local key pair?

It's not a requirement but it definitely makes it easier. You're correct, that anytime you're using a remote signer, you will need to allow a new local client from the signer. That "allow" flow will be different for each type of signer. Nsecbunker's default is to see a request show up in the Admin UI and then approve. If you're using the OAuth-like flow, Nsecbunker sends back an auth challenge so that the user can visit a webpage (hosted by the bunker) to approve (it's using the same mechanism under the hood).

You can imagine other signers, like phones, etc that might send you a push notification and then you'd approve there. But yeah, there's always a step on the remote signer to allow the access.

When the user types a NIP-05 the client:

- Queries the `/.well-known/nostr.json` file from the domain for the NIP-05 address provided to get the user's pubkey (this is the **remote user pubkey**)
- In the same `/.well-known/nostr.json` file, queries for the `nip46` key to get the relays that the remote signer will be listening on.
Copy link
Collaborator

Choose a reason for hiding this comment

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

how do you query for the NIP-46 key?

Copy link
Member

Choose a reason for hiding this comment

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

it works just like relays, you want to get the nip46.$pubkey to get the array of relays for this pubkey:

$ wget -q -O - https://highlighter.com/.well-known/nostr.json | jq .names.pablo
"bab85564b5d198c3d3a9f856a3fe9dc8e2f9ca3952ba6e7dd60bc98030321103"

$ wget -q -O - https://highlighter.com/.well-known/nostr.json | jq .nip46.bab85564b5d198c3d3a9f856a3fe9dc8e2f9ca3952ba6e7dd60bc98030321103
[
  "wss://relay.nsecbunker.com",
  "wss://relay.damus.io"
]

Copy link
Collaborator

Choose a reason for hiding this comment

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

ohh the nip46 key is the service provider key. ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh wait, there is a nip46 field as well. Interesting.

Copy link
Member

@staab staab Feb 20, 2024

Choose a reason for hiding this comment

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

@vitorpamplona
Copy link
Collaborator

@monlovesmango
Copy link
Member

since we are adopting @pablof7z 's restructuring, can we also simplify the terminology section as he suggested in #1047 (comment). so only need local pubkey and remote pubkey (no longer need to distinguish between remote user pubkey and remote signer pubkey)

@alexgleason
Copy link
Member

alexgleason commented Feb 20, 2024

So we went back to not using different kinds for each command? Good, because I had a better idea in #940 (comment)

Instead of some commands (like nip44.decrypt) taking multiple tuples of arguments, it should be possible to batch any number of NIP-46 commands in a single request. You can encrypt, decrypt, and sign multiple events all in the same call. This would allow NIP-07 to stay atomic, while improving NIP-46 performance. An adapter between NIP-07 and NIP-46 would simply batch the calls with a time delay.

It's basically the same, but the content is an array of RPC calls instead of a single one. EDIT: for backwards compat, this can be a different "request" kind and the old kind can be deprecated.

@monlovesmango
Copy link
Member

@alexgleason maybe we can move the conversation around how nip44 calls within nip46 should be structured to #983.

think this convo should stay focused on updating NIP-46 clarity. the nip44 calls discussion seems to get pretty involved.

@fiatjaf fiatjaf merged commit c6cd655 into nostr-protocol:master Feb 20, 2024
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.