Add clamping to X25519 keys

[justincormack]

X25519 keys are not exactly a random 256 bit string, there are a few clamping operations that need to be done after to make a conformant key, as documented in the specification.

Signed-off-by: Justin Cormack justin.cormack@docker.com

[nrvnrvn]

right, I was actually going to do the same initially but then noticed that this is already done for us.

https://github.com/amaizfinance/secreter/blob/ed1b5a2c5740a3da4f15570b620c35bbab80425a/vendor/golang.org/x/crypto/curve25519/doc.go#L21-L23

https://github.com/amaizfinance/secreter/blob/ed1b5a2c5740a3da4f15570b620c35bbab80425a/vendor/golang.org/x/crypto/curve25519/doc.go#L14-L16

https://github.com/amaizfinance/secreter/blob/ed1b5a2c5740a3da4f15570b620c35bbab80425a/vendor/golang.org/x/crypto/curve25519/curve25519.go#L785-L791

and also

https://github.com/amaizfinance/secreter/blob/ed1b5a2c5740a3da4f15570b620c35bbab80425a/vendor/golang.org/x/crypto/curve25519/mont25519_amd64.go#L62-L67

So, on the one hand, it is absolutely right to perform clamping on the newly generated [32]bytes. On the other hand the underlying library is fool-proof and will perform clamping either way. That's why I decided to leave private keys as mere random strings...

Please correct me if I am wrong.

[justincormack]

Yeah I was digging in the x25519 implementation and found that it clamps, although this is not documented. I am inclined to see if they will accept a GenerateKey function to x25519 to remove the confusion or otherwise document it. Up to you if you want to merge this...

[nrvnrvn]

I've also found the same implementation in libsodium: 1, 2

NaCl box also treats scalars as random 32-byte slices.

RFC 7748 clearly states that scalars are assumed to be randomly generated bytes.

But I agree it is not clear from the docs.

In the end it turns out to be implementation-specific. Most of the libraries do not rely on correctness of the scalars, create a copy of the scalar and perform clamping against the copy just before proceeding with actual scalar mult.

In case of x/crypto it is not needed but it is worth raising this issue with them @justincormack I think there is some work in progress in regards to reworking the X25519 API. Raising this question seems more than relevant.

Will close it for now.