Skip to content

Fixes #26996: Add argon2id support for local hash #6407

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2025
Merged

Fixes #26996: Add argon2id support for local hash #6407

merged 1 commit into from
Jun 26, 2025

Conversation

@amousset
Copy link
Member

@amousset amousset commented May 28, 2025 •
edited
Loading

https://issues.rudder.io/issues/26996

  • The way the migration currently works prevent users from modifying the hash algorithm used for new passwords as the value is overridden at each file load. I just updated the tests to hardcoded argon2id value instead of bcrypt as it should be transparent for users.
  • Removing verifyHash which does not appear to be used, and I don't see any reason to do this kind of check.
  • Improved a bit the tests. Especially check that wrong passwords are not accepted. Add some property based-testting instead of testing a second hardcoded password.
  • Centralize configuration values in PasswordEncoderType.
  • Introduce opaque type usage for argon2 user-configurable parameters.

@amousset
Copy link
Member Author

amousset commented May 30, 2025

PR updated with a new commit

1 similar comment
@amousset
Copy link
Member Author

amousset commented Jun 3, 2025

PR updated with a new commit

@amousset amousset force-pushed the arch_26996/add_argon2id_support_for_local_hash branch from f4150d9 to e068135 Compare June 17, 2025 15:21
@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

2 similar comments
@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

@amousset amousset marked this pull request as ready for review June 18, 2025 13:47
@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

4 similar comments
@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 18, 2025

PR updated with a new commit

@amousset amousset requested a review from fanf June 18, 2025 15:06
@amousset amousset force-pushed the arch_26996/add_argon2id_support_for_local_hash branch from ae300e2 to 2209869 Compare June 18, 2025 15:22
@amousset amousset requested a review from clarktsiory June 18, 2025 15:23
fanf
fanf requested changes Jun 19, 2025
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Really nice PR ! The logic is OK and tests too. Some things can be improved regarding scala idiomatic usage.

clarktsiory
clarktsiory requested changes Jun 20, 2025
View reviewed changes
Copy link
Contributor

@clarktsiory clarktsiory left a comment

Choose a reason for hiding this comment

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

the change is great ! I only have some additional suggestions

@amousset
Copy link
Member Author

amousset commented Jun 22, 2025

PR updated with a new commit

4 similar comments
@amousset
Copy link
Member Author

amousset commented Jun 23, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 23, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 24, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 24, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 24, 2025

PR updated with a new commit

@amousset amousset force-pushed the arch_26996/add_argon2id_support_for_local_hash branch from 8b7102e to cbfb454 Compare June 24, 2025 16:26
@amousset
Copy link
Member Author

amousset commented Jun 24, 2025

PR updated with a new commit

4 similar comments
@amousset
Copy link
Member Author

amousset commented Jun 24, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 24, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 25, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 25, 2025

PR updated with a new commit

@amousset
Copy link
Member Author

amousset commented Jun 25, 2025

PR updated with a new commit

fanf
fanf reviewed Jun 26, 2025
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

It's shapping nicely! But I don't get why you didn't go all in for the opaque type and use them everywhere they could have been ?

Plus some details

@amousset
Copy link
Member Author

amousset commented Jun 26, 2025

PR updated with a new commit

1 similar comment
@amousset
Copy link
Member Author

amousset commented Jun 26, 2025

PR updated with a new commit

fanf
fanf reviewed Jun 26, 2025
View reviewed changes
webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/users/Argon2.scala
parallelism: Argon2Parallelism,
hashSize: Argon2HashSize,
saltSize: Argon2SaltSize
)
Copy link
Member

@fanf fanf Jun 26, 2025

Choose a reason for hiding this comment

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

🚀

@fanf
Copy link
Member

fanf commented Jun 26, 2025

LGTM, appart the magnitude factor in the memory parameter in argon algo

@amousset
Copy link
Member Author

amousset commented Jun 26, 2025

PR updated with a new commit

1 similar comment
@amousset
Copy link
Member Author

amousset commented Jun 26, 2025

PR updated with a new commit

@Normation-Quality-Assistant
Copy link
Contributor

Normation-Quality-Assistant commented Jun 26, 2025

OK, squash merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant force-pushed the arch_26996/add_argon2id_support_for_local_hash branch from bd8cc8f to 2bdec40 Compare June 26, 2025 15:40
@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 2bdec40 into Normation:branches/rudder/9.0 Jun 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fanf fanf fanf requested changes

@clarktsiory clarktsiory clarktsiory requested changes

Assignees

No one assigned

Labels

Ready for merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amousset @fanf @Normation-Quality-Assistant @clarktsiory