Support authentication switch request and basic auth plugin. #1776

elemount · 2017-07-20T05:54:11Z

The new code supports mysql_native_password and mysql_old_password as potential authentication methods.
And for mysql_old_password, will still need {inscure:true}.
It fixed #1396 and also fixed authentication against Azure Database for MySQL: #1729 .

The implementation refer the pull request #1730 .

dougwilson

I'm traveling, but since I am constantly gettting emails from new commits on this PR, I figured I'd at least try to take a quick look on my phone. I only spotted one obvious error, and will take a closer look as soon as I can.

Great work on this!

dougwilson · 2017-07-20T17:55:45Z

lib/protocol/Auth.js

+      // mysql_old_password only need first 8 bytes scremble.
+      return this.scramble323(pluginData.slice(0, 8), password);
+    default:
+      throw 'Unknown auth plugin: ' + pluginName;


This should throw an error object, with a proper code and fatal flag so users can handle it like the other errors.

dougwilson · 2017-07-21T17:20:36Z

Awesome, thanks @elemount ! The last question I had here is that I believe you are the one who asked about sha256_password authentication plugin support. This pull request doesn't add that, but does this pull request implement the plugin system talked about so you'd be able to code sha256_password authentication plugin support in your code? If so, there's no documentation about how one would go about this in the pull request. If not, what is the plan to add sha256_password authentication plugin (or any custom plugin users may have setup in their MySQL)? Just curious because once this lands, if adding plugin support on top of this is hard, probably going to be very plainful to land when we already know user plugins is a use-case.

dougwilson · 2017-07-21T17:20:58Z

/cc @sidorares to take a look at this implementation as well for parity with mysql2.

elemount · 2017-07-22T13:56:12Z

@dougwilson , you raise a great problem for me.

In fact, I still want to add sha256_password_plugin. But with some thought, I think put the sha256_password_plugin in this pull request is not wise.

The main problem is the tests. In this pull request, all the code is both in UT and integration test. But for sha256_password_plugin, it hard to use integration tests. I have a local MySQL environment to test the sha256_password_pluign, to make other one test this plugin, I try to found a MySQL docker image with openssl build support, which can fully test the sha256_password_plugin, but I failed. Now I decide to build such docker image by myself, and can deliver to other people to test it easily. But it also needs some time.

Another one is the design. I see both Golang driver and Python driver PyMySQL has a pluggable auth plugin like that pull request, I think this pluggable auth plugin is a good design for that.

In my plan, I want to

Support basic client plugin auth attribute in this pull request.
Support pluggable auth plugin in other pull request including make auth plugin can both provide by users or by other contributors. To make add auth plugin easily, in my mind, contributors should only just modify one definition file and just add a new file. And user can provide a class with a name to add their personal auth plugin.
Add sha256_password_plugin by the pluggable auth plugin mechanism.

The pluggable auth plugin must be supported in future because I see MariaDB is planning to have another plugin https://mariadb.org/history-of-mysql-mariadb-authentication-protocols/ . And if user want to use Amazon RDS with IAM plugin, they can also just provide a class to the connection string.

But I do not want to merge the client plugin auth attribute and the pluggable auth plugin in one pull request. The main problem is complex, and I also concern that you may not accept my pluggable auth plugin design. The client plugin auth attribute is the first step, but also the basic step. Mark the attribute ON firstly and then in next pull request we have a pluggable auth plugin and then sha256_password_plugin and other auth plugins, step by step.

Give me your comments on my plan, or what's your plan on it?

sidorares · 2017-07-23T02:37:00Z

@elemount @dougwilson I am going to change auth plugin api to make it easier to configure especially if you need to handle multiple plugins - sidorares/node-mysql2#560 (comment)

Current api documented here: https://github.com/sidorares/node-mysql2/blob/master/documentation/Authentication-Switch.md

const conn = mysql.createClient({
  authPlugins: {
    some_custom_plugin: (data, cb) => { /* ... * /},
    // mysql_native_plugin and sha256_password_plugin automaticlly merged to here unless you do something like
    // mysql_native_plugin: null
    // 'mysql_clear_password' not enabled by default:
    mysql_clear_password: mysql.authPlugins.mysql_clear_password
  },
  connectAuthPluginName: 'mysql_native_password' // this would be default, but if you need to start with sha256_password_plugin for example you need to put this option
})

would be good to add AD and PAM plugins to examples but no need to bundle them with driver

elemount · 2017-07-27T01:44:21Z

@sidorares I think this design is great.
And @dougwilson , what's your options on the pluggable auth plugin design?
Do you think that I should implement this pluggable auth plugin in this pull request?

sidorares · 2017-07-27T02:13:24Z

@elemount note that in theory there might me multiple data roundtrips between server and auth plugins, not just single helloFromServer(plugin name, hello data) => helloFromClient( client hello response data ) . Maybe there should be extra parameter? (data, cb, sequenceId) => {} ? (though this can be solved using closure counter )

elemount · 2017-07-27T03:01:08Z

@sidorares , In fact, I do not think ({pluginName, pluginData, sequenceId}, cb) is better than ({pluginName, pluginData}, cb).
For example, in sha256_password_plugin, this plugin must know which phase it locates, and the phase is not only depends on the sequenceId, it depends on the previous state of the auth plugin (for example, received the public_key can be on sequenceId 1 or 2).
So I think sequenceId can simplify some auth plugin, but it is not a common solution.

In my mind, if we describe the connection as a state machine, the pluggable auth plugin is a node which we impose to contributors and users to implement. If the auth plugin is just a node(a function), for complicated auth plugin, it must be a node of stack based state machine, which means it can pop its last push state and push its newest state. SequenceId is means that the when function is called, then it pop a id, and after it finished, it push the next id, not flexible enough, and also not usable enough.

I advise you can describe how to implement complicated auth plugin in the document sample, instead of the uncompleted solution sequenceId for just simplify a few plugins.

sidorares · 2017-07-27T03:52:45Z

 const createPlugin = () => {
  let state = 'start';
  return (data, cb) => {
    if (state === 'start') {
       state = 'start2';
       return cb(null, 'foo');
    }
    if (state === 'start2') {
       return cb(null, 'bar');
    }
  });

  const conn = mysql.createClient({
    authPlugins: {
      foo_bar_plugin = createPlugin()
    }
  })

elemount · 2017-07-27T06:08:08Z

Or what about that, @sidorares ?

function ({pluginName, pluginData, extraPluginInfo}, cb) {
    if (pluginName === 'ssh-key-auth') {
      getPrivateKey(function (key) {
        var response = encrypt(key, pluginData);
        if (extraPluginInfo == 'phase1') {
          return cb(null, response, 'phase2' /*this is the new extraPluginInfo*/);
        } else {
          // continue handshake by sending response data
          // respond with error to propagate error to connect/changeUser handlers
          cb(null, response, 'end')
        };
      });
    } else {
      const err = new Error(`Unknown AuthSwitchRequest plugin name ${pluginName}`);
      err.fatal = true;
      cb(err);
    }

elemount · 2017-08-03T08:28:14Z

@sidorares Could you help to review the code first?

sidorares · 2017-08-03T13:44:16Z

looks good to me. Re parity with mysql2 - this pr currently does not add user-supplied pluggable auth and only implements standard mysql_native_password and mysql_old_password plugins, so no new api at the moment

sidorares · 2017-08-03T13:45:07Z

@elemount can you rebase pr please?

elemount · 2017-08-03T14:24:38Z

@sidorares I've rebased it.

dougwilson · 2017-08-03T14:32:30Z

@elemount does this PR take into account all the comments to PR #1730 you linked to? For example, one thing we talked about was adding an integration test so we can know if at some point the string[EOF] vs string[NUL] bug is fixed in MySQL, but I'm not sure I see that in this PR, but maybe I'm missing it? Can you point me to it?

dougwilson · 2017-08-03T14:34:21Z

lib/protocol/packets/ComChangeUserPacket.js

 }

 ComChangeUserPacket.prototype.parse = function(parser) {
  this.command       = parser.parseUnsignedNumber(1);
  this.user          = parser.parseNullTerminatedString();
  this.scrambleBuff  = parser.parseLengthCodedBuffer();
  this.database      = parser.parseNullTerminatedString();
-  this.charsetNumber = parser.parseUnsignedNumber(1);
+  this.charsetNumber = parser.parseUnsignedNumber(2);
+  if (!parser.reachedPacketEnd()) {


Shouldn't this key off the fact that plugin auth is used, not if packet end was reached? For example, this will not work with CLIENT_CONNECT_ATTRS because if client auth is off but CLIENT_CONNECT_ATTRS is on, then the packet end is indeed not reached, but the rest of the packet is not related to client plugin auth.

Docs http://web.archive.org/web/20160604101506/http://dev.mysql.com/doc/internals/en/com-change-user.html

Sure. Let me change the parse.

dougwilson · 2017-08-03T14:44:36Z

And forgive me if you answered my question, but even re-reading your responses I'm still not sure: what is the plan to add sha256_password authentication plugin (or any custom plugin users may have setup in their MySQL)? Just curious because once this lands, if adding plugin support on top of this is results in a breaking change, it may end up never landing...

dougwilson · 2017-08-03T14:55:20Z

Also, forgot to note: please add some information in the README about this, for example the list of plugins supported. Also update the https://github.com/mysqljs/mysql#connection-flags section with the changes you made to the default flags.

elemount · 2017-08-03T14:58:00Z

@dougwilson , I use another way to avoid the string[EOF] and string[NUL] bug problem.

In protocol level, the AuthSwitch packet contains 'auth_plugin_name\x00auth_plugin_data', 'auth_plugin_name\x00' is a string[NUL] and the 'auth_plugin_data' is string[EOF].

In real world level, the 'auth_plugin_data' which 'mysql_native_password' is 20bit scramble and another '\x00' which is 21bit, that is a string[NUL], the 'mysql_old_password' is 8bit scramble and another '\x00' is 9bit, that is not match the protocol.

But fortunately we do not need to handle this mismatch. How? Also by the protocol.
I follow the protocol 'auth_plugin_name\x00' is a string[NUL] and the 'auth_plugin_data' is string[EOF]. The '\x00' ends the auth_plugin_data, but we do not need to care about it.
MySQL describe the what mysql_native_password and mysql_old_password need to accept.
In the page Old Password Authentication and Native Password Authentication, mysql_old_password needs first 8 byte of auth_plugin_data and mysql_native_password needs first 20 byte of auth_plugin_data. We do not need care the 9th bit and the 21th bit, we do not need care its value, even it is '\x00'.

Thus I avoid the real world mismatch by following the MySQL client protocol.

dougwilson · 2017-08-03T15:02:23Z

Right, I'm fine with that slice, but what I'm talking about is the underlying packet read. Because the protocol and the MySQL server implementation disagree, this is an important aspect to make sure we have an actual integration test, and not simply a unit test. This is important when we test against other implementations like MarisDB, Azure, and any others that popup (like various MySQL proxy software) to be aware when we run aground.

elemount · 2017-08-03T15:12:53Z

@dougwilson , I will create PR on pluggable auth plugin and sha256_password auth support in this month, I have enough time to finished it this month and I'll do it, at least have a pull request with ut and integration test.

And also I can make sure it will not have a breaking change which introduces the pluggable auth plugin.

elemount · 2017-08-03T15:25:19Z

@dougwilson , I do not see any underlying packet read problem.
In my mind, which may be wrong. The protocol is string[EOF], so we just readPacketToEnd, readPacketToEnd is the only correct one because it follows the protocol. And we slice to get the scramble, then we do auth. No packet read problem, the protocol describe a string[EOF], then why we need to care about the content of string[EOF], do we need to know it there any \x00 in the string? The scramble can be \0\0....\0, but it is not a series of string[NUL]s, it just a string[EOF], we follow it and any other server also aspect it. Then can add \0 in the end of the string[EOF], but not breaking anyone.

Additional, the protocol will not be string[NUL], because scramble is not string, it is random bits, it may contains \0 during the bits.
And other server will send 21 bits in fact, otherwise it will break all existing mysql-connector-c/libmariadbclient lib.

What's your insight?

elemount · 2017-08-07T05:08:11Z

Hi @dougwilson , @sidorares , I resolved all the comments, please help to review it again.

twocode · 2017-08-07T08:23:21Z

Great to this pr. I created an issue on this a while ago and have been using mysql2 ever since thanks to @sidorares supported it. I am looking forward to see this pr gets merged. Great job, @elemount !

dougwilson

P.S. I love the integration test you added 👍

dougwilson · 2017-08-07T17:41:40Z

lib/protocol/packets/ComChangeUserPacket.js

 }

 ComChangeUserPacket.prototype.parse = function(parser) {
  this.command       = parser.parseUnsignedNumber(1);
  this.user          = parser.parseNullTerminatedString();
  this.scrambleBuff  = parser.parseLengthCodedBuffer();
  this.database      = parser.parseNullTerminatedString();
-  this.charsetNumber = parser.parseUnsignedNumber(1);
+  if (!parser.reachedPacketEnd()) {


Why this check here? Shouldn't the charsetNumber just be outside the if like it used to be, or is there a reason behind this change?

Hi @dougwilson , this is for the document COM_CHANGE_USER packet, it include the charset in the if more data condition.

Oh, sorry :) I was thinking the handshake. Nevermind on this, then :)

I'm writing a test to exercise this case, there the charsetNumber may not get populated.

dougwilson · 2017-08-07T17:42:44Z

lib/protocol/sequences/Handshake.js

+
+function HandshakeInsecureAuthError() {
+  var err   = new Error(
+    'MySQL server is requesting the old and insecure pre-4.1 auth mechanism.' +


Looks like the space that was at the end of this sentence got removed.

Sure. Fixed it.

dougwilson

It won't let me comment on the line, but PLUGIN_AUTH is still listed in the README under the doesn't work section.

elemount · 2017-08-10T01:46:51Z

@dougwilson , I've fixed the README issue and rebased the code. Could you merge this code after check?

dougwilson · 2017-08-10T01:48:15Z

I am working through your code locally for the past half an hour or so now, with the intention of merging. I saw the README issue which is why I pinged you on it, though I see you squashed all the commits, so will need to fix up my local copy here as well.

dougwilson · 2017-08-10T02:35:20Z

lib/protocol/sequences/ChangeUser.js

+    return Packets.ErrorPacket;
+  }
+
+  if (firstByte === 0xfe) {


I am fixing up this edge case where the server can actually respond here with the old password packet, of 0xfe doesn't always mean auth switch as implemented here, so just gotta add the same logic from the handshake.

dougwilson · 2017-08-10T02:36:10Z

I see you pushed again. I'm going to comment on the places where I am making changes to help show that I'm busy working on this. If you're making changes too, let me know so I can stop for now while you make changes...

dougwilson · 2017-08-10T02:37:03Z

lib/protocol/packets/AuthSwitchPacket.js

+module.exports = AuthSwitchPacket;
+function AuthSwitchPacket(options) {
+  options = options || {};
+  this.scrambleBuff = options.scrambleBuff;


Since this is greenfield, I'm just changing these props to have the same name as in the protocol docs for sanity.

dougwilson · 2017-08-10T02:41:21Z

lib/protocol/packets/HandshakeInitializationPacket.js

  }
-  writer.writeNullTerminatedBuffer(this.scrambleBuff2);


This was a miss in the first, and since it's fixed in here, I'm trying to write a test case for this bug.

dougwilson · 2017-08-10T03:06:54Z

lib/Pool.js

    connection.config = this.config.newConnectionConfig();
+    connection.config.clientPluginAuth = previousClientPluginAuth;


I need to rework how this is tracked so it stops changing the hidden class of the config object.

dougwilson · 2017-08-10T04:05:38Z

Hey @elemount it is getting very late in my time zone and I'm likely to head out soon. I pushed up the PR #1730 code to master and I had this PR rebased on top of it already, but haven't yet rebased again the latest squash you pushed up. I need to do that still with the changes I noted above. Running the tests on this project just really eats up a lot of time :S

elemount · 2017-08-30T03:15:57Z

@dougwilson , I add a commit on mysql_clear_password support. But faced merged issue. What should I do, merge the current code, or just left it?

And the I'm working on the pluggable auth plugin, how should I give the pull request, based on this pull request or have an another which its code based on it? Or I should fork on another branch?

dougwilson · 2018-07-05T01:53:58Z

@dougwilson , I add a commit on mysql_clear_password support. But faced merged issue. What should I do, merge the current code, or just left it?

Wow, not sure how I missed this, but was just going through open PRs / issues tonight to see where they are at. The best would have been to rebase the PR. I'm working on doing that now, because I don't see anything wrong with this PR, just needs to be rebased now.

And the I'm working on the pluggable auth plugin, how should I give the pull request, based on this pull request or have an another which its code based on it? Or I should fork on another branch?

So it would depending I guess on what is needed. Ideally it would be a separate PR either way. If it needs what is in this PR, then yea, just branch from this PR and submit a PR. Yes, at first that RP will include all changes since they are not in our master, but after this gets merged, it would only have the diff of the two. Adding it to this PR would just stretch out landing this PR, which is the downside to mixing.

rhummelmose · 2018-09-27T10:17:47Z

So is this actually fixed in the newest version?

closes #1776

dougwilson requested changes Jul 20, 2017

View reviewed changes

cohowap mentioned this pull request Jul 24, 2017

Support for Azure Database for MySQL (New PaaS in Azure) sequelize/sequelize#7633

Closed

novemberborn mentioned this pull request Jul 28, 2017

Recognize fatal mysql2 connection errors knex/knex#2175

Closed

dougwilson requested changes Aug 3, 2017

View reviewed changes

dougwilson requested changes Aug 7, 2017

View reviewed changes

dougwilson requested changes Aug 10, 2017

View reviewed changes

Support authentication switch request.

2254018

dougwilson reviewed Aug 10, 2017

View reviewed changes

dougwilson added the feature label Aug 10, 2017

dougwilson self-assigned this Aug 10, 2017

elemount mentioned this pull request Aug 10, 2017

Error: ER_NOT_SUPPORTED_AUTH_MODE with auth_socket #1507

Open

prashanthmadi mentioned this pull request Aug 22, 2017

azure mysql database authentication is not working prashanthmadi/azure-ghost#1

Closed

elemount mentioned this pull request Aug 28, 2017

Problems connecting to AWS RDS Instance using new IAM Authentication Feature #1715

Closed

Support mysql_clear_password auth plugin

a337109

dougwilson force-pushed the master branch 2 times, most recently from 946727b to 37fbbdd Compare November 18, 2018 02:26

dougwilson closed this in c144dda Apr 17, 2019

dougwilson pushed a commit that referenced this pull request Apr 17, 2019

Support auth switch in change user flow

d916479

closes #1776

		connection.config = this.config.newConnectionConfig();
		connection.config.clientPluginAuth = previousClientPluginAuth;

Support authentication switch request and basic auth plugin. #1776

Support authentication switch request and basic auth plugin. #1776

Conversation

elemount commented Jul 20, 2017

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Jul 21, 2017

dougwilson commented Jul 21, 2017

elemount commented Jul 22, 2017

sidorares commented Jul 23, 2017 • edited Loading

elemount commented Jul 27, 2017

sidorares commented Jul 27, 2017

elemount commented Jul 27, 2017

sidorares commented Jul 27, 2017

elemount commented Jul 27, 2017

elemount commented Aug 3, 2017

sidorares commented Aug 3, 2017

sidorares commented Aug 3, 2017

elemount commented Aug 3, 2017

dougwilson commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Aug 3, 2017

dougwilson commented Aug 3, 2017

elemount commented Aug 3, 2017

dougwilson commented Aug 3, 2017

elemount commented Aug 3, 2017

elemount commented Aug 3, 2017 • edited Loading

elemount commented Aug 7, 2017

twocode commented Aug 7, 2017

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson left a comment

Choose a reason for hiding this comment

elemount commented Aug 10, 2017

dougwilson commented Aug 10, 2017

Choose a reason for hiding this comment

dougwilson commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Aug 10, 2017

elemount commented Aug 30, 2017

dougwilson commented Jul 5, 2018

rhummelmose commented Sep 27, 2018

sidorares commented Jul 23, 2017 •

edited

Loading

elemount commented Aug 3, 2017 •

edited

Loading