ph.hash hanging indefinitely in 18.3.0? about argon2-cffi HOT 8 CLOSED

@hynek that is awesome:)! With that, I will for sure come to your defense if anyone even remotely tries to complain like I did.

from argon2-cffi.

Could you try whether 18.3.0 with 18.1.0's hash parameters still works? I.e. PasswordHasher(time_cost=2, memory_cost=512, parallelism=2, type=argon2.Type.I)

If so, maybe try to isolate which parameter is to blame?

from argon2-cffi.

@hynek Works fine with the 18.1.0 parameters. Seems the issue is the memory cost. The original memory cost was 512 the new default memory cost in 18.3.0 is 102400? Are you sure you meant to increase the default memory cost that much or is it a typo and you wanted to double it to 1024? Things start breaking for us at a memory cost of 2**16.

I think what could be happening to us is that we have memory constraints on our container. If the memory is spiking that could cause problems. Our containers are set to about 100MB. Not sure how the memory cost translates to used ram.

from argon2-cffi.

Yeah it was on purpose, because the RFC recommends even 1–4 GB so I went for 100 MB as a compromise. So the answer is basically that the hasher made your app swap? :)

from argon2-cffi.

Yes. I would say the answer is that we had a 100MB memory constraint on our containers. 18.1.0 had a peak ram usage of 512kB and 18.3.0 now has a peak usage >100MB, which was right at the limit of our container and caused swaps to begin to happen.

The default values for argon2_cffi went from requiring 512kB of ram to produce a single hash to requiring 100MB of ram for every hash that is generated. This is a 2 orders of magnitude increase, which for sure caught us off guard. I don't think programs changing default memory requirements 2 orders of magnitude should be done lightly. At a minimum I would make this pretty explicit in the docs. And I'll still say the problem is on us for not setting these parameters explicitly in the first place.

The RFC recommendations state

The Argon2id variant with t=1 and maximum available memory is recommended as a default setting for all environments

More memory is simply better for security, but has tradeoffs. Your default setting now says that the absolute minimum RAM necessary to run the default argon2_cffi is 100MB. That is up to you, but that kind of change should be more pronounced.

I think a better default would be something like 1024 meaning that someone trying out argon_cffi won't see these large memory consumptions and then put a comment that "RFC recommendations suggest using the maximum memory available for memory cost for maximum security". Let the user set that parameter to whatever they feel is their available maximum available memory.

from argon2-cffi.

I can assure you that that decision hasn’t been made lightly. In fact, I had started working on it at the PyCon US 2018 sprints in May and was procrastinating on it because it is a touchy topic.

My three alternatives basically were:

1. ignore RFC, get angry issue from security folks that I’m being irresponsible and the world is ending due to my neglect
2. (somewhat) implement RFC, with the risk of getting issues like this one

I think I picked the right option but the execution might be lacking despite months of work.

Let the user set that parameter to whatever they feel is their available maximum available memory.

This goes against the spirit of PasswordHasher though. You’re supposed to be safe if you use it and again I prefer to introduce some obvious failure than being silently insecure. I’m sure we’ll agree that if I rely on people figuring out their parameter than nobody will do anything at all.

All that said the question is how to proceed since obviously it’s not my intention to tell people “lol your own fault for not reading docs”. The only thing I can think of is a docs & changelog update that warn about the default memory requirement.

from argon2-cffi.

@hynek I think the best option is to put a short description of default parameters on the main page of the docs. A kind of "these are the defaults" and I do think telling your users that they need a minimum of 100MB to run PasswordHasher with the defaults is a good idea.

My point is only that making changes that impact RAM by 2 orders of magnitude probably deserves a pretty bold description.

I get that you probably deal with some pretty stingy security folk. I still think that any security folk worth their salt agrees that the parameters used do not guarantee anything other than some minimum amount of computation time/memory to try to crack given hashes. Is 100MB really more secure than 93MB, or less secure than 110MB? Why not 1GB default, which would be another 10x more memory difficult to break or even 10GB? The reason not 1GB or 10GB is because there are practical tradeoffs that have to always be made. It is pretty impractical to have 10GB of RAM dedicated to every single hash. That said, I understand that you want want a high bar for security as a default in PasswordHasher, but 'safe' isn't a real thing. It is all just some amount of computational power/time to thwart reasonable effort in the future given the resources of today.

As far as I'm concerned this issue is closed from our end. Thanks for making PasswordHasher!

from argon2-cffi.

Better late than never, I've just added a big fat warning: https://argon2-cffi.readthedocs.io/en/latest/api.html

Thank you for reporting this!

from argon2-cffi.