Add authentication plugin support #353

grooverdan · 2015-06-28T05:47:14Z

This extracts a plugin name from the server and exposes this as
connection.get_plugin_name().

The client side here adds capabilities PLUGIN_AUTH and
PLUGIN_AUTH_LENENC_CLIENT_DATA.

Because of this the HandshakeResponse response has been altered to use
the server_capabilities a bit more strictly.

The immediate upshot is that plugins like unix_socket are immediately
supported where they previously would of returned and error as without
PLUGIN_AUTH being advertised on the client side, the server only accepts
mysql_native_password and mysql_old_password.

To support other plugins some more work is needed to implement these.

I didn't intentionally break 4.X support however I haven't tested it either.

This extracts a plugin name from the server and exposes this as connection.get_plugin_name(). The client side here adds capabilities PLUGIN_AUTH and PLUGIN_AUTH_LENENC_CLIENT_DATA. Because of this the HandshakeResponse response has been altered to use the server_capabilities a bit mroe strictly. The immediate upshot is that plugins like unix_socket are immediately supported where they previously would of returned and error as without PLUGIN_AUTH being advertised on the client side, the server only accepts mysql_native_password and mysql_old_password. To support other plugins some more work is needed to implement these.

grooverdan · 2015-07-26T23:51:44Z

Is there some reassurance I can help with the acceptance of this patch?

methane · 2015-08-23T18:22:17Z

pymysql/util.py

@@ -17,3 +17,18 @@ def join_bytes(bs):
        for b in bs[1:]:
            rv += b
        return rv
+
+# https://dev.mysql.com/doc/internals/en/integer.html#packet-Protocol::LengthEncodedInteger
+def lenenc_int(i):


util module shouldn't know about MySQL protocol.
Please move this function to connections module.

lenenc-int appears in other places outside connection like https://dev.mysql.com/doc/internals/en/com-query-response.html, https://dev.mysql.com/doc/internals/en/binary-protocol-resultset.html and https://dev.mysql.com/doc/internals/en/table-map-event.html

All of these are protocol related.
And all code relating to protocol is in connections module for now.

methane · 2015-08-23T18:26:07Z

Is there some reassurance I can help with the acceptance of this patch?

This PR is large.
I don't have time to test authentication plugins.

methane · 2015-08-23T18:30:28Z

pymysql/connections.py

@@ -345,7 +353,7 @@ def is_eof_packet(self):
        # http://dev.mysql.com/doc/internals/en/generic-response-packets.html#packet-EOF_Packet
        # Caution: \xFE may be LengthEncodedInteger.
        # If \xFE is LengthEncodedInteger header, 8bytes followed.
-        return len(self._data) < 9 and self._data[0:1] == b'\xfe'
+        return self._data[0:1] == b'\xfe'


Why? Have you read comment above?

When the auth switch is done the length isn't < 9. http://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::AuthSwitchRequest

I've found the <9 reference here: http://dev.mysql.com/doc/internals/en/packet-OK_Packet.html. I'll see what can be done to avoid changing this bit and support the AuthSwitchRequest.

methane · 2015-08-23T18:36:12Z

PyMySQL doesn't have enough test suite about authentication.
So I'm afraid of merging such large PR.

methane · 2015-08-23T18:48:56Z

pymysql/connections.py

+            handler = None
+        if plugin_name == "mysql_native_password":
+            # https://dev.mysql.com/doc/internals/en/secure-password-authentication.html#packet-Authentication::Native41
+            data = _scramble(self.password.encode('latin1'), auth_packet.read_all()) + int2byte(0)


int2byte(0) should be b'\0'

grooverdan · 2015-08-25T10:27:28Z

PyMySQL doesn't have enough test suite about authentication.

Ok. no problem. I can add some more. Might get some code coverage there to identify the bits that need testing a little better.

I don't have time to test authentication plugins.

I've extended the test suite a bit more to cover it.

So I'm afraid of merging such large PR.

ok. Hoping adding more tests is a sufficient way to test this.

…y plugin name occurs with PLUGIN_AUTH meaning old passwords

grooverdan · 2015-08-30T09:37:51Z

Despite appearances this is still passing Travis tests and builds https://travis-ci.org/grooverdan/PyMySQL/builds/77866544 with a significant portion of the code added being covered https://coveralls.io/builds/3435133/source?filename=pymysql%2Fconnections.py .

I'll merge the changes with #369.

Is a sufficient test coverage of code added a suitable reassurance for merging this PR?

Would you prefer this to be broken down into smaller PRs?

…pam use of that name. Recreate afterwards

grooverdan · 2015-10-25T22:52:33Z

I'm finished. Questions/queries or requests for fixing code welcome.

nicklasb · 2015-11-19T11:53:03Z

Just lurking this...
In my understanding, it is currently not possible to authenticate over sockets, which seems like a really severe problem(at least it is for me), which this PR seems to address.

And while neither I am not fond of huge PR:s, they aren't atomic but consists of lots of commits, and therefore things can be selectively rolled back.

To put it short, please merge this. :-)

grooverdan · 2015-11-20T00:12:02Z

it got quite big quickly in the number of commits. Happy to rebase into a single (or a few basic increments) commits. All of the code adds rigour to the protocol and callouts to where authentication plugins are required. In the mean time @nicklasb , the first commit 2d1da09 of adding PLUGIN_AUTH to the CAPABILITIES is sufficient to get socket_auth working.

nicklasb · 2015-11-20T01:28:11Z

Thanks for your response, @grooverdan .
Well, sometime that happens, especially if one continues to improve it.

I am not sure that the maintainers have time to tell you how to break it apart..but as a maintainer of other projects myself, and if I were you, I would try to first break out each or all bug fixes in a separate PR, as those could then be released in a minor version.

Then add structural changes and new functionality in another PR.

As there is not visible develop branch for the new stuff, at least that is the way I'd do it.

I can't deploy non-pip-stuff, but I can at least try it.

nicklasb · 2015-11-20T01:50:25Z

Sorry, didn't work with just pasting over the 0.6.7 code, and I don't have time to clone and try with git now, perhaps I can test it tomorrow instead.

nicklasb · 2015-12-16T13:49:16Z

Hi @grooverdan and @methane, I have now tried with this branch, and it works.
However, it says that the MariaDB issue(#243) is resolved by this, but as this isn't merged, that isn't strictly true, and should as such be reopended.

Add authentication plugin support

grooverdan · 2016-01-03T05:48:44Z

thanks @methane

methane · 2016-01-03T10:47:10Z

While I've merged this pull request, I couldn't get how connection.next_packet works.
Until I understand it and refactoring it for future maintenance, I won't release next version.

nicklasb · 2016-01-03T22:19:00Z

It's OK. Cool kids use #master. :-)

grooverdan · 2016-01-04T08:34:17Z

I couldn't get how connection.next_packet works.

Its the next sequence number in the packet protocol (https://dev.mysql.com/doc/internals/en/sequence-id.html).

It seems that pkt_nr is cast down to uchar during use (despite being a uint in the code) - most of its use in this file: https://github.com/mysql/mysql-server/blob/5.7/sql/net_serv.cc#L789

methane · 2016-01-04T08:35:53Z

Thanks for reply.
But I have researched it already and refactoring it now.
See #405.

grooverdan added 2 commits June 28, 2015 15:43

mysql and mariadb compatible test

128d308

grooverdan mentioned this pull request Jun 29, 2015

insufficient completions for SHOW dbcli/mycli#65

Closed

grooverdan added 2 commits July 4, 2015 14:06

Merge branch 'expose_auth_plugin_name' into expose_auth_plugin_name_1

42484ad

Add Plugin API and implement standard plugins

bdeade9

This was referenced Jul 23, 2015

"Client does not support authentication protocol requested by server; consider upgrading MySQL client" when attempting to authenticate over sockets #243

Closed

CONJ 164 plugin implementation mariadb-corporation/mariadb-connector-j#22

Closed

grooverdan changed the title ~~Add basic connection plugin support~~ Add basic authentication plugin support Jul 26, 2015

merge from master

c46538b

methane reviewed Aug 23, 2015
View reviewed changes

grooverdan added 6 commits August 24, 2015 09:07

int2byte(0) -> b\0

bd1ba65

formatting and handling errors/existing users easier

4a94edd

remove spurious print statement

59f29f3

restore is_eof_packet implementation. Add is_auth_switch_request method

0a1763f

tests: upgrade plugin tests to new class. Test against mysql-5.6 too

4cc3ca7

int2byte(0) -> b'\0'

8cc9cdf

grooverdan force-pushed the expose_auth_plugin_name branch from 5901faf to 8cc9cdf Compare August 25, 2015 12:10

grooverdan added 6 commits August 25, 2015 22:20

py3+ fix for TypeError: Type str doesnt support the buffer API

28fc9b9

str -> byte in _my_crypt

ea45a73

add tests for old_password. Fix py3 old password authentication. empt…

a52e1b7

…y plugin name occurs with PLUGIN_AUTH meaning old passwords

remove code coverage from bits we cant test

d3136d8

merge from master

e196c4e

duplicate copy of travis.databases.json that breaks 5.6 builds

258cf1f

grooverdan added 6 commits August 29, 2015 17:51

test cases to cover more of the authentication plugin server framework

2069599

plugin sha256 - cannot handle yet

bcea1a6

add PAM plugin to test suite

93fdb75

5.7 has warnings on old_passwords, even for sha256_passwords

ebc87c5

in 5.7, set password = PASSWORD() is deprecated

98b659d

correct object reference

aa23fbb

grooverdan changed the title ~~Add basic authentication plugin support~~ Add authentication plugin support Aug 30, 2015

grooverdan added 6 commits August 31, 2015 08:28

merge from master

a578bb4

env vars to test pam/cleartext plugin

0590ce9

explicit close on socket in test_defer_connect

5d3904b

add pam plugin in cleartext mode to test mysql_cleartext_password plugin

3d8dfc1

travis: show syslog(with PAMCLEAR) and error log on success and failure

23135cd

test: auth: correct identation. Drop $USER@localhost before creating …

c09e8b4

…pam use of that name. Recreate afterwards

move lenenc_int from util.py to connections.py

ee78b2d

methane added a commit that referenced this pull request Jan 3, 2016

Merge pull request #353 from grooverdan/expose_auth_plugin_name

1893637

Add authentication plugin support

methane merged commit 1893637 into PyMySQL:master Jan 3, 2016

grooverdan deleted the expose_auth_plugin_name branch January 3, 2016 05:48

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication plugin support #353

Add authentication plugin support #353

grooverdan commented Jun 28, 2015

grooverdan commented Jul 26, 2015

methane Aug 23, 2015

grooverdan Aug 23, 2015

methane Dec 26, 2015

methane commented Aug 23, 2015

methane Aug 23, 2015

grooverdan Aug 23, 2015

methane commented Aug 23, 2015

methane Aug 23, 2015

grooverdan commented Aug 25, 2015

grooverdan commented Aug 30, 2015

grooverdan commented Oct 25, 2015

nicklasb commented Nov 19, 2015

grooverdan commented Nov 20, 2015

nicklasb commented Nov 20, 2015

nicklasb commented Nov 20, 2015

nicklasb commented Dec 16, 2015

grooverdan commented Jan 3, 2016

methane commented Jan 3, 2016

nicklasb commented Jan 3, 2016

grooverdan commented Jan 4, 2016

methane commented Jan 4, 2016

Add authentication plugin support #353

Add authentication plugin support #353

Conversation

grooverdan commented Jun 28, 2015

grooverdan commented Jul 26, 2015

methane Aug 23, 2015

Choose a reason for hiding this comment

grooverdan Aug 23, 2015

Choose a reason for hiding this comment

methane Dec 26, 2015

Choose a reason for hiding this comment

methane commented Aug 23, 2015

methane Aug 23, 2015

Choose a reason for hiding this comment

grooverdan Aug 23, 2015

Choose a reason for hiding this comment

methane commented Aug 23, 2015

methane Aug 23, 2015

Choose a reason for hiding this comment

grooverdan commented Aug 25, 2015

grooverdan commented Aug 30, 2015

grooverdan commented Oct 25, 2015

nicklasb commented Nov 19, 2015

grooverdan commented Nov 20, 2015

nicklasb commented Nov 20, 2015

nicklasb commented Nov 20, 2015

nicklasb commented Dec 16, 2015

grooverdan commented Jan 3, 2016

methane commented Jan 3, 2016

nicklasb commented Jan 3, 2016

grooverdan commented Jan 4, 2016

methane commented Jan 4, 2016