Do not enable mysql_clear_password by default

[MasterOdin]

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

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]

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

[sidorares]

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]

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

[sidorares]

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

What we discuss I guess is

• if server replies with "hey client, let's use cleartext auth" auth switch only agree if 1) enableCleartextPlugin: true is passed to a config or potentially LIBMYSQL_ENABLE_CLEARTEXT_PLUGIN environment variable is set
• if not explicitly enabled, throw connection error with message explaining how to enable

sounds reasonable to me

[MasterOdin]

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]

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:

node-mysql2/lib/commands/auth_switch.js

Line 59 in 0229368

authPlugin = standardAuthPlugins[pluginName];

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

[sidorares]

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

[MasterOdin]

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]

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]

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]

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

• add enableCleartextPlugin to ConnectionConfig somewhere near

  node-mysql2/lib/connection_config.js

  Line 115 in 74cb142

  this.enableKeepAlive = !!options.enableKeepAlive;

  ( default false )
• add the logic to only proceed with cleartext AuthSwitch if its enabled, otherwise error ( error message should include explanation how to add flag enabling cleartext, or maybe a link to readme with documentation and context )
• 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, might need addition of the whole server part of auth switch. If you do want to tackle that scenario hit me for help @MasterOdin

[MasterOdin]

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:

• https://github.com/sidorares/node-mysql2/blob/master/test/integration/connection/test-connect-sha1.js
• https://github.com/sidorares/node-mysql2/blob/master/test/integration/test-auth-switch.js

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

[sidorares]

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]

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