dalek-cryptography / curve25519-dalek

A pure-Rust implementation of group operations on Ristretto and Curve25519
Other
853 stars 422 forks source link

ed: Change `SigningKey::to_scalar` back to `SigningKey::to_scalar_bytes` #599

Closed rozbb closed 8 months ago

rozbb commented 8 months ago

As mentioned in the conversation in https://github.com/dalek-cryptography/curve25519-dalek/issues/565, it is currently not clear how you would use a clamped and reduced scalar from SigningKey::to_scalar() to build an x25519 keypair. In fact, the test code in that thread shows that x25519_dalek::PublicKey::from(StaticScalar::from(sk.to_scalar().to_bytes())) does not match the ed25519 public key. This is because the second public key computation does clamping again, so it's clamp -> reduce -> clamp, which is not a no-op.

This PR replaces SigningKey::to_scalar() with SigningKey::to_scalar_bytes(), which returns the unreduced scalar bytes determined by the ed25519 secret key. This is actually how things used to be before https://github.com/dalek-cryptography/curve25519-dalek/pull/545, and what the docs (currently erroneously) reflect.

I've also added a regression test to the ed25519 tests.

tarcieri commented 8 months ago

This is a breaking change that would break running code, namely @dignifiedquire is using these conversions to instantiate a crypto_box using a Scalar originally computed as an Ed25519 key, via these APIs:

https://github.com/dalek-cryptography/ed25519-dalek/pull/296

That's all working just fine and the existing API is useful.

If you really want to go through with this, you either need to deprecate to_scalar instead of just outright deleting it, or bump ed25519-dalek's major version to 3.

tarcieri commented 8 months ago

To be clear: I'm fine with adding to_scalar_bytes, but I think it should be a purely additive change.

rozbb commented 8 months ago

Ok agreed. I think simply adding it would be fine. And updating the docs on to_scalar

rozbb commented 8 months ago

Alright, ready to review again