feat(softsign): expanded secret key can be used for signing #891
Conversation
| 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(), |
There was a problem hiding this comment.
This is a trick so I don't have to reimplement the verifying key for expanded secret keys separately:
- Convert the ExpandedSecretKey into ed25519-dalek's VerifyingKey (trait implemented in ed25519-dalek)
- Get the bytes from the VerifyingKey, turn them into a slice
- 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.
|
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 |
|
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. |
| pub enum SigningKey { | ||
| /// Ed25519 signing key. | ||
| Ed25519(ed25519_consensus::SigningKey), | ||
| /// Ed25519 expanded signing key. | ||
| Ed25519Expanded(ExpandedSecretKey), | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
| pub enum SigningKey { | |
| /// Ed25519 signing key. | |
| Ed25519(ed25519_consensus::SigningKey), | |
| /// Ed25519 expanded signing key. | |
| Ed25519Expanded(ExpandedSecretKey), | |
| } | |
| pub struct SigningKey(ExpandedSecretKey); |
|
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. |
We've had many requests to leverage `ed25519-dalek`'s `ExpandedSecretKey` functionality in order to support keys exported from YubiHSMs. See #978, #891, and #742. I have been a bit wary of the implementation though, because it has always involved having both `ed25519-consensus` and `ed25519-dalek`, often with enums over multiple key types, which seems incredibly overcomplicated just to implement this feature. We originally switched to `ed25519-consensus` because `tendermint-rs` did, and it makes sense there where ZIP-215 provides consensus-critical signature verification rules that don't diverge between implementations. However that's a bit irrelevant for our purposes as this is a signing service, and ZIP-215 allows us to use whatever signing implementation we want. Now that we've vendored `tendermint-p2p` as `tmkms-p2p` we can unilaterally switch the Ed25519 implementation (back) to `ed25519-dalek`, which should make adding `ExpandedSecretKey` support much easier and avoid having to worry about two implementations. This additionally switches (back) to upstream `curve25519-dalek` so as to support X25519 as part of the SecretConnection protocol.
We've had many requests to leverage `ed25519-dalek`'s `ExpandedSecretKey` functionality in order to support keys exported from YubiHSMs. See #978, #891, and #742. I have been a bit wary of the implementation though, because it has always involved having both `ed25519-consensus` and `ed25519-dalek`, often with enums over multiple key types, which seems incredibly overcomplicated just to implement this feature. We originally switched to `ed25519-consensus` because `tendermint-rs` did, and it makes sense there where ZIP-215 provides consensus-critical signature verification rules that don't diverge between implementations. However that's a bit irrelevant for our purposes as this is a signing service, and ZIP-215 allows us to use whatever signing implementation we want. Now that we've vendored `tendermint-p2p` as `tmkms-p2p` we can unilaterally switch the Ed25519 implementation (back) to `ed25519-dalek`, which should make adding `ExpandedSecretKey` support much easier and avoid having to worry about two implementations. This additionally switches (back) to upstream `curve25519-dalek` so as to support X25519 as part of the SecretConnection protocol.
We've had many requests to leverage `ed25519-dalek`'s `ExpandedSecretKey` functionality in order to support keys exported from YubiHSMs. See #978, #891, and #742. I have been a bit wary of the implementation though, because it has always involved having both `ed25519-consensus` and `ed25519-dalek`, often with enums over multiple key types, which seems incredibly overcomplicated just to implement this feature. We originally switched to `ed25519-consensus` because `tendermint-rs` did, and it makes sense there where ZIP-215 provides consensus-critical signature verification rules that don't diverge between implementations. However that's a bit irrelevant for our purposes as this is a signing service, and ZIP-215 allows us to use whatever signing implementation we want. Now that we've vendored `tendermint-p2p` as `tmkms-p2p` we can unilaterally switch the Ed25519 implementation (back) to `ed25519-dalek`, which should make adding `ExpandedSecretKey` support much easier and avoid having to worry about two implementations. This additionally switches (back) to upstream `curve25519-dalek` so as to support X25519 as part of the SecretConnection protocol.
2 participants
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-unwrapapplication, 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:
(
xxdwill convert the hexstring output of shasum into bytes.)