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 hazmat module with ExpandedSecretKey, raw_sign, raw_sign_prehashed #299

Merged
merged 22 commits into from
May 15, 2023

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Apr 10, 2023

This splits out the signing functionality we have into two new functions. Nothing changes underlyingly. The only difference is now users can use raw_sign() and raw_sign_prehashed() if you enable the raw_sign feature (and digest feature as well for the latter).

Also this renames ExpandedSecretKey::nonce to hash_prefix, because that's what it is, and it is absolutely used more than once.

Fixes #298.

Why expose these functions

Our removal of the public ExpandedSecretKey API in #205 made it impossible for users to sign using an ExpandedSecretKey. The only way to sign now is SigningKey::sign. The issue here is that some downstream users, as noted in #298, never actually saved their SigningKey, and only saved the ExpandedSecretKey. You cannot recover a SigningKey from an ExpandedSecretKey, since the former is the seed whose hash is expanded to the latter. Thus, it is necessary to expose some lower-level way of computing signatures. There were a few levels of abstraction we could have picked here, and I'm open to suggestions.

Separately, and I don't know how important this is, but these functions would allow someone to build BIP32-Ed25519 on top of this library, since scalar and hash_prefix are explicit inputs to the raw_sign() function.

@rozbb rozbb requested a review from tarcieri April 10, 2023 07:22
@tarcieri
Copy link
Contributor

My preference would be to stick the pub functions in a hazmat module with a big fat warning, similar to this:

https://docs.rs/ecdsa/latest/ecdsa/hazmat/index.html

@rozbb
Copy link
Contributor Author

rozbb commented Apr 10, 2023

That looks good. Will do

src/signing.rs Outdated Show resolved Hide resolved
src/signing.rs Outdated Show resolved Hide resolved
src/signing.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Contributor

@rozbb if you do extract a hazmat module it'd be good to link to https://github.com/MystenLabs/ed25519-unsafe-libs and document how things could go wrong if the public key isn't computed from the secret key

@benma
Copy link

benma commented Apr 21, 2023

but these functions would allow someone to build BIP32-Ed25519 on top of this library

Fyi, I made such a library (you can find it here), and I would appreciate if this PR made it into 2.0, as I previously relied on ExpandedSecretKey::from_bytes to use the BIP32 expanded private key to sign a message here.

@jplatte
Copy link

jplatte commented Apr 27, 2023

@rozbb gentle ping, are you expecting to finish this soon?

Alternatively, could we maybe get a patch release on x25519-dalek with the zeroize dependency unpinned? It seems like it causes a new problem once every week 😢

@rozbb
Copy link
Contributor Author

rozbb commented May 1, 2023

@jplatte I apologize, I've been swamped at work lately. I will definitely get to this this week. The existing PR is pretty close to the finish line so it shouldn't take long.

@rozbb
Copy link
Contributor Author

rozbb commented May 6, 2023

Ok, I think all concerns should be addressed, pending 1 unresolved conversation.

To address #298, I brought back ExpandedSecretKey. One notable change is that it uses Scalar::from_bytes_modulo_order() for deserialization rather than the Scalar::from_bits() function. Strictly speaking, this is a meaningful change. But the only way you'd notice the difference is if you were somehow serializing unreduced expanded secret key scalars. I really doubt that'll be an issue.

@rozbb rozbb changed the title Add raw_sign and raw_sign_prehashed Add hazmat module with ExpandedSecretKey, raw_sign, raw_sign_prehashed May 6, 2023
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

From my point of view this is all we need and looks great. Thanks a lot for bringing back ExpandedSecretKey as well.

@rozbb
Copy link
Contributor Author

rozbb commented May 7, 2023

Alright, @tarcieri ready to review. Sorry for the huge edits. This weirdly affected a lot of things.

src/hazmat.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Contributor

One nit on from_bytes accepting a slice but otherwise this looks good to me.

Co-authored-by: Tony Arcieri <bascule@gmail.com>
Copy link
Contributor

@tarcieri tarcieri 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 now, although for bonus points an ExpandedSecretKey::from_slice method and/or TryFrom<&[u8]> impl would be nice.

@rozbb
Copy link
Contributor Author

rozbb commented May 15, 2023

Good idea, thanks!

@rozbb rozbb merged commit 4afbf09 into dalek-cryptography:main May 15, 2023
9 checks passed
@tarcieri tarcieri mentioned this pull request May 15, 2023
4 tasks
@jplatte
Copy link

jplatte commented May 24, 2023

Any plans for the next (pre-)release? :)

@rozbb rozbb mentioned this pull request Jun 11, 2023
@benma
Copy link

benma commented Jun 19, 2023

Any plans for the next (pre-)release? :)

Seconded :)

@tarcieri
Copy link
Contributor

It's blocked on dalek-cryptography/curve25519-dalek#531

@tarcieri
Copy link
Contributor

It's out

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.

Reintroduce ExpandedSecretKey funcionality
5 participants