Allow initializing SigningKey from a raw key

[jakob]

Currently, SigningKey must be initialized from a 32 byte seed.

However, sometimes you already have the 64 byte Ed25519 private key, and not the seed. Then it's very inconvenient to create a SigningKey object from the key bytes.

This problem was initially reported by @dmerejkowsky in issue #419. Recently, @BlackTurtle123 opened issue #639, which is also about the same problem.

The problem is that most applications probably store the 64 byte Ed25519 key rather than the 32 byte seed, since that should be faster as you don't need to re-compute the public key every time.

This Pull Request adds an additional argument key to the SigningKey initializer. You can use this argument to create a SigningKey directly from the raw Ed25519 private key bytes.

The new signature proposed in this PR is:
SigningKey(seed=None, encoder=encoding.RawEncoder, key=None)

key is an optional 3rd argument in order to maintain backward compatibility with existing code.

Example usage of the new code:

raw_key = b'64 byte Ed25519 key from somewhere'
signing_key = SigningKey(key=raw_key)

[dmerejkowsky]

Looks good to me - thanks

[jakob]

An alternative would be to just check the length of the argument -- if it's 32 bytes, assume it's a seed, and if it's 64 bytes assume it's a full Ed25519 secret key (including the public key). That would also be backwards compatible, and may be a bit more intuitive. The version implemented in this PR requires reading the docs.

[reaperhulk]

I think calling it key is misleading since the 32-byte value is called a key basically everywhere that isn't libsodium. libsodium itself uses secret key and seed, which is perhaps a reasonable path.

Other than that I'm okay with this approach.

[jakob]

@reaperhulk OK. So should I change the argument from key to secret_key? We could also call it sk, like in the libsodium docs.

(Aside from that, I realised that I used the term "private key" in the docs. I guess I should probably replace that with "secret key".)

[reaperhulk]

Let’s do secret_key

[jakob]

OK, I've changed the argument name from key to secret_key.

I didn't change the term "Private Key" in the docs to "Secret Key" since "Private Key" is used everywhere in the docs.

[reaperhulk]

Looks like this needs a black format

[reaperhulk]

Two final requests: instead of an assertion error I’d prefer a value error when the values are both None (you can do this with raising=exc.ValueError) and also please add a changelog entry. Thanks for working on this!

[covert-encryption]

Should also accept 32 byte plain keys. This 64-byte combined key format of libsodium is quite annoying, and too often I end up doing hacks like sk + bytes(32) to keep pynacl happy when the function I am calling does not actually use the public key half at all.

[jakob]

Should also accept 32 byte plain keys. This 64-byte combined key format of libsodium is quite annoying, and too often I end up doing hacks like sk + bytes(32) to keep pynacl happy when the function I am calling does not actually use the public key half at all.

@covert-encryption Can you be more specific what you mean with "32 byte plain keys"? It sounds like you are talking about the value that PyNaCl and libsodium call "seed", so you could just create a SigningKey(seed = [32 byte plain key])

What this PR adds is allowing to create a SigningKey from the 64 byte value that libsodium calls a secret key (which is just the 32 byte seed and the 32 byte precomputed public key concatenated).

[jakob]

OK, but does this have anything to do with this PR?

[reaperhulk]

@jakob I can approve and merge this once the two small comments in the review are fixed!

[jagerman]

The documentation in this PR is quite misleading:

:param seed: [:class:bytes] Random 32-byte value for generating Ed25519 private key
:param secret_key: [:class:bytes] A previously generated 64-byte Ed25519 private key

This seems to be implying that the seed "generates" something, but neither of these are used to "generate" anything, nor are they actually any different as the description is implying. The starting point everywhere in libsodium is the seed and is really the only thing that ever should be stored. The only difference is that, for computational convenience, libsodium wants to have a precomputed public key so that it doesn't have to be computed in many different functions.

I'm also pretty skeptical of the claim that

it's very inconvenient to create a SigningKey object from the key bytes.

How does seed=raw_key[:32] qualify as "very inconvenient"?

Perhaps, rather than adding two separate arguments for almost identical purposes, the seed parameter could just be made to be smarter when passed a 64-byte value?

[jakob]

How does seed=raw_key[:32] qualify as "very inconvenient"?

Neither libsodium nor pynacl mention in the docs that this is possible. You have to read the source code to know that. I didn't know that when I started on this PR.

It seems that libsodium tries to hide the fact that a secret key is just the concatenation of seed and publickey. Why else would they have functions like crypto_sign_ed25519_sk_to_seed?

[jagerman]

Neither libsodium nor pynacl mention in the docs that this is possible. You have to read the source code to know that. I didn't know that when I started on this PR.

That's fair. I'd suggest at least a change to "secret key" instead of "private key" for the args/documentation then. Private key immediately makes me think of the private key scalar, which is what a pynacl PrivateKey is (and, somewhat perversely, the PrivateKey that you get back from a to_curve25519_private_key actually is also the private key scalar for ed25519).