common icon indicating copy to clipboard operation
common copied to clipboard

Asymmetric encryption/decryption feature

Open LaurentTrk opened this issue 3 years ago • 10 comments

Hi all,

This work is an alternative way to add the asymmetric encryption/decryption feature than the one already implemented, which does not work with sr25519 (see workaround proposal by @RoyTimes).

It works more like the eth_decrypt function from Metamask, where only one public key/private key pair is involved : you can encrypt data with the public key of the recipient, and only this recipient can decrypt the data with his private/secret key (vs the already implemented solution using nacl.box which requires 2 keypairs).

What has been done ?

  • Add encrypt/decrypt methods to util-crypto\sr25519 following ECIES
  • Add encrypt/decrypt methods to util-crypto\ed25519 using existing encryption feature, but with an ephemeral key (similar as ECIES)
  • Add encrypt method to keyring (encrypt data without the need of a keypair)
  • Add encrypt/decrypt methods to keyringPair

Implementation details

Following SR25519 Encryption/Decryption following Elliptic Curve Integrated Encryption Scheme (ECIES)
See https://cryptobook.nakov.com/asymmetric-key-ciphers/ecies-public-key-encryption

The algorithms used were chosen among those already used in this library.

  1. Ephemeral Key generation Generate new keypair using the wasm sr25519KeypairFromSeed function, with a random seed from mnemonicGenerate
  2. Key Agreement Use wasm sr25519Agree function between the generated ephemeral private key and the recipient public key
  3. Key Derivation Use pbkdf2 (random salt is generated, default 2048 rounds) to derive a new secret from the previous step output The derived secret is split into :
    • MAC key (first 32 bytes)
    • encryption key (last 32 bytes)
  4. Encryption Use nacl.secretbox api symmetric encryption (xsalsa20-poly1305) to encrypt the message with the encryption key generated at step 3. A nonce (24 bytes) is randomly generated.
  5. MAC Generation HMAC SHA256 (using the MAC key from step 3) of the concatenation of the encryption nonce, ephemeral public key and encrypted message

The encrypted message is the concatenation of the following elements :

  • nonce (24 bytes) : random generated nonce used for the symmetric encryption (step 4)
  • keyDerivationSalt (32 bytes) : random generated salt used for the key derivation (step 3)
  • public key (32 bytes): public key of the ephemeral generated keypair (step 1)
  • macValue (32 bytes): mac value computed at step 5
  • encrypted (remaining bytes): encrypted message (step 4)

Feel free to share your comments and feedbacks :pray:

Nota : Some folks were interested in this feature:

LaurentTrk avatar Dec 20 '21 23:12 LaurentTrk

Generally good. However I have two suggestions:

  1. We need better documentation. The interface exposed is encrypt and decrypt, which hides all the details behind. We should at least include the basic algorithm and message format used in the comments, including:

    • How the ephemeral key pair is generated?
    • How the key is derived? Does it follow any crypto standard?
    • Which symmetric encryption algorithm and parameter is used?

    This is important because once the ecosystem grows, polkadotjs will not be the only library to implement the cryptographic. It's important to have clear guide for other SDKs to implement the standard with common crypto libraries. The later requirement also implies we should try our best to stick to popular crypto standard and algorithm.

  2. Expose the key agreement interface. Per ECIES, under the hood the symmetric key is generated by two parties. In this PR, it only addresses a special case where one party is represented by an ephemeral key. It assumes all the developer has to use an ephemeral key. However, I believe the key agreement is very useful as well. An idea is to leave the key agreement exposed, and then add a new function to generate the ephemeral key. So the two use cases can be fully covered. An example API design:

    • keypair.encrypt(otherPartyPubkey, plainMessage)
    • keypari.decrypt(otherPartyPubkey, encryptedMessage)

    The otherPartyPubkey can be easily generated by:

    const mnemonic = mnemonicGenerate();
    const pair = keyring.addFromUri(mnemonic, { name: 'first pair' }, 'sr25519');
    

h4x3rotab avatar Dec 24 '21 04:12 h4x3rotab

Thanks @h4x3rotab ! I will add implementation details shortly.

Regarding your proposal on exposing the agreement, I would like to propose the 2 options (ephemeral key as ECIES and 2nd party key), depending on the way you call the encrypt method:

  • keypair.encrypt(otherPartyPubkey, plainMessage) : use the private key of the key pair (no need of an ephemeral keypair)
  • keyring.encrypt(otherPartyPubkey, plainMessage) : generate an ephemeral keypair (as specified in ECIES)

In both options, the decrypt() method does not need the public key argument, as the public key used (either from an ephemeral key or a 2nd party key) is embedded in the encrypted message.

Edit: add implementation details to PR description.

LaurentTrk avatar Dec 24 '21 08:12 LaurentTrk

Thanks for this work @LaurentTrk - I'm not able to add any coding or encryption contributions but from a usability point eth_decrypt requires the MetaMask user to decrypt each message by approving the popup dialogue; it would simpler for a user to permit decryption once and allow the dApp to encrypt decrypt at will from thereon. Although this comment may be relevant to the UI implementation following your work here.

defliction avatar Jan 07 '22 19:01 defliction

Hello @LaurentTrk, thanks a lot for this contribution! I reviewed the code and IMHO, everything is fine (documentation, style, tests). Is there something preventing you to change the status of this PR to pending? If not, I suggest that you do so so that maintainers can review it as well. Remove also the [WIP] is the PR's title. ;-)

gdethier avatar Mar 25 '22 11:03 gdethier

Thanks for your comment @gdethier ;)
I was expecting more reviews, but it seems that we are ready now...

LaurentTrk avatar Mar 25 '22 15:03 LaurentTrk

Hello @jacogr ! Did you already have a look at this PR?

We would really love to see this feature integrated as we rely on it in this fork of the Polkadot JS extension which brings encryption/decryption: https://github.com/LaurentTrk/extension/tree/DecryptionFeature (we are actually waiting for this PR to be merged before creating the one on the extension).

If there is anything we can do to make your life easier, please do not hesitate to tell.

gdethier avatar Apr 27 '22 12:04 gdethier

I am also anxiously awaiting this feature, into this library and the corresponding one on the extension. thank you!

mgravitt avatar May 17 '22 23:05 mgravitt

Will bump it up the overflowing task queue. Sorry for the long time getting here, it is on the list, but not a lot of progress seems to be made on my side getting through.

jacogr avatar May 18 '22 06:05 jacogr

Hello @jacogr and thank you very much for your review. Sorry to come back to you so late but I had to test a few things. I tried to implement encryption/decryption without any prior knowledge about the key type but was not completely successful.

Here is what I came up with: https://github.com/LaurentTrk/common/pull/1 (in another PR to keep this one "clean" of my experiments). In the code, when no type is provided, the key type is guessed. However, that method does not work in all cases (this test passes but this one does not).

I am no cryptographer so I think that I reached the limits of what I can achieve. Suggestions are more than welcome.

gdethier avatar Jun 28 '22 10:06 gdethier

Hi @gdethier !
Thanks for your work, that's could be great yes, but unfortunately, sometimes just checking that a key could fit in one or the other type is not enough, some keys fit in both, but that leads to incorrect encryption/decryption.
That was the problem in the very initial version of this feature: sr22519 were forced to be treated as ed25519, but that just not work (despite the unit tests proving so, that were using keys that fitted both key types).

LaurentTrk avatar Jul 04 '22 15:07 LaurentTrk

Closing, removing this functionality in https://github.com/polkadot-js/common/pull/1762

jacogr avatar Mar 14 '23 05:03 jacogr

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

polkadot-js-bot avatar Apr 04 '23 07:04 polkadot-js-bot