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' #2225

Closed
wants to merge 4 commits into from

Conversation

nmggithub
Copy link

@nmggithub nmggithub commented May 29, 2019

This addition is two-fold. First of all, it allows user-set flags to enable flags that were disabled by default. This allows users to enable the PLUGIN_AUTH flag. Second and finally, it adds a case auth handler for the 'mysql_clear_password' auth plugin. This case simply returns the password, in plain text, as a buffer. These both work in conjunction to allow for users to connect to databases that request the 'mysql_clear_password' plugin. This solves #2001 This enables support for MariaDB servers using the PAM Authentication Plugin, the user just has to add flags: ['+PLUGIN_AUTH'] to their options object. I could have changed the default for this flag, but I am unaware of the consequences of enabling it by default, so I left it up to the user to enable it. This is tested and works on my machine for a MariaDB database provided by my university for my database course.

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

Awesome, thanks so much! Before we land this we need some kind of tests in our test suite for this auth method and how it is working so we can make sure it stays working for you.

You're welcome to update your pr to add this, can wait for someone else to add this, or if there are questions you can ask and I can try and help you make the tests 👍

@nmggithub
Copy link
Author

@dougwilson Ok, I would like to help with the tests, but I don't know how to start. I've tested this with my school's database, but I obviously would not want to included my credentials for it in a test. What would we use to test it?

@dougwilson
Copy link
Member

Hi @nmggithub ! So this module contains an automated test suite. The overview of what to include in a contribution can be found in our contributing section of the README: https://github.com/mysqljs/mysql#contributing

Ideally you could add a new integration test to test against a database that is set up with a clear password auth pugin, but you can also make a unit test that uses the fake server and you'll need to implement how the server does clear auth if you go that route, though.

@dougwilson
Copy link
Member

dougwilson commented May 30, 2019

Also @nmggithub can you clarify how this solves #2001 ? The user who opened the issue is showing they are using unix_socket auth, but this seems to implement a different type of auth from the issue #2001 .

@nmggithub
Copy link
Author

@dougwilson Apologies for the late reply, as I have been busy. Looking closer, it does seem I was mistaken is saying this solves #2001 , but only inasmuch as it pertains to the unix_socker auth. What this is doing is adding support for MariaDB PAM authentication. However, the same exact error is given in the case of trying to login to a MariaDB server using PAM authentication, which makes me think my fix is, at least in part, a fix for #2001 . I can play around with a MariaDB install to see if I can replicate the error using unix_socket and will update the PR with any additional code that may need to be added.

@nmggithub
Copy link
Author

nmggithub commented May 30, 2019

@dougwilson Ok I found solution for #2001 (but seeing as I am locked from commenting, I didn't know where to put this, so I'm putting it here). The error in that issue is due to the fact that the auth method is unix_socket. MariaDB (and I'm assuming every other flavor of MySQL), doesn't allow remote connections to users with the unix_socket auth method. This can be seen by enabling the PLUGIN_AUTH flag, and seeing the new rejection messages, which see the user trying to login as user@theirownip not user@serverip. So the user that started the issue would have to run an ALTER USER query from the machine that is running the server. Then that will allow remote connections to the host.

TL;DR: This pull request does NOT fix #2001 as that is a server-side issue. This pull request DOES add support for MariaDB servers that use the PAM authentication plugin, though.

P.S. sorry for the mess

@nmggithub nmggithub force-pushed the master branch 5 times, most recently from 32882a2 to 3607c4c Compare May 31, 2019 00:35
@nmggithub
Copy link
Author

@dougwilson tests have been added. Apologies for the multiple commits. I was having issues with my git client. Anyway, I spun up a DigitalOcean droplet, and setup a server there and tested against it. It works, but I don't want to send the credentials publicly here. Is there a way I can privately send them to you / show how to run the tests?

@nmggithub
Copy link
Author

Deleted bad test.

@dougwilson
Copy link
Member

Hi @nmggithub sorry I have been away over the weekend. So I don't need your credentials to your own test servers :) The idea for the tests is that they will run on the CI infrastructure here and that they would test the supported features using the CI servers provided by the Travis CI platform. The idea is that if you want to make an integration test (based on the files checked in) then you'll want to probably edit the .travis.yml file in this project to create a user which uses the given plugin in which the underlying integration test will then test again.

I hope that makes sense. Please let me know if you need any help. I'll be back online again on Monday :) !

@nmggithub
Copy link
Author

@dougwilson The issue is the plugin is actually serverside. If a user is authenticated via PAM, as previously explained, the server will ask via that plugin. So I cannot make such a user on the client side.

@dougwilson
Copy link
Member

Hi @nmggithub that is unfortinate. I will close this PR for now and you can always re-open if you can get some automated tests created. The tests are run on the CI server along with the server-side so it should be possible to do. Perhaps you could even open an issue about this feature to describe it and someone else (perhaps me when I get some time) can help implement the support in this module along with the proper tests?

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

2 participants