ed25519-dalek: make ExpandedSecretKey fields private
#544
Conversation
| @@ -106,6 +120,18 @@ impl ExpandedSecretKey { | |||
| .into() | |||
| }) | |||
| } | |||
|
|
|||
| /// Construct an `ExpandedSecretKey` from a scalar and hash prefix. | |||
| pub fn from_scalar_and_prefix(scalar: Scalar, hash_prefix: HashPrefix) -> Self { | |||
There was a problem hiding this comment.
I thought from_scalar_and_hash_prefix was a bit unwieldy but it would be more consistent.
This method could probably use a longer comment explaining its use cases (e.g. Ed25519-BIP32).
There was a problem hiding this comment.
I think naming is fine.
More docs here. I do feel weird about linking to niche cryptocurrency specs from our repo though, especially in the hazmat section. I included the text anyway. I vote we cut the whole second paragraph, but I'll defer to you.
Construct an
ExpandedSecretKeyfrom a scalar and hash prefix. In the spec, these are derived a SHA-512 hash. Specifically, the scalar is derived by clamping and reducing the first 32 bytes of the hash. And the prefix is set equal to the second 32 bytes.This constructor might be useful when the given
scalaris derived by some arithmetic means, such as in the BIP-32 hierarchical key derivation scheme.
| impl From<(Scalar, HashPrefix)> for ExpandedSecretKey { | ||
| fn from(parts: (Scalar, HashPrefix)) -> ExpandedSecretKey { | ||
| ExpandedSecretKey::from_scalar_and_prefix(parts.0, parts.1) | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels a bit debatable
There was a problem hiding this comment.
My opinion is that there shouldn't be a constructor with unnamed types unless the struct is overtly made from those parts. Since everything is pub(crate) now (as, I agree, it should be), this makes less sense imo.
Motivation can be found in this comment: dalek-cryptography/ed25519-dalek#293 (review) Replaces public fields with private fields + accessor methods. Also adds an `ExpandedSecretKey::from_scalar_and_prefix` constructor which makes it possible to construct `ExpandedSecretKey` using a `Scalar` again, which is useful for protocols that derive the scalar such as Ed25519-BIP32. Additionally defines a `HashPrefix` type alias to make it semantically clear which `[u8; 32]` is involved in type signatures.
tarcieri
force-pushed
the
ed25519-dalek/make-expanded-secret-key-fields-private
branch
from
5138ef0 to
58f2855
Compare
| @@ -40,14 +43,15 @@ pub struct ExpandedSecretKey { | |||
| // modulo l. | |||
| pub(crate) scalar_bytes: [u8; 32], | |||
There was a problem hiding this comment.
Sidebar: perhaps this should be canonical_scalar_bytes?
There was a problem hiding this comment.
Actually, now that you bring this up again, I'm not sure if this was even the right choice. The original reason I included scalar_bytes was because section 5.1.5 didn't say you do a modular reduction of the clamped integer when computing the pubkey. So I kept this by the letter of the law.
But this detail doesn't matter! Yes it is true that sometimes s.reduce() * H != s * H, but this is only for H that are not in the prime subgroup. The only place scalar_bytes is used (outside of defining scalar) is in the pubkey generation step EdwardsPoint::mul_base_clamped(scalar_bytes). Since the generator is in the prime order subgroup, I should be able to just do scalar * ED25519_BASEPOINT.
Does this make sense? Basically I think we should remove scalar_bytes. Lmk and I'll push to this branch
There was a problem hiding this comment.
#545 already does it as an alternative to this PR
|
Alternatively (and alternatively to #541) we could get rid of the I'm not sure what the motivation is for having it at all. Serializing this format on the wire is an antipattern, IMO. |
The reason `ExpandedSecretKey` needs a private `scalar_bytes` field is to retain the canonical scalar bytes as output by SHA-512 during key expansion so they can be serialized by the `to_bytes` method. However, `ExpandedSecretKey`s should not be serialized to the wire. Removing this method allows the private field to be removed, which allows `ExpandedSecretKey` to be constructed entirely from public fields. This provides an alternative to #544 for use cases like Ed25519-BIP32 where the private scalar is derived rather than clamped from bytes. One other change is needed: `to_scalar_bytes` was changed to `to_scalar` as the canonical scalar bytes are no longer retained, however this has no impact on its main use case, X25519 Diffie-Hellman exchanges, where the `Scalar` should NOT be written to the wire anyway.
|
I opened #545 as an alternative to this PR to try just getting rid of the private field, which makes all of the fields |
* ed25519-dalek: remove `ExpandedSecretKey::to_bytes` The reason `ExpandedSecretKey` needs a private `scalar_bytes` field is to retain the canonical scalar bytes as output by SHA-512 during key expansion so they can be serialized by the `to_bytes` method. However, `ExpandedSecretKey`s should not be serialized to the wire. Removing this method allows the private field to be removed, which allows `ExpandedSecretKey` to be constructed entirely from public fields. This provides an alternative to #544 for use cases like Ed25519-BIP32 where the private scalar is derived rather than clamped from bytes. One other change is needed: `to_scalar_bytes` was changed to `to_scalar` as the canonical scalar bytes are no longer retained, however this has no impact on its main use case, X25519 Diffie-Hellman exchanges, where the `Scalar` should NOT be written to the wire anyway. * Added scalar byte comparison back to ed25519-dalek x25519 test --------- Co-authored-by: Michael Rosenberg <[email protected]>
|
Superseded by #545 |
Motivation can be found in this comment:
dalek-cryptography/ed25519-dalek#293 (review)
Replaces public fields with private fields + accessor methods.
Also adds an
ExpandedSecretKey::from_scalar_and_prefixconstructor which makes it possible to constructExpandedSecretKeyusing aScalaragain, which is useful for protocols that derive the scalar such as Ed25519-BIP32.Additionally defines a
HashPrefixtype alias to make it semantically clear which[u8; 32]is involved in type signatures.