NIP07 - add nip44 calls

[monlovesmango]

add nip44 calls to NIP07 spec

https://github.com/monlovesmango/nips/blob/NIP07-Add-nip44/07.md

@paulmillr does the version need to be added to these calls?

[paulmillr]

I think it's only:

async window.nostr.nip44.v2.getConversationKey(pubkey): string // takes third party pubkey and returns shared secret key as specified in nip-44

Because encrypt and decrypt take conversationKey directly - not priv/pubKeys.

[monlovesmango]

I think it would be better to include the version as an argument so we aren't having to update the specification with each new version. it would probably also be much easier on clients to have a single call rather than a unique call for each version. what do others think? @staab @vitorpamplona @pablof7z

encrypt and decrypt only make sense if they take pubkey. if they take conversation key there is no point of these functions (because the app can do the encryption/decryption itself). also it is good to give apps the option to NOT store the conversation keys (which isn't possible if encrypt/decrypt only take conversation key).

as for naming, I would prefer getSharedKey over getConversationKey (because it will have use cases outside of just messaging). but whichever we choose we should keep it consistent with NIP46 (which is just nip44_get_key right now).

[vitorpamplona]

on the version, here's what I am doing:

conversationKey: version is optional. It should return the newest version, but to decrypt locally, the caller will need specific versions based on what's on the ciphertext.

encrypt should encrypt with the newest version all the time.

decrypt has the version in the ciphertext and must decode accordingly (if the extension supports it).

Both decrypt and encrypt should only be called with the client cannot perform these operations by itself.

[paulmillr]

also it is good to give apps the option to NOT store the conversation keys

No. It's great to store conversation keys and it's not great to store master private key, which is used to sign all events and generate all conversation keys.

That was my thinking when making nip44. getConversationKey was explicitly made caching-friendly. It is supposed to be cached and stored, for performance reasons. It is really slow.

This also allows building trezor-like external signers. They would just sign the events themselves and won't encrypt individual messages. They would be able to generate conversation keys, which the frontend would store / cache in itself.

[paulmillr]

As for the version stuff: I don't know how v3 could look like. Will it use method similar to getConversationKey at all? I'm not sure - maybe not. We are talking about mixing post-quantum-cryptography keys, maybe ephemeral secrets, or something. So it's not certain. v2.getConverastionKey is descriptive and local.

I would prefer getSharedKey over getConversationKey

You should have voiced this concern in nip44. Conversation key term is used all over it, that includes test vectors and implmenentations.

[monlovesmango]

@vitorpamplona this makes sense. when you call encrypt/decrypt are you using the pubkey or the conversation key?

@paulmillr agree its good to store conversation keys. in my mind not storing conversation keys doesn't mean you have to store master private key either. however, I hear you and understand that the conversation key should be cached locally. from the convo in nip44 it seemed that this would be optional, not required.

as for convsersationKey verses sharedKey verses getKey, I guess I don't care how we refer to it in the technical aspects but we should give thought to the naming of the public facing interface. getConversationKey wasn't in the nip44 spec and I had tried to add it in here. I only chose getKey because that was what had been added to nip46 and was trying to stay consistent.

[vitorpamplona]

when you call encrypt/decrypt are you using the pubkey or the conversation key?

Pubkey. Then I expect the signer to cache the conversation key to avoid recreating all the time.

[monlovesmango]

per @staab from prior closed pr #922 (want to consolidate the conversation):

I haven't been able to track all the changes to this file and NIP 46 that have been proposed, what's the argument for getKey? The downside is that an evil client can exfiltrate conversation keys for all DMs, and since we have no forward secrecy or post-compromise security that gives them access to snoop forever.

@paulmillr is saying we should primarily rely on getKey only and have apps/clients cache it. however I agree with @staab that we should be cautious of having apps/clients cache the shared key by default. I lean towards the browser extension (or signing device in the case of NIP46) to do the caching instead (like @vitorpamplona is doing) and not force the use of getKey by apps/clients.

[staab]

Right, the whole point of signers is to minimize trust, so I feel that we should... minimize trust. What clients choose to do is irrelevant, if clients have access to getConversationKey it can be abused.

[vitorpamplona]

Right, the whole point of signers is to minimize trust, so I feel that we should... minimize trust.

The signer can have an option where the user authorizes the client to decrypt all content. In that case, the client should have access to the Conversation key directly. If the GetConversation Key fails, then the client can encrypt/decrypt directly with the extension.

[staab]

The signer can have an option where the user authorizes the client to decrypt all content.

But why? How is that different from "remember my choice and don't ask again"?

[vitorpamplona]

The reason is performance. Especially for NIP-46.

[staab]

Because of shared key generation? Caching of conversation keys can be done in the extension/provider the same way it can be done on the client.

[vitorpamplona]

Yep, but calling decrypt for every chat message and calling just once to get the key and decrypting them all locally have very different performance impacts. It's definitely true on NIP-46 and on the android signer, maybe less on the extension.

[monlovesmango]

the only difference in performance would be network latency, right?

@staab I tend to agree with you (latency is worth not having shared keys cached in every app) however I don't think its worth fighting against getKey/conversationKey since the browser extension or signing app can always restrict this on their side. so ultimately the user can prevent clients/apps from caching the shared secrets very easily.

does anyone opinions on the naming of the getKey call? should it be consistent with nip46 (or is it just me and my OCD that cares about consistency)? @vitorpamplona I know you already started implementing something

I prefer getConversationKey over getKey (I just used getKey bc it was in nip46). but I think getSharedKey is better than both.

[vitorpamplona]

@paulmillr why did you choose "conversation" key instead "shared" key?

I tend to agree with @monlovesmango that shared key is a better name for a secret shared between two keys. Conversation key only works on one-on-one chats. Group chats using GiftWraps will have conversation keys for each pair of users inside that group. So, it's not really a single "conversation" key.

[paulmillr]

I picked it because there was no discussion or differing opinions on the naming. That's mostly it. It's possible to still change it, if that's the final decision.

My reasoning was mostly that there's a key which should only be used for conversations. It's separate from all other keys. encrypt and decrypt only use this key and not some "private" or "public" key. If there was a public key in encrypt/decrypt, the question is: whose public key it is? etc

[fiatjaf]

I like the name "conversation key".

[staab]

Yep, but calling decrypt for every chat message and calling just once to get the key and decrypting them all locally have very different performance impacts. It's definitely true on NIP-46 and on the android signer, maybe less on the extension.

So we're talking network latency here? I don't think that would be substantial in NIP 07. And for NIP 46, the correct solution would be to add encrypt_bulk type methods. That way you only experience the network latency once per chunk.

No one has addressed the security issue I mentioned, is it not an issue for anyone that clients can exfiltrate conversation keys? It seems to defeat most of the purpose of a signer.

[monlovesmango]

No one has addressed the security issue I mentioned, is it not an issue for anyone that clients can exfiltrate conversation keys? It seems to defeat most of the purpose of a signer.

signer can easily limit use of or disallow conversation key requests (and only respond to encrypt/decrypt)

I do think bulk encrypt/decrypt methods might be nice

[vitorpamplona]

No one has addressed the security issue I mentioned

GiftWraps come from random keys. So, there is a getKey for every single message. The getKey of the inner Seal (which comes from the real author) is the one that will provide performance improvements.

That means, even if the client shares the conversation key with a "snooper" client, it still has to share the conversation key of each individual wrap. There is no way to just listen in to the conversation by sharing the inner key.

Also, it's impossible to sign a valid event (as the participant in a chat) with just the conversation key. You need the master private key to forge any encrypted message.

[staab]

signer can easily limit use of or disallow conversation key requests (and only respond to encrypt/decrypt)

Then why have it? This will also break clients that assune a signer implements the full interface.

GiftWraps come from random keys.

NIP 44 doesn't assume single-use keys. The issue is mitigated with gift wraps, but still exists because as you say, the seal's key can be re-used. But for non-gift wrap applications of NIP 44 this doesn't apply.

[vitorpamplona]

Well, I don't have a good view of non-giftwrapped uses of NIP-44. Maybe there are cases where the metadata leak is ok, but I don't know.

I will leave the decision to you guys.

Maybe next time we can create a Conversation key that changes weekly (it must match the year+week of the event). In that way if the conversation key leaks, it can only be used to decrypt events in that week.

[vitorpamplona]

Maybe next time we can create a Conversation key that changes weekly (it must match the year+week of the event). In that way if the conversation key leaks, it can only be used to decrypt events in that week.

Ok, I am starting to think this is a great idea. For v3 (sorry for bringing a new version this soon), what if we used the first 7 digits of the .created_at as the conversation key salt that today is fixed as nip44-v2? That seems to create new keys at every 11 days. So, those can be used to create shared secrets that automatically rotate based on date. We can even use 8 or 9 digits and make it rotate every day or 3 hours, respectively. Maybe event kinds can choose their own salt rotation based on the security needs they have. And Signers can approve the client with a start and expiration date.

Then conversation keys can be shared with clients without much fear of clients going rogue with them.

And if they ever leak, there will always be a time-based protection into them.

[paulmillr]

I am out of loop on the wrapping. Can someone give a 2-sentence overview of what’s the goal with the key sharing here?

[vitorpamplona]

@paulmillr sorry for the long explanation.

The goal of the key sharing between applications (signer app and the nostr app) is to avoid too many calls between those two apps. Usually, the signer app has the user's private key and the nostr app calls decrypt in every chat message before displaying. If there is network latency in each call, things get slow very quickly. Sharing the conversation key with the nostr app then allows the app to just call getKey once and decrypt all messages locally. But that comes at a risk that the client might be ill-designed and leak the conversation key. With the control of the conversation key, an attacker could then decrypt all past and future messages between the two people.

Now, the wrapping design offers some protections because each message is encrypted and signed twice. The outer event that gets transferred through Nostr relays (the wrap) always uses a random private key to encrypt and sign and thus the conversation key is unique to each message. However, the inner event (the seal) is signed by the sender's main private key to the receiver's main pub key. Sharing the inner key between apps can provide significant performance improvements and if it leaks the attacker still must decrypt all the outer events with their individual keys. Thus, when using GiftWraps, it's ok to share the conversation key.

The issue happens when Giftwraps are not being used AND the conversation key is being shared. Take NIP-51 Lists for instance. There is a private part to lists where we encrypt the information to my own user. The conversation key is between the user's private key and the user's pubkey. If that conversation key leaks, an attacker can then watch the user's private payload forever.

Which leads to the idea of using the salt in the conversation key design. If we allow that to be flexible, each NIP can have a way to determine the salt based on information in the signed event. We would just add a third parameter to the conversation key that defaults to nip44-v2 if nothing is provided. By using the first X chars of the .createdat timestamp, each event kind can choose a time-based rotating salt to reduce the impacts of a conversation key leakage if it ever happens.

The List NIP for instance, could specify the use of the first 7 digits of the .created_at as salt for the encryption. This means that the conversation key will be different at every ~11 days and if it leaks only the messages inside that 11 day block will be decrypted by an attacker. We could reduce that further to a day by specifying the use of the first 8 digits as salt.

This seems beneficial and it would only slightly change the NIP44 design. We would just offer the salt as a parameter in the get_conversation_key function with the current default value if NIPs don't care about specifying one.

From:

def get_conversation_key(private_key_a, public_key_b):
  shared_x = secp256k1_ecdh(private_key_a, public_key_b)
  return hkdf_extract(IKM=shared_x, salt=utf8_encode('nip44-v2'))

To:

def get_conversation_key(private_key_a, public_key_b, salt=utf8_encode('nip44-v2')):
  shared_x = secp256k1_ecdh(private_key_a, public_key_b)
  return hkdf_extract(IKM=shared_x, salt)

[paulmillr]

I like it. Good idea. Make sure you've got integer encoding rules correct: amount of bytes and endianness (BE in other parts of spec).

And we should keep the version prefix for future proofness. So, instead of salt=created_at, it would be salt=concat(encode('nip44-v2'), created_at).

[arthurfranca]

What about the.......... salt?

[staab]

your comment talks about returning an array of objects as well

Just for extensibility. It's always possible we would want to return more information. But it probably doesn't matter, and it occurs to me that as bad as positional arguments are, tuples are a more compact representation. Maybe we should go back to tuples (sorry).

What about the.......... #1020?

I think we agreed that that can be added later when there's a specific proposal that uses it with some buy-in. From @monlovesmango:

as for salt, I think that should only be added after there is a nip44 version that uses salt.

[fiatjaf]

Shouldn't we do the same thing NIP-46 is doing?

[staab]

nip 46 should do what we're doing. The array notation doesn't actually do any good for NIP 07, it's only for consistency with what NIP 46 should do, which is accept bulk requests to reduce round trips.

[monlovesmango]

yep, what @staab said. ultimately the design decision we made here was for the benefit of NIP46, so that NIP46 would work well and NIP07 can be consistent with it.

[fiatjaf]

So can we merge this?

[staab]

Waiting on implementations ahem fiatjaf/nos2x#47

[fiatjaf]

Merged.

But not deployed. I must go back to nos2x and review the other open PR and then deploy everything.

[vitorpamplona]

Let's go! 🚀

[fiatjaf]

Shouldn't we have the getConversationKey here too, even if as optional?

[monlovesmango]

we had a lot of discussion about this. tbh I think getConversationKey should be removed from nip46 too, not added here as well. if people really feel strongly about it, I am ok adding it back in (since it should be easy for the signer to prevent usage of this method if users don't like conversation key exposed). but I lean towards keeping the spec simpler since there isn't much performance gain, creates another attack vector if clients maliciously use the conversation key, and adds complexity to client implementation (and no guarantee clients will handle scenario where getConversationKey is not available properly).

if we do expose this, we should at least have a good reason for doing so. since encrypt/decrypt are now batched method calls there isn't much efficiency gain by having getConversationKey. whats another reason to have the getConversationKey?

again, most of the considerations around the NIP07 method calls were really had with NIP46 in mind, so that NIP07 would align with NIP46.

I know @staab felt strongly that it shouldn't be exposed, and agree with the sentiment.

[fiatjaf]

@monlovesmango what do you think of this? #1047 (comment)

Basically just make the two ways of decrypting optional and have clients implement one but fall back to the other.

The most important thing is having a uniform interface between NIP-07 and NIP-46 to prevent too much spaghetti.

[staab]

The most important thing is having a uniform interface between NIP-07 and NIP-46 to prevent too much spaghetti.

Definitely this. I still haven't heard a compelling reason to expose the conversation key if we have bulk methods, but it would be faster and less prone to network failures. I'd prefer to leave it out still, but @fiatjaf's compromise would also be fine (although I think it will just result in get_conversation_key being implemented and used everywhere).

[alexgleason]

I will also just add real quick, that letting you encrypt/decrypt multiple at once leads to the possibility that the signer will return an array with a different length than the one you passed in. It's not really a big deal since clients need to validate the response anyway.

[monlovesmango]

if theres an error, I would think that returned array should retain the same number of values and just put a blank value for the one that err'ed. would probably be good to explicitly say that here.

do you think thats a valid way of handling it?

[alexgleason]

If any individual message has an error, the whole function call should throw.

[alexgleason]

The more I think about it, the more I think it's actually a problem. The client has no way to validate messages before sending them to the signer, right? So any invalid text can cause the signer to throw.

First I would question if we really need to support decrypting multiple messages in a single call. Is it just because Alby prompts every time?

If we really need to do it, it should have a return type of Promise<string>[] instead of Promise<string[]> so individual calls can be either resolved or rejected.

[monlovesmango]

no, it is because for the NIP44 calls in NIP46 it would be extremely inefficient to fetch one decryption at a time. so it is better for NIP46 to have these calls be arrays, and we want the NIP07 calls to be consistent with the NIP46 calls. hence why the NIP44 calls have array parameters.

can you return a promise from a browser extension? returning a promise definitely wouldn't work for NIP46 either, which we are trying to stay consistent with.

[alexgleason]

Found the Alby MR and commented here: getAlby/lightning-browser-extension#2653 (review)

[staab]

What does Alby do about failed decrypt calls at the moment

That's a super good point, it throws.

Alby doesn't have to do batching unless it's converting calls to NIP-46

Alby may want to do batching for UX rather than performance reasons. So instead of 300 pop ups, they should really show "Coracle wants to decrypt 300 messages. ok?"

[alexgleason]

It should just do what it's currently doing for nip04. Users will check "remember this choice", which is not the best UX, but it can be fixed on the Alby side later. We don't need to fix that problem in NIP-07.

[staab]

🤔 I might agree with you. I know @fiatjaf currently wants the interface the same. Maybe he would be convinced by your argument.

[vitorpamplona]

FYI, Amber, the Android Signer, also does batching to avoid creating a new dialog for each approval. The experience is pretty cool and makes a lot of sense since, depending on the user's data, Amethyst might blast the signer with 500 decrypt calls in a second.

[erskingardner]

Does the proposal to add a salt actually fix the issue with unconstrained conversation key reuse? (I haven’t looked at it at all yet). If it does, then it might make sense to just add the conversation key method once that’s released. On Feb 19, 2024, at 17:06, Alex Gleason ***@***.***> wrote: @alexgleason commented on this pull request. In 07.md:

@@ -20,6 +20,8 @@ Aside from these two basic above, the following functions can also be implemente

async window.nostr.getRelays(): { [url: string]: {read: boolean, write: boolean} } // returns a basic map of relay urls to relay policies async window.nostr.nip04.encrypt(pubkey, plaintext): string // returns ciphertext and iv as specified in nip-04 (deprecated) async window.nostr.nip04.decrypt(pubkey, ciphertext): string // takes ciphertext and iv as specified in nip-04 (deprecated) +async window.nostr.nip44.encrypt([[pubkey1, plaintext1], [pubkey2, plaintext2], ...]): string[] // takes array of [pubkey, plaintext] tuples, returns array of ciphertexts as specified in nip-44 +async window.nostr.nip44.decrypt([[pubkey1, ciphertext1], [pubkey2, ciphertext2], ...]): string[] // takes array of [pubkey, ciphertext] tuples, returns array of plaintexts as specified in nip-44 I will also just add real quick, that letting you encrypt/decrypt multiple at once leads to the possibility that the signer will return an array with a different length than the one you passed in. It's not really a big deal since clients need to validate the response anyway. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[staab]

Does the proposal to add a salt actually fix the issue with unconstrained conversation key reuse?

No, it only increases the number of keys an attacker would have to generate.

[monlovesmango]

The most important thing is having a uniform interface between NIP-07 and NIP-46 to prevent too much spaghetti.

agree.

added the getConversationKey method back in. would be nice to have a strong rational for exposing this, but really don't feel strongly about it one way or another. right now the method only takes a single pubkey, do people think it should be changed to take an array as well? or fine as is?

[staab]

An array is probably a good idea, especially if we add salt. I'm still nack on it, but I suppose if it's ever exploited signers can drop support for it.

[monlovesmango]

ok updated getConversationKey to take array instead. I went ahead and made it an array of tuples in anticipation of adding salt in a future PR, so its easy to extend the method to use salt. if it was just a simple array of pubkeys we would need a second array of just salts once salt is added. however I am ok with that if thats what people would prefer (but it would be a significantly different pattern than what we have for encrypt/decrypt.

I'm still nack on it, but I suppose if it's ever exploited signers can drop support for it.

same :)

[arthurfranca]

TLDR: Exposing conversation key (CK) without salt arg is bad similar to exposing the privkey. Changing salt too often defeats the purpose (caching) of exposing the CK. Conclusion: a client should use the same random salt for a period to be able to use a cached CK for encryption/decryption of many events.

would be nice to have a strong rational for exposing this

Fact is that If people don't change salt from time to time, exposing conversation key (CK) is bad, cause the client can leak the CK and who has it can decrypt all events that used (and will in the future use) the CK for encryption.

The only anticipated use case (that wasn't debunked atleast) for exposing CK (with salt arg) is for a theoretical NIP-46 signer that is shared by thousands of users that don't wan't to cache CKs (without salt) from all these users or even with the cached CKs don't want to decrypt too many events (to save compute resources).

If it was not for the above (that may or may not be reasonable, not sure), for my DM use case I would be ok with not exposing CK but just being able to pass a salt arg to nip44encrypt/decrypt functions.

So, imo if people want to expose CKs to cache them on clients, clients should reuse a random salt for some time (keep it locally stored and change from time to time) and add it to a public ["salt", <...>] tag on encrypted events (to be able to later decrypt them).

Because if a client changes the salt too often, it won't be able to take advantage of the CK caching. And if it is the case, it would defeat the purpose of "safely" exposing the CK, cause the client would have to ask the signer to generate many CKs, one for each used salt.

So, imo if we want to expose the CK: without salt (in practice it uses one default salt) is bad; with one different salt per event is bad; with one salt per many events is good.

[monlovesmango]

fyi @alexgleason has opened alternative version of this PR with atomic calls #1063

[monlovesmango]

closing in favor of #1063