Closed poljar closed 1 year ago
@poljar what's your use case for to_bytes
? Could we just get rid of it as I proposed here? (Edit: see also #545)
It's worth doing https://gitweb.torproject.org/torspec.git/tree/proposals/224-rend-spec-ng.txt#n2135 but this could be expressly supported if whatever way you like. Ed25519-BIP32 is a mess, so maybe ignore it.
@poljar what's your use case for
to_bytes
? Could we just get rid of it as I proposed here? (Edit: see also #545)
As explained in https://github.com/dalek-cryptography/ed25519-dalek/issues/298#issuecomment-1500616102, libolm
does in fact serialize and persists the unclamped bytes that are now in the scalar_bytes
field of the ExpandedSecretKey
type. The persisted scalar_bytes
are used for a long-term identity key.
We are migrating from libolm
to a Rust-based library, but we need to support existing accounts, which means supporting deserialization and serialization of the scalar_bytes
. This was previously done using the Serialize
/Deserialize
implementation of the ExpandedSecretKey
type. Currently it's done using the ExpandedSecretKey::to_bytes()
and ExpandedSecretKey::from_bytes()
methods.
So the methods are useful to me, of course I can work around to_bytes()
being removed with a wrapper:
struct MyExpandedSecretKey {
source: Box<[u8; 64]>,
inner: ExpandedSecretKey
}
But it does feel a bit strange to not have this method since as far as I can tell, every other type in the ecosystem can be converted to/from a byte slice or array.
The downside is supporting this incredibly niche use case requires making a second copy of the secret key in memory for all users, just to store the unclamped version. It makes the type 50% larger in memory, and it’s one more thing that needs zeroized (which is also currently missing, I believe)
I also proposed in the comments of #544 that we remove scalar_bytes
entirely. Deserialization should work, but if you load up "legacy" keys then to_bytes()
might output something different
Well, as I said, I can live with this. Just need to implement the above mentioned code snippet in my lib.
Should I close this then or do you guys want to merge this to have correct behavior until the other PRs are ready?
@rozbb #545 is an alternative to #544 that removes scalar_bytes
(and with it to_bytes
)
@poljar I think we need to pick one of the above PRs. #544 would benefit from this PR I think.
We decided to get rid of to_bytes
entirely. Turns out that keeping the unprocessed scalar bytes was unnecessary.
Superseded by #545
A mistake snuck in with 9b166b75e0bb0c22bd782665f63638efef72556a. The new
scalar_bytes
field of theExpandedSecretKey
is not used in theExpandedSecretKey::to_bytes()
method, instead the, now clamped,scalar
is used.This in turn means that the following snippet won't produce the same
ExpandedSecretKey
:Down the line, it means that serializing/deserializing this struct will produce differing keys and thus spurious signature verification errors.
Transferred from: https://github.com/dalek-cryptography/ed25519-dalek/pull/308.