Computing X25519 Secret Keys

[cbluth]

Hello! Nice project, i love the code.

I am reading here : func GenerateKeypair(privateKey *[32]byte) *KeyPair

that it reads from crypto/rand and returns a random result for the private key.

but, i am also reading here: https://cr.yp.to/ecdh.html

Computing secret keys. Inside your program, to generate a 32-byte Curve25519 secret key, start by generating 32 secret random bytes from a cryptographically safe source: mysecret[0], mysecret[1], ..., mysecret[31]. Then do

     mysecret[0] &= 248;
     mysecret[31] &= 127;
     mysecret[31] |= 64;

is this byte manipulation required?

[mimoo]

Hey @cbluth ! and thanks for the issue.

In golang the curve25519 clamping is done during the scalar multiplication: https://github.com/golang/crypto/blob/70a84ac30bf957c7df57edd1935d2081871515e1/curve25519/curve25519_generic.go#L783

which might indeed contrast with how other implementations do it.

re-visiting this code I see one more issue, the API says generate keypair, but what type of keypair? We should correctly name functions and types to reflect that these are key exchanges keypairs

Would you be interested in submitting a PR to fix this key generation (so that other implementations that do not do the clamping during scalar multiplication are not affected) and potentially the naming problem?

[mimoo]

btw I see that libsodium also doesn't do the clamping during generation. @jedisct1 is there a reason and should we not do it for disco too?

[mimoo]

another thing is that since we use ristretto for signatures, we could also potentially just use that for key exchanges. But that's not in the noise specification... (cc @trevp)

[jedisct1]

In libsodium, clamping is done during generation. crypto_scalarmult_base() is where it happens, right after the line you referenced.

[mimoo]

If I understand correctly, the secret key is passed as a const to this function and is thus clamped temporarily to produce the public key.

I'm guessing you clamp it temporarily again when you perform scalarmult without the base :o and my worry is that not every implementation does that

[jedisct1]

But the secret key, as returned by the key generation function, is not clamped if this is what you mean.

Mainly because we want to always do it before a scalar multiplication, in case the secret was not generated using the dedicated function. So, this avoids doing it twice.

This also prevents bits from being lost if the secret is hashed with a bunch of other data.

[jedisct1]

NaCl/TweetNaCl also don't returned clamped scalars. Some implementations may do it, but clamping only before a multiplicatiton is probably the most common pattern.

[jedisct1]

I'm guessing you clamp it temporarily again when you perform scalarmult without the base.

Yes, this is obviously also done when using a non-fixed base.

[jedisct1]

Clamping not being done by the scalarmult function for an arbitrary base would delegate the responsibility to the application, which is usually what we want to avoid.

Clamping being done in scalarmul and not by scalarmult_base would be inconsistent.

So I guess most implementation are doing it unconditionally in both functions anyway. If not... they probably should.

[mimoo]

OK I see, thanks for chiming in @jedisct1 :)

So I guess I wouldn't be opposed to do it at generation (even if it's redundant), like this is we export the private key we won't have interop issues with an implementation that does not do the clamping during scalar mult.

[cbluth]

Would you be interested in submitting a PR to fix this key generation (so that other implementations that do not do the clamping during scalar multiplication are not affected) and potentially the naming problem?

@mimoo

i have a branch here with the renamed functions and types: https://github.com/mimoo/disco/compare/master...cbluth:fix/keypair_naming?expand=1 ^ how does it look? im new to golang and crypto.

(so that other implementations that do not do the clamping during scalar multiplication are not affected)

i dont quite understand what you mean by this, would my current changes fix this too?