puppetlabs / puppetlabs-mysql

MySQL Puppet Module / Manifests + Types & Providers
Apache License 2.0
380 stars 790 forks source link

Add ability to use hex hash with caching_sha2_password plugin #1612

Open C24-AK opened 8 months ago

C24-AK commented 8 months ago

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 commented 8 months ago

CLA assistant check
All committers have signed the CLA.

Jimadine commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@bastelfreak done! Looking forward for the review

Jimadine commented 6 months ago

@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 commented 6 months ago

@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

Jimadine commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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! :smiley:

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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

C24-AK commented 5 months ago

Any Reviewers available? @bastelfreak @alexjfisher

bastelfreak commented 5 months ago

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

nwton commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

C24-AK commented 3 months ago

Push

bastelfreak commented 3 months ago

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

C24-AK commented 3 months ago

@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.