Fix hashed passwords being returned by get_existing_authentication() via the plugin_auth_string variable instead of plugin_hash_string

[laurent-indermuehle]

SUMMARY

Fix hashed passwords being returned by get_existing_authentication() via the plugin_auth_string variable instead of plugin_hash_string. This method is used in user_add(), which doesn't seem appropriate. I would have expected to find it in user_mod() instead. I hope it is never used and doesn't require to treat this PR as a major_change.
The only other place where get_existing_authentication() is used is in mysql_info while utilizing the users_info filter introduced in 3.8.0 (#580). Since this feature is relatively new, I don't expect it to have widespread usage yet.

This doesn't f-i-x #621 but the issue was reported there. It makes more sense to use the right variable anyway.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

mysql_user, mysql_info

ADDITIONAL INFORMATION

Using mysql_info with community.mysql 3.9.0:

users_info:
    - host: localhost
      name: root
      plugin: mysql_native_password
      plugin_auth_string: '*7D2ABFF56C15D67445082FBB4ACD2DCD26C0ED57'
      priv: '*.*:ALL,GRANT'

Using mysql_info with this PR:

users_info:
    - host: localhost
      name: root
      plugin: mysql_native_password
      plugin_hash_string: '*7D2ABFF56C15D67445082FBB4ACD2DCD26C0ED57'  # <------ Fixed
      priv: '*.*:ALL,GRANT'

[Andersson007]

@laurent-indermuehle thanks for the PR! Makes sense to change, though looks like a breaking change as we're changing the interface here.
Maybe it's time to plan the major release and incorporate it in https://github.com/ansible-collections/community.mysql/milestone/1 ?

For now, can we have both the keys having the same content (i.e. change your PR to have both)
if it's possible, we could add a deprecation message to the changelog under major_changes/breaking changes now saying that the wrong one will go away in 4.0.0.
In several month we could release 4.0.0 - it'll be at least a bit expected for users.

I'm not insisting though. Please decide how you think it'll be OK and I'll share the responsibility in case of any complaints (which might not even happen, not sure if anyone checks that key in playbook conditions).

[laurent-indermuehle]

@Andersson007 I'm lost. The original method get_existing_authentication returns plugin and plugin_auth_string. But then... can you explain this line: https://github.com/ansible-collections/community.mysql/blob/main/plugins/module_utils/user.py#L155 ? Where auth_string comes from?

We should be more careful when we add a functionality that is not supported by MySQL and MariaDB. It adds complexity for no obvious reasons. For instance, I've read #344 and feel like it could have been solved in the playbook with 1-2 tasks and loop. I thank @betanummeric for this feature that adds to the value of c.mysql. But it is now hard to maintain.

All the tests are passing before and after I change the variables. I've lost my confidence about this change.
I think it's because in user_add() and user_mod() plugin_hash_string has a higher precedence over plugin_auth_string.

But yes, you're right. I can add plugin_hash_string and keep plugin_auth_string as before. I'm changing my PR accordingly. I push to see if CI is all green, but I'm unhappy with the complexity of the password handling.

[Andersson007]

@laurent-indermuehle let's emphasize the (potentially) breaking change in the changelog.

[Andersson007]

@betanummeric any concerns on this PR?

[betanummeric]

Oh, it looks like the "update_password: on_new_username" feature of mysql_user which was introduced by #365 (@laurent-indermuehle auth_string comes from there) was broken by #580, so since v3.8.0. :(

The on_new_username feature attempts to reuse an existing password when the username matches, even if the host of the user doesn't match.
For example, say the user foo@'192.0.2.1' already exists with password asdf.
When I execute mysql_user to create a new user foo@'192.0.2.100', mysql_user with update_password: on_new_username will set the password of the exiting foo@'192.0.2.1' also for foo@'192.0.2.100', regardless of what new password I specified in the module arguments.
Then I manually set a different password, only for foo@'192.0.2.100'.
When I then create a third user foo@'192.0.2.99', also with update_password: on_new_username, reusing the existing password will not work because there are multiple distinct options. In this case the password from the module arguments is used for the new user foo@'192.0.2.99'.

The feature is currently broken because

• when the user does not already exists, an IndexError will be raised here: https://github.com/ansible-collections/community.mysql/blob/main/plugins/module_utils/user.py#L121
• The host part must be ignored, it should check for all users with the same name: https://github.com/ansible-collections/community.mysql/blob/main/plugins/module_utils/user.py#L152
• The get_existing_authentication function should only return something when there is exactly one distinct password/authentication_string, to avoid ambiguity. (Therefore the limit 2 in the SQL).

I am using this feature in production so I am happy I did not upgrade to the broken version yet and I would be glad if we can fix this.

Sure, it would be possible to work around this outside of the module, but I thought others could use this feature too. I think the logic is simple enough to maintain, but apparently the documentation could have been better.

[laurent-indermuehle]

@betanummeric thanks for your reply.
I'm sorry if my PR #580 broke something. To be honest, when I wrote it I was unaware of the difference between MySQL and MariaDB for handling roles and also the update_password feature.

But, if the tests still passes, it means they are incomplete! We should fix that.

[laurent-indermuehle]

@betanummeric could you confirm something please?
When reading the documentation:

- "C(on_new_username) works like C(on_create), but it tries to reuse an existing password: If one different user
        with the same username exists, or multiple different users with the same username and equal C(plugin) and
        C(authentication_string) attribute, the existing C(plugin) and C(authentication_string) are used for the
        new user instead of the I(password), I(plugin), I(plugin_hash_string) or I(plugin_auth_string) argument."

It's not clear to me what will happen if I have 2 accounts with same username but different passwords and I'm creating a 3rd account with update_password: on_new_username. And if I can leave the password unset.

The integration tests here suggests that the plugin will use the password set in the arguments.

But I thought this feature would be used without setting any password at all. Since you explicitly want to reuse an existing one? I was expecting a failure if the account already exists with different passwords.

When I will understand this, I will be able to decide what to do with the current PR. Let me know if you feel that this PR could be merged and we fix update_password in another one.

[betanummeric]

@laurent-indermuehle

It's not clear to me what will happen if I have 2 accounts with same username but different passwords and I'm creating a 3rd account with update_password: on_new_username.

In that case the module arguments password, plugin, plugin_hash_string and plugin_auth_string are applied to the user as if it had been update_password: on_create.

I intentionally built on_new_username as a best-effort attempt which will fallback to on_create if it finds multiple different passwords. Changing that to fail upon ambiguity would be a breaking change.

[laurent-indermuehle]

@betanummeric thank you for confirming the first part and the explanation about limit 2.

I get that the root cause is that get_existing_authentication() only searched for usernames and in v3.8.0 I added the hostname as well. So in the tests for update_password: on_new_username, user_add() search for an existing password, doesn't found any, so it apply test_password3. Which, by accident, is what the test expects.

So, I think we need to:

• Add a test when no previous user exists (IndexError)
• Refactor get_existing_authentication() to support both host and retrurn_all_hosts. Or split this into 2 methods get_user_host_existing_authentication() and get_user_existing_authentation().
• Add a warning/failure if multiple accounts with same username but different passwords exists

Will that works?

[betanummeric]

• When no previous user exists, update_password: on_new_username should set the password from the arguments without error. Yes, there should be a test for that.
• I would make get_existing_authentication support both usecases, but splitting it into two functions would work as well.
• We can add a warning when multiple different passwords are found, but please no failure.

[Andersson007]

any updates on this? there's another PR depending on this one

[laurent-indermuehle]

I'm busy on something else. I won't have time before few weeks. @betanummeric have you some time to add the missings tests?

[laurent-indermuehle]

Ok @betanummeric I know what's wrong... the tasks file test_update_password.yml that contains your tests is not included in main.yml... Just by adding that include, I can see the IndexError!

So, I'm going to merge this PR that fix another real issue. And open a new one that address the regression!

[Andersson007]

@laurent-indermuehle +1