[x13a]

This pull adds scrypt key derivation function and basic phc-string parser for kdf(s).

With much help of @jedisct1 .

[jedisct1]

Errrr... I don't think it is ready yet :)

[x13a]

error: expected type 'u5', found 'u6'

const n = @as(usize, 1) << param.log_n;

log_n is u6 which max is (1 << 6) - 1 == 63
usize max is 0xffff_ffff_ffff_ffff
0xffff_ffff_ffff_ffff > (1 << 63)

What's wrong with my math? :(

[jedisct1]

usize is platform-dependent. Let's keep discussing this in the repo :)

[jedisct1]

This is awesome work!

Just one thing. Returning strings that have to be deallocated is not very common in Zig, especially when the length is a constant for a given set of parameters.

In the current code, the same allocator is also used for two completely different things (allocating gigabytes of aligned memory for the duration of a computation vs returning a 64 byte string).

So, maybe the functions can accept an existing array and return the corresponding slice? For the Unix crypt format, the size is constant. For the PHC format, the upper bound is known and the exact size can be computed from the parameters.

[x13a]

Let's wait for base64 to be merged.

[jedisct1]

Yep, this is going to remove a bit of code duplication.

[jedisct1]

Hi @x13a,

CI may be acting up, but otherwise, the code looks really good!

Once CI's happy, do you think this is ready to merge, or are there more changes you would like to do?

[x13a]

Hi @jedisct1 ,

Suppose it's ready. Some things I have to mention..

Scrypt kdf, as well, as argon2 requires allocator. If we want to return exact error in pwhash, we have to add mem.Allocator.Error to crypto.Error.

pub const Error = @import("crypto/error.zig").Error || std.mem.Allocator.Error;

Or we can transform it to some internal error.

Another thing, besides I have added pwhash to crypto/benchmark, I couldn't run it. Maybe I missing something, but on compile I get:

error: import of file outside package path: '../std.zig'

If I change it to @import("std"), I get:

Assertion failed at /Users/user/dev/zig/src/stage1/ir.cpp:22773 in ir_analyze_container_member_access_inner. This is a bug in the Zig compiler.thread 2390872 panic:

<traceback here>

It will be nice if you can check if crypto/benchmark is ok.

[jedisct1]

Scrypt kdf, as well, as argon2 requires allocator. If we want to return exact error in pwhash, we have to add mem.Allocator.Error to crypto.Error.

No need to add mem.Allocator.Error to crypto.Error. Relevant functions can return crypto.Error || mem.Allocator.Error.

[x13a]

Lol, really didn't think that we can merge errors on return :) Will update tomorrow.

[x13a]

Is there any way to understand what is the reason for error in ziglang.zig?

2021-04-01T18:17:21.0853863Z 1777 passed; 43 skipped.
2021-04-01T18:21:47.5480829Z The following command terminated unexpectedly:
2021-04-01T18:21:47.5624979Z /home/vsts/work/1/s/build/release/bin/zig test /home/vsts/work/1/s/lib/std/std.zig --test-name-prefix std-native-ReleaseSafe-bare-multi  -OReleaseSafe --cache-dir /home/vsts/work/1/s/zig-cache --global-cache-dir /home/vsts/.cache/zig --name test -I /home/vsts/work/1/s/test --override-lib-dir /home/vsts/work/1/s/lib 
2021-04-01T18:21:47.5630825Z error: the following build command failed with exit code 1:
2021-04-01T18:21:47.5634995Z /home/vsts/work/1/s/zig-cache/o/3c4dee4be353affb186fa64b16bc2978/build /home/vsts/work/1/s/build/release/bin/zig /home/vsts/work/1/s /home/vsts/work/1/s/zig-cache /home/vsts/.cache/zig test -Denable-qemu -Denable-wasmtime
2021-04-01T18:21:47.9445360Z ##[error]Bash exited with code '1'.
2021-04-01T18:21:48.0000186Z ##[section]Finishing: Build and test

[jedisct1]

It may be an error happening at comptime.

Or, in the case of your function, if may be that the instance is running out of memory.

The way I usually deal with these is by having setup Azure Pipelines for a copy of the repository, with the CI scripts reduced to the bare minimum (only running the Linux tasks, running the relevant tests first, not running non-native environments).

You may get more informative messages that way, but it also makes it faster to get feedback after having made a change.

[x13a]

Hello @jedisct1 ,

Merging with master helped: "All checks have passed".

[jedisct1]

Yay! This is great news!

Everything looks really nice!

Just one thing: with these changes, bcrypt now requires an allocator, even though the function itself has a constant-size state and always returns a constant-size string. It the allocator even used here?

[x13a]

This is done for bcrypt, scrypt and argon2 to have the same signatures in strHash and strVerify, as two last kdf's require allocator. PhcHasher is generic and requires allocator for .create() and .verify() methods, because of scrypt and argon2. In bcrypt allocator is not used. It is a mock pointer.

[jedisct1]

Wait, what? bcrypt is not a KDF.

[x13a]

Let's look..

wikipedia bcrypt tells us that: bcrypt is a password-hashing function designed by.., if we click on password-hashing function we get redirected to Key derivation function. Where we can read this:

In cryptography, a key derivation function (KDF) is a cryptographic hash function that derives one or more secret keys from a secret value such as a main key, a password, or a passphrase using a pseudorandom function.
...
The first[citation needed] deliberately slow (key stretching) password-based key derivation function was called "crypt" (or "crypt(3)" after its man page), and was invented by Robert Morris in 1978.
...
Various algorithms have been designed specifically for this purpose, including bcrypt, scrypt and, more recently, Lyra2 and Argon2 (the latter being the winner of the Password Hashing Competition).

[jedisct1]

bcrypt is a little bit different: it was only made to be a password hashing function.

bcrypt as a KDF generally refers to the bcrypt-pbkdf function.

[jedisct1]

Got it.

Requiring a "mock pointer" feels a little bit odd.

How about including the allocator in the options set of functions requiring one?

[x13a]

I though that allocator is always passed as first argument to functions that require allocations. Maybe do not save the same signature between bcrypt and (scrypt, argon2) and remove allocator from bcrypt?

[jedisct1]

I though that allocator is always passed as first argument to functions that require allocations

Not necessarily.

For example std.json.parse() also has an options parameter (ParseOptions), with an allocator field. As long as it doesn't have a default value, it will be required by the functions needing it.

[Mouvedia]

You are using Error || mem.Allocator.Error 9 times.
Could you extract/name it?

[andrewrk]

30+ days old draft; closing

[x13a]

@andrewrk Merged with upstream cause I wanted to complete it in few days. Could you reopen or I have to create new?

upd: I will create a new one with reference to this. It will be better.