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

Add in ability to encrypt/decrypt msg with keyring #1070

Merged
merged 7 commits into from
Aug 2, 2021
Merged

Add in ability to encrypt/decrypt msg with keyring #1070

merged 7 commits into from
Aug 2, 2021

Conversation

RoyTimes
Copy link
Contributor

Ref: #1068

  • 1 dependency added to 'util-crypto' - ed2curve by the author of tweetnacl.js
  • tests included
  • covers sr25519 & ed25519, secp256k1 to be added.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Funky hack to make this operational in sr/ed/eth.

Some small comments.

The only think I feel is missing is possibly explicit tests for various keypair types -

  • Ethereum
  • sr25519
  • ed25519

packages/keyring/src/pair/index.ts Outdated Show resolved Hide resolved
packages/keyring/src/pair/index.ts Outdated Show resolved Hide resolved
packages/util-crypto/src/nacl/index.ts Outdated Show resolved Hide resolved
@RoyTimes
Copy link
Contributor Author

I have included test cases for cross sr & ed. I'm fairly positive if the tests can be added and work as expected in a similar fashion as the test for cross sr & ed. However, I'm not sure if it's the right cryptographic thing to do when forcibly converting private keys of the same size from x25519 to secp256k1. Two options here: add in an assert to reject usage on secp256k1 curve; OR just do the conversion and compose test cases for them. What do you think?

@jacogr
Copy link
Member

jacogr commented Jul 30, 2021

As a first-step, rather play safe and assert them away.

We can re-visit if need be or add an alternative approach for them (which would probably be the correct approach here).

@RoyTimes
Copy link
Contributor Author

Done.

On a better non-hacky approach: I have no doubt that all of ed25519, sr25519 and secp256k1 can do ECDH; on a worse side, the library for secp256k1 supports ECDH but not encryption/decryption and I might have to implement a portion of the cryptographic myself. On a better side, introduce new dependencies that is well trusted to do ed25519 sr25519 ECDH. Tweetnacl does not have it built in. For sr25519, that will involve messing with the Rust implementation.

Long story short, it's a pretty heavy load of work to be done. I'm happy to revisit this when I'm more available.

When this is merged, I'll jump to extension.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

LGTM. That you very much.

Like mentioned, this is an interesting approach, certainly not something I thought about to get over the sr25519-doesn't-have-that-in-the-libs (yet).

@polkadot-js-bot
Copy link

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 polkadot-js locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants