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 X25519 key and verification method support #735

Merged
merged 20 commits into from
Mar 22, 2022
Merged

Add X25519 key and verification method support #735

merged 20 commits into from
Mar 22, 2022

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Mar 20, 2022

Description of change

Adds support for X25519 cryptographic key pairs for performing Elliptic Curve Diffie-Hellman (ECDH) key exchange operations and the X25519KeyAgreementKey2019 verification method type.

There are some open questions about the design of the KeyExchange trait which uses PhantomData rather than generics (in line with current conventions for Sign and Verify) and several TODOs related to removing MerkleKeyCollections in #715.

Added

  • Add MethodType::X25519KeyAgreementKey2019.
  • Add KeyType::X25519.
  • Add MethodSecret::X25519.
  • Add X25519 KeyExchange implementation.
  • Add Diffie-Hellman key exchange example.
  • Add MethodData Wasm bindings.

Changed

  • Change KeyPair::try_from_ed25519_bytes to try_from_private_key_bytes.
  • Rename MethodData::new_b58 to new_base58.
  • Rename VerificationMethod::key_type to type_.
  • Rename VerificationMethod::key_data to data.

Removed

  • Remove KeyPair::new_ed25519.

Links to any relevant issues

Fixes #671.

Type of change

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Local tests and new examples work.

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@cycraig cycraig added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Mar 20, 2022
@cycraig cycraig marked this pull request as draft March 20, 2022 18:59
@cycraig cycraig changed the title [WIP] Add X25519 key type and verification method [WIP] Add X25519 key and verification method support Mar 21, 2022
@cycraig cycraig added Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog and removed Added A new feature that requires a minor release. Part of "Added" section in changelog labels Mar 21, 2022
KeyPair::try_from_private_key_bytes
@cycraig cycraig marked this pull request as ready for review March 21, 2022 14:41
@cycraig cycraig changed the title [WIP] Add X25519 key and verification method support Add X25519 key and verification method support Mar 21, 2022
Copy link
Contributor

@HenriqueNogara HenriqueNogara left a comment

Choose a reason for hiding this comment

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

LGTM

identity-core/src/crypto/exchange/key_exchange.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Some comments regarding the outstanding questions.

identity-core/src/crypto/exchange/x25519.rs Outdated Show resolved Hide resolved
Comment on lines +18 to +20
// TODO: refactor with exact types for each key type? E.g. Ed25519KeyPair, X25519KeyPair etc.
// Maybe a KeyPair trait with associated types? Might need typed key structs
// like Ed25519Public, X25519Private etc. to avoid exposing pre-1.0 dependency types.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that makes sense, at least in the current situation. Typically, we cannot use a KeyPair interchangeably, e.g. a document can only be created from an Ed25519KeyPair. On the other hand, for generation or storage we don't care about the type, so perhaps we have to retain the KeyPair type as an abstraction? (Not thought through.)

Copy link
Contributor Author

@cycraig cycraig Mar 22, 2022

Choose a reason for hiding this comment

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

Yeah I'm hesitant to do a KeyPair refactor at this point since it has sweeping implications everywhere. One concern I have is that with introducing specific types we multiply the number of structs we have to make bindings for.

E.g. currently we have KeyPair with Public and Private keys represented as strings/UInt8Array soon, so basically one type exposed to the bindings. With a refactor we might have 6 different structs:

  • Ed25519KeyPair
  • Ed25519Private
  • Ed25519Public
  • X25519KeyPair
  • X25519Private
  • X25519Public

And more for each new KeyType we choose to support. Note that the specific types for keys may be important for places where we only want/need the public key, such as creating a new verification method or only allowing Ed25519 for creating a new IotaDocument or IotaDID. We've largely moved away from requiring KeyPairs in function parameters where the private key is not required.

If we do go ahead with such a refactor I think a KeyPair trait would be useful for generalization, e.g.

trait KeyPair {
  type Public;
  type Private;

  fn public(&self) -> &Self::Public;
  fn private(&self) -> &Self::Private;
  fn key_type(&self) -> KeyType;
}

I don't think that will be in this PR, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanations. It's fine to leave this for a future PR. Turning KeyPair into a trait is what I had in mind, too, that sounds good! I agree that we need to carefully consider the impact before going ahead, specifically for Wasm.

identity-core/src/utils/ed25519.rs Show resolved Hide resolved
identity-did/src/verification/method_type.rs Outdated Show resolved Hide resolved
@cycraig cycraig merged commit 742eba5 into dev Mar 22, 2022
@cycraig cycraig deleted the feat/x25519 branch March 22, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] Support X25519KeyAgreementKey2019
3 participants