There was a problem hiding this 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 👍
|
Thanks for the quick feedback; it is super helpful!
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. |
|
Added one new test (in |
|
I reinstated |
|
@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): |
|
Disclaimer: although I've worked with the MySQL protocol, I have never worked with pluggable auth (except for handling the base case of What should The signature for |
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).
Would you accept this PR simply as a bugfix for buggy servers (Azure and apparently some MySQL proxies), without the full
Once |
Currently it's full auth switch handler responsibility - similar to for example node http request handler - if you don't do |
|
@dougwilson Have you had a chance to review the updated code, or comment on the questions I asked earlier? |
|
I have not yet. I have been working to fix a security vulnerability elsewhere, so this has fallen to my backburner. |
|
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) . . . . |
|
+1 for merge |
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. |
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? |
I completely agree. But I would like to make this library usable by developers who have to code against those buggy servers.
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.
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 |
AuthSwitchRequest is documented as having "auth_method_data (string.EOF)" but MySQL Server 5.7 actually sends 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 A similar slice is already performed in HandshakeInitializationPacket.js here, apparently for compatibility across multiple servers that follow the documented protocol slightly differently.
I think this would be possible to test by setting |
|
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? |
|
@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. |
|
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. :-( |
|
Do there any update on this issue @dougwilson ? Or is there any things which you think that @bgrainger should do firstly? |
|
@dougwilson thanks for you reply. And @bgrainger , for check item Please mark it completed. I've verified that MariaDB also use string[EOF] for mysql_native_password and mysql_old_password. mpvio->write_packet(mpvio, (uchar*)thd->scramble, SCRAMBLE_LENGTH + 1)So the response will contains 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 |
|
@bgrainger , for check item If you use the more |
|
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 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? |
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 |
|
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. |
|
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) { |
There was a problem hiding this comment.
Why not just use challenge[0:20] for all the time. Just describe in Secure Password Auth
There was a problem hiding this comment.
because it may not be match 20 bytes if package is broken
638d79d to
88bade0
Compare
|
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. |
|
FYI @bgrainger your PR landed on master and will be in the next minor release. |
|
This commit works fine for me using a connection pool, thanks. But if I manually start a connection with |
|
@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. |
|
@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. |
|
@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
@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? |
|
@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. |
|
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 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. |
|
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. |
|
@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. |
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? |
|
Hi @dougwilson , I'm familar with a Azure MySQL guy.
How you think about it? |
The new code supports
mysql_native_passwordandmysql_old_passwordas potential authentication methods (but still requiresinsecureAuth: truefor the latter).This fixes authentication against Azure Database for MySQL: #1729
mysql_native_passwordstring[NUL]value forauth_method_dataalthough it's documented asstring[EOF].