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

[refactor] #3422: Ursa replacement #4047

Merged
merged 14 commits into from
Nov 20, 2023

Conversation

DCNick3
Copy link
Contributor

@DCNick3 DCNick3 commented Nov 9, 2023

Description

Ursa is a rust cryptographic library that iroha has been using for a lot of time. It combines several cryptographic libraries from crates.io and provides a unified interface. Unfortunately, it is no longer maintained, so we have to migrate to other ecosystem crates.

This PR does this by importing the parts of ursa code that we have been using into iroha_crypto. It then proceeds to clean the code up a bit, removing unused functionality, updating dependencies and simplifying internal APIs.

In the end, as ursa was just a unifying layer, iroha_crypto ends up using almost the same underlying cryptographic function implementations (except for a switch from C-based secp256k1 to pure-rust k256 for easier use as a wasm library in JS SDK later #4046).

Linked issue

Closes #3422

Benefits

Not having an unmaintained dependency is obviously a plus, allowing us to benefit from updates to upstream libraries.

Future work

#4046, #4045

Checklist

  • make ci pass

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Nov 9, 2023
@DCNick3 DCNick3 marked this pull request as ready for review November 9, 2023 11:40
@DCNick3 DCNick3 marked this pull request as draft November 9, 2023 15:43
@DCNick3 DCNick3 force-pushed the ursa-replacement branch 2 times, most recently from 5f24836 to b082523 Compare November 10, 2023 11:55
@coveralls
Copy link

coveralls commented Nov 10, 2023

Pull Request Test Coverage Report for Build 6927938662

  • 711 of 836 (85.05%) changed or added relevant lines in 19 files are covered.
  • 6821 unchanged lines in 127 files lost coverage.
  • Overall coverage decreased (-3.2%) to 56.281%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/test_network/src/lib.rs 0 1 0.0%
crypto/src/encryption/chacha20poly1305.rs 63 64 98.44%
p2p/src/lib.rs 0 1 0.0%
tools/kagami/src/config.rs 0 1 0.0%
crypto/src/signature/bls/implementation.rs 111 117 94.87%
crypto/src/kex/x25519.rs 54 62 87.1%
crypto/src/signature/bls/mod.rs 26 34 76.47%
crypto/src/signature/ed25519.rs 102 110 92.73%
crypto/src/lib.rs 39 51 76.47%
p2p/src/peer.rs 0 17 0.0%
Files with Coverage Reduction New Missed Lines %
config/base/derive/src/view.rs 1 99.37%
config/src/block_sync.rs 1 95.0%
config/src/network.rs 1 93.75%
config/src/torii.rs 1 95.45%
config/src/wasm.rs 1 87.5%
core/src/smartcontracts/isi/block.rs 1 87.5%
config/src/kura.rs 2 79.41%
config/src/lib.rs 2 0.0%
ffi/src/option.rs 2 71.43%
config/src/genesis.rs 3 72.92%
Totals Coverage Status
Change from base Build 5423219773: -3.2%
Covered Lines: 22921
Relevant Lines: 40726

💛 - Coveralls

@DCNick3 DCNick3 marked this pull request as ready for review November 10, 2023 12:18
@DCNick3 DCNick3 changed the title Ursa replacement [refactor] #3422: Ursa replacement Nov 10, 2023
Cargo.toml Outdated Show resolved Hide resolved
crypto/Cargo.toml Outdated Show resolved Hide resolved
crypto/Cargo.toml Outdated Show resolved Hide resolved
@mversic mversic self-assigned this Nov 13, 2023
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

lgtm, not much to add

crypto/Cargo.toml Outdated Show resolved Hide resolved
crypto/src/encryption/chacha20poly1305.rs Outdated Show resolved Hide resolved
@DCNick3 DCNick3 force-pushed the ursa-replacement branch 2 times, most recently from 01d470a to 5876e75 Compare November 13, 2023 10:57
crypto/Cargo.toml Outdated Show resolved Hide resolved
client_cli/Cargo.toml Outdated Show resolved Hide resolved
@DCNick3 DCNick3 force-pushed the ursa-replacement branch 2 times, most recently from e1a8dac to 15e47a7 Compare November 14, 2023 11:04
crypto/src/lib.rs Show resolved Hide resolved
@mversic mversic requested review from Erigara and mversic November 20, 2023 07:20
…pt to iroha_crypto types

Signed-off-by: Nikita Strygin <[email protected]>
…20Poly1305 code from ursa, migrate iroha_p2p to use it

Signed-off-by: Nikita Strygin <[email protected]>
…o blocks

This also removes bls signature aggregation and rogue key mitigation, as iroha does not and probably will not use aggregated signatures

Signed-off-by: Nikita Strygin <[email protected]>
This makes all the dependency tree of iroha_crypto wasm-compatible (except `getrandom`, which either requires a "js" feature enabled when used from web, or a custom getrandom implementation in other cases)

Signed-off-by: Nikita Strygin <[email protected]>
…tests

Clean up the API a bit:
- hide the implementation details of signatures (they are only used through the PublicKey, PrivateKey and Signature types)
- remove even more unused API functions
- add missing documentation items

Signed-off-by: Nikita Strygin <[email protected]>
- make the signature implementations not use the &self, they are all stateless anyway
- remove redundant Result returns
- add docs on errors

Signed-off-by: Nikita Strygin <[email protected]>
… crate, allows removing dep of iroha_p2p on aead. Also remove buffer-based API from iroha_crypto, it's not used anyways.

Signed-off-by: Nikita Strygin <[email protected]>
…edundant result.is_ok() checks

Signed-off-by: Nikita Strygin <[email protected]>
… dependency from `iroha_crypto` and introduce configurable tls backends to `iroha_client`

openssl-sys was previously added to `iroh_crypto` to allow static builds of openssl with musl libc.

This was somewhat a kludge though, as `iroha_crypto` does not depend on `openssl` (or at least it stopped depending on it after removing `ursa` dependency).

It was used, however, in the client to allow connecting to iroha nodes via HTTPS.

This commit gives the user more freedom in choosing their TLS implementation by providing four features: `tls-native`, `tls-native-vendored`, `tls-rustls-native-roots` and `tls-rustls-webpki-roots`, which mirror corresponding features of `attohttpc` and `tokio-tungstenite`.

Unlike previously, none of the TLS implementations are enabled by default, which is a breaking change

Signed-off-by: Nikita Strygin <[email protected]>
@DCNick3 DCNick3 merged commit df41226 into hyperledger-iroha:iroha2-dev Nov 20, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants