Default crypto config params used when registering a new user in the admin group

[jamieburchell]

Which branch are you using? 3

What commit hash are you on? 14a9bb1

What CodeIgniter version are you using? v3

What PHP version are you using? 7.4

Describe the bug When registering a new user in an admin group, the "default" hash config params are used. I believe this to be because the hash_password method only uses the identity to lookup a user's group, which is not passed at registration.

To Reproduce Register a user in an admin group, the crypto settings for the default group are used.

Expected behaviour The admin settings should be used for users registered in the admin group.

[benedmunds]

Can you do a little troubleshooting here to see if this is being called correctly:

https://github.com/benedmunds/CodeIgniter-Ion-Auth/blob/14a9bb12ceda4806d56e0915423d4b26ee9b737a/models/Ion_auth_model.php#L880

This linked code is what sets the groups that the user is in when registering a new user.

This doesn't affect their hashing config though, the hashing config is shared between all users regardless of the groups they are in.

[jamieburchell]

There is a hashing config for "default" and "admin" groups:

$config['bcrypt_default_cost']
$config['bcrypt_admin_cost']
$config['argon2_default_params']
$config['argon2_admin_params']

The issue stems from here: https://github.com/benedmunds/CodeIgniter-Ion-Auth/blob/3/models/Ion_auth_model.php#L837

No identity is passed to the method which would interrogate the user to find out which group and therefore hashing config to use. As no user exists at that point, it can't be passed. So, even if a user is in an admin group they will always initially get the default config.

[benedmunds]

Ohhh I see what you mean. This is going to take some digging from me to figure out how to solve... I'll get back with you next week.

[jamieburchell]

Yeah no worries - I had a good look myself to see if I can find a simple solution but it either meant calling the rehash method immediately after, thus potentially wasting time or refactoring the method which expects an identity.

[jamieburchell]

I've been thinking a little more on this. Why prioritise the sensitivity of a user's password based on if they are a "normal" user or an "admin" user in the first place? Shouldn't all user passwords be treated with the most sensitivity? My vote would be to remove the functionality and have the strongest possible values in place for all password hashing.

As an aside, with PHP 7.4 and php-sodium, the default argon2_default_params configuration fails as threads can only be 1 and if this is set to 1, the value for time_cost is then too small (which causes password_hash to fail). I appreciate users are encouraged to set their own values. In my case I decided to set the defaults to those that PHP recommend, which may depend on the underlying library and which may change over time:

'memory_cost'   => PASSWORD_ARGON2_DEFAULT_MEMORY_COST,
'time_cost' => PASSWORD_ARGON2_DEFAULT_TIME_COST,
'threads'   => PASSWORD_ARGON2_DEFAULT_THREADS

This may not be suitable for everyone though, because those constants might not be available if argon2 support is missing.

[benedmunds]

I agree with you that having different configs for regular users and admin users doesn't make sense. I also like the idea of defaulting these to the PHP constants when possible.

There shouldn't be any BC issues since we will rehash as needed and password_verify doesn't take the params as input.

Here is a PR with the proposed changes, lmk what you think: https://github.com/benedmunds/CodeIgniter-Ion-Auth/pull/1498

NOTE: Once this is smoothed out I need to make similar changes in v4.

[benedmunds]

Closing due to inactivity. Feel free to re-open if this is still an issue.