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

Add support for 'mysql_clear_password - continued #2327

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ZJONSSON
Copy link
Contributor

Thanks @nmggithub for the mysql_clear_password branch. This PR is a continuation of #2225 with some unit tests added. Also I'm rejecting mysql_clear_password unless it's on a secure socket, which seems to be conventional among database clients. Maybe we should allow an override to that condition, or just drop the check altogether?

@dougwilson I didn't find a simple way to get access to whether the socket is secure or not in a generic fashion. I implemented it in a relatively hacky way here (setting this._tls = true) for the handshake, but changeUser might still be missing access to whether the socket is secure or not.

@ZJONSSON ZJONSSON changed the title Master nmggithub Add support for 'mysql_clear_password - continued Mar 28, 2020
Copy link
Member

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Thank you for picking this back up.

lib/ConnectionConfig.js Outdated Show resolved Hide resolved
test/FakeServer.js Outdated Show resolved Hide resolved
options = options || {};

switch (name) {
case 'mysql_native_password':
return Auth.token(options.password, data.slice(0, 20));
case 'mysql_clear_password':
if (!isSecure) {
throw new Error('Authentication method mysql_clear_password not supported on insecure connections');
Copy link
Member

Choose a reason for hiding this comment

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

I get where this is coming from, but is probably going to be limiting to users in the long run. What about just adding an option the user can use to enable / disable this? I'm thinking along the lines of our insecureAuth option (perhaps clearAuth?) and how the mysql command line program requires a flag to enable it (https://dev.mysql.com/doc/refman/8.0/en/mysql-command-options.html#option_mysql_enable-cleartext-plugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the --enable-cleartext-plugin is just to enable the plugin itself. Are you thinking the insecureAuth would allow clear_password even if we are not on a tls socket?

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking to have it behave inline with the mysql command line program: add an option (like perhaps clearAuth?) that that user must enable to use the clear password auth. That would be the only requirement -- the user can choose to use that over SSL or not as their own choice, just like in the mysql command line program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. Looking at the python module, if you specify auth_plugin='mysql_clear_password' then cleartext is used whether SSL is on or not. So maybe that would be the expected behavior.

I still think there is a big difference between saying: I want to use cleartext only inside secure channel, vs I want to use cleartext no matter what. For example, some of the GUI Sql clients I tested would bail on cleartext if SSL is not turned on.

Maybe the right way is to use clearAuth = true to enable it for secure channel, and insecureAuth to allow clear to be in an unencrypted socket?

@ZJONSSON
Copy link
Contributor Author

Thanks @dougwilson - just to clarify my interest in this - we are doing a proxy mysql server and want to handle the auth using bcrypt, as credentials are shared with other services. The only realistic way to use bcrypt is to required clear_password from a secure channel. Let me take another spin at this, really appreciate all the suggestions you have.

nmggithub and others added 9 commits March 28, 2020 20:28
Originally, would only allow user-set flags that disable a flag enabled by default. Now it will also allow user-set flags that enable a flag that is disabled by default.
Allow insecure cleartext if options.insecureAuth == true
* Specific SSL flag set for clear pwd tests
Copy link

@jbreckman jbreckman left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@jbreckman jbreckman left a comment

Choose a reason for hiding this comment

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

still lgtm

@ZJONSSON
Copy link
Contributor Author

@jbreckman is it still LGTM?

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

Successfully merging this pull request may close these issues.

4 participants