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

Support caching sha2 password #794

Merged
merged 4 commits into from May 19, 2018

Conversation

Projects
None yet
4 participants
@nakagami
Copy link
Contributor

nakagami commented May 15, 2018

Description

Support chaching_sha2_password authentication plugin. It's MySQL 8.0 default plugin.

Related Issue #785

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
@methane

This comment has been minimized.

Copy link
Contributor

methane commented May 15, 2018

Thanks for your contribution.

There is #552 which implements auth plugin support (without caching_sha2 implementation).
But author of the pull request is not active and there are some unfixed review comments.

@julienschmidt How do you think about next version? MySQL 8.0 is GA already.

  1. Don't merge this and #552 until next version is released.
  2. Merge this before next version. And add auth plugin support after next version is released.
  3. Fix #552 and implement caching_sha2 based on it, before releasing next version.

@julienschmidt julienschmidt added the bug label May 15, 2018

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

@julienschmidt

This comment has been minimized.

Copy link
Member

julienschmidt commented May 15, 2018

Thank you for this pull-request!

@methane I changed the scheduled release date for v1.4.0 to 1 week from now. We should get it out ASAP. My preference would be option 3. I'll look into it later today, to see how feasible that is. Otherwise we'll fall back to option 2. Making a release without MySQL 8 support would be a bad joke.

@@ -1842,7 +1842,7 @@ func TestSQLInjection(t *testing.T) {

dsns := []string{
dsn,
dsn + "&sql_mode='NO_BACKSLASH_ESCAPES,NO_AUTO_CREATE_USER'",

This comment has been minimized.

@julienschmidt

julienschmidt May 15, 2018

Member

is this a required change?

This comment has been minimized.

@nakagami

nakagami May 15, 2018

Contributor

NO_AUTO_CREATE_USER SQL mode seems removed at MySQL 8.0
https://dev.mysql.com/doc/refman/8.0/en/mysql-nutshell.html

My linux box show that message

--- FAIL: TestSQLInjection (0.92s)
        driver_test.go:161: error on exec CREATE TABLE test (v INTEGER): Error 1231: Variable 'sql_mode' can't be set to the value of 'NO_AUTO_CREATE_USER'

@julienschmidt julienschmidt self-assigned this May 15, 2018

@julienschmidt
Copy link
Member

julienschmidt left a comment

@methane let's do it the other way round. As we will merge this PR anyway in some form, let's merge it as it is first and then implement the general auth plugin support. The benefit would be that we hopefully already get some feedback if this breaks something before we make the release.

packets.go Outdated
plain := make([]byte, 20)
for k, v := range []byte(mc.cfg.Passwd) {
plain[k] = byte(v)
}

This comment has been minimized.

@methane

methane May 17, 2018

Contributor

When len(mc.cfg.Passwd) > 20, this code may overflow.
Could we just use copy(plain, mc.cfg.Passwd) here?

packets.go Outdated
plain[k] = byte(v)
}
for i := range plain {
plain[i] ^= cipher[i]

This comment has been minimized.

@methane

methane May 17, 2018

Contributor

len(plain) == 20, but how about len(cipher)?
Shouldn't we do cipher[i % len(cipher)]?

This comment has been minimized.

@nakagami

nakagami May 17, 2018

Contributor

This comment has been minimized.

@methane

methane May 17, 2018

Contributor

I see. Please fix only above. (len(mc.cfg.Passwd) > 20 case).

This comment has been minimized.

@nakagami

nakagami May 17, 2018

Contributor

Sorry, There is no limitation about password length (< 20) when caching_sha2_password.
Now fix it 84f6018

@methane

This comment has been minimized.

Copy link
Contributor

methane commented May 17, 2018

@nakagami Would you add MySQL 8.0 test to .travis.yml?
It may be very similar to MySQL 5.7 test.

@nakagami

This comment has been minimized.

Copy link
Contributor

nakagami commented May 17, 2018

Would you add MySQL 8.0 test to .travis.yml?

Sorry, it is difficult for me.

nakagami added some commits May 17, 2018

@julienschmidt

This comment has been minimized.

Copy link
Member

julienschmidt commented May 18, 2018

@nakagami

This comment has been minimized.

Copy link
Contributor

nakagami commented May 18, 2018

Sorry, gofmt & push now

@julienschmidt

This comment has been minimized.

Copy link
Member

julienschmidt commented May 19, 2018

@methane any other requested changes or is this PR ready for merge now?

@methane

This comment has been minimized.

Copy link
Contributor

methane commented May 19, 2018

I want to add MySQL 8 test on Travis, using caching_sha2 auth.

@julienschmidt

This comment has been minimized.

Copy link
Member

julienschmidt commented May 19, 2018

See https://github.com/go-sql-driver/mysql/tree/travis-mysql8 / https://travis-ci.org/go-sql-driver/mysql/jobs/381084434. I'll submit a separate PR which also fixes the LOAD DATA LOCAL INFILE test.

@julienschmidt julienschmidt merged commit f557730 into go-sql-driver:master May 19, 2018

2 of 3 checks passed

coverage/coveralls Coverage decreased (-1.05%) to 76.09%
Details
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@julienschmidt

This comment has been minimized.

Copy link
Member

julienschmidt commented May 19, 2018

Thanks a lot @nakagami!

arvenil added a commit to arvenil/mysql-test that referenced this pull request May 21, 2018

@doug-ess

This comment was marked as off-topic.

Copy link

doug-ess commented May 25, 2018

Are we going to get a version bump for this at some point? Last bump was in 2016.

@methane

This comment was marked as off-topic.

Copy link
Contributor

methane commented May 25, 2018

@doug-ess Please try master branch to stabilize next release. It will help us to make next version early and stable :)

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