Skip to content

Fix #78969 Add password_default_algo() function #5104

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

Closed
wants to merge 1 commit into from

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Jan 21, 2020

I think it makes sense adding a way for users to determine the actual default password hashing algorithm.

I chose password_default_algo() to return not just the algorithm's name, but the ID too (which are different in case of Bcrypt). I think it is worth to return the default options as well. This way, password_default_algo() will be in line with the data structure returned by password_get_info().

@kocsismate kocsismate force-pushed the password-default-algo branch from 9089c32 to 99f134f Compare January 22, 2020 07:53
@nikic
Copy link
Member

nikic commented Jan 22, 2020

As I asked on the ML, is there any reason why we can't make PASSWORD_DEFAULT equal to PASSWORD_BCRYPT again, instead of null? I'd prefer doing that, as it keeps better compatibility with PHP < 7.4.

The password_default_algo() conflates two things: Default algorithm and default options. I think the default algorithm should be provided by the constant. The default options should not be coupled to the default algorithm, as that means you can only get the options for the default algorithm. There would be no way to fetch the default options for argon2i under this design. (Whether getting the default options is useful is another matter ... but it shouldn't be coupled...)

@kocsismate
Copy link
Member Author

kocsismate commented Jan 22, 2020

as that means you can only get the options for the default algorithm

Yes! I was considering to add password_algo(?string $id): array too exactly for this purpose, but didn't have time to ask if it would be useful. By having it, one could retrieve all the necessary information for any algo by calling password_algo(PASSWORD_DEFAULT) or password_algo(PASSWORD_BCRYPT).

Speaking about the PASSWORD_DEFAULT.. I would also be in favour of making it non-null.. However it doesn't look very right to change the value of constants in every release :/ (although a major version would be a not-that-bad target for this change 🤔). So if we can do that then instead of password_algo(), we could add password_algo_default_options(string $id).

@nikic
Copy link
Member

nikic commented Jan 22, 2020

@kocsismate I'm suggesting to change PASSWORD_DEFAULT for 7.4.3, as a fix for an unintentional BC break in 7.4.0.

@kocsismate
Copy link
Member Author

@nikic I got your point now. I'll get this ready in this PR.

@kocsismate
Copy link
Member Author

I chose to create a new PR instead.

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.

2 participants