Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

z3bra
Copy link

@z3bra z3bra commented Oct 21, 2022

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
Copy link
Owner

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
Copy link
Author

z3bra commented Oct 21, 2022

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
Copy link
Owner

jedisct1 commented Oct 22, 2022

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
Copy link
Author

z3bra commented Oct 22, 2022

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

@jedisct1 jedisct1 force-pushed the master branch 4 times, most recently from 82c35a0 to 6d1d7ed Compare November 14, 2022 21:16
@jedisct1 jedisct1 closed this Dec 11, 2022
Repository owner locked and limited conversation to collaborators Mar 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants