@Findus23 opened this Pull Request on February 7th 2021 Member

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

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@Findus23 commented on February 8th 2021 Member

Ah, I am not so familiar with the whole braces positioning discussion (I mostly write in Python which doesn't have this issue :slightly_smiling_face: ), so I might have ticked a few to many "in new line" boxes.
It should be looking better now.

@tsteur commented on February 8th 2021 Member

@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)

@Findus23 commented on February 9th 2021 Member

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 (https://github.com/nextcloud/server/issues/17241). Also it seems like some CPUs are much slower with argon2 (https://github.com/nextcloud/server/issues/22886).
So having a fallback for those few people should be helpful. Even though they are probably fewer with Matomo as on one hand fewer people run it on very-low-end servers like Raspberry Pis and on the other hand it might be that Nextcloud needs to run password_hash on every API request, which Matomo doesn't.

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).

@tsteur commented on February 9th 2021 Member

Thanks for clarifying @Findus23

Powered by GitHub Issue Mirror