unifying the crypto keypair APIs #21002

jakubDoka · 2024-08-08T20:43:49Z

Related to #20953

I changed all keypair APIs I could find to init for the pure derivation from seed, and added initWithRandomSeed that's mostly used in tests/benchmarks.

Note: I noticed docs on the new functions are missing, though, I feel like they are unnecessary.

andrewrk

Thanks! Let's take this opportunity to also fix something else that I noticed.

andrewrk · 2024-08-08T22:10:12Z

lib/std/crypto/25519/ed25519.zig

@@ -506,7 +507,7 @@ test "signature" {
 test "batch verification" {
    var i: usize = 0;
    while (i < 100) : (i += 1) {
-        const key_pair = try Ed25519.KeyPair.create(null);
+        const key_pair = try Ed25519.KeyPair.initWithRandomSeed();


unit tests should not use std.crypto.random, instead they should use std.testing.random_seed.

this is a problem with master branch, but it should be fixed in this PR before merging.

I noticed https://github.com/ziglang/zig/blob/master/lib/std/crypto/salsa20.zig#L539 will need to be changed to take a seed, the initWithRandomSeed should probably be removed to reduce the likelihood of similar mistakes.

jakubDoka · 2024-08-08T23:21:38Z

I have repeated the initTestKeypair to keep it private and also moved the seed_length into the KeyPair scope.

jedisct1 · 2024-08-08T23:40:36Z

I strongly disagree with these changes.

Virtually all applications should always use non-deterministic keys. We should absolutely not encourage people to provide their own seed, possibly from insecure random number generators or constant seeds. This is a recipe for disaster.

Aa create() function that doesn't take any seed for the common case, and createDeterministic() for deterministic keys would be way better.

jedisct1 · 2024-08-08T23:42:20Z

lib/std/crypto/25519/x25519.zig

@@ -19,8 +19,6 @@ pub const X25519 = struct {
    pub const public_length = 32;
    /// Length (in bytes) of the output of the DH function.
    pub const shared_length = 32;
-    /// Seed (for key pair creation) length in bytes.
-    pub const seed_length = 32;


That shouldn't be removed.

It's was moved to the keypair to unify things

jedisct1 · 2024-08-08T23:44:15Z

lib/std/crypto/ml_kem.zig

@@ -229,8 +229,6 @@ fn Kyber(comptime p: Params) type {
        pub const shared_length = common_shared_key_size;
        /// Length (in bytes) of a seed for deterministic encapsulation.
        pub const encaps_seed_length = common_encaps_seed_length;
-        /// Length (in bytes) of a seed for key generation.
-        pub const seed_length: usize = inner_seed_length + shared_length;


Why remove these useful constants?

andrewrk · 2024-08-09T00:19:41Z

Virtually all applications should always use non-deterministic keys.

Testing is an extremely common use case.

We should absolutely not encourage people to provide their own seed, possibly from insecure random number generators or constant seeds. This is a recipe for disaster.

You really think people are going to reach for Ed25519 encryption, and then pass the system time or something like that? ...you know what now that I think of it, yes, people are absolutely that stupid. Point taken.

Aa create() function that doesn't take any seed for the common case, and createDeterministic() for deterministic keys would be way better.

create typically means "heap allocated" or allocation of another resource which is not the case here. As a rule of thumb, if there is no "destroy" then there should be no "create".

However, let us accomplish your goal while keeping existing conventions, with init and initDeterministic. Naming aside, the key changes here are good, making those two different functions rather than one with an optional.

jakubDoka · 2024-08-09T00:47:49Z

Besides humans, AI now also writes code so this is even more important 😂. I'll get to this tomorrow

jedisct1 · 2024-08-09T05:51:47Z

create typically means "heap allocated" or allocation of another resource which is not the case here. As a rule of thumb, if there is no "destroy" then there should be no "create".

Not really. Maybe in other languages, but in Zig, init() is typically what gets an allocator, with a corresponding deinit() function.

init() also conveys the idea that the result with be initialized with default values, not with something different every time.

There's nothing wrong with create(). This actually creates a new key every time.

… a random seed

alterstep · 2024-08-09T13:38:08Z

lib/std/crypto/benchmark.zig

 pub fn benchmarkSignature(comptime Signature: anytype, comptime signatures_count: comptime_int) !u64 {
    const msg = [_]u8{0} ** 64;
-    const key_pair = try Signature.KeyPair.create(null);
+    const key_pair = try initBenchKeyPair(Signature);


The benchmark can simply use Signature.KeyPair.create().

alterstep · 2024-08-09T13:49:37Z

lib/std/crypto/25519/ed25519.zig

 test "batch verification" {
+    var prng = std.Random.DefaultPrng.init(std.testing.random_seed);


People often refer to tests to understand how to use things.

So perhaps use DefaultCsprng instead, and rename prng to test_prng to clarify that this code is intended solely for testing.

andrewrk · 2024-08-09T20:24:46Z

Not really. Maybe in other languages, but in Zig, init() is typically what gets an allocator, with a corresponding deinit() function.

Frank, please do a couple greps and see that this is completely wrong.

jakubDoka · 2024-08-09T20:28:53Z

And once again I am renaming things🍷

andrewrk · 2024-08-09T20:29:42Z

@jakubDoka I recommend for you to sit back and do nothing until we work this out.

jakubDoka · 2024-08-09T20:38:26Z

So, does the create/destroy pattern signify that the object holds an allocation for the full lifetime, whereas init/deinit hints that the initialization never allocates and/or allocation is not required for the object to exist?

andrewrk · 2024-08-09T20:42:08Z

The create convention generally indicates that the object itself is heap allocated and the return value will be a pointer.

init does not hint anything other than that the return type will be of the parent namespace type and not be a pointer.

jedisct1 · 2024-08-09T20:45:37Z

Would generate() work? This is quite a common name for creating keys.

andrewrk · 2024-08-09T21:12:34Z

"generate" is fine. Mainly, I care about separate functions rather than optionals (the pattern in #20953).

I understand the desire to help people write better code (really, I do), but the bottom line is that entropy is an externally-provided dependency, and it needs to be recognized as one in the API. That's just a fact of reality in programming.

I would like to avoid the convention-breaking create name if possible. However if you want to have your own convention in std.crypto, I think that's ... acceptable.

jedisct1 · 2024-08-09T21:22:03Z

Let's opt for generate and generateDeterministic, then.

jakubDoka · 2024-08-09T23:24:02Z

Is this all?

Our key pair creation API was ugly and inconsistent between ecdsa keys and other keys. The same `generate()` function can now be used to generate key pairs, and that function cannot fail. For deterministic keys, a `generateDeterministic()` function is available for all key types. Fix comments and compilation of the benchmark by the way. Fixes ziglang#21002

jakubDoka and others added 2 commits August 8, 2024 22:34

unifiing the crypto keypair APIs

3782fc0

Merge branch 'master' into unify-crypto-keypair-apis

9ab7fd0

jakubDoka changed the title ~~unifiing the crypto keypair APIs~~ unifying the crypto keypair APIs Aug 8, 2024

andrewrk requested changes Aug 8, 2024

View reviewed changes

make the crypto test pure again

688ca1d

jakubDoka requested a review from andrewrk August 8, 2024 23:22

fixing tired code

edf62ac

jedisct1 reviewed Aug 8, 2024

View reviewed changes

jakubDoka and others added 2 commits August 9, 2024 13:57

renaming again and adding the short-name function to create keys from…

e34b9f3

… a random seed

Merge branch 'master' into unify-crypto-keypair-apis

c5aec0a

jakubDoka requested a review from jedisct1 August 9, 2024 12:00

cleaning up the tls client generation

10787ba

alterstep reviewed Aug 9, 2024

View reviewed changes

Merge branch 'master' into unify-crypto-keypair-apis

21f3753

changing create to generate for key constructors

5ef58c0

Merge branch 'master' into unify-crypto-keypair-apis

ec3c481

jedisct1 mentioned this pull request Nov 10, 2024

std.crypto: make the key pair API creation consistent #21955

Merged

jedisct1 closed this in #21955 Nov 19, 2024

jedisct1 closed this in 8a00bd4 Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unifying the crypto keypair APIs #21002

unifying the crypto keypair APIs #21002

jakubDoka commented Aug 8, 2024

andrewrk left a comment

andrewrk Aug 8, 2024

jakubDoka Aug 8, 2024

jakubDoka commented Aug 8, 2024

jedisct1 commented Aug 8, 2024

jedisct1 Aug 8, 2024

jakubDoka Aug 9, 2024

jedisct1 Aug 8, 2024

jakubDoka Aug 9, 2024

andrewrk commented Aug 9, 2024 •

edited

Loading

jakubDoka commented Aug 9, 2024

jedisct1 commented Aug 9, 2024

alterstep Aug 9, 2024

alterstep Aug 9, 2024 •

edited

Loading

andrewrk commented Aug 9, 2024

jakubDoka commented Aug 9, 2024

andrewrk commented Aug 9, 2024

jakubDoka commented Aug 9, 2024 •

edited

Loading

andrewrk commented Aug 9, 2024

jedisct1 commented Aug 9, 2024

andrewrk commented Aug 9, 2024

jedisct1 commented Aug 9, 2024

jakubDoka commented Aug 9, 2024

		test "batch verification" {
		var prng = std.Random.DefaultPrng.init(std.testing.random_seed);

unifying the crypto keypair APIs #21002

unifying the crypto keypair APIs #21002

Conversation

jakubDoka commented Aug 8, 2024

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Aug 8, 2024

Choose a reason for hiding this comment

jakubDoka Aug 8, 2024

Choose a reason for hiding this comment

jakubDoka commented Aug 8, 2024

jedisct1 commented Aug 8, 2024

jedisct1 Aug 8, 2024

Choose a reason for hiding this comment

jakubDoka Aug 9, 2024

Choose a reason for hiding this comment

jedisct1 Aug 8, 2024

Choose a reason for hiding this comment

jakubDoka Aug 9, 2024

Choose a reason for hiding this comment

andrewrk commented Aug 9, 2024 • edited Loading

jakubDoka commented Aug 9, 2024

jedisct1 commented Aug 9, 2024

alterstep Aug 9, 2024

Choose a reason for hiding this comment

alterstep Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

andrewrk commented Aug 9, 2024

jakubDoka commented Aug 9, 2024

andrewrk commented Aug 9, 2024

jakubDoka commented Aug 9, 2024 • edited Loading

andrewrk commented Aug 9, 2024

jedisct1 commented Aug 9, 2024

andrewrk commented Aug 9, 2024

jedisct1 commented Aug 9, 2024

jakubDoka commented Aug 9, 2024

andrewrk commented Aug 9, 2024 •

edited

Loading

alterstep Aug 9, 2024 •

edited

Loading

jakubDoka commented Aug 9, 2024 •

edited

Loading