Skip to content

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