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

feat(softsign): expanded secret key can be used for signing #891

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

greg-szabo
Copy link

@greg-szabo greg-szabo commented Apr 3, 2024

This is a reimplementation of #742.

Unfortunately, ed25519_consensus doesn't support signing with the expanded secret key, so I had to rely on ed25519-dalek. The new version of that crate hid the ExpandedSecretKey struct behind the hazmat feature, and other crates seem not to support signing with the expanded secret key. (Everyone's hashing internally.)

This is still the least painful way of using secrets exported from YubiHSMs.

Edit:
Key size assumptions on softsign consensus key:

If the input key is 32 bytes, it is assumed to be a seed key. (This is the original behavior.)
If the input key is 64 bytes, it is assumed to be an expanded secret key. (SHA512(seed key))
If the input key is 96 bytes, it is assumed to be an export of the yubihsm-unwrap application, which encodes the first 32 bytes in little-endian (as stored on the YubiHSM device) and adds the public key to the file. The application will reverse these bytes and use them as a regular expanded secret key.

I wanted to preserve the option of using sha512-hashed keys while supporting the YubiHSM-exported keys, because it's so easy to create them even using the command line.

Example:

tmkms softsign keygen seed.key
cat seed.key | base64 -d | shasum -a 512 -b | cut -d' ' -f1 | xxd -r -p | base64 > expanded.key

(xxd will convert the hexstring output of shasum into bytes.)

VerifyingKey(self.0.verification_key())
match &self {
SigningKey::Ed25519(signing_key) => VerifyingKey(signing_key.verification_key()),
SigningKey::Ed25519Expanded(signing_key) => Into::<ed25519_dalek::VerifyingKey>::into(signing_key).as_bytes().as_slice().try_into().unwrap(),
Copy link
Author

Choose a reason for hiding this comment

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

This is a trick so I don't have to reimplement the verifying key for expanded secret keys separately:

  1. Convert the ExpandedSecretKey into ed25519-dalek's VerifyingKey (trait implemented in ed25519-dalek)
  2. Get the bytes from the VerifyingKey, turn them into a slice
  3. Use the trait defined in ed25519-consensus to turn a slice of bytes into an ed25519-consensus VerifyingKey.

The verifying key implementation already falls back to this function so this way I could implement both types of verifying keys in one go.

@tony-iqlusion
Copy link
Member

Hmm, it seems not super great to have two Ed25519 libraries. I've also been planning to refactor the softsign backend and this would complicate things.

Perhaps you could inquire about getting this functionality added to ed25519-consensus?

@greg-szabo
Copy link
Author

I talked to Henry, and he considers the ed25519-consensus library done. All he could promise me is that he'll take a look at a PR if someone implements the feature.

It's discouraging to pass through this moving goalpost. I have implemented the feature to tmkms twice, and now I need to do it a third time. I have no confidence that the feature will land in two repositories and be released in a timely manner for validators to use. It would be nice to have a measurable definition of done that I can work against to get this done.

@iqlusioninc iqlusioninc deleted a comment from tarcieri Apr 5, 2024
Comment on lines +10 to +15
pub enum SigningKey {
/// Ed25519 signing key.
Ed25519(ed25519_consensus::SigningKey),
/// Ed25519 expanded signing key.
Ed25519Expanded(ExpandedSecretKey),
}
Copy link
Member

@tony-iqlusion tony-iqlusion Apr 5, 2024

Choose a reason for hiding this comment

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

Instead of having two signing codepaths that duplicate each other and use different dependencies to accomplish the same thing depending on whether or not the key has been pre-expanded, I think this PR would be a lot less messy if it moved (back) to ed25519-dalek wholesale, since apparently there is no path forward on supporting ExpandedSecretKey-like functionality in ed25519-consensus.

Copy link
Member

Choose a reason for hiding this comment

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

My preference would still be to unify these somehow. You should be able to initialize an ExpandedSecretKey from either a seed or its SHA-512 expansion.

Then this could just be:

Suggested change
pub enum SigningKey {
/// Ed25519 signing key.
Ed25519(ed25519_consensus::SigningKey),
/// Ed25519 expanded signing key.
Ed25519Expanded(ExpandedSecretKey),
}
pub struct SigningKey(ExpandedSecretKey);

@greg-szabo
Copy link
Author

Thanks for the ideas, Tony!

I have implemented one version to discuss with Henry in ed25519-consensus (PR 12). It's incomplete; serialization can be cumbersome in a no_std world.

Depending on Henry's opinion, I might need to do a different version, but as long as he's open to some version of it, you could keep using ed25519-consensus and have both key types supported.

I hope we can make it feasible in ed25519-consensus, but reverting back to ed25519-dalek would definitely be an easy solution.

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.

2 participants