feat: add NIP 44 encryption support #2653

staab · 2023-08-11T20:54:18Z

I've added added for NIP-44 encryption and decryption so that users can access nostr features built on the new primitives.

Fixes #2652

feat: New feature (non-breaking change which adds functionality)

How has this been tested?

No testing yet

Checklist

Self-review of changed code
Manual testing
Added automated tests where applicable
Update Docs & Guides

socket-security · 2023-08-11T20:55:55Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@scure/[email protected]	None	`0`	46.4 kB	paulmillr
npm/[email protected]	None	`0`	11.4 kB	feedic
npm/[email protected]	filesystem	`0`	3.01 kB	sindresorhus
npm/[email protected]	None	`0`	10.4 kB	isaacs
npm/[email protected]	None	`0`	22.8 kB	doowb
npm/[email protected]	None	`0`	206 kB	dougwilson
npm/[email protected]	None	`0`	2.97 kB	thejameskyle
npm/[email protected]	network	`+9`	47.3 MB	fiatjaf
npm/[email protected]	None	`0`	19.1 kB	indutny
npm/[email protected]	None	`0`	10.3 kB	dougwilson
npm/[email protected]	None	`0`	8.46 kB	dougwilson
npm/[email protected]	None	`0`	42.3 kB	chalker
npm/[email protected]	None	`0`	148 kB	7rulnik
npm/[email protected]	None	`0`	11 kB	dougwilson
npm/[email protected]	None	`0`	4.31 kB	dougwilson

🚮 Removed packages: npm/@types/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

reneaaron · 2023-08-12T09:02:18Z

@staab Thanks for the PR! 🙌 Are there already any clients that implement NIP44?

staab · 2023-08-12T13:17:19Z

Yep, Coracle does (private key login only right now for obvious reasons). Amethyst, Snort, and 0xChat are actively working on implementations.

staab · 2023-12-22T21:26:57Z

Ok, I think this is ready for review. It took a while to finalize the NIP 44 spec, but it was finally merged this week. I did add nostr-tools as a dependency, let me know if you'd like to copy the nip44 code into the project instead.

reneaaron · 2024-01-08T22:51:05Z

Hey @staab, thanks again for the PR - there are currently some typescript checks failing:

https://github.com/getAlby/lightning-browser-extension/actions/runs/7303976512/job/20279852843?pr=2653

I'll try to get my hands on it this week and do some more testing.

nostr-tools should be fine to include as we also use it already in @getalby/sdk (which is also used in the extension)

bumi · 2024-01-08T23:40:27Z

yarn.lock

+    mitata "^0.1.6"
+    nostr-wasm v0.1.0
+
+[email protected]:


hmm, why is mitata and nostr-wasm a hard dependency of nostr-tools?

Not sure about mitata, but nostr-wasm enables an optimization which speeds up signature validation. These should probably be, what, peer deps?

I am always just a bit hesitant to have a wasm blob in the dependencies.hmm

We can extract nip 44 and drop nostr-tools, shouldn't be too bad

Any update on this? @bumi should we extract nip-44 from nostr-tools?

The wasm is fine, it's just Bitcoin's secp256k1 library compiled to wasm. It's faster. Check: https://github.com/fiatjaf/nostr-wasm?tab=readme-ov-file#is-libsecp256k1-modified

bumi · 2024-01-08T23:41:16Z

yarn.lock

  integrity sha512-EbqwksQwz9xDRGfDST86whPBgM65E0OH/pCgqW0GBVzO22bNE+NuIbeTb714+IfSjU3aRk47EUvXIb5bTsenKA==

-"@noble/hashes@^1.1.5":
+"@noble/hashes@1.3.2", "@noble/hashes@^1.1.5":


we should check what uses "@noble/hashes@^1.1.5" and if we can update it.

Looks like @bitcoinerlab/secp256k1@^1.0.5

rolznz · 2024-01-09T07:11:08Z

@staab as soon as I navigate to send a DM to someone on coracle using this branch I get a popup:

I do not know what I am signing or why this is needed, and it should only popup to sign once I have typed a DM and pressed send, right?

pavanjoshi914 · 2024-01-09T07:22:33Z

Pr's still uses nip04 encryption as you don't specify calls in content scripts and instead nip04 prompts are executed can you please try and fix this?

pavanjoshi914 · 2024-01-09T07:24:13Z

also there are many codewise fixes we need to do here. for eg this function has 3 arguments passed but function expects only two

pavanjoshi914 · 2024-01-09T07:24:36Z

also we need to make this pr in sync with new nostr signing improvements where we split screens for encrypt and decrypt

staab · 2024-01-09T20:12:29Z

I'll try to address some of these issues today, as far as

as soon as I navigate to send a DM to someone on coracle using this branch I get a popup

You can ignore that WRT to this PR, that's an event Coracle publishes to track when the user last opened a thread.

staab · 2024-01-09T21:08:33Z

Ok, pushed an update with the new separated dialogs. The new commit also changes the api to fit what's proposed here. Unfortunately this makes the prompt potentially overwhelming since it might include hundreds of messages. I'm not sure how you want to handle that. Let me know if there's anything else you spot that I can fix! Also, there are a number of type errors and a couple failing tests that appear to be unrelated to this PR.

rolznz · 2024-01-10T02:59:18Z

You can ignore that WRT to this PR, that's an event Coracle publishes to track when the user last opened a thread.

@staab it was not a standard sign event request though (with a kind, etc), but a NIP-04 encryption

staab · 2024-01-10T04:02:35Z

Yep, it encrypts the payload before sending to keep read receipts private.

staab · 2024-02-05T18:03:35Z

Hey guys, anything I can do to push this forward? Also interested in feedback about the bulk use case.

rolznz · 2024-02-06T04:47:54Z

The bulk case makes sense for remote signing and is similar to the new multi_ methods for split payments introduced in NIP-47. For decryption would the user need to really review what they are decrypting (and would they understand it anyway)? maybe we can simplify the dialog there and say "decrypt X messages from <npub>".

For bulk encryption, this would just be for bulk DMs, right (e.g. for advertising?) or is there another common usecase?

staab · 2024-02-06T05:04:10Z

Yeah, that UX would make sense I think. Use cases would be anything wrapped, so DMs, encrypted chat (NIP 17 draft), key exchange for encrypted groups (NIP 87 draft), anything else requiring encryption. The bulk use case is helpful because a lot of the time these events are requested in the background and many need to be encrypted to get the user up to speed.

rolznz · 2024-02-06T05:09:20Z

a lot of the time these events are requested in the background and many need to be encrypted to get the user up to speed

How does the user control this though? I am worried they just end up using the "remember" option and automatically signing everything if they get too many random popups, which in my opinion defeats the point (and you might as well just trust or not trust specific apps).

pavanjoshi914 · 2024-02-19T10:11:33Z

@staab any updates on my feedback? do you need any help?

staab · 2024-02-19T15:55:23Z

It should all be addressed, please review the PR and let me know if anything's missing

src/extension/background-script/nostr/index.ts

socket-security · 2024-02-20T21:54:49Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

staab · 2024-02-20T22:02:19Z

After much debate we have decided to stick to the same interface that nip 04 uses: nostr-protocol/nips#1063

Lots of linting is failing from upstream I believe, let me know if there's anything I can do to improve the PR.

rolznz · 2024-02-21T03:07:59Z

@staab can you please update this PR so we can directly push updates to it?

I created a PR here for some fixes: coracle-social#2

rolznz · 2024-02-21T03:15:44Z

src/extension/background-script/router.ts

@@ -128,6 +128,8 @@ const routes = {
      getRelays: nostr.getRelays,
      encryptOrPrompt: nostr.encryptOrPrompt,
      decryptOrPrompt: nostr.decryptOrPrompt,
+      nip44EncryptOrPrompt: nostr.nip44EncryptOrPrompt,


should we rename the existing methods to be prefixed with nip04?

I can do that if you want, just let me know

package.json

src/extension/background-script/nostr/index.ts

src/extension/background-script/actions/nostr/nip44EncryptOrPrompt.ts

pavanjoshi914 · 2024-02-21T10:42:37Z

@staab do you know why nip44 encryption is not triggered on coracle on scoial? if i try it on console functionality works great. but i always get nip04 prompt when trying on website. thanks!

pavanjoshi914 · 2024-02-21T10:43:26Z

can you please merge Roland's follow up pr so that tests and functionality fix is present in the pr

Fix: coracle nip44

staab · 2024-02-22T17:19:06Z

Got that PR merged.

i always get nip04 prompt when trying on website

04 will still be used for most things, since it depends on new NIPs to invoke nip44. I would suggest sending a DM to yourself. If window.nostr.nip44 is defined, Coracle will ask if you want to send a legacy dm or nip44 one. Click NIP44 to encrypt then reload the page to test decrypt.

alexgleason

API is sorted out in NIPs now. LGTM.

pavanjoshi914 · 2024-03-11T07:57:02Z

@staab can you please sync your fork as with upstream. so that i can push latest changes on your fork

pavanjoshi914 · 2024-03-11T09:16:18Z

@staab i created a new pr for nip44 support and based it off from alby's master branch. i also extracted nip44 into extension itself cause we don't want to introduce nostr2.x now. we do it when alby-js-sdk is upgraded to 2.x. thanks a lot for the contributions and pr. closing this in favour of #3075

staab force-pushed the master branch from 0f97ae0 to 9584cc6 Compare August 16, 2023 20:14

staab force-pushed the master branch 4 times, most recently from 404cf04 to abc683b Compare December 22, 2023 21:25

staab marked this pull request as ready for review December 22, 2023 21:26

bumi requested review from reneaaron and pavanjoshi914 January 8, 2024 22:00

bumi reviewed Jan 8, 2024

View reviewed changes

staab force-pushed the master branch from abc683b to 20977f5 Compare January 9, 2024 21:06

staab force-pushed the master branch from 20977f5 to 72e3605 Compare February 5, 2024 18:02

staab mentioned this pull request Feb 5, 2024

NIP07 - add nip44 calls nostr-protocol/nips#940

Closed

alexgleason reviewed Feb 20, 2024

View reviewed changes

src/extension/background-script/nostr/index.ts Show resolved Hide resolved

Jonathan Staab and others added 4 commits February 20, 2024 13:53

feat: add NIP 44 encryption support

f24eade

Use separate encrypt/decrypt dialogs for nip44

f3bb66d

fix: make functionality work

9318232

Switch back to simple interface for nip44

51538c3

staab force-pushed the master branch from 0664062 to 51538c3 Compare February 20, 2024 21:54

staab mentioned this pull request Feb 20, 2024

NIP-07: add NIP-44 calls nostr-protocol/nips#1063

Merged

rolznz added 3 commits February 21, 2024 10:02

fix: encryptOrPrompt args

19182ba

chore: run prettier

fde0634

chore: update yarn.lock

6959be9

rolznz reviewed Feb 21, 2024

View reviewed changes

package.json Show resolved Hide resolved

rolznz reviewed Feb 21, 2024

View reviewed changes

src/extension/background-script/nostr/index.ts Outdated Show resolved Hide resolved

rolznz reviewed Feb 21, 2024

View reviewed changes

src/extension/background-script/actions/nostr/nip44EncryptOrPrompt.ts Outdated Show resolved Hide resolved

rolznz reviewed Feb 21, 2024

View reviewed changes

src/extension/background-script/actions/nostr/nip44EncryptOrPrompt.ts Show resolved Hide resolved

staab and others added 2 commits February 22, 2024 08:56

Merge pull request #2 from getAlby/fix/coracle-nip44

0d65641

Fix: coracle nip44

Rename pk to peerPubkey

f5893ab

alexgleason approved these changes Mar 2, 2024

View reviewed changes

pavanjoshi914 mentioned this pull request Mar 11, 2024

NIP44 Encryption Support #3075

Merged

pavanjoshi914 closed this Mar 11, 2024

feat: add NIP 44 encryption support #2653

feat: add NIP 44 encryption support #2653

Conversation

staab commented Aug 11, 2023 • edited Loading

How has this been tested?

Checklist

socket-security bot commented Aug 11, 2023 • edited Loading

reneaaron commented Aug 12, 2023

staab commented Aug 12, 2023

staab commented Dec 22, 2023

reneaaron commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolznz commented Jan 9, 2024

pavanjoshi914 commented Jan 9, 2024

pavanjoshi914 commented Jan 9, 2024

pavanjoshi914 commented Jan 9, 2024

staab commented Jan 9, 2024

staab commented Jan 9, 2024

rolznz commented Jan 10, 2024

staab commented Jan 10, 2024

staab commented Feb 5, 2024

rolznz commented Feb 6, 2024 • edited Loading

staab commented Feb 6, 2024

rolznz commented Feb 6, 2024

pavanjoshi914 commented Feb 19, 2024

staab commented Feb 19, 2024

socket-security bot commented Feb 20, 2024 • edited Loading

staab commented Feb 20, 2024

rolznz commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavanjoshi914 commented Feb 21, 2024

pavanjoshi914 commented Feb 21, 2024

staab commented Feb 22, 2024

alexgleason left a comment

Choose a reason for hiding this comment

pavanjoshi914 commented Mar 11, 2024

pavanjoshi914 commented Mar 11, 2024 • edited Loading

staab commented Aug 11, 2023 •

edited

Loading

socket-security bot commented Aug 11, 2023 •

edited

Loading

rolznz commented Feb 6, 2024 •

edited

Loading

socket-security bot commented Feb 20, 2024 •

edited

Loading

pavanjoshi914 commented Mar 11, 2024 •

edited

Loading