pwhash: Simplify strHash/strVerify APIs #24112
Conversation
There's no real reason for these to be generic functions - the only reason they're written as such is for the benchmarking code. Otherwise, being generic harms their usability and contorts these functions in a way that violates Zig Zen. This commit rewrites the three pwhash functions and their associated structs to be non-generic, allowing for each API to properly communicate their intended inputs in their function arguments and type fields.
Greatly simplifies the API for most use cases.
BrainBlasted
force-pushed
the
cdavis/pwhash-api-tweaks
branch
from
838d754 to
3c9404a
Compare
| params: Params, | ||
| mode: Mode = .argon2id, | ||
| encoding: pwhash.Encoding = .phc, | ||
| strhash_max_bytes: usize = 128, |
There was a problem hiding this comment.
Based on crypto_pwhash_argon2id_STRBYTES.
| const buf_len = comptime if (options.strhash_max_bytes) |len| len else switch (options.encoding) { | ||
| .crypt => hash_length, | ||
| .phc => hash_length * 2, | ||
| }; |
There was a problem hiding this comment.
These default values were based on the comments for the hash_length constant and the tests. Please let me know if something else should be chosen.
| params: Params, | ||
| encoding: pwhash.Encoding, | ||
| strhash_max_bytes: usize = 128, |
There was a problem hiding this comment.
This one is also based on the tests.
|
CC: @jedisct1 |
|
/cc @x13a I don't like the fact that an allocator becomes mandatory, while functions like bcrypt don't need any dynamic allocation, though. |
|
Dynamic allocation isn't necessary for bcrypt, but it is for What are your thoughts on having both |
|
Or: |
This is even more confusing. The nullable allocator in |
|
Since hash strings are guaranteed to be short, if we want to avoid returning a slice, a BoundedArray could be a good fit. |
|
any reason you decided an allocator was better instead of a std.BoundedArray? I agree with @jedisct1 that making an allocator here mandatory feels unnecessary. |
| .crypt => hash_length, | ||
| .phc => hash_length * 2, | ||
| }; | ||
| var buf: [buf_len]u8 = undefined; |
There was a problem hiding this comment.
maybe provide strHash and strHashAlloc for bcrypt? We still use the stack based approach in std, but don't give the developer any choice if they know what they're doing
These APIs were "generic" in a way that obfuscated their intended use (e.g. using runtime errors for invalid parameters), and made use of a combination of output parameters and a return value that's lead to confusion in multiple projects.
This PR aims to simplify the APIs such that each one accurately communicates intent and is harder to misuse.
See #23994 for context and rationale