Nem cipher encode and decode. #59

AnthonyLaw · 2021-09-06T11:51:30Z

Added

shared key hash method on nacl-fast.
encode and decode message method.
refactor unpack
- add isNegate (bool) params in unpack method

rg911 · 2021-09-07T08:05:31Z

src/core/nem/NemCrypto.ts

+     *
+     * @returns Keccak-256 hash
+     */
+    private static key_derive(shared: Uint8Array, salt: Uint8Array, privateKey: Key, publicKey: Key): number[] {


I guess we use camel rather than snake case elsewhere?

Also, can the shared be an internal var as only zero bytes (new Uint8Array(32)) get passed in ?

y, don't see reason not to.

rg911 · 2021-09-07T09:38:03Z

src/core/nem/external/nacl-fast.js

@@ -651,6 +651,22 @@
        return 0;
    }

+    function crypto_shared_key_hash(shared, pk, sk, hash) {


shared_key => shared_secret? or perhaps just call it get_scalar_mult_result?

I copied this from the old SDK.
All custom methods added crypto_ as a prefix.

maybe call crypto_scalar_mult_result ?

rg911 · 2021-09-07T09:39:44Z

src/core/nem/external/nacl-fast.js

@@ -811,6 +827,55 @@
      return 0;
    }

+    function unpack(r, p) {


perhaps export unpackneg from js script rather than duplicating it?

thanks, make sense 👍🏼

I did a double-check on the code, compare with unpackneg vs unpack most of the code is the same but here is the different part.

in unpackneg
if (par25519(r[0]) === (p[31]>>7)) Z(r[0], gf0, r[0]);

in unpack
if (par25519(r[0]) !== (p[31]>>7)) Z(r[0], gf0, r[0]);

ah dang, right unpackneg() = unpack AND negate, technically we could extract and/or parametrize this, but not sure if's good idea. cc @Jaguar0625

yea, i'd recommend refactoring unpack and unpackneg

@gimre-xymcity or could we negate after calling unpackneg?

rg911 · 2021-09-07T09:47:04Z

src/core/nem/NemCrypto.ts

+        const message = payload.slice(48);
+
+        const shared = new Uint8Array(32);
+        const key = this.key_derive(shared, salt, privateKey, publicKey);


same as above comments

gimre-xymcity · 2021-09-07T16:54:27Z

src/core/nem/NemCrypto.ts

+     *
+     * @returns Keccak-256 hash
+     */
+    private static key_derive(shared: Uint8Array, salt: Uint8Array, privateKey: Key, publicKey: Key): number[] {


y, don't see reason not to.

gimre-xymcity · 2021-09-07T16:57:35Z

src/core/nem/external/nacl-fast.js

@@ -811,6 +827,55 @@
      return 0;
    }

+    function unpack(r, p) {


gimre-xymcity · 2021-09-07T16:59:28Z

test/core/nem/NemCrypto.spec.ts

+            expect(Converter.uint8ToUtf8(decrypted)).equal(message);
+        });
+
+        it('Encoding throw error if message exceed 976 btyes', () => {


add a tests checking that 976 is actually passing ;)

'bytes' (multiple)

Jaguar0625 · 2021-10-06T15:13:50Z

src/core/nem/external/nacl-fast.js

@@ -811,6 +827,55 @@
      return 0;
    }

+    function unpack(r, p) {


yea, i'd recommend refactoring unpack and unpackneg

Jaguar0625 · 2021-10-06T15:15:04Z

src/core/nem/external/nacl-fast.js

@@ -811,6 +827,55 @@
      return 0;
    }

+    function unpack(r, p) {


@gimre-xymcity or could we negate after calling unpackneg?

Jaguar0625 · 2021-10-06T15:18:27Z

src/core/nem/NemCrypto.ts

+        const salt = payload.slice(0, 32);
+        // 16 byte for iv
+        const iv = payload.slice(32, 48);
+        // 32 byte for payload


comment is wrong

Jaguar0625 · 2021-10-06T15:20:18Z

test/BasicVectorTest.template.ts

+                const encoded = salt
+                    ? NemCrypto.encode(privateKey, otherPublicKey, clearText, iv, salt)
+                    : encode(privateKey, otherPublicKey, clearText, iv);
+
+                const decoded = salt
+                    ? NemCrypto.decode(privateKey, otherPublicKey, Converter.hexToUint8(item.salt + item.iv + item.cipherText))
+                    : decode(privateKey, otherPublicKey, encoded);


why is all this conditional based on salt?

So this vector test is shared between Symbol and Nem.

salt is only used in nem.

all the conditionals look awkward to me, especially for tests, so i'd prefer two functions. but ask @gimre-xymcity his opinion.

I agree with what @Jaguar0625 has written, would rather split in 2

Jaguar0625 · 2021-10-06T15:22:45Z

test/core/nem/NemCrypto.spec.ts

+            expect(Converter.uint8ToUtf8(decrypted)).equal(message);
+        });
+
+        it('Encoding throw error if message exceed 976 btyes', () => {


'bytes' (multiple)

This reverts commit 102141d.

sonarqubecloud · 2022-01-10T12:19:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

gimre-xymcity

please revert "isNegate" as discussed on slack

AnthonyLaw added 2 commits September 6, 2021 15:52

feat: add shared key hash on nacl-fast lib

2825500

feat: message encode & decode cipher method

0b865ba

AnthonyLaw force-pushed the nem-cipher branch from b8b9f46 to b88bd99 Compare September 6, 2021 12:43

test: encode & decode message

a1e7f5e

AnthonyLaw force-pushed the nem-cipher branch from b88bd99 to a1e7f5e Compare September 6, 2021 15:01

AnthonyLaw added 2 commits September 7, 2021 04:07

fix: add more validation

bcd808d

feat: add more test case

ae54bb6

AnthonyLaw marked this pull request as ready for review September 7, 2021 07:27

AnthonyLaw requested review from 0x6861746366574, gimre-xymcity, Jaguar0625 and rg911 September 7, 2021 07:28

rg911 reviewed Sep 7, 2021

View reviewed changes

gimre-xymcity reviewed Sep 7, 2021

View reviewed changes

AnthonyLaw added 2 commits September 8, 2021 17:16

refactor: rename method

640e6c1

test: add message size check

06f20b0

AnthonyLaw requested review from rg911 and gimre-xymcity September 8, 2021 09:17

AnthonyLaw added 3 commits October 5, 2021 15:16

Merge branch 'main' into nem-cipher

303f4c2

feat: add crypto_shared_key interface

8b906f7

test: add cipher vector test in nem.

6dadb16

Jaguar0625 reviewed Oct 6, 2021

View reviewed changes

AnthonyLaw added 3 commits January 7, 2022 14:56

fix: typo

285932d

fix: split vector test template

b63b6d1

fix: refactor unpack

102141d

AnthonyLaw force-pushed the nem-cipher branch 2 times, most recently from 4c79e29 to 102141d Compare January 7, 2022 20:53

AnthonyLaw requested a review from Jaguar0625 January 7, 2022 20:53

Merge branch 'main' into nem-cipher

2d8d2b7

AnthonyLaw added 2 commits January 10, 2022 18:18

Revert "fix: refactor unpack"

cb3dd4b

This reverts commit 102141d.

fix: refactor unpack

aed8479

gimre-xymcity reviewed Jan 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nem cipher encode and decode. #59

Nem cipher encode and decode. #59

AnthonyLaw commented Sep 6, 2021 •

edited

Loading

rg911 Sep 7, 2021 •

edited

Loading

gimre-xymcity Sep 7, 2021

rg911 Sep 7, 2021

AnthonyLaw Sep 8, 2021

rg911 Sep 7, 2021

gimre-xymcity Sep 7, 2021

AnthonyLaw Sep 8, 2021

AnthonyLaw Sep 8, 2021

gimre-xymcity Sep 9, 2021

Jaguar0625 Oct 6, 2021

Jaguar0625 Oct 6, 2021

rg911 Sep 7, 2021

gimre-xymcity Sep 7, 2021

gimre-xymcity Sep 7, 2021

gimre-xymcity Sep 7, 2021

Jaguar0625 Oct 6, 2021

Jaguar0625 Oct 6, 2021

Jaguar0625 Oct 6, 2021

Jaguar0625 Oct 6, 2021

Jaguar0625 Oct 6, 2021

AnthonyLaw Oct 6, 2021

Jaguar0625 Oct 6, 2021

gimre-xymcity Nov 9, 2021

Jaguar0625 Oct 6, 2021

sonarqubecloud bot commented Jan 10, 2022

gimre-xymcity left a comment

Nem cipher encode and decode. #59

Are you sure you want to change the base?

Nem cipher encode and decode. #59

Conversation

AnthonyLaw commented Sep 6, 2021 • edited Loading

Added

rg911 Sep 7, 2021 • edited Loading

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

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

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

sonarqubecloud bot commented Jan 10, 2022

gimre-xymcity left a comment

Choose a reason for hiding this comment

AnthonyLaw commented Sep 6, 2021 •

edited

Loading

rg911 Sep 7, 2021 •

edited

Loading