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
Allow selecting password_hash algorithm #17199
Conversation
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.
LGTM, there are some code formatting issues (if
blocks aren't formatted like the rest of matomo), but code looks good and the change makes sense. 👍
Ah, I am not so familiar with the whole braces positioning discussion (I mostly write in Python which doesn't have this issue 🙂 ), so I might have ticked a few to many "in new line" boxes. |
@Findus23 quick question: Do you know how much it would make things slower to simply use the highest settings by default? Not sure how often we use the password_verify but might be only on log in etc and maybe we could try to simply use the strongest setting by default so we don't need a config for it? Just a random thought (I'm not much into these options) |
There are no highest settings, all three argon2 options are open ended. The nextcloud docs also mention that using more threads than the server has CPUs makes no sense (as it slows things down a lot). And memory is of course limited by the memory limit. As to why we maybe shouldn't just hardcode argon2 without an option to use bcrypt: The nextcloud team added them afterwards as quite a few people reported a high CPU usage (as is intended with argon2), but their server couldn't handle the load ( And there is no perfect setting for everyone as the whole point is to slow down the calculation of the hash by a specific time and depending on the CPU speed of the server this might be a bit more or less. (e.g. KeepassXC, which of course can afford higher complexity sets the parameter to a value so that one hash takes one second on the current CPU). |
Thanks for clarifying @Findus23 |
@diosmosis seems you have accidentally added the screenshot to git directly instead of git lfs. I'll change that on 4.x-dev branch directly. |
@sgiehl 👍 thanks, I guess I forgot to set lfs up on my new machine |
Some people might want their Matomo passwords to be hashed with a slower, more computationally complex algorithm than bcrypt to make sure that in case attackers ever get access to the matomo_user table, bruteforcing the hashes needs a lot of computational power even for more insecure passwords.
Argon2 allows this (and is supported since PHP 7.2) and also provides three options to tune the CPU and memory usage of the hash calculation. Increasing this value might increase load on the server during login, but this isn't something that should happen very often on a typical Matomo instance.
Also one day we could change the default to argon2 (like Nextcloud did), but this feel more like a breaking change than just allowing customization.
Just changing the setting works out of the box as Matomo already checks on login if the stored hash uses the correct algorithm and hashes and stores the password again if not. So after changing the settings, the next time a user loggs in their hash is updated.
As I am not so familar with PHP and the codesyle, this should be more seen as a proof of concept than the definitive implementation.
Also I tried to keep it conceptually similar to https://github.com/nextcloud/server/blob/master/lib/private/Security/Hasher.php
Review