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

[suggestion]: Use concrete key types in iroha_crypto #4045

Closed
DCNick3 opened this issue Nov 9, 2023 · 0 comments
Closed

[suggestion]: Use concrete key types in iroha_crypto #4045

DCNick3 opened this issue Nov 9, 2023 · 0 comments
Assignees
Labels
api-changes Changes in the API for client libraries crypto iroha2-dev The re-implementation of a BFT hyperledger in RUST Optimization Something isn't working as well as it should Refactor Improvement to overall code quality

Comments

@DCNick3
Copy link
Contributor

DCNick3 commented Nov 9, 2023

(note that this only makes sense to do after the PR for #3422 lands)

Background

For most asymmetric cryptographic algorithm, the keys are not just "an array of bytes" but they actually have some internal structure. Constructing them from bytes requires a parsing step that can fail. Currently we do not reflect this structure in PublicKey and PrivateKey types, storing them as an array of bytes tagged with an algorithm name.

This is unfortunate because we end up copying & deserializing the keys each time we do any cryptographic operation with it. This:

  1. might have some performance impact
  2. delays the key validation errors to the stage where we actually try to use the key

The suggestion

I suggest to change the key types to instead be something akin to:

pub enum PublicKey {
    Ed25519(ed25519_dalek::VerifyingKey),
    Secp256k1(k256::PublicKey),
    ...
}

In this case the keys are stored in native representations of the crypto traits we use and do not require any conversion to work with. The conversions would now happen at the serialization/deserialization stage.

The smart contract problem

This would, however, require a change to how we treat iroha_crypto in smart contracts. Currently smart contracts can manipulate the cryptographic keys, but cannot do actual cryptographic operations (relying on the host to validate everything for them). Changing the PublicKey type like this would, however, require to actually include at least the parsing part of cryptographic libraries to the smart contracts.

We are, however, focused on reducing the wasm binary size, so this is not feasible as-is.

One solution I can think of is to have separate types for unparsed/parsed key representations. This seems feasible on the surface, but should be investigated further.

@DCNick3 DCNick3 added iroha2-dev The re-implementation of a BFT hyperledger in RUST api-changes Changes in the API for client libraries Refactor Improvement to overall code quality labels Nov 9, 2023
@DCNick3 DCNick3 changed the title [suggestion] Use concrete key types in iroha_crypto [suggestion]: Use concrete key types in iroha_crypto Nov 10, 2023
@mversic mversic added the Optimization Something isn't working as well as it should label Nov 21, 2023
@Arjentix Arjentix self-assigned this Dec 5, 2023
Arjentix added a commit to Arjentix/iroha that referenced this issue Dec 28, 2023
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Dec 28, 2023
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 10, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@DCNick3 DCNick3 added the crypto label Jan 10, 2024
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 11, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 22, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 23, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 23, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 24, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 24, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Arjentix added a commit to Arjentix/iroha that referenced this issue Jan 28, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to Arjentix/iroha that referenced this issue Feb 6, 2024
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit that referenced this issue Feb 6, 2024
* [refactor] #4045: Use concrete key types

* [feature]: Allow `iroha_crypto` in WASM

* [refactor]: Replace amcl_wrapper with w3f-bls

* [refactor]: Hide random key generation under feature flag

* [ci]: Fix docker compose consistency check

* [refactor]: Remove `Box` from `PrivateKey`

* [refactor] #4199: Sign by reference

* [refactor]: Reduce key size with Box

---------

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@AlexStroke AlexStroke self-assigned this Feb 7, 2024
@alexstroke1 alexstroke1 assigned alexstroke1 and unassigned AlexStroke Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries crypto iroha2-dev The re-implementation of a BFT hyperledger in RUST Optimization Something isn't working as well as it should Refactor Improvement to overall code quality
Projects
None yet
Development

No branches or pull requests

5 participants