Skip to content

Conversation

@MichaIng
Copy link
Member

@MichaIng MichaIng commented May 1, 2020

We check and apply Argon2 settings from config.php in case of Argon2i hashing. Since NC19 Argon2id hashing is used, when available, which uses the same settings, hence we should apply them for Argon2id as well: #20710 (comment)

To be sure, the minimum values could be retested for Argon2id explicitly: #19023 (comment)

@MichaIng MichaIng added enhancement 3. to review Waiting for reviews labels May 1, 2020
@MichaIng MichaIng added this to the Nextcloud 19 milestone May 1, 2020
@MichaIng MichaIng requested review from blizzz, kesselb and rullzer May 1, 2020 09:47
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

I think if argon2id is there argon2i is also there but still

@rullzer rullzer mentioned this pull request May 1, 2020
2 tasks
@kesselb
Copy link
Contributor

kesselb commented May 1, 2020

I think if argon2id is there argon2i is also there but still

Yes. But that's probably not true the other way around :)

@rullzer
Copy link
Member

rullzer commented May 1, 2020

I think if argon2id is there argon2i is also there but still

Yes. But that's probably not true the other way around :)

sure but that is fine. If i is there (as in the original check) then all is already good. But doesn't hurt to be sure

@MichaIng
Copy link
Member Author

MichaIng commented May 1, 2020

I think if argon2id is there argon2i is also there but still

Very likely, however now we are failsafe in case Argon2i is dropped in the feature or somehow disabled in certain circumstances.

@rullzer
Copy link
Member

rullzer commented May 1, 2020

Exactly hence my approval :D

@rullzer rullzer merged commit 8c023a6 into master May 1, 2020
@rullzer rullzer deleted the enh/argon2id-options branch May 1, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants