Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Add method to derive x25519 from an ed25519 private key #112

Merged

Conversation

andrewwhitehead
Copy link
Contributor

This method is compatible with libsodium's crypto_sign_ed25519_sk_to_curve25519.

This PR also adds Ed25519Sha519::keypair_from_secret which is compatible with libsodium's crypto_sign_seed_keypair. @mikelodder7 I didn't add this as a separate KeyGenOption as it didn't seem to be generally applicable, but I'll let you be the judge.

@andrewwhitehead andrewwhitehead changed the title Add method to derive x25519 from a private key Add method to derive x25519 from an ed25519 private key Apr 8, 2020
Copy link
Contributor

@mikelodder7 mikelodder7 left a comment

Choose a reason for hiding this comment

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

Have you tested this with indy to verify compatibility?

@andrewwhitehead
Copy link
Contributor Author

Yes, as much as I can. The last test is deriving a common Indy keypair.

Copy link
Contributor

@mikelodder7 mikelodder7 left a comment

Choose a reason for hiding this comment

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

Could you create a branch in indy-sdk in your own fork with tests showing this working?

@andrewwhitehead
Copy link
Contributor Author

andrewwhitehead commented Apr 15, 2020

@mikelodder7 Tested here: https://github.com/andrewwhitehead/indy-sdk/tree/test/derive-key

In particular indy_create_key_works_for_seed and the tests in indy-utils/src/crypto/ed25519_sign/sodium.rs

@mikelodder7
Copy link
Contributor

@andrewwhitehead I'm not seeing that function in that branch in that file. Can you provide the direct link?

@andrewwhitehead
Copy link
Contributor Author

@mikelodder7 It's over in https://github.com/andrewwhitehead/indy-sdk/blob/test/derive-key/libindy/tests/crypto.rs

The whole test suite runs anyway, apart from some unrelated pool genesis transaction tests. indy_crypto_sign_works might be more relevant as it's generating a keypair from a seed, signing data and comparing the result to an expected value.

@kdenhartog
Copy link
Contributor

kdenhartog commented Apr 18, 2020

The function that does the bilateral mapping by dalek-X25519 can be found here: https://github.com/dalek-cryptography/curve25519-dalek/blob/17698df9d4c834204f83a3574143abacb4fc81a5/src/edwards.rs#L473

On Indy-sdk side I've tracked it down to using this libsodium function: https://github.com/jedisct1/libsodium/blob/927dfe8e2eaa86160d3ba12a7e3258fbc322909c/src/libsodium/crypto_sign/ed25519/ref10/keypair.c#L46

They appear to share the same math function of (1+Y_P)/(1-Y_P) although the dalek implementation seems to be doing a different equation of (Z+Y)/(Z-Y) which depends on "Z" that's considered equivalent in the comments.

So from what I can tell they're both doing this mapping of points, but using different math operations to do so is all.

@mikelodder7
Copy link
Contributor

@andrewwhitehead I see the test but what I'm interested in is whether this code you are pushing to Ursa is functionally the same. I actually think this code could be moved to Indy and kept out of Ursa since this is an Indy specific implementation.

@kdenhartog
Copy link
Contributor

@andrewwhitehead I see the test but what I'm interested in is whether this code you are pushing to Ursa is functionally the same. I actually think this code could be moved to Indy and kept out of Ursa since this is an Indy specific implementation.

This is quite common functionality in libraries. From what I know, libsodium, monocipher, and dalek-cryptography currently support this. I see it as beneficial to be kept within ursa.

@andrewwhitehead
Copy link
Contributor Author

I think the sign_key_to_key_exchange method is pretty much essential in order to do anything useful with verkey_to_key_exchange which is already in there.

I don't think keypair_from_secret can be implemented in Indy without adding ed25519_dalek as an explicit dependency on top of Ursa (or rewriting it). It's also not an Indy-specific method for creating the secret key, it just expands the secret part to a full keypair.

@mikelodder7
Copy link
Contributor

@andrewwhitehead and I met over zoom to discuss the details. I'm happy with it now.

@@ -34,6 +34,36 @@ impl Ed25519Sha512 {
))),
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments here to document the purpose of the sign_key_to_key_exchange function and how to use it.

let secret = x25519_dalek::StaticSecret::from(output);
Ok(PrivateKey(secret.to_bytes().to_vec()))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments here to document the purpose of the expand_keypair function and how to use it

…ey creation

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
@brentzundel brentzundel merged commit 5444d41 into hyperledger-archives:master Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants