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

Feature Request: Allow specifying Argon2 parameters #119

Open
TylerWanta opened this issue Jun 14, 2024 · 15 comments
Open

Feature Request: Allow specifying Argon2 parameters #119

TylerWanta opened this issue Jun 14, 2024 · 15 comments

Comments

@TylerWanta
Copy link

I noticed that the default parameters are being used for the argon2 package, which seem to be a bit below the recommended parameters outlined in the protocol.

Could an update be made to support specifying different parameters for argon2?

@TylerWanta
Copy link
Author

TylerWanta commented Jul 1, 2024

I created a Pull Request with the proposed solution.

@nikgraf
Copy link
Member

nikgraf commented Jul 2, 2024

hey @TylerWanta, sorry for the late reply. If the recommended parameters are too low, I think we should focus on fixing the defaults than allowing different parameters.

My current assessment:

The defaults in https://docs.rs/argon2/0.5.3/argon2/struct.Params.html#associatedconstant.DEFAULT_M_COST are

DEFAULT_M_COST: u32 = 19_456u32
DEFAULT_T_COST: u32 = 2u32
DEFAULT_P_COST: u32 = 1u32
DEFAULT_OUTPUT_LEN: usize = 32usize

Spec currently recommends:

Argon2id(S = zeroes(16), p = 4, T = Nh, m = 2^21, t = 1, v = 0x13, K = nil, X = nil, y = 2)

https://datatracker.ietf.org/doc/draft-irtf-cfrg-opaque/

Conclusion:

P (paralellism) cost is 1, but should be 4
t (iterations) cost is 2, but should be 1
M (memory) cost is 19456 KiB, but should be (2^21) = 2097152 KiB
V0x13 (already the default)

Is this correct? If so I think we should update the defaults in the library to match the spec. Or do you have any good reasons to use other parameters?

@TylerWanta
Copy link
Author

Admittedly, I was only thinking about the low parallelism cost when originally looking at the parameters. Now, seeing the jump in memory (~20MBs -> ~2GBs) is kind of concerning.

Doing some research, it seems like 2GBs is quite a high cost. Testing this locally, it takes 14.71s to fun the algorithm with the recommended parameters (t=1, p=4, m=2097152). My computer specs are even quite high:

Graphics Card: NVIDIA GeForce RTX 4080
Processor: 13th Gen Intel(R) Core(TM) i9-13900KF 3000 Mhz, 24 Core(s)
Ram: 32 GBs

Considering this is an OPAQUE package, updating the parameters to the recommended still seems like a good idea to me, but, with the memory cost as high as it is, allowing developers to tone it done a bit would be needed. Otherwise, forcing the defaults could easily prevent developers from being able to utilize the package due to the abnormally high resource usage.

FWIW, the default argon2 parameters take me ~250ms to run, which is a bit low. Usually the sweet spot is ~1s from my understanding.

Another argument for allowing to specify argon2 parameters is that hardware advances. What may take ~1s today, could take ~250ms in a couple years. In that case, the parameters would want to be increased. From my knowledge, monitoring algorithm times and adjusting parameters is a fairly common thing.

I guess to flip the question, did you have a specific reason for not allowing parameters to be specified? The parameters are still going through validation, so it should be easy to tell to someone if they are trying to use parameters that are incompatible.

@nikgraf
Copy link
Member

nikgraf commented Jul 2, 2024

To answer your question: From my experience sane defaults and less options is usually better. Especially when it comes to cryptography. I think us exploring this is a good example how complex it is and how is to screw it up.

We could still make it flexible, but the most important thing should be to have good defaults. The 2GB memory is indeed concerning. Will see if I can check with other people who use OPAQUE in production and what's their experience.

@nikgraf
Copy link
Member

nikgraf commented Aug 1, 2024

Hey @TylerWanta, sorry for the delay

Got some feedback: the initial discussion on the parameters happened here: cfrg/draft-irtf-cfrg-opaque#376

I think you are right in making on configurable after reading more into the topic.

There are two ways forward for default configurations that sound sensible:

  1. use scrypt(N = 32768, r = 8, p = 1) - this is a recommended setting and could be much faster (but needs testing first)
  2. stick to Argon2id variant with t=3 and 64 MiB memory (also needs testing)

Why this setting? It's the second recommended option in the RFC draft https://www.rfc-editor.org/rfc/rfc9106.html#name-recommendations

7.4.  Recommendations

   The Argon2id variant with t=1 and 2 GiB memory is the FIRST
   RECOMMENDED option and is suggested as a default setting for all
   environments.  This setting is secure against side-channel attacks
   and maximizes adversarial costs on dedicated brute-force hardware.
   The Argon2id variant with t=3 and 64 MiB memory is the SECOND
   RECOMMENDED option and is suggested as a default setting for memory-
   constrained environments.

What are your thoughts?

@TylerWanta
Copy link
Author

Hey @nikgraf.

Good catch on the second set of parameter recommendations in the paper. My vote would be for using those parameters for Argon2id.

Do you still plan on allowing users of the package to specify their own parameters on top of this change?

@nikgraf
Copy link
Member

nikgraf commented Aug 5, 2024

@TylerWanta good question, not sure. We could offer predefined sets of parameters? I'm worried that exposing the full parameter-set causes more harm than does good. Very tricky API decision

@TylerWanta
Copy link
Author

TylerWanta commented Aug 5, 2024

@nikgraf offering predefined sets of parameters would be useful. We could do

  1. The first set of recommended parameters from the paper (Highest security, lowest efficiency)
  2. The second set of recommended parameters from the paper. (Medium security, medium efficiency)
  3. The default parameters that are being used now (Lowest security, highest efficiency)

We could then default them to the first set of recommended parameters but allow switching between any of the 3.

We could also allow slight tweaking of the predefined ones, but only with options from a curated list. So if someone wanted something slightly more secure than the second option, they could pick from a memory cost of [128, 256, 512, 1048] mbs instead?

@czeidler
Copy link

I am interested in this as well. I would like to let the user select the parameters during registration. For this it would be really nice if the user can choose the parameters to their liking, e.g. in the UI the user can choose how many seconds or how much memory it would take to stretch their password on their machine.

Btw. once the KDF parameters are selected how are their retrieved from the server? Are they encoded in the loginResponse? or do they need to be transferred separately?

@TylerWanta
Copy link
Author

@nikgraf FWIW, it looks like Bitwarden (password manager) allows users to specify the parameters used for Argon2id

@nikgraf
Copy link
Member

nikgraf commented Sep 23, 2024

yeah, I think we probably want to allow a union of pre-defined defaults or a custom object

@czeidler no the KDF parameters are currently not encoded in the loginResponse. Especially since the handling of these might vary a lot from application to application I think they should be handled by the application developer. What do you think?

sorry for being so unresponsive, busy times

@TylerWanta
Copy link
Author

@nikgraf Did you want me to tweak my current pull request to support both pre-defined defaults and custom params, or do you have a plan and want to tackle it yourself?

@nikgraf
Copy link
Member

nikgraf commented Sep 30, 2024

@TylerWanta In case you have time and are motived, would be great if you could do it! I'm super busy for 2 more weeks.

As a first step we should define the API. I was thinking to have an optional parameter:

keyStrechingFunction and it could be a discriminated type union for to ensure good type safety on the TS side.

e.g.

type KeyStrechingFunction = {
  name: "argon2-custom";
  iterations: number;
  memory: number;
  parallelism: number
} | {
  name: "argon2-recommended"
};

This would be a minimal version. argon2 recommended is the default in case keyStrechingFunction is not defined and the params from the RFC are used.

In addition we could add a argon2-web which uses the t=3 and 64 MiB memory. The second recommendation from the Argon2 RFC.

What do you think? Feedback is very welcome

@nikgraf
Copy link
Member

nikgraf commented Oct 25, 2024

@TylerWanta @czeidler I created a PR: #127 - feedback would be much appreciated

@TylerWanta
Copy link
Author

@nikgraf Sorry, was very busy for a while there and wasn't tracking this as closely. Thanks for getting this done, it looks great so far! I left just a few comments on the pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants