Closed benedmunds closed 3 years ago
I'm not sure this sits well with the requirement to pass in the respective hash method's params (as defined in the IonAuth config).
If we don't specify a hash_method
in the config (so we make use of PASSWORD_DEFAULT
in your revision) _get_hash_parameters
may return FALSE
since the hash_method
could then be something other than bcrypt
or argon2
in the future.
Also, maybe for compatibility in your lib, if you've specified argon2
it should stay as argon2
but provide the option for argon2id
?
Something like:
protected function _get_hash_algo()
{
$algo = FALSE;
switch ($this->hash_method)
{
case 'bcrypt':
$algo = PASSWORD_BCRYPT;
break;
case 'argon2':
$algo = PASSWORD_ARGON2I;
break;
case 'argon2id':
$algo = PASSWORD_ARGON2ID;
break;
default:
// Do nothing
}
return $algo;
}
In _get_hash_parameters
you'd need something like
...
case 'argon2':
case 'argon2id':
$params = $is_admin ? $this->config->item('argon2_admin_params', 'ion_auth')
: $this->config->item('argon2_default_params', 'ion_auth');
break;
...
Good point on the hash params. In hindsight I should've just let users pass it an override straight to PHP's functions and had sane defaults otherwise.
I don't see a good solution that maintains BC at this point though, so I'm going to go with your suggestions. Please review when you can.
One thing to note is that I'm using the Argon2 hash config for both Argon2 and Argon2id. Lmk if you think that doesn't make sense.
Thanks for all the help here!
I'm glad I could help, even if I wasn't confident enough to implement the changes myself!
Update hash algo to default to the PASSWORD_DEFAULT constant if not specified.
Also changed the ARGON default to ARGON2ID.