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

Switch to Argon2 for password hashing #3353

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

tomhughes
Copy link
Member

@tomhughes tomhughes commented Oct 27, 2021

It's been five years since we last updated our password hashing and things have moved on - the current OWASP recommendation is to use Argon2.

Argon2 takes care of recording the salt and hash parameters as part of the password so the separate salt is no longer needed except for legacy passwords.

We're using the default parameters (64Mb memory, 2 iterations, 1 degree of parallelism) which exceeds the OWASP recommended values and upgrading will be automatic as the defaults change over time.

There is also support for an optional "pepper" which means that a leak of hashed passwords would be useless without the pepper (a shared secret included in the hashes) which is not present in the database or on the database servers.

@mmd-osm
Copy link
Collaborator

mmd-osm commented Oct 29, 2021

Does this have some impact on CGImap?

@tomhughes
Copy link
Member Author

Possibly if it does password validation, which I guess it must?

@mmd-osm
Copy link
Collaborator

mmd-osm commented Oct 29, 2021

Yes, definitely, I needs to validate passwords independent of what's going on in the Rails port.

@tomhughes
Copy link
Member Author

@mmd-osm
Copy link
Collaborator

mmd-osm commented Oct 31, 2021

I would like to propose two changes for a halfway sane implementation on the CGImap side:

  • Drop the support for PEPPER, as long as libargon2 doesn't provide a public API for it (see verify with secret P-H-C/phc-winner-argon2#314).
    The ruby gem does a lot of magic behind the scenes and re-implements parts of the argon2 code, or directly calls into non-public parts of the code. This is not a feasible approach for CGImap.
  • Use exactly one of the 3 possible algorithms (i, d, or id), to avoid parsing of argon2 hash values on CGImap side.
    Again, the issue is there's no official API to parse argon2 hash values, and I want to avoid any custom code to do so.

@tomhughes tomhughes added changes requested Changes are needed before the PR will be merged or reviewed again work-in-progress Pull request is not ready to be merged labels Nov 2, 2021
@gravitystorm
Copy link
Collaborator

gravitystorm commented Nov 3, 2021

I'm happy to switch algorithms. The main thing I wanted to check is how this would impact (postively or not) on migrating to use Devise to take care of these sorts of things for us.

They don't currently use argon2 for password hashing (they use bcrypt), but I found that they have a plugin system for their password encryptors, and someone else has written a devise-argon2 gem. So as far as I can tell, if/when we move to devise we could write our own custom encryptor, in order to handle all the fallbacks and whatever arguments we are passing to argon2. Although, ideally, I would let someone upstream worry about password algorithms for us.

Talking of fallbacks, I know that we support silent upgrades of old password hashes. Is there merit in limiting how far back we do this, from the perspective of code simplicity and maintainability? I don't think it would be too bad to say if you haven't logged in at some point over the last X (e.g. 10?) years then you need to reset your password.

@tomhughes
Copy link
Member Author

Well the risk is that the older the account the more likely somebody no longer has access to the email, although they may well still know the password.

There is some discussion at https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#upgrading-legacy-hashes where the suggestion is that ideally you would expire old insecure passwords and force a reset while also noting that it may cause an increased support load.

@tomhughes
Copy link
Member Author

tomhughes commented Nov 3, 2021

As suggested by @mmd-osm I have updated this drop the pepper support.

As to the second request the ruby bindings are actually fixed at argon2id already but I have added a test to verify that in case a future version of the gem changed it and also to verify the version as if a new version appeared we would need to make sure that cgimap was using a library that supported it before we could switch to it.

@tomhughes tomhughes removed work-in-progress Pull request is not ready to be merged changes requested Changes are needed before the PR will be merged or reviewed again labels Nov 3, 2021
@tomhughes
Copy link
Member Author

I did a quick survey of the historic passwords - the results are:

Scheme Users Last Password Change Last Login
Unsalted MD5 ~7000 Before Aug 2007 Before Aug 2013
Salted MD5 ~1500000 Before Aug 2013 Before Aug 2013
1000 Round PBKDF2 ~3000000 Before Nov 2016 Before Nov 2016
10000 Round PBKDF2 ~10000000 Since Nov 2016 Since Nov 2016

Based on that I suspect we could reasonable wipe the passwords for the first two groups and certainly for the first group - anybody in those groups hasn't logged in or changed their password in eight years.

@mmd-osm
Copy link
Collaborator

mmd-osm commented Nov 5, 2021

Can we somehow deploy this to the dev instance? To merge the cgimap pull request, libargon2-dev would need to be installed on the dev instance first. I added argon2 as a hard dependency for now. No worries, I will include the additional dependency as a breaking change in the release notes - but it would still be an annoyance for some people out there.

@tomhughes
Copy link
Member Author

I've setup https://argon2.apis.dev.openstreetmap.org/ with both the web site and cgimap branches.

@mmd-osm
Copy link
Collaborator

mmd-osm commented Nov 8, 2021

Thanks! I tested Basic auth in JOSM, and this still appears to be working fine. I cannot test an upgrade to Argon2, as every new website user would immediately start using Argon2.

As we have unit test coverage on the Rails port for the upgrade to Argon2, as well as unit tests on cgimap to use different hashes, this should be ok for now.

@tomhughes tomhughes merged commit e342314 into openstreetmap:master Nov 16, 2021
@tomhughes tomhughes deleted the argon2 branch December 16, 2021 18:45
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

Successfully merging this pull request may close these issues.

3 participants