Support authentication switch request. Fixes #1396

[bgrainger]

The new code supports mysql_native_password and mysql_old_password as potential authentication methods (but still requires insecureAuth: true for the latter).

This fixes authentication against Azure Database for MySQL: #1729

• Add test for authentication method switch that "switches" to mysql_native_password
• Maintain backwards compatibility
• Test whether MySQL server sends a string[NUL] value for auth_method_data although it's documented as string[EOF].

[dougwilson]

This is great! I'm going to head off, but just wanted to give some feedback in case you have time before I'm on again. I really just see two missing items in the PR:

(1) No server should ever even be sending back a auth switch packet to a client that is not setting the CLIENT_PLUGIN_AUTH flag in the handshake packet, and this module is not setting that flag. It sucks that servers just ignore the flag and send it anyway, but now since this implements handing it (even through just one plugin), we should set that flag in our handshake packet now.

(2) There really needs to be at least one test for the new functionality added, otherwise it can accidentally be broken. I would expect some test that does exactly what the Azure server was doing: sends back an auth switch to use mysql_native_password plugin.

(2a) The compatibility note in your mysql_native_password w.r.t the string[EOF] trick really needs a test for that going against the real MySQL servers so we can make sure that quick is correct and that if it is ever "fixed" in a new MySQL version, we know that the code needs to be changed.

And finally, it is really unfortunate that this pull request is not backwards-compatible, as this would in the current form require a major version bump in order to publish, which would mean there is no timeline to publish right now, since there are several refactorings for 3.0 happening. Can it be implemented in a back-compat way at all? The breaking part is the removal of the UseOldPasswordPacket from the source and from the old password flow. Since users can listen into the packet flow, it's possible they can be interacting with this packet. If the old password part of the auth switch can just continue to flow through with that packet, that should be good enough.

Great job 👍

[bgrainger]

Thanks for the quick feedback; it is super helpful!

1. Good point, I should have realised that.

2. I should have made a note about the absence of tests when I opened the PR. Once I got to the "works on my machine" stage, I wanted to open the PR a) for exactly this kind of feedback and b) to let people know a fix was on the way and avoid duplicating effort. Understanding the test framework is just taking a bit more time but I think I should have a basic test up tonight.

2a. Agreed; I'll need to check the docs to see if I can force this to happen in an integration test.

RE: backwards compatibility, I think I'm following what you're saying here, but I may need to do some more reading on hooking into the packet flow to get the right fix.

[bgrainger]

Added one new test (in test/unit/connection/test-force-auth-switch.js); feedback welcome. Other PR review items are not yet addressed.

[bgrainger]

I reinstated UseOldPasswordPacket in a way I think should be backwards compatible (though I'm not sure how to add a test for that, but the existing tests run without changes).

[sidorares]

@bgrainger if you decide to continue working on this I would like to hear feedback on authSwitch request api currently used in mysql2 so it's compatible with mysqljs/mysql (I'm open to change api on my side if required):
https://github.com/sidorares/node-mysql2/blob/master/documentation/Authentication-Switch.md sidorares/node-mysql2#560

[bgrainger]

@sidorares

Disclaimer: although I've worked with the MySQL protocol, I have never worked with pluggable auth (except for handling the base case of mysql_old_password and mysql_native_password). So any concerns I raise are purely theoretical.

What should authSwitchHandler do if it doesn't recognise data.pluginName? If it doesn't call cb then would logging in hang if no handler can process the server's auth plugin? Or should it call cb with an error for any unsupported plugin? (The example code doesn't demonstrate this.)

The signature for authSwitchHandler looks like it should be able to be supported by an extension to my new code (instead of issuing an error, call authSwitchHandler if it's provided); that is, I don't think any API changes are needed to be supported by this code.

[bgrainger]

we should set [CLIENT_PLUGIN_AUTH] in our handshake packet now

Would you accept this PR simply as a bugfix for buggy servers (Azure and apparently some MySQL proxies), without the full CLIENT_PLUGIN_AUTH implementation? I have some working code for the latter here but I'm concerned that it's riskier than this bug fix needs to be, due to setting a new client flag. (All the integration tests pass but it's really hard to say that this won't break some client in the wild. Additionally, I introduced a lot of code duplication in ChangeUser.prototype['UseOldPasswordPacket'] and I'm not sure where that code should live; suggestions?)

The compatibility note in your mysql_native_password w.r.t the string[EOF] trick really needs a test for that going against the real MySQL servers so we can make sure that quick is correct and that if it is ever "fixed" in a new MySQL version, we know that the code needs to be changed.

Once CLIENT_PLUGIN_AUTH is set (in the commit I linked above), a ComChangeUserPacket request will be answered with an UseOldPasswordPacket (now really misnamed) containing the new challenge. This code transforms a NULL-terminated 21-byte buffer into the 20-byte buffer (specified by the documentation) that's needed for the password hashing. Did you have a specific idea in mind of an integration test you'd like to see, or is this sufficient?

[sidorares]

What should authSwitchHandler do if it doesn't recognise data.pluginName? If it doesn't call cb then would logging in hang if no handler can process the server's auth plugin? Or should it call cb with an error for any unsupported plugin? (The example code doesn't demonstrate this.)

Currently it's full auth switch handler responsibility - similar to for example node http request handler - if you don't do res.end('something') browser just waits until timeout. Similarly if you don't call callback with results mysql server kills connection after timeout. There is built in mysql_native_password switch handler that is used automatically and you are expected to handle every possible other method in your custom handler ( or just close connection with "don't know how to deal with this plugin" - yes, need to document that scenario)

[bgrainger]

@dougwilson Have you had a chance to review the updated code, or comment on the questions I asked earlier?

[dougwilson]

I have not yet. I have been working to fix a security vulnerability elsewhere, so this has fallen to my backburner.

[MWaser]

Do we have an ETA for merging/releasing this fix? It's pretty critical that Node.JS can't connect to MySQL on Azure (the second largest cloud provider) . . . .

[christophmayrhofer]

+1 for merge

[dougwilson]

Would you accept this PR simply as a bugfix for buggy servers (Azure and apparently some MySQL proxies), without the full CLIENT_PLUGIN_AUTH implementation?

To me, the bug is in those servers not here. The bug here is that this module incorrectly is stating that it is trying to use the old auth, so if there is a partial big fix, correcting that to say auth switch is not support would be good. Otherwise this should be an implementation to support auth switch if the purpose I to support servers needed auth switch.

[dougwilson]

This code transforms a NULL-terminated 21-byte buffer into the 20-byte buffer (specified by the documentation) that's needed for the password hashing. Did you have a specific idea in mind of an integration test you'd like to see, or is this sufficient?

I mean, the tests should be able to tell if we removed that slice for example, and it should be done against a real MySQL server (we test against multiple versions in Travis CI), not against a mock. It feels really weird that you are performing a slice there, especially with a magic number. Doesn't the parser already have a method for reading a nul terminated buffer? Why does this code need to slice it?

[bgrainger]

To me, the bug is in those servers not here.

I completely agree. But I would like to make this library usable by developers who have to code against those buggy servers.

The bug here is that this module incorrectly is stating that it is trying to use the old auth, so if there is a partial big fix, correcting that to say auth switch is not support would be good.

This would be a better error message, but still doesn't accomplish the ultimate goal of allowing mysqljs to be used with Azure Database for MySQL.

Otherwise this should be an implementation to support auth switch if the purpose I to support servers needed auth switch.

I may be misunderstanding what you're saying here, but that's exactly what this PR is (in my mind): the minimum implementation of auth switch necessary to support servers that need auth switch. Hence my question "Would you accept this PR simply as a bugfix for buggy servers?"

If the answer is "no", and you really want the full CLIENT_PLUGIN_AUTH implementation, I can clean up the other commit I mentioned; I just think it's a much higher-risk change, as it introduces a client behavior change (setting that flag) for all servers.

[bgrainger]

It feels really weird that you are performing a slice there, especially with a magic number. Doesn't the parser already have a method for reading a nul terminated buffer? Why does this code need to slice it?

AuthSwitchRequest is documented as having "auth_method_data (string.EOF)" but MySQL Server 5.7 actually sends string[NUL].

I could change the code to match the observed behaviour (and just read a NUL-terminated string) but I was concerned that this could break interoperability with other servers that follow the protocol documentation (possibly MariaDB for example).

In this specific case, the mysql_native_password authentication method is documented as hashing 20 bytes so it seemed OK to perform the slice. (The code doesn't work if all 21 bytes, including the NUL byte, are hashed.)

A similar slice is already performed in HandshakeInitializationPacket.js here, apparently for compatibility across multiple servers that follow the documented protocol slightly differently.

I mean, the tests should be able to tell if we removed that slice for example, and it should be done against a real MySQL server (we test against multiple versions in Travis CI), not against a mock.

I think this would be possible to test by setting CLIENT_PLUGIN_AUTH in the handshake response and supplying an invalid plugin name, so the server is forced to request an auth switch.

[MWaser]

So . . . . I now have a large project that is seriously considering abandoning MySQL due to its incompatibility with Azure (and the MySQL team's apparent lack of concern and unwillingness to work with a developer attempting to solve the problem). Is this what the MySQL team wants? If not, what is the ETA for an Azure compatible version?

[bgrainger]

@MWaser this project is not owned by "the MySQL team"; dougwilson is (AFAIK) a volunteer open source maintainer. If you look at his profile it looks like he may be taking a vacation (for the first time in a year).

There does appear to be an "official" JavaScript connector at mysql/mysql-js; I haven't used it so I can't say whether it supports Azure Database for MySQL or not. You may be able to get a response from the MySQL team there.

There's also another open source project, mysql2, which may offer better compatibility with Azure. It claims its "mostly API compatible with mysqljs" so it may be an easy drop-in replacement in your code.

[MWaser]

Thanks for replying @bgrainger.

The connector at mysql/mysql-js is for the NoSQL Database Jones.

There is, however, a mysql/mysql-connector-nodejs. Looks like an amateur project compared to this though.

Looks like we'll be moving away from MySQL. :-(

[elemount]

Do there any update on this issue @dougwilson ? Or is there any things which you think that @bgrainger should do firstly?

[elemount]

@dougwilson thanks for you reply. And @bgrainger , for check item

Test whether MySQL server sends a string[NUL] value for auth_method_data although it's documented as string[EOF].

Please mark it completed. I've verified that MariaDB also use string[EOF] for mysql_native_password and mysql_old_password.
Now let me explained it by the code of MariaDB.
For current version 10.2 of MariaDB, all the auth code is in sq_acl.cc. Change Plugin Packet function send_plugin_request_packet is called by function server_mpvio_write_packet and this function is called by a server authentication plugin, when it wants to send data to the client. All this function are just pass the data of the plugins. native_password_authenticate and old_password_authenticate are both use code looks like

mpvio->write_packet(mpvio, (uchar*)thd->scramble, SCRAMBLE_LENGTH + 1)

So the response will contains string[NUL].

For both MariaDB and MySQL use the string[NUL] instead of string[EOF], please take it easy as the default options. We can assume all other tools will use this case.

Btw, if you concern on it, please treat the auth_method_data as the string, but this string is consist of the salt and another \0.

[elemount]

@bgrainger , for check item

Set CLIENT_PLUGIN_AUTH in initial handshake

If you use the more risky code, what is the risk of it? Or can I provide some effort to mark it as no risk.
You may need more test? And anything else?

[bgrainger]

I think it would be best (for stability reasons) if the goal of this PR isn't to support pluggable authentication (but just to handle buggy auth switch request methods sent from buggy servers), so
I've removed that checkbox from this PR. I've checked the other (as per @elemount).

In my mind, this PR is feature-complete and accomplishes the low-risk goal of supporting an auth-switch-request (to an authentication method that is already supported) during initial authentication. This will fix the proxy problem in #1396 and Azure Database for MySQL in #1729.

@dougwilson is that satisfactory?

[bgrainger]

If you use the more risky code, what is the risk of it? Or can I provide some effort to mark it as no risk.

It changes the way this client connects to every DB server for all clients who have installed this library. I simply don't know how that might affect connections to every version of MySQL Server, MariaDB, Percona, Amazon Aurora, etc. that are being used in the wild; it's just a large surface area to test.

I suppose we could mitigate the risk by making the PLUGIN_AUTH flag off by default, and allowing clients who need it to turn it on manually; my commit didn't do that.

[dougwilson]

I'm at least still expecting "Test whether MySQL server sends a string[NUL] value for auth_method_data although it's documented as string[EOF]." to be done, as we discussed above. I'm expecting this to be an actual test in our test suite, so for example when new MySQL servers come out we can validate that we still work correctly, since we're saying that what is implemented here is different from what the documented protocol is.

I also disagree on your assessment that we should support the buggy servers like this. Either support the auth switch protocol or it is on those servers to stop being buggy.

[dougwilson]

As per your comment above, if there is a fear of breaking by implementing the auth switch, it can be implemented behind an opt-in flag, as you suggested. That makes sense.

[elemount]

Why not just use challenge[0:20] for all the time. Just describe in Secure Password Auth

[xukai286]

because it may not be match 20 bytes if package is broken

[bgrainger]

I'm not going to be able to implement the requested fixes in the near future, so I'm going to close this for now. I'm happy for anyone to take my code (in this PR and in the other commit I've linked to) and rework it into something better.

Or, if I do find the time to work on this, I'll reopen the PR when I've written the updated code.

[dougwilson]

FYI @bgrainger your PR landed on master and will be in the next minor release.

[alejandromav]

This commit works fine for me using a connection pool, thanks.

But if I manually start a connection with connection.connect(), calling connection.end() causes an ECONNRESET error.

[elemount]

@alex030293 , let me repro it and fix it, could you help to provide some info on how to repro it? And if possible, could you use the https://github.com/elemount/mysql to test again, it is an advanced commit on the auth, and may be merged into next release.

[alejandromav]

@elemount for me it's failing with the example in the Readme:

var mysql      = require('mysql');
var connection = mysql.createConnection({ config });

connection.connect();

connection.query('SELECT 1 + 1 AS solution', function (error, results, fields) {
  if (error) throw error;
  console.log('The solution is: ', results[0].solution);
});

connection.end();

Throws ECONNRESET when calling connection.end(). Anyway, I'm using a pool, which works fine.

Thanks.

[elemount]

@alex030293 , I test on my local MySQL server, everything is expected. And only Azure MySQL have this issue. Due to my sniffer, it is for the Azure MySQL send a RST, ACK after COM_QUIT.

RST, ACK is a force shutdown, which is not match with the local MySQL server which provide a grateful shutdown. In another hand, force shutdown is a also shutdown, which may be one of expected results of COM_QUIT.

@dougwilson , what's your idea on it? Could I have a pull request on it? Or we should not handle this RST. Could you share your idea on it?

[alejandromav]

@elemount thanks for for time. Yes, I meant this was only an issue with Azure MySQL but we can still use a connection pool, which works as expected and it's even a best practice. Please let me know if I can help with anything.

[dougwilson]

That is a good question: if the RST should be reported as an error or not. Ideally it should be an error if that packet doesn't have the ACK flag set, acknowledging that the QUIT command was actually received, but I'm not sure it's possible to know that distinctions in the Node.js networking code.

Are we sure this is not just an issue with the Azure implementation that can just be fixed? The MySQL protocol only says that the response to a QUIT is an OK packet or the connection closing, though it doesn't really say if the closing should be FIN or RST or if that matters.

Definitely a RST without the ACK set should be reported as an error here, I believe, because users are free to choose to ignore the error from the .end() method by providing a callback and ignoring the error -- if we don't raise any error, they don't have a choice to opt into it.

Back to the Azure-specific issue, if Azure doesn't believe this is an issue they should fix and Node.js can actually tell us that the ACK flag was set, then we can handle it, but that would bring up the whale of the Azure support: we have no tests against Azure in our CI. If Azure is going to be a first-class citizen of this module, we really need to look into what it will take to add Azure to the CI system, otherwise it's on the module users to be the QA for issues like this. I would expect that the existing CI tests would have shown this issue if Azure was a part of the CI.

[dougwilson]

P.S. if there is a desire to continue talking about this RST issue, please open a new issue, otherwise having it be here, at the bottom of a closed pull request, it's really hard to find.

[alejandromav]

@dougwilson I'd say testing Azure integration is up to the user and not CI. But the debate about Azure importance is really interesting. Anyhow, I've just opened #1811.

Thanks for your answers both of you.

[dougwilson]

I'd say testing Azure integration is up to the user and not CI.

Then that means it is up to the users to diagnose their issues and provide the code fixes, because otherwise, for example, how can I work on a fix for your issue without the ability to run the scenario you are referring to?

[elemount]

Hi @dougwilson , I'm familar with a Azure MySQL guy.
Talk with him, the RST, ACK is by design. And he has a request for you.

It is possible that we can provide a totally free Azure MySQL server for mysqljs to do CI? This server is totally free for any usage and will be kept forever. We will make sure all the parameters is set properly, and make the CI pass for the current release.

How you think about it?