Add support for MySQL 8.0 and support for TLS/SSL for both Server and Client #312

michael2008 · 2018-08-19T13:14:01Z

This PR:

Added support for MySQL 8.0 for the Client: starting from MySQL 8.0.4, MySQL uses 'caching_sha2_password' as default authentication method, which caused the connection to fail. This PR fixed the problem by supporting three mainstream auth methods 'mysql_native_password', 'caching_sha2_password', and 'sha256_password', which covers a wide range of MySQL versions from MySQL 5.5 to MySQL 8.0.
Added support of new auth methods for the Server: the server now accepts 'mysql_native_password', 'caching_sha2_password', and 'sha256_password' as auth methods. Other old and deprecated auth methods are not supported.
Supports TLS/SSL for the Client and the Server: the new design maintains compatibility with the previous releases. Customizations are optional.

Since the upgrade of auth methods affects the Client and Server design, I made lots of changes to the code while trying to reuse the old code. Some minor existing bugs are fixed too. For instance, the buffer reader for the net.Conn is now removed because it causes the SSL handshake to fail.

Changing and refactoring can be a bad thing though as they can introduce new bugs. However, I tried my best to make the old tests pass and added new feature tests for different MySQL versions using docker compose to increase testing coverage. For now all test cases passed.

…bug when sha256_password is empty

…o 8.0

siddontang · 2018-08-19T15:09:31Z

Great work, thank you @michael2008

siddontang · 2018-08-20T00:17:44Z

Hi @michael2008

seem you have supported two features - 8.0 and TLS here, can we split into two PRs?

michael2008 · 2018-08-20T02:37:04Z

@siddontang About two PRs, a bit difficult. The reason to supporting TLS/SSL is that it is required for sha256_password and caching_sha2_password authentication. Basically MySQL supports two ways to send the client password to the server: one is to send as plain text via an established TLS channel; the other one is to send as encrypted text using a public key retrieved from the server. So the support of TLS connection may be taken as the side-effect of supporting new authentication methods. However, for the users, it seems that you can use TLS at the very beginning of connecting to the server.

IANTHEREAL · 2018-08-20T06:00:14Z

Great work @michael2008, we would review and test it

michael2008 · 2018-08-20T07:53:11Z

@GregoryIan I am do more tests with clients other than go-sql-driver. There are still some incompatibilities. I am working on them. I will give you feedback when it's done.

…ompatibility issues

michael2008 · 2018-08-20T13:38:47Z

@GregoryIan Hi, more tests are done. Now the Server is more compatible with MySQL server regarding the authentication part. Tested using the native MySQL Client (both version 5.7.23 and 8.0.12) against the Server. Tests all passed and showed promising results.

I think my works are almost done here. If you guys should find any problems, please let me know and I will fix them.

siddontang · 2018-08-21T22:40:52Z

client/auth.go

+	}
+
+	// encode length of the auth plugin data
+	var authRespLEIBuf [9]byte


what does LEI mean?

it's Length-Encoded-Integer(LEI), I will add comment here

siddontang · 2018-08-21T22:41:41Z

client/auth.go

 func (c *Conn) writeAuthHandshake() error {
+	if c.authPluginName != "mysql_native_password" && c.authPluginName != "caching_sha2_password" && c.authPluginName != "sha256_password" {


can you extract to a function like authPluginAllowed(name)

siddontang · 2018-08-21T23:00:58Z

client/auth.go

@@ -5,11 +5,16 @@ import (
 	"crypto/tls"
 	"encoding/binary"

+	"fmt"


remove the above blank line to group the standard libraries.

siddontang · 2018-08-21T23:01:23Z

client/auth.go

@@ -50,9 +61,8 @@ func (c *Conn) readInitialHandshake() error {

 		c.status = binary.LittleEndian.Uint16(data[pos : pos+2])
 		pos += 2
-
+		//capability flags (upper 2 bytes)


// capability

leave a space here

added space before comments

siddontang · 2018-08-21T23:10:45Z

client/auth.go

+			return nil, true, nil
+		}
+		if c.tlsConfig != nil || c.proto == "unix" {
+			// write cleartext auth packet


clear text?

any reference for it?

see here: https://dev.mysql.com/doc/refman/8.0/en/sha256-pluggable-authentication.html

If the connection is secure, an RSA key pair is unnecessary and is not used. This applies to encrypted connections that use TLS. The password is sent as cleartext but cannot be snooped because the connection is secure. If the connection is not secure, and an RSA key pair is available, the connection remains unencrypted. This applies to unencrypted connections without TLS. RSA is used only for password exchange between client and server, to prevent password snooping. When the server receives the encrypted password, it decrypts it. A scramble is used in the encryption to prevent repeat attacks. If a secure connection is not used and RSA encryption is not available, the connection attempt fails because the password cannot be sent without being exposed as cleartext.```

I added ref as comment in the code

siddontang · 2018-08-21T23:19:22Z

client/auth.go

 	}

 	return nil
 }

+// generate auth response data according to auth plugin
+func (c *Conn) genAuthResponse(authData []byte) ([]byte, bool, error) {


which does bool mean here? can you add a comment here?

That's a good question. Sure, the bool determines whether to add a \NUL to the end of data. That is quite tricky actually because mysql dev doc is very unclear about the message endings. The rules here are interpreted from MySQL server response packets. Besides, the MySQL implementation does not quite follow their docs in some response messages.

see: https://dev.mysql.com/doc/internals/en/string.html

siddontang · 2018-08-21T23:30:49Z

client/client_test.go

+
+// the certificates are in go-mysql/docker/resources
+
+var caPem = []byte(`-----BEGIN CERTIFICATE-----


I find that many places use the same keys, can we use move them to one place to reduce the code?

maybe move to the siddontang/mysql folder? but I think it's a bit confusing since the keys are for testing purpose only and the mysql folder is an actual working code base and if we do move the keys there, we have to expose them outside the package and that for users will be an unnecessary leak.

I prefer to create a test_util mod which only the tests can use it.

ok that will do

already fixed

siddontang · 2018-08-21T23:34:08Z

client/resp.go

+	}
+
+	// handle caching_sha2_password
+	if c.authPluginName == "caching_sha2_password" {


better to use const for all plugin names to avoid a typo. :-)

all replace in Client and Server

siddontang · 2018-08-21T23:37:30Z

server/server_test.go


-	s.db, err = sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s)/%s", *testUser, *testPassword, *testAddr, *testDB))
+	s.db, err = sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s)/%s?tls=%s", *testUser, *testPassword, *testAddr, *testDB, s.tlsPara))


must we use tls for the test now?

oh, it's controlled by the test configs. some tests set this value to 'false' to use non-tls connections and for TLS tests the value is set to 'skip-verify'

siddontang · 2018-08-23T19:30:07Z

PTAL @GregoryIan @holys

This PR is awesome, I can't wait to merge it, but I know we need detailed reviews and hope we can merge it next week. :-)

@michael2008 can you give us your email? we can keep in touch later.

michael2008 · 2018-08-24T00:37:24Z

@siddontang Sure, I will send you an email. Love to collaborate with you guys. BTW, PingCap is really great and you guys are awesome. You've contributed a lot to the open source community.

IANTHEREAL · 2018-09-06T03:49:42Z

sorry to have waited so long, we would focus on it next week @michael2008 @siddontang

IANTHEREAL · 2018-10-26T08:42:25Z

client/conn.go

@@ -109,6 +112,18 @@ func (c *Conn) Ping() error {
 	return nil
 }

+// use default SSL
+// pass to options when connect
+func (c *Conn) UseSSL(insecureSkipVerify bool) {


how about insecureVerify? while I call UseSSL((true), I think it need to verify

func UseSSL() means we use secure link. For golang TLS, the default 'insecureSkipVerify=false' requires certificates verification for security reasons. If one needs to use self-signed certs, the 'insecureSkipVerify' must be explicitly set to true. So I think it better be the same as golang crypto lib.

See https://golang.org/pkg/crypto/tls/#Config

IANTHEREAL · 2018-10-27T03:56:17Z

seem some wrong with table map event in 8.0.12 MySQL. can u solve it ? @michael2008
table structure
CREATE TABLE bank2_accounts ( id int(11) NOT NULL, balance int(11) NOT NULL, name varchar(32) DEFAULT NULL, remark varchar(128) DEFAULT NULL, PRIMARY KEY (id), UNIQUE KEY byName (name) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci
error logs
Header &replication.EventHeader{Timestamp:0x5bd3c052, EventType:0x13, ServerID:0x3, EventSize:0x48, LogPos:0x87d, Flags:0x0}, Data ">\x00\x00\x00\x00\x00\x01\x00\x04test\x00\x0ebank2_accounts\x00\x04\x03\x03\x0f\x0f\x04\x80\x00\x00\x02\f\x01\x01\x00\x02\x03\xfc\xff\x00", Err: EOF github.com/siddontang/go-mysql/replication/binlogsyncer.go:704:

siddontang · 2018-11-03T04:08:07Z

hi @GregoryIan

can we fix it by ourselves?

michael2008 · 2018-11-06T06:38:54Z

seem some wrong with table map event in 8.0.12 MySQL. can u solve it ? @michael2008
table structure
CREATE TABLE bank2_accounts ( id int(11) NOT NULL, balance int(11) NOT NULL, name varchar(32) DEFAULT NULL, remark varchar(128) DEFAULT NULL, PRIMARY KEY (id), UNIQUE KEY byName (name) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci
error logs
Header &replication.EventHeader{Timestamp:0x5bd3c052, EventType:0x13, ServerID:0x3, EventSize:0x48, LogPos:0x87d, Flags:0x0}, Data ">\x00\x00\x00\x00\x00\x01\x00\x04test\x00\x0ebank2_accounts\x00\x04\x03\x03\x0f\x0f\x04\x80\x00\x00\x02\f\x01\x01\x00\x02\x03\xfc\xff\x00", Err: EOF github.com/siddontang/go-mysql/replication/binlogsyncer.go:704:

I think this is a different issue from MySQL Authentication. More of a binlog format changing in MySQL 8. Should we open a new issue for this?

IANTHEREAL · 2018-12-08T08:46:24Z

ok, we can merge it now, we would fix it later @michael2008 @siddontang

siddontang · 2018-12-14T13:16:22Z

can you solve the conflict? @michael2008

michael2008

resolve conflict and merge error

michael2008 · 2018-12-20T11:34:12Z

can you solve the conflict? @michael2008

@siddontang conflict resolved

jianjunxie and others added 15 commits August 13, 2018 00:11

new testing env with docker-compose for interation with MySQl 5.7, 8.0

1a4c540

remove unused code

c04a594

update

fc03190

add support for caching_sha2_password and sha256_password, still has …

73c0ccc

…bug when sha256_password is empty

caching_sha2_password and sha256_password passed tests on MySQL 5.5 t…

8d9f198

…o 8.0

fix typo

f499d71

ready for test and debug

3a7a3c1

update vendor

132892e

refactor to compatible with MySQL server: add credential provider

339542d

update

596a8a1

update: add test cases

0c8fd9d

more tests

1d17f87

more tests

d8c39ba

add content to README

38a02a2

cleanup the code

175e49b

michael2008 mentioned this pull request Aug 19, 2018

try to support MySQL 8.0 #270

Open

jianjunxie added 2 commits August 19, 2018 22:02

empty cache for testing

11d31d6

fix a race condition for Server conf

6c1264d

fix rare RSA decryption error when read packet wrong

2cb184e

michael2008 added 2 commits August 20, 2018 21:28

test server against MySQL native clients (MySQL 5.7 & 8.0); resolve c…

91dd27b

…ompatibility issues

fix test instance

408fef7

siddontang reviewed Aug 21, 2018

View reviewed changes

Merge branch 'master' into master

aefe5e7

siddontang reviewed Aug 21, 2018

View reviewed changes

jianjunxie added 4 commits August 22, 2018 09:32

update according to reviews

181d73a

fix the import in example

fb4f72a

move all the test RSA keys to the test_util mod

be1f7e6

make the example server handle error connection instead of panic

d8791c8

siddontang mentioned this pull request Sep 20, 2018

NO_KEY_OPTIONS is remove from mysql8.0 #321

Open

Merge branch 'master' into master

670b057

IANTHEREAL reviewed Oct 26, 2018

View reviewed changes

michael2008 added 2 commits December 20, 2018 19:21

Merge branch 'master' into master

738bef3

change to client.Conn's func SetTLSConfig()

b08b5fa

michael2008 commented Dec 20, 2018

View reviewed changes

siddontang merged commit 8b47113 into go-mysql-org:master Dec 21, 2018

Ryan-Git mentioned this pull request Dec 30, 2018

Why use cgo? #348

Closed

shuhaowu mentioned this pull request May 19, 2020

Bump go-mysql dependency Shopify/ghostferry#187

Merged

		func (c *Conn) writeAuthHandshake() error {
		if c.authPluginName != "mysql_native_password" && c.authPluginName != "caching_sha2_password" && c.authPluginName != "sha256_password" {


		// the certificates are in go-mysql/docker/resources

		var caPem = []byte(`-----BEGIN CERTIFICATE-----


		s.db, err = sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s)/%s", testUser, testPassword, testAddr, testDB))
		s.db, err = sql.Open("mysql", fmt.Sprintf("%s:%s@tcp(%s)/%s?tls=%s", testUser, testPassword, testAddr, testDB, s.tlsPara))

Add support for MySQL 8.0 and support for TLS/SSL for both Server and Client #312

Add support for MySQL 8.0 and support for TLS/SSL for both Server and Client #312

Conversation

michael2008 commented Aug 19, 2018

siddontang commented Aug 19, 2018

siddontang commented Aug 20, 2018

michael2008 commented Aug 20, 2018

IANTHEREAL commented Aug 20, 2018

michael2008 commented Aug 20, 2018

michael2008 commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Aug 23, 2018

michael2008 commented Aug 24, 2018

IANTHEREAL commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL commented Oct 27, 2018 • edited Loading

siddontang commented Nov 3, 2018

michael2008 commented Nov 6, 2018

IANTHEREAL commented Dec 8, 2018

siddontang commented Dec 14, 2018

michael2008 left a comment

Choose a reason for hiding this comment

michael2008 commented Dec 20, 2018

IANTHEREAL commented Oct 27, 2018 •

edited

Loading