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
expose Argon2 options (as we did for bcrypt) #19023
Conversation
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 with @kesselb's suggestions :)
Yep lets use the suggestions form @kesselb and then I'm fine with it! |
Co-Authored-By: kesselb <mail@danielkesselberg.de> Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
3e52d1c
to
56c3ba6
Compare
/backport to stable18 |
/backport to stable17 |
/backport to stable16 |
You can't backport that far. Or then you need some hardening. As this is only available starting at php 7.2 |
@rullzer 16 supports 7.2, so that should work. Provided, that unused parameters are being ignored, I am going to double check that. |
Checked again, no conflicts. Because hash_password fails when minimum values are undershot, I'll add some checks. |
… this should also fix CI now |
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
b35b45d
to
f92ba2c
Compare
Bonus points if you add the minimum value to |
but but but but it is document in co… … oh. I see. Okay, for the Karma! |
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Added the label for proper documentation as suggested in #18677 (comment) (even though this is rather for admins) |
backport to stable18 in #19094 |
backport to stable17 in #19095 |
backport to stable16 in #19096 |
cc @nachoparker @MichaIng probably something for NextcloudPi and DietPi |
@kesselb Just to give some impressions from RPi2:
Btw important to know:
|
if (\defined('PASSWORD_ARGON2I')) { | ||
// password_hash fails, when the minimum values are undershot. | ||
// In this case, ignore and revert to default | ||
if ($this->config->getSystemValueInt('hashingMemoryCost', PASSWORD_ARGON2_DEFAULT_MEMORY_COST) >= 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be >= 8 * $hashingThreads
: #19023 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @blizzz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaIng mind to send a patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this, it helped me a lot - after all, after reducing the hashing costs by this parameters for argon2, my server is responding to requests again! :-) |
Yes good question. Argon2 is a password hash method, so used to hash and compare sensitive strings, not to encrypt data with the purpose of later decrypt. I was checking the code to see where this hash function is used (surely missed many):
Generally of course where an authentication with password against Nextcloud is done or a new password is set, so there is only a hashed version stored and the input hashed and compared. File integrity check is obvious.
Did you recognise anything else then when making new connections with clients or web login? The above cases I found should e.g. not affect browsing the web UI or performance of file transfer after connection has been established, but probably I have overseen an important use. EDIT: Having a closer look, even in many other cases different
That is also what I am most interested in. Some readings: https://en.wikipedia.org/wiki/Argon2
Now I am confused, and it seems I am not the only one, sadly... On my RPi, time cost and time in seconds matched with max execution time when considering the additional time for connection, script processing and such. Another test on much stronger system should give quick verification which is true. Ah found the Nextcloud docs also saying that it is about the amount of seconds. What I don't understand, if this is true, as asked above already, is how one knows the amount of iterations that need to be done for hashing, when you want to compare the result to a stored hash, as there is only one Regardless, the articles link several possible attacks and how Argon2i vs Argon2d vs Argon2id make it harder, how much iterations are considered as safe etc. But while I lack insights, it seems like an attacker needs direct access to the hardware or needs to execute the hash function repeatedly on your system to apply any of those attacks. In regular cases the default values obviously can be reduced significantly (above link: So now I want to know what which half of docs is wrong... |
Okay, the
This totally makes sense as, when one wants to verify the hash, this is what one must know, and not how much time the particular machine required or had as limit to create the hash in its particular load state... @blizzz I think something got mixed up here, I guess also due to the bid unclear (likely as well due to misunderstanding by author) PHP documentation, but this should be definitely fixed in code comment and documentation. But probably I am wrong and you can review and verify first as this is all based on logic thoughts, the fact that the same input, especially And indeed it would be nice to know in which cases this is used, so one has a good basis to find reasonable numbers for ones particular environment and use case. I could also add the small script to the docs so one can test which values lead to which times. Also recommendations could be added, like:
There are many more cases that show that either me and PHP wiki are wrong or the whole rest of the world 😄:
All talk about execution time or time in seconds... Here someone got it right, calling it the "Number of iterations t" that of course affect processing time: https://crypto.stackexchange.com/a/37140 |
+ DietPi-Software | Nextcloud: Add Argon2 hashing tweaks. Use all available CPU threads for now, which radically reduces hashing time one multi-core SBCs, wait with memory cost and time cost until getting some clarification, especially since those are security relevant and defaults do not break anything: nextcloud/server#19023 (comment)
helps with
#17131#17241 at leastAt the moment we already expose the settings for bcrypt ('hashingCost'), but since PHP 7.2 we use Argon2 instead. In the issue linked at the beginning there are complaints about high CPU usage. I saw similar when profiling a different case.
The PHP default config (on my system) is to take up to 64 MB RAM on a single thread for up to 4 seconds. You feel it. Now, for more powerful systems it is reasonable to allow using more threads, while for weaker machines less RAM (or a shorter time) might make sense.
Default PHP config is followed if no override is given in config.php.
For >=18 we should removed the hashingCost config. I intend a follow up config, so this one can be backported easily.