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

feat(crypto)!: libsodium crypto implementation #574

Merged
merged 4 commits into from
Sep 7, 2023

Conversation

AngelCastilloB
Copy link
Member

@AngelCastilloB AngelCastilloB commented Jan 10, 2023

Context

Currently, the SDK uses the Cardano Multiplatform Lib to perform all Ed25519-related operations. We want to replace this dependency by implementing our own package that uses libsodium for cryptographic operations.

Proposed Solution

Introduce a new set of ed25519 classes to the crypto package based on libsodium. There is also a new BIP32 set of classes implemented natively in javascript for key public and private key derivation.

Critical Segments

This PR introduces some code that must be audited and verified, as we consider it to be critical:

  • BIP-32 key derivation: The core logic that does the derivation is contained in this ts file Bip32KeyDerivation.ts , and the following classes are implemented in terms of the functions defined in such file: Bip32PrivateKey.ts and Bip32PublicKey.ts
  • Ed25519 Extended: Libsodium doesn't support out-of-the-box extended keys in its high-level API (crypto_sign and crypto_sign_detached), so we had to implement our own sign method using the Ed25519 primitives provided by the libsodium library, said function can be found here: signExtendedDetached, the following classes will be also a point of interest for reviewers: Ed25519PrivateKey.ts and Ed25519PublicKey.ts

Important Changes Introduced

  • A new set of classes were introduced to the 'crypto' package, which contains several classes and utilities to perform Ed25519 cryptographic operations using libsodium.
  • Added a new implementation of the Bip32Ed25519 interface.
  • Unit tests and e2e test now uses the new SodiumBip32Ed25519 concrete class by default.

@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch 2 times, most recently from 891659d to 3d4ae6a Compare January 10, 2023 23:02
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch from 3fbd797 to 6900223 Compare January 11, 2023 20:50
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch from 4cb292b to 18e1e2d Compare January 12, 2023 17:01
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Fantastic work here @AngelCastilloB

packages/crypto/LICENSE Outdated Show resolved Hide resolved
packages/crypto/NOTICE Outdated Show resolved Hide resolved
packages/crypto/README.md Outdated Show resolved Hide resolved
packages/crypto/README.md Outdated Show resolved Hide resolved
packages/crypto/package.json Outdated Show resolved Hide resolved
packages/crypto/package.json Outdated Show resolved Hide resolved
packages/crypto/package.json Outdated Show resolved Hide resolved
packages/crypto/package.json Outdated Show resolved Hide resolved
packages/crypto/src/Bip32/Bip32KeyDerivation.ts Outdated Show resolved Hide resolved
packages/crypto/src/Bip32/Bip32KeyDerivation.ts Outdated Show resolved Hide resolved
@rhyslbw rhyslbw changed the title [WIP]Feat/implement crypto pachage [WIP]Feat/implement crypto package Jan 13, 2023
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch from c9244ac to a11bdd9 Compare January 14, 2023 00:04
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch 3 times, most recently from a747730 to 5ebe69d Compare January 17, 2023 17:36
@AngelCastilloB AngelCastilloB changed the title [WIP]Feat/implement crypto package Feat/implement crypto package Jan 18, 2023
@AngelCastilloB AngelCastilloB requested review from mkazlauskas, rhyslbw and a team January 18, 2023 04:08
@AngelCastilloB AngelCastilloB changed the title Feat/implement crypto package feat(crypto): Implement crypto package Jan 18, 2023
@AngelCastilloB AngelCastilloB changed the title feat(crypto): Implement crypto package feat(crypto)!: Implement crypto package Jan 18, 2023
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch from 5ebe69d to a27b4ab Compare January 20, 2023 05:07
Copy link

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

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

First round of comments.

packages/crypto/src/Ed25519e/Ed25519KeyHash.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519e/Ed25519PrivateKey.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519e/Ed25519PrivateKey.ts Outdated Show resolved Hide resolved
packages/crypto/src/Ed25519e/Ed25519PrivateKey.ts Outdated Show resolved Hide resolved
packages/crypto/src/Bip32/Bip32KeyDerivation.ts Outdated Show resolved Hide resolved
packages/crypto/src/Bip32/Bip32KeyDerivation.ts Outdated Show resolved Hide resolved
packages/crypto/src/Bip32/Bip32PrivateKey.ts Show resolved Hide resolved
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Some additional refactoring is needed:

  • typedHex() should probably live in util package just like opaqueTypes (seems to be duplicated in core & crypto packages right now)
  • Base hash types (e.g. Hash32ByteBase16) should probably live in crypto package

@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch 2 times, most recently from d740a48 to 702d341 Compare January 27, 2023 16:51
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch 2 times, most recently from 6737b0d to ea79235 Compare February 7, 2023 12:51
@AngelCastilloB
Copy link
Member Author

Some additional refactoring is needed:

  • typedHex() should probably live in util package just like opaqueTypes (seems to be duplicated in core & crypto packages right now)
  • Base hash types (e.g. Hash32ByteBase16) should probably live in crypto package

This was addressed in this PR #589

@rhyslbw rhyslbw changed the title feat(crypto)!: Implement crypto package feat(crypto)!: libsodium crypto implementation Feb 8, 2023
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch 7 times, most recently from 0d51847 to ce79d4e Compare July 18, 2023 10:08
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Let's add a notification in the crypto package README, warning users the implementation has not yet been audited, and use at this stage is at own risk. Can also add this in the package.json description.

@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch 2 times, most recently from 606100f to 29dc5a5 Compare September 7, 2023 11:36
packages/crypto/README.md Outdated Show resolved Hide resolved
@AngelCastilloB AngelCastilloB force-pushed the feat/implement-ed25519-crypto-provider-interface branch from 5770cec to e11a9a8 Compare September 7, 2023 13:16
@rhyslbw rhyslbw requested review from iquerejeta, rhyslbw and mkazlauskas and removed request for iquerejeta September 7, 2023 15:59
@rhyslbw rhyslbw dismissed mkazlauskas’s stale review September 7, 2023 16:01

Feedback addressed

@rhyslbw rhyslbw merged commit b0f9072 into master Sep 7, 2023
@rhyslbw rhyslbw deleted the feat/implement-ed25519-crypto-provider-interface branch September 7, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants