Auth refactoring and bug fixes #807

julienschmidt · 2018-05-24T16:17:39Z

Description

This PR refactors the existing auth code and separates it from other code (currently the auth code is spread over driver.go, utils.go and packets.go. This refactoring also serves as a preparation for adding more auth plugins, such as sha256_password (#625) or dialog (#803) and an exported interface for adding custom auth plugins, as proposed in #552.

It further fixes many bugs: the following new tests fail when backported to the old code (see https://travis-ci.org/go-sql-driver/mysql/jobs/383270883):

--- FAIL: TestAuthFastCachingSHA256PasswordEmpty (0.00s)
	auth_test.go:118: got error: malformed packet
--- FAIL: TestAuthFastCleartextPasswordNotAllowed (0.00s)
	auth_test.go:235: expected ErrCleartextPassword, got unknown authentication plugin name 'mysql_clear_password'
--- FAIL: TestAuthFastCleartextPassword (0.00s)
	auth_test.go:251: unknown authentication plugin name 'mysql_clear_password'
--- FAIL: TestAuthFastCleartextPasswordEmpty (0.00s)
	auth_test.go:289: unknown authentication plugin name 'mysql_clear_password'
--- FAIL: TestAuthFastNativePasswordNotAllowed (0.00s)
	auth_test.go:328: expected ErrNativePassword, got <nil>
--- FAIL: TestAuthSwitchCachingSHA256PasswordCached (0.00s)
	auth_test.go:428: got error: this authentication plugin is not supported
	auth_test.go:438: got unexpected data: []
--- FAIL: TestAuthSwitchCachingSHA256PasswordEmpty (0.00s)
	auth_test.go:461: got error: this authentication plugin is not supported
	auth_test.go:466: got unexpected data: []
--- FAIL: TestAuthSwitchCachingSHA256PasswordFullRSA (0.00s)
	auth_test.go:497: got error: this authentication plugin is not supported
	auth_test.go:513: got unexpected data: []
--- FAIL: TestAuthSwitchCachingSHA256PasswordFullSecure (0.00s)
	auth_test.go:543: got error: this authentication plugin is not supported
	auth_test.go:556: got unexpected data: []

This PR is partially based on the work done in #552.

Fixes #806

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

Updates #795

julienschmidt · 2018-05-26T09:28:38Z

@arnehormann @methane I know this is a huge changeset to review, but it would be great if you would find time to do so soon. Much of it is just moved code and very similar tests anyway. This PR and the follow-up PR #808 should make the auth system much more stable and fix several currently existing bugs.
These two PRs are the only remaining planned changes before the v1.4.0 release (except the changelog itself).

julienschmidt · 2018-05-26T09:39:57Z

auth.go

+	return message1
+}
+
+func (mc *mysqlConn) auth(authData []byte, plugin string) ([]byte, bool, error) {


The actual changes start here. The code above was moved from utils.go

methane · 2018-05-29T11:33:20Z

packets.go

-		return readAuthSwitch(data)
+		if len(data) > 1 {
+			pluginEndIndex := bytes.IndexByte(data, 0x00)
+			plugin := string(data[1:pluginEndIndex])


if pluginEndIndex < 0 { return nil, "", errors.New("invalid AuthSwitchRequest packet") }

methane · 2018-05-29T11:33:20Z

packets.go

-		copy(b[:], cipher)
-		return b[:], pluginName, nil
+		copy(b[:], authData)
+		return b[:], plugin, nil


b instead of b[:]. (While old code was same...)

copy requires slices as parameters but b is an array and not a slice.

arvenil · 2018-05-29T12:19:20Z

Just FYI. I've used master and this PR in one of our repos and some tests on master failed with malformed packet but this PR seems to fix the issues and now tests pass. I didn't dig what really in this PR fixed it but clearly for us it works ;) Thanks for great work.

* log missing auth plugin name * refactor auth handling * auth: fix AllowNativePasswords * auth: remove plugin name print * packets: attempt to fix writePublicKeyAuthPacket * packets: do not NUL-terminate auth switch packets * move handleAuthResult to auth * add old_password auth tests * auth: add empty old_password test * auth: add cleartext auth tests * auth: add native auth tests * auth: add caching_sha2 tests * rename init and auth packets to documented names * auth: fix plugin name for switched auth methods * buffer: optimize default branches * auth: add tests for switch to caching sha2 * auth: add tests for switch to cleartext password * auth: add tests for switch to native password * auth: sync NUL termination with official connectors * packets: handle missing NUL bytes in AuthSwitchRequests Updates #795

julienschmidt added 18 commits May 21, 2018

log missing auth plugin name

d6fe90f

Updates #795

refactor auth handling

36c70eb

auth: fix AllowNativePasswords

cf52770

auth: remove plugin name print

f3b65ae

packets: attempt to fix writePublicKeyAuthPacket

eb54ab9

packets: do not NUL-terminate auth switch packets

86adb9b

move handleAuthResult to auth

ceb1236

add old_password auth tests

fa931ce

auth: add empty old_password test

66f8d59

auth: add cleartext auth tests

de51fdd

auth: add native auth tests

Loading status checks…

d0d482d

auth: add caching_sha2 tests

Loading status checks…

9be743a

rename init and auth packets to documented names

Loading status checks…

7242d1e

auth: fix plugin name for switched auth methods

d09ac83

buffer: optimize default branches

fba0ad9

auth: add tests for switch to caching sha2

Loading status checks…

f252087

auth: add tests for switch to cleartext password

a27458c

auth: add tests for switch to native password

Loading status checks…

caae030

julienschmidt added bug enhancement labels May 24, 2018

julienschmidt added this to the v1.4.0 milestone May 24, 2018

julienschmidt requested a review from methane May 24, 2018

julienschmidt mentioned this pull request May 24, 2018

Missing plugin name leads to panic #806

Closed

auth: sync NUL termination with official connectors

Loading status checks…

8985303

julienschmidt force-pushed the auth_refactoring branch from 0dcbff2 to 8985303 May 25, 2018

julienschmidt mentioned this pull request May 25, 2018

sha256_password auth support #808

Merged

5 of 5 tasks complete

julienschmidt changed the title ~~Auth refactoring~~ Auth refactoring and bug fixes May 25, 2018

julienschmidt reviewed May 26, 2018

View changes

auth.go

return message1

}

func (mc *mysqlConn) auth(authData []byte, plugin string) ([]byte, bool, error) {

This comment has been minimized.

Sign in to view

julienschmidt May 26, 2018
Author Member

The actual changes start here. The code above was moved from utils.go

julienschmidt mentioned this pull request May 27, 2018

Release v1.4.0 #809

Merged

2 of 2 tasks complete

methane requested changes May 29, 2018

View changes

packets: handle missing NUL bytes in AuthSwitchRequests

Loading status checks…

2846c61

methane approved these changes May 29, 2018

View changes

zwpaper mentioned this pull request May 30, 2018

github.com/go-sql-driver/mysql auth update leads to panic go-gorm/gorm#1922

Closed

julienschmidt deleted the auth_refactoring branch Oct 2, 2018

This was referenced Mar 8, 2019

API for adding custom auth plugins #931

Open

add support for authentication plugins. #552

Closed

go-sql-driver / mysql

Auth refactoring and bug fixes #807

Auth refactoring and bug fixes #807

julienschmidt commented May 24, 2018 •

edited

julienschmidt commented May 26, 2018 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

arvenil commented May 29, 2018

go-sql-driver / mysql

Auth refactoring and bug fixes #807

Auth refactoring and bug fixes #807

Conversation

julienschmidt commented May 24, 2018 • edited

Description

Checklist

julienschmidt commented May 26, 2018 • edited

This comment has been minimized.

julienschmidt May 26, 2018 Author Member

This comment has been minimized.

methane May 29, 2018 Member

This comment has been minimized.

julienschmidt May 29, 2018 Author Member

This comment has been minimized.

methane May 29, 2018 Member

This comment has been minimized.

julienschmidt May 29, 2018 Author Member

arvenil commented May 29, 2018

julienschmidt commented May 24, 2018 •

edited

julienschmidt commented May 26, 2018 •

edited

julienschmidt May 26, 2018
Author Member

methane May 29, 2018
Member

julienschmidt May 29, 2018
Author Member

methane May 29, 2018
Member

julienschmidt May 29, 2018
Author Member