Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to use hex hash with caching_sha2_password plugin #1612

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

C24-AK
Copy link

@C24-AK C24-AK commented Dec 28, 2023

Summary

This is a pull request to add the ability for users to use hex hashes to authenticate with caching_sha2_password plugin.

How does it work:

mysql::server::users:
  native@localhost:
    password_hash: '*8A47DD31CC0202C1682AA9C78AAD8D031E817BCB'  # this is mysql_native_password
  caching@localhost:
    plugin: caching_sha2_password
    password_hash: '0x244124303035244A306D441C066316643B317F7C361706720F2D6A4C45772F7050666A7032656B7443544A5543683556386A654A3367427336484E4A2E3361666C6868454743' # this is a hex hash for caching_sha2_password

As for now, you can't use plain passwords, since the module does not create a valid hash for you.
But it is possible to use a generated hex hash.

Why not the original authentication string?

The original authentication string when printed out contains non-printable binary characters.
Since the hex format always contains a unique string with printable characters, this is working.

How to generate this hash?

Locally I used this to generate my hash and then use it in puppet:

CREATE USER 'caching'@'localhost' IDENTIFIED WITH 'caching_sha2_password' BY '<your-password>';
SET print_identified_with_as_hex=1; 
SHOW CREATE USER 'caching'@'localhost'; 
DROP USER 'caching'@'localhost'; 
SET print_identified_with_as_hex=0;

This gives you the output. It is necessary to have the 0x prefix in front of the hash, so mysql syntax will not be broken.

I appreciate every commit and help for this PR. I am by no mean a ruby expert and used it for the first time.

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2023

CLA assistant check
All committers have signed the CLA.

@Jimadine
Copy link

This would be a very useful improvement ❤

To add to this, looking at the defined type mysql::db, it appears that there is no user plugin parameter as there is for mysql_user. mysql::db uses mysql_user, so I don't think there's any reason why it can't. So an improvement to your PR would be to add a user plugin parameter to mysql::db.

I've quickly modified manifests/db.pp with what I think would need changing (Lines 8,23,24,55 & 110 have been added). For comparison, here's the original from main.

I'm not sure if the new plugin parameter I've added should be an enum, or whether it should be a bit more relaxed as it is in mysql_user, which is a regex matching %r{\w+}.

Note that, in mysql::db, the existing password parameter value can be either a password or password hash, as it uses mysql_user.

@C24-AK
Copy link
Author

C24-AK commented Jan 23, 2024

Hey @Jimadine ,
I appreciate the answer!
Ye I think that's a good idea. I would prefer the plugin to be more relaxed, since we don't know every plugin (maybe there are custom plugins?), regex matching is fine.

I encountered a bug which I will focus first and come back to the request later. :)

@CiderAndWhisky
Copy link

Can we get this merged? Would be helpful for us, we use this branch in prod already, and we want to be able to update without losing the changes.

@bastelfreak
Copy link
Collaborator

@C24-AK thanks for the PR! Can you take a look at the rubocop annotations and rebase against the latest main branch (not a merge commit)?

@C24-AK C24-AK force-pushed the feature/caching_sha2_password_hash branch from 52e36b6 to 491fca1 Compare February 23, 2024 14:59
@C24-AK
Copy link
Author

C24-AK commented Feb 23, 2024

@bastelfreak done! Looking forward for the review

@Jimadine
Copy link

@C24-AK - Do you think you'd be able to add a plugin parameter to $user_resource of the mysql::db defined type, per my earlier comment, as part of this PR, or would you prefer to keep this separate?

@C24-AK
Copy link
Author

C24-AK commented Feb 26, 2024

@C24-AK - Do you think you'd be able to add a plugin parameter to $user_resource of the mysql::db defined type, per my earlier comment, as part of this PR, or would you prefer to keep this separate?

Done, I appreciate if you could test the changes and see if everything works as expected

manifests/db.pp Outdated Show resolved Hide resolved
@Jimadine
Copy link

@C24-AK - Do you think you'd be able to add a plugin parameter to $user_resource of the mysql::db defined type, per my earlier comment, as part of this PR, or would you prefer to keep this separate?

Done, I appreciate if you could test the changes and see if everything works as expected

Thank you! I've looked at your changes and they look good. I'm attempting to test, but am facing a problem related to setting the mysql root password:

Error: can't modify frozen String: "ALTER USER 'root'@'localhost' IDENTIFIED WITH"
Error: /Stage[main]/Mysql::Server::Root_password/Mysql_user[root@localhost]/password_hash: change from [old password hash redacted] to [new password hash redacted] failed: can't modify frozen String: "ALTER USER 'root'@'localhost' IDENTIFIED WITH"

I'm not sure if this problem relates to the changes in this PR, but I tried 15.0.0 from Puppet Forge and didn't see the problem. The Puppet resource relevant to the error is:

class {'::mysql::server':
  package_name            => 'mysql-server',
  service_name            => 'mysql',
  remove_default_accounts => true,
  restart                 => true,
  service_enabled         => true,
  config_file             => '/etc/mysql/my.cnf',
  create_root_user        => true,
  create_root_my_cnf      => true,
  root_password           => $dlib_atom::mysql_root_password,
  override_options        => {
    mysqld => {
      default-authentication-plugin => $mysql_user_auth_plugin,
      sql_mode                      => 'ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION',
      optimizer_switch              => '\'block_nested_loop=off\'',
      disable_log_bin               => '',
      mysqlx                        => 'off',
      log_error_verbosity           => '1'
    }
  },
}

Apologies - I guess this is likely a problem limited to the specific scenario (an internal module) where I'm using the module! Hopefully I'll work it out.

@C24-AK
Copy link
Author

C24-AK commented Feb 28, 2024

@C24-AK - Do you think you'd be able to add a plugin parameter to $user_resource of the mysql::db defined type, per my earlier comment, as part of this PR, or would you prefer to keep this separate?

Done, I appreciate if you could test the changes and see if everything works as expected

Thank you! I've looked at your changes and they look good. I'm attempting to test, but am facing a problem related to setting the mysql root password:

Error: can't modify frozen String: "ALTER USER 'root'@'localhost' IDENTIFIED WITH"
Error: /Stage[main]/Mysql::Server::Root_password/Mysql_user[root@localhost]/password_hash: change from [old password hash redacted] to [new password hash redacted] failed: can't modify frozen String: "ALTER USER 'root'@'localhost' IDENTIFIED WITH"

I'm not sure if this problem relates to the changes in this PR, but I tried 15.0.0 from Puppet Forge and didn't see the problem. The Puppet resource relevant to the error is:

class {'::mysql::server':
  package_name            => 'mysql-server',
  service_name            => 'mysql',
  remove_default_accounts => true,
  restart                 => true,
  service_enabled         => true,
  config_file             => '/etc/mysql/my.cnf',
  create_root_user        => true,
  create_root_my_cnf      => true,
  root_password           => $dlib_atom::mysql_root_password,
  override_options        => {
    mysqld => {
      default-authentication-plugin => $mysql_user_auth_plugin,
      sql_mode                      => 'ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION',
      optimizer_switch              => '\'block_nested_loop=off\'',
      disable_log_bin               => '',
      mysqlx                        => 'off',
      log_error_verbosity           => '1'
    }
  },
}

Apologies - I guess this is likely a problem limited to the specific scenario (an internal module) where I'm using the module! Hopefully I'll work it out.

Hey, I appreciate testing this PR!
This error is unfortunately regarding how I concat the strings. Still don't know how ruby works :D
I didn't know I reference a frozen string which I cannot add a new string to.
I will check out the ruby docs and see if I can fix this asap

@Jimadine
Copy link

Hey, I appreciate testing this PR! This error is unfortunately regarding how I concat the strings. Still don't know how ruby works :D I didn't know I reference a frozen string which I cannot add a new string to. I will check out the ruby docs and see if I can fix this asap

Thank you, your recent commit d2e2b29 fixed it! 😃

I guess it would mean yet another change to allow changing the authentication plugin for the root user? I think it would mean a new param in server.pp e.g. root_plugin and a change to root_password.pp - add new plugin attribute after line 35 e.g. plugin => $mysql::server::root_plugin,. What do you think?

@C24-AK
Copy link
Author

C24-AK commented Feb 28, 2024

Hey, I appreciate testing this PR! This error is unfortunately regarding how I concat the strings. Still don't know how ruby works :D I didn't know I reference a frozen string which I cannot add a new string to. I will check out the ruby docs and see if I can fix this asap

Thank you, your recent commit d2e2b29 fixed it! 😃

I guess it would mean yet another change to allow changing the authentication plugin for the root user? I think it would mean a new param in server.pp e.g. root_plugin and a change to root_password.pp - add new plugin attribute after line 35 e.g. plugin => $mysql::server::root_plugin,. What do you think?

Good to hear that it works :)
Regarding plugin for root user, I noticed too that root password is always the same password. I can add a new parameter to that and make it adjustable.

@Jimadine I appreciate if you could test the root plugin :)

@Jimadine
Copy link

@Jimadine I appreciate if you could test the root plugin :)

Thank you! I've tested it and it appears to be trying to change the plugin for root, but it's returning the following errors:

Error: Execution of '/usr/bin/mysql --database=mysql -e ALTER USER 'root'@'localhost' IDENTIFIED WITH 'caching_sha2_password' AS X'244124303035242816411A493F2969174D3D2D0825173421533D17662F4A6C687333456F484239596D4B43724D6A5375644964325673642E7074533052705436747943656B35'' returned 1: ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)
Error: /Stage[main]/Mysql::Server::Root_password/Mysql_user[root@localhost]/plugin: change from 'auth_socket' to 'caching_sha2_password' failed: Execution of '/usr/bin/mysql --database=mysql -e ALTER USER 'root'@'localhost' IDENTIFIED WITH 'caching_sha2_password' AS X'244124303035242816411A493F2969174D3D2D0825173421533D17662F4A6C687333456F484239596D4B43724D6A5375644964325673642E7074533052705436747943656B35'' returned 1: ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)

@C24-AK
Copy link
Author

C24-AK commented Feb 28, 2024

@Jimadine I appreciate if you could test the root plugin :)

Thank you! I've tested it and it appears to be trying to change the plugin for root, but it's returning the following errors:

Error: Execution of '/usr/bin/mysql --database=mysql -e ALTER USER 'root'@'localhost' IDENTIFIED WITH 'caching_sha2_password' AS X'244124303035242816411A493F2969174D3D2D0825173421533D17662F4A6C687333456F484239596D4B43724D6A5375644964325673642E7074533052705436747943656B35'' returned 1: ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)
Error: /Stage[main]/Mysql::Server::Root_password/Mysql_user[root@localhost]/plugin: change from 'auth_socket' to 'caching_sha2_password' failed: Execution of '/usr/bin/mysql --database=mysql -e ALTER USER 'root'@'localhost' IDENTIFIED WITH 'caching_sha2_password' AS X'244124303035242816411A493F2969174D3D2D0825173421533D17662F4A6C687333456F484239596D4B43724D6A5375644964325673642E7074533052705436747943656B35'' returned 1: ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)

I think I revert this commit since I cant actually test it right now, maybe we can implement this in another PR

@Jimadine
Copy link

I think I revert this commit since I cant actually test it right now, maybe we can implement this in another PR

Yeah, I agree, best to revert. FWIW, the same error was apparent when attempting to set root's plugin to mysql_native_password:

Error: Execution of '/usr/bin/mysql --database=mysql -e ALTER USER 'root'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '*B638EC5422004FCF44EE84FABA603D29A2259BC0'' returned 1: ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)
Error: /Stage[main]/Mysql::Server::Root_password/Mysql_user[root@localhost]/plugin: change from 'auth_socket' to 'mysql_native_password' failed: Execution of '/usr/bin/mysql --database=mysql -e ALTER USER 'root'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '*B638EC5422004FCF44EE84FABA603D29A2259BC0'' returned 1: ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)

@C24-AK
Copy link
Author

C24-AK commented Feb 29, 2024

Good morning @bastelfreak, is there anything else to do for this PR?

@C24-AK C24-AK requested a review from bastelfreak March 6, 2024 07:17
@C24-AK
Copy link
Author

C24-AK commented Mar 19, 2024

Any Reviewers available? @bastelfreak @alexjfisher

@bastelfreak
Copy link
Collaborator

@C24-AK thanks for the patch! Is there a chance for you to add a test for this change?

@nwton
Copy link

nwton commented Apr 2, 2024

@C24-AK, it's a good job, I am very glad that someone has started implementing this important thing, but I think we can do a little better.

We don't need to run mysql to convert a string to HEX

@password = mysql_caller("SELECT CONCAT('0x',HEX('#{@password}'))", 'regular').chomp

Instead of running an external binary, you can use ruby like this:

@password = '0x' + @password.each_byte.map { |b| b.to_s(16) }.join

Or even we can immediately get the HEX from the MYSQL something like this, and then decode it back if necessary (since mysql_native_password is promised to be completely disabled in future versions of mysql, then this may be the best option)

      if !mysqld_version.nil? && newer_than('mysql' => '5.7.6', 'percona' => '5.7.6')
        query = "SELECT MAX_USER_CONNECTIONS, MAX_CONNECTIONS, MAX_QUESTIONS, MAX_UPDATES, SSL_TYPE, SSL_CIPHER, X509_ISSUER, X509_SUBJECT, HEX(AUTHENTICATION_STRING), PLUGIN FROM mysql.user WHERE CONCAT(user, '@', host) = '#{name}'"
      ### a little bit code skipped
      end
      @max_user_connections, @max_connections_per_hour, @max_queries_per_hour, @max_updates_per_hour, ssl_type, ssl_cipher,
      x509_issuer, x509_subject, @password_hex, @plugin, @authentication_string = mysql_caller(query, 'regular').chomp.split(%r{\t})

      if @plugin == 'caching_sha2_password'
        # Convert from mysql HEX to normal hex
        @password = '0x' + @password_hex
      else
        # Convert from mysql HEX to string back
        @password = @password_hex.scan(/../).map { |x| x.hex.chr }.join
      end

@nwton
Copy link

nwton commented Apr 2, 2024

We can also replace this general regexp:

%r{0x[A-F0-9]+$}.match?(password)

with a more explicit one:

%r{0x24412430303524[A-F0-9]{63}$}.match?(password)

The magic sequence 24412430303524 is HEX for $A$005$, followed by 20 characters of SALT and 43 characters for the SHA DIGEST. It will remain in this format until some global changes are made to the MYSQL code.

@C24-AK
Copy link
Author

C24-AK commented Apr 15, 2024

Hello,
I added the suggestions from @nwton.
But I am not really familiar with acceptance tests.
I would highly appreciate someone who could do it, so we can merge this PR asap :)

@bastelfreak
Copy link
Collaborator

@C24-AK can you please rebase against main to get rid of the merge commit?

@C24-AK C24-AK force-pushed the feature/caching_sha2_password_hash branch 2 times, most recently from 4542ee7 to dcafc70 Compare April 15, 2024 07:27
@C24-AK C24-AK requested a review from bastelfreak April 26, 2024 08:07
@C24-AK C24-AK force-pushed the feature/caching_sha2_password_hash branch from dcafc70 to 9d1bb5f Compare May 27, 2024 06:18
@C24-AK
Copy link
Author

C24-AK commented May 27, 2024

Push

@bastelfreak
Copy link
Collaborator

@C24-AK any chance you can add a test for this?

@C24-AK
Copy link
Author

C24-AK commented May 27, 2024

@C24-AK any chance you can add a test for this?

As I mentioned above I am not familiar with acceptance tests and was looking forward for help here.
Otherwise I can read about it in the next days and try to figure out how to write these tests.

@CiderAndWhisky
Copy link

This has been going back and forth for 3/4 of a year - what is missing to get it merged? We use this branch on prod since December and need to always check to make sure not to overwrite it when we do module updates - would be nice to have it in the main branch. :-D

Thank you for the module!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants