Add mysql_clear_password auth plugin

[rathboma]

This seems like it comes up fairly frequently here (and has come up on my repo too, so figured I'd add this as a core supported plugin.

Not sure if you want to do this or not, but figured I'd open a PR just to see. Seems like a simple fix.

Let me know!

[sidorares]

yeah, this was discussed few times. I'm keen to have it included, thanks for working on this @rathboma ! Not sure if I want to have mysql_clear_password accepted by default or require explicit configuration ( and have a good error message explaining how to enable it ), something like this:

const { createConnection, AuthPlugins } = require('mysql2');
  createConnection({
     user: 'user',
     password: 'password'
    authPlugins: {
        mysql_clear_password: AuthPlugins.mysql_clear_password
    }
  });

also would be good to add an integration test ( I guess that would require creating a new mysql_clear_password user from a test )

[rathboma]

Yeah honestly I had no idea how to test it! Ok I'll take the approach you suggest and see if I can figure testing out.

[sidorares]

what's your opinion on risk vs convenience of having mysql_clear_password configured by default @rathboma ? In some scenarios it would be good to warn/error about potentially insecure connection but in some other scenarios it might be very intentional and secure ( connection over ssh tunnel to a legacy server for example ). What I potentially want to prevent is users thinking that connection is secure while sever accidentally configured to offer mysql_clear_password as first auth method

[rathboma]

I'm struggling with the same question in my app to be honest.

It's not enabled by default in the c library, so my gut is that it should throw an error but say that it can be enabled by setting a flag to true.

Then also put it in the docs so it appears for google searchers.

That seems like the most sensible to me?

I think that's the approach I'm going to take for Beekeeper.

[sidorares]

I'm leaning towards the same. I'm not sure how common this auth type is in the wild and how inconvenient would it be to require some actions from a user to enable but I feel it's safer approach ( and more future proof - we can make it default in the future without breaking anything but not the other way around )

[sidorares]

note that mysql_clear_password is also used with AWS.RDS.signer - see #1116

[sidorares]

also slightly different topic but I believe pre-mysql4.2 auth is currently not supported by this module, not sure if anyone requested this from you @rathboma - #30

[rathboma]

This is 'mysql_old_password' looks like?

Probably lack of support is turning some folks away. So thanks for the heads up!

[sidorares]

This is 'mysql_old_password' looks like?

yes, and potentially if we want to implement it probably should be part of normal handshake, not auth_switch/plugins as likely is used with legacy servers that might not support plugin auth. I see it as relatively low priority but would be good to add eventually

[sidorares]

as a safety check we could add here if (!connection.config.insecureAuth) { throw new Error(...)}

not sure if its worth having two different flag names - insecureAuth for mysql_old_password and something else for mysql_clear_password or one is enough

see

node-mysql2/lib/connection_config.js

Line 90 in 21ad545

this.insecureAuth = options.insecureAuth || false;

[MasterOdin]

I would not expect that the insecureAuth be tied to anything but the pre-4.2 authentication. It is an inverse of the secure_auth system variable for configuring the MySQL server to allow using mysql_old_password on a MySQL 5.6 or earlier installation.

[BrysonRafael]

@rathboma just following up on this PR. Is this good to go?

[silverbullettruck2001]

@rathboma @sidorares Can this PR be merged yet?

[silverbullettruck2001]

@sidorares Just following up on this. Is there something that needs to be done to get this merged?

[sidorares]

@silverbullettruck2001 the change itself looks good for me to be added however really would like to see an integration test. The script would need to use standard root credentials to create another clear_password user and then make a second connection as that user

[silverbullettruck2001]

@rathboma are you able to add those integration tests?

[tkchk]

I'm also on this. Can this PR be merged?

[tkchk]

I did tests - changes here doesn't work. The password sould be returned as a function, not the password itself.

diff --git a/lib/auth_plugins/mysql_clear_password.js b/lib/auth_plugins/mysql_clear_password.js
index 0a13828..31ad40e 100644
--- a/lib/auth_plugins/mysql_clear_password.js
+++ b/lib/auth_plugins/mysql_clear_password.js
@@ -4,5 +4,5 @@ module.exports = pluginOptions => ({ connection, command }) => {
   const password =
     command.password || pluginOptions.password || connection.config.password;

-  return Buffer.from(`${password}\0`)
-};
\ No newline at end of file
+  return () => Buffer.from(`${password}\0`)
+};

[silverbullettruck2001]

@sidorares How do you want to proceed with this? @rathboma doesn't seem to be responding this and we cannot update this PR to reflect the necessary changes. I can create a new branch with the changes and then create a new PR if that's what is needed to complete this? @tkchk could you provide the tests that you did to verify that this wasn't working so I can include them in my PR?

[rathboma]

Hey! Sorry I've been super slammed lately.

I want to do this but not sure I have time in the next few weeks, if someone wants to take over the fork to push it through, please do.

Matthew

[silverbullettruck2001]

@rathboma do you want to give me access to the fork to get the changes made? I don't believe I have permissions on your fork to commit any changes.

[sidorares]

@silverbullettruck2001 if you want to continue you don't need @rathboma permission - just pull his branch:

git remote add beekeeper [email protected]:beekeeper-studio/node-mysql2.git
git fetch beekeeper mysql_clear_password
git co mysql_clear_password
( ... add your changes / commits )
git push origin mysql_clear_password // push mysql_clear_password branch with your extra changes to your fork

and just create another PR and reference this one. The initial commit history will be preserved and will show @rathboma as an author

[silverbullettruck2001]

@sidorares thanks for the suggestion! I went ahead and followed your suggestion and created a new branch and have a PR created for this:
#1552

I don't have any tests included as I was hoping to get the tests from @tkchk as I am not familiar with building tests. If I don't hear back from him I may try taking a stab at it though to get this across the finish line.

[tkchk]

@sidorares How do you want to proceed with this? @rathboma doesn't seem to be responding this and we cannot update this PR to reflect the necessary changes. I can create a new branch with the changes and then create a new PR if that's what is needed to complete this? @tkchk could you provide the tests that you did to verify that this wasn't working so I can include them in my PR?

Sure. This is what I get with this PR unchanged.
But, when I apply my patch, it works and I see that connection is OK.

ConnectionError [SequelizeConnectionError]: connection._authPlugin is not a function
    at ConnectionManager.connect (/Users/aleksandr/Desktop/test-db-connection/node_modules/sequelize/lib/dialects/mysql/connection-manager.js:102:17)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async ConnectionManager._connect (/Users/aleksandr/Desktop/test-db-connection/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:216:24)
    at async /Users/aleksandr/Desktop/test-db-connection/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:174:32
    at async ConnectionManager.getConnection (/Users/aleksandr/Desktop/test-db-connection/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:197:7)
    at async /Users/aleksandr/Desktop/test-db-connection/node_modules/sequelize/lib/sequelize.js:303:26
    at async Sequelize.authenticate (/Users/aleksandr/Desktop/test-db-connection/node_modules/sequelize/lib/sequelize.js:414:5)
    at async /Users/aleksandr/Desktop/test-db-connection/index.js:20:5 {
  parent: TypeError: connection._authPlugin is not a function
      at Object.authSwitchRequest (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/commands/auth_switch.js:67:30)
      at ClientHandshake.handshakeResult (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/commands/client_handshake.js:155:22)
      at ClientHandshake.execute (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/commands/command.js:45:22)
      at Connection.handlePacket (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/connection.js:456:32)
      at PacketParser.onPacket (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/connection.js:85:12)
      at PacketParser.executeStart (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/packet_parser.js:75:16)
      at Socket.<anonymous> (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/connection.js:92:25)
      at Socket.emit (events.js:400:28)
      at addChunk (internal/streams/readable.js:293:12)
      at readableAddChunk (internal/streams/readable.js:267:9) {
    code: 'AUTH_SWITCH_PLUGIN_ERROR',
    fatal: true
  },
  original: TypeError: connection._authPlugin is not a function
      at Object.authSwitchRequest (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/commands/auth_switch.js:67:30)
      at ClientHandshake.handshakeResult (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/commands/client_handshake.js:155:22)
      at ClientHandshake.execute (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/commands/command.js:45:22)
      at Connection.handlePacket (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/connection.js:456:32)
      at PacketParser.onPacket (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/connection.js:85:12)
      at PacketParser.executeStart (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/packet_parser.js:75:16)
      at Socket.<anonymous> (/Users/aleksandr/Desktop/test-db-connection/node_modules/mysql2/lib/connection.js:92:25)
      at Socket.emit (events.js:400:28)
      at addChunk (internal/streams/readable.js:293:12)
      at readableAddChunk (internal/streams/readable.js:267:9) {
    code: 'AUTH_SWITCH_PLUGIN_ERROR',
    fatal: true
  }
}```

[tkchk]

@silverbullettruck2001 this is my test script

const { Sequelize } = require('sequelize');

const dbConfig = {
  database: 'dbname',
  host: 'dbhost',
  port: '3306',
  username: 'dbuser',
  password: 'dbpass',
};

const sequelize = new Sequelize(
  dbConfig.database,
  dbConfig.username,
  dbConfig.password,
  { host: dbConfig.host, port: dbConfig.port, dialect: 'mysql'},
);

(async () => {
  try {
    await sequelize.authenticate();
    console.log('Connection has been established successfully.');
  } catch (error) {
    console.error('Unable to connect to the database:');
    throw error;
  }

  const data = await sequelize
    .query(
      'select * from table limit 1',
      { type: sequelize.QueryTypes.SELECT },
    ) || [];

  return data;
})()
  .then((data) => {
    console.log('Success', data);
  })
  .catch((error) => {
    console.log('Error', error.message);
    console.error(error);
  });

[tkchk]

any news?

[summer-wu]

@tkchk I found the same bug, and submitted a PR #1644

[sidorares]

@tkchk I merged @summer-wu fix, can you try master again?