search

sidorares / node-mysql2

:zap: fast mysqljs/mysql compatible mysql driver for node.js
https://sidorares.github.io/node-mysql2/
MIT License
4.07k stars 619 forks source link

Do not enable mysql_clear_password by default #1617

Open MasterOdin opened 2 years ago

MasterOdin commented 2 years ago

1552 added support for the mysql_clear_password auth plugin, but made it enabled by default. Per the manual docs on it:

Sending passwords as cleartext may be a security problem in some configurations. To avoid problems if there is any possibility that the password would be intercepted, clients should connect to MySQL Server using a method that protects the password. Possibilities include SSL (see Section 6.3, “Using Encrypted Connections”), IPsec, or a private network.

To make inadvertent use of the mysql_clear_password plugin less likely, MySQL clients must explicitly enable it.

I would propose to make it disabled by default, and only enable it implicitly if the LIBMYSQL_ENABLE_CLEARTEXT_PLUGIN environment variable is set to 1, Y or y (per the docs), or that a library consumer can add it themselves via config.authPlugins (following #1497 being merged to make it easy to reference).

MasterOdin commented 2 years ago

With the v3.0.0-rc.1 release that includes the support for mysql_clear_password, I'd like to bump up this issue before a final v3.0.0 goes out such that doing this would then constitute a breaking change.

cc @sidorares

sidorares commented 2 years ago

thanks for the ping, yeah wanted to review that before final release, after that its going to be more difficult to change api

sidorares commented 2 years ago

I don't think there is any expectation from this library to respect LIBMYSQL_ENABLE_CLEARTEXT_PLUGIN or any other environment variable libmysql expects, but would be good to have some similar behaviour and consistent names for createConnection/createPool config flags

sidorares commented 2 years ago

Looping in plugin users - @rathboma @silverbullettruck2001 - wdyt?

sidorares commented 2 years ago

What I see the main attack vector is dns poisoning and mysql honeypot server stealing passwords.

What we discuss I guess is

sounds reasonable to me

MasterOdin commented 2 years ago

I agree with the above. Happy to put up a PR to tackle that, though would appreciate a pointer for (2) on where you'd like to see that in the code.

For anything dealing with databases, given how sensitive the data that might live in it, having the mechanisms to connect start as secure as possible and then only weaken security at the user's request is important to me.

sidorares commented 2 years ago

I agree with the above. Happy to put up a PR to tackle that, though would appreciate a pointer for (2) on where you'd like to see that in the code.

Probably here:

https://github.com/sidorares/node-mysql2/blob/0229368d4002a5dcf193d59193ef561c28b7627c/lib/commands/auth_switch.js#L59

add something along the lines of if (pluginName === 'mysql_clear_password') { // add steps to see if OK to accept

sidorares commented 2 years ago

I'm a bit hesitant re LIBMYSQL_ENABLE_CLEARTEXT_PLUGIN but at the same time not against it

image

MasterOdin commented 2 years ago

I'd be perfectly fine with having it be the library consumer who has to manually opt into using cleartext, and so they would have to deal with LIBMYSQL_ENABLE_CLEARTEXT_PLUGIN or whatever other mechanism they care to have in their program.

rathboma commented 2 years ago

Hey all, I know I started the original PR that was merged to add cleartext support, but I agree with @MasterOdin that it makes sense for it to be disabled by default.

If possible, could it be enabled in some way when instantiating the client on a connection-by-connection basis, rather than through an environment variable, or another static method?

rathboma commented 2 years ago

So as @sidorares suggested higher up the thread, adding a config parameter to createConnection or createPool sounds perfect.

mysql.createPool({
  enableCleartextPlugin: true
  ...
})
// this would be amazing
sidorares commented 2 years ago

@MasterOdin would you be able to provide PR with this change?

MasterOdin commented 1 year ago

end to end test covering happy and error path. This is probably the most complicated step. I haven't found an easy way to configure real mysql server with mysql_clear_password auth, possible option is to have mysql2.createServer server but not sure how much work is to have that running

Yeah, neither have I, without also configuring something like ldap alongside it. I agree the best option is to use mysql2.createServer. My plan was to base a test on one of:

I think this is shouldn't be too bad, just haven't yet found time, hoping to at some point this week.

sidorares commented 1 year ago

sorry this has not received enough attention and I did release 3.0 in the end. I'll try to get the above functionality done and release as 3.1

sidorares commented 1 year ago

probably depends on #1113 in order to be able to add e2e test