jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.06k stars 1.72k forks source link

Add parallelism setting to crypto_pwhash_argon2i() and crypto_pwhash_argon2id() #1221

Closed z3bra closed 1 year ago

z3bra commented 1 year ago

This effectively add a new parameters "joblimit" to the Argon2 specific fonctions exposed by the crypto_pwhash API.

Parallelism is one of the 3 parameters that define the Argon2 algorithm, but it cannot be changed using the API. This makes libsodium incompatible with any other implementation, and incompatible with RFC 9106 recommended settings for Argon2.

In order to keep compatibility with previous version, I didn't change the crypto_pwhash() function, which is now where the parallelism value of 1U is hardcoded. By using the argon2 specific function, users can can change this parameter, which is already fully implemented and working internally. This MR only expose the parameter to the API.

jedisct1 commented 1 year ago

Existing APIs cannot be changed. That would break all existing applications and bindings.

For Argon2 and other functions that can use multiple threads, we may want additional functions that accept the parallelism parameter, but also the actual maximum number of threads to use. And actual threading still has to be implemented (and not just on Unix).

But the low level functions are only there for consistency with NaCl. Functions that applications should use are the high-level crypto_pwhash functions only.

Different hash functions may accept different parameters in addition to the memory limit and work factor.

So, like what was done in the Zig standard library, high-level crypto_pwhash functions could accept an opaque options parameter. For Argon2, the parallelism and actual. number of threads could go there.

Maybe that would be a good opportunity to make these slow operations interruptible/resumable by the way.

So, something like

int crypto_pwhash_str(
    crypto_pwhash_state *state,
    char out[crypto_pwhash_STRBYTES],
    const char * const passwd, unsigned long long passwdlen,
    const crypto_pwhash_options *options)

state can be NULL. If not, one of the options could be the maximum number of interactions to run before the function returns an EINPROGRESS error code.

z3bra commented 1 year ago

On one hand you say API cannot be changed, and on the other, that crypto_pwhash() prototype could be changed to accept an opaque "options" struct. I'm not sure to understand your idea.

Do you mean to add new functions, like crypto_pwhash_extended() or whatever the name would be, that could accept more parameters ?

I'd be totally fine with working on that if that's what you mean.

As for actual threading/interruptions, it's probably out of scope regarding the issue, especially given that threading doesn't exist at all within the library for now.

jedisct1 commented 1 year ago

Yes, of course, it should be a different set of crypto_pwhash functions.

And threading has to be added for the parallelism parameter to make sense.

z3bra commented 1 year ago

Agreed that it would make more sense to do "real" parallelism. But internally it seems to be already handled, albeit not in parallel. So maybe the first step could be to just expose the parameter, and then add actual threading later on ? This would keep changes easier to test and review