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

[frontend] Adding the use of Argon2id for password hashing #978

Merged
merged 14 commits into from
Feb 5, 2024

Conversation

AlexandreDoneux
Copy link
Contributor

Related to issue #358

This pull request contains changes regarding multiple elements :

  • Adding a new hashing function hash_password_argon2id and renaming the old one to hash_password_sha512.
  • Supporting user authentication regardless of the algorithm hashing their password in the database.
  • Using the argon2 hash for new users (via self registering, administrator registering or during the installation process).
  • Using the argon2 hash when a users resets it's password or changes it

Password hashes with argon2id are now stored in the database using a prepend "argon2id-" to allow future changes.

@AlexandreDoneux AlexandreDoneux changed the title Adding the use of Argon2id for password hashing [frontend] Adding the use of Argon2id for password hashing Oct 24, 2023
inginious/frontend/installer.py Outdated Show resolved Hide resolved
inginious/frontend/pages/preferences/profile.py Outdated Show resolved Hide resolved
inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
Copy link
Member

@anthonygego anthonygego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also rebase this PR so that it has no more conflicts ?

inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
inginious/frontend/user_manager.py Show resolved Hide resolved
@AlexandreDoneux
Copy link
Contributor Author

AlexandreDoneux commented Dec 8, 2023

Rebase on 2e692aa.

inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
error = True
msg = _("Incorrect old password.")
return result, msg, error
user = self.user_manager.auth_user(self.user_manager.session_username(), data["oldpasswd"], False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit confused with prior code already, but can you confirm that a user with no password (registered through social networks for instance) can actually set a password to use password authentication ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can now. I changed the structure for handling user password change in this PR. Tell me if your ok with it.

@AlexandreDoneux
Copy link
Contributor Author

Rebase on e2de1c3.

Copy link
Member

@anthonygego anthonygego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd recommend moving the latest_method prepending into the hash_password method as suggested, to keep the same behaviour for sha512 when saved, and to avoid repeating the identifier in the hashing method itself.

I let you test and confirm this is OK for you and then we'll be able to merge.

inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
inginious/frontend/user_manager.py Outdated Show resolved Hide resolved
@AlexandreDoneux
Copy link
Contributor Author

I'd recommend moving the latest_method prepending into the hash_password method as suggested

Done

@anthonygego anthonygego merged commit f40d36b into UCL-INGI:master Feb 5, 2024
1 of 3 checks passed
@nrybowski nrybowski added this to the v0.9.0 milestone Feb 8, 2024
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