Add salt parameter to hash generation for sha256 plugins

[Aohzan]

SUMMARY

Add a salt parameter to handle internal hash generation for caching_sha2_password or sha256_password plugin to avoid idempotence failure

Fixes #621

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

mysql_user

ADDITIONAL INFORMATION

mysql_user:
  name: myuser
  host: '%'
  plugin: caching_sha2_password
  plugin_auth_string: mypassword
  salt: TDwqdanU82d0yNtvaabb

will generate same hash for each ansible run and be idempotent

[laurent-indermuehle]

@Aohzan thank you very much for taking the time to share your work.
If you have questions about the CI (GitHub Action) and the integrations tests, feel free to ask me. You can find me on Elements/Matrix in #mysql:ansible.com or here as well (though I come only once per day here).

FYI, mysql is still upset about the format of the hashed password. You can see this by searching for the pattern "Failed" in the output of one of the integration tests that use MySQL up until you reach one without the light-blue "...ignoring".

[Aohzan]

Yes I have a problem to send the hash in the hex format as quote are added in the MySQL command, I will check that next week

edit:PyMySQL doesn't seem to allow to not insert quote arround args : https://github.com/PyMySQL/PyMySQL/blob/b4ed6884a1105df0a27f948f52b3e81d5585634f/pymysql/cursors.py#L129

[Aohzan]

it's finally working @laurent-indermuehle, I let you review
I still have failed CI, if you can help me on it please, my new tests are ok

[laurent-indermuehle]

Sure @Aohzan, thank you for your great work! It's almost perfect as is and it feel like you're experienced with plugin development in Python. I'll suggest minor changes like using the fully qualified collection name for tasks and I saw a repetition in your error handling.

But first, the failing CI integration test comes from this task:

• task: test_mysql_role : Create role0 in check_mode again
  error: user_mod() missing 1 required positional argument: 'password_expire_interval'

The error comes from the positional arguments of user_mod() line 932 of plugins/modules/mysql_role.py. There is one None, missing. I look all the files in the project and found 3 calls to user_mod() in mysql_role.py and mysql_user.py and 1 call to user_add(). So after that fix we should be good.

[Aohzan]

Sure @Aohzan, thank you for your great work! It's almost perfect as is and it feel like you're experienced with plugin development in Python. I'll suggest minor changes like using the fully qualified collection name for tasks and I saw a repetition in your error handling.

Do you want me to use FQCN for all tasks or just tasks that I added ?

[laurent-indermuehle]

I prefer to use one task instead of 2 when assert only test one conditions.

[codecov]

Codecov Report

Attention: Patch coverage is 82.41758% with 16 lines in your changes missing coverage. Please review.

Project coverage is 76.08%. Comparing base (50e7413) to head (4d1d552).

❗ Current head 4d1d552 differs from pull request most recent head b8e0239

Please upload reports for the commit b8e0239 to get more accurate results.

Files	Patch %	Lines
plugins/module_utils/user.py	42.85%	4 Missing and 4 partials ⚠️
plugins/modules/mysql_user.py	25.00%	3 Missing and 3 partials ⚠️
plugins/module_utils/implementations/mysql/hash.py	97.10%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #631      +/-   ##
==========================================
+ Coverage   75.26%   76.08%   +0.81%     
==========================================
  Files          18       19       +1     
  Lines        2515     2542      +27     
  Branches      642      642              
==========================================
+ Hits         1893     1934      +41     
+ Misses        425      417       -8     
+ Partials      197      191       -6     

Flag	Coverage Δ
integration	75.37% <82.41%> (+0.86%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Aohzan]

ready @laurent-indermuehle 😄

[laurent-indermuehle]

I finally found the solution to the hundred of ... ignored in the output of the CI. So please, use it in order to help me to stop this madness :)

More importantly, I was about to ask you to remove if plugin in ['caching_sha2_password', 'sha256_password']: from the modules utils but then realized that we may add an additional auth plugin that supports salt in the future. So it's a good thing you kept it. But please, review my comment about the generated_hash_string variable.

[laurent-indermuehle]

Thanks @Aohzan for the changes.
A late question, have you tried https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html ?

Before we can merge your PR, I'll need to merge #629 then you can rebase on it an launch integrations tests a last time.

[Aohzan]

Thanks @Aohzan for the changes. A late question, have you tried https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html ?

Yes, it's not the same format than the one MySQL want 😞

[Aohzan]

Thanks @Aohzan for the changes.
A late question, have you tried https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html ?

Before we can merge your PR, I'll need to merge #629 then you can rebase on it an launch integrations tests a last time.

Hello
Any news 🙂

[laurent-indermuehle]

@Aohzan I hope to finish the big project I'm on next week. After that I may have some time. We need to add tests for MySQL 8.4, Mariadb 10.11 and fix the regression with the user password. So, fun times ahead ;)

[laurent-indermuehle]

@Aohzan I've merge the other PR. Can you rebase your PR and see if tests still pass please?

[Aohzan]

@Aohzan I've merge the other PR. Can you rebase your PR and see if tests still pass please?

done, and all tests are ok 👍

[Andersson007]

@Aohzan thanks a lot for the contribution! LGTM
Could you please add a changelog fragment? See the currently present fragment for the precise format

[Aohzan]

done

[laurent-indermuehle]

@Aohzan thanks again for your contribution. I'm merging now.
The next steps are:

• For me to add tests for MySQL 8.1, 8.2 or 8.3 (don't know yet)
• For me to add tests for Python 3.11 and 3.12
• Release the collection

So before you can use your feature it is possible that I may ask for your help in case something doesn't work, if that's ok for you and you have the time? But maybe hopefully it will work without requiring any modification, who knows.

[Aohzan]

Yes no problem

[Andersson007]

@Aohzan @tchernomax @laurent-indermuehle thanks for the contribution!