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

Support authentication switch request. Fixes #1396 #1730

Closed
wants to merge 1 commit into from

Conversation

bgrainger
Copy link
Contributor

@bgrainger bgrainger commented May 14, 2017

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

Copy link
Member

@dougwilson dougwilson left a comment

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
Copy link
Contributor Author

@bgrainger bgrainger commented May 14, 2017

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
Copy link
Contributor Author

@bgrainger bgrainger commented May 14, 2017

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

@bgrainger
Copy link
Contributor Author

@bgrainger bgrainger commented May 14, 2017

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
Copy link
Member

@sidorares sidorares commented May 15, 2017

@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
Copy link
Contributor Author

@bgrainger bgrainger commented May 15, 2017

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

Add optional auth plugin name and data to UseOldPasswordPacket. Support
mysql_native_password and mysql_old_password as potential authentication
methods (but still require 'insecureAuth:true' for the latter).
@bgrainger
Copy link
Contributor Author

@bgrainger bgrainger commented May 15, 2017

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
Copy link
Member

@sidorares sidorares commented May 15, 2017

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
Copy link
Contributor Author

@bgrainger bgrainger commented May 18, 2017

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

@dougwilson
Copy link
Member

@dougwilson dougwilson commented May 18, 2017

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

@MWaser
Copy link

@MWaser MWaser commented Jun 2, 2017

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
Copy link

@christophmayrhofer christophmayrhofer commented Jun 4, 2017

+1 for merge

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jun 4, 2017

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
Copy link
Member

@dougwilson dougwilson commented Jun 4, 2017

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
Copy link
Contributor Author

@bgrainger bgrainger commented Jun 6, 2017

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
Copy link
Contributor Author

@bgrainger bgrainger commented Jun 6, 2017

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
Copy link

@MWaser MWaser commented Jun 20, 2017

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
Copy link
Contributor Author

@bgrainger bgrainger commented Jun 20, 2017

@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
Copy link

@MWaser MWaser commented Jun 20, 2017

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
Copy link
Contributor

@elemount elemount commented Jun 28, 2017

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

@elemount
Copy link
Contributor

@elemount elemount commented Jun 28, 2017

@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
Copy link
Contributor

@elemount elemount commented Jun 28, 2017

@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
Copy link
Contributor Author

@bgrainger bgrainger commented Jun 28, 2017

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
Copy link
Contributor Author

@bgrainger bgrainger commented Jun 28, 2017

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
Copy link
Member

@dougwilson dougwilson commented Jun 28, 2017

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
Copy link
Member

@dougwilson dougwilson commented Jun 28, 2017

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.

// null-terminated byte array for mysql_native_password; we only need to hash with
// the first 20 bytes
var challenge = packet.pluginData;
if (challenge.length === 21 && challenge[20] === 0) {
Copy link
Contributor

@elemount elemount Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 638d79d to 88bade0 Compare Jun 29, 2017
@bgrainger
Copy link
Contributor Author

@bgrainger bgrainger commented Jul 10, 2017

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
Copy link
Member

@dougwilson dougwilson commented Aug 10, 2017

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

@alejandromav
Copy link

@alejandromav alejandromav commented Aug 25, 2017

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
Copy link
Contributor

@elemount elemount commented Aug 28, 2017

@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
Copy link

@alejandromav alejandromav commented Aug 28, 2017

@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
Copy link
Contributor

@elemount elemount commented Aug 30, 2017

@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
Copy link

@alejandromav alejandromav commented Aug 30, 2017

@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
Copy link
Member

@dougwilson dougwilson commented Aug 30, 2017

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
Copy link
Member

@dougwilson dougwilson commented Aug 30, 2017

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
Copy link

@alejandromav alejandromav commented Aug 31, 2017

@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
Copy link
Member

@dougwilson dougwilson commented Aug 31, 2017

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
Copy link
Contributor

@elemount elemount commented Sep 1, 2017

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?

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

Successfully merging this pull request may close these issues.

None yet

8 participants