privileges: Upgrade stored password format to Compatible with MySQL. #3292

louishust · 2017-05-18T03:54:23Z

Password stored in mysql.user of TIDB is the format of SHA1(pwd),
which is not compatible with MySQL.
Different password format will cause user migrate data from MySQL
login failed on TIDB.

Password stored in mysql.user of TIDB is the format of SHA1(pwd), which is not compatible with MySQL. Different password format will cause user migrate data from MySQL login failed on TIDB.

shenli · 2017-05-20T08:34:34Z

@tiancaiamao PTAL Please test updating an exist cluster with this branch.

tiancaiamao · 2017-05-20T13:28:32Z

bootstrap.go

@@ -376,6 +381,13 @@ func upgradeToVer9(s Session) {
 	s.Execute("UPDATE mysql.user SET Trigger_priv='Y'")
 }

+func upgradeToVer10(s Session) {
+	_, err := s.Execute("UPDATE mysql.user SET `password` = old_password_upgrade(`password`)")


I don't think we need introduce the old_password_upgrade builtin function.
This is not a standard MySQL function, define it as a local function or put it to util package is enough, no need to implement as builtin.

if use local function, password can not be updated in one UPDATE statement, we should fetch rows from mysql.user and update each rows.

You can begin; select; update; commit

ok, i'll try that way

tiancaiamao · 2017-05-20T13:36:28Z

expression/builtin_encryption.go

+}
+
+// eval evals an upgrade from old password format stored in TIDB to MySQL format
+func (b *builtinOldPasswordUpgradeSig) eval(row []types.Datum) (d types.Datum, err error) {


You can put it to util package, or bootstarp.go in which it's used.

tiancaiamao · 2017-05-20T13:37:37Z

expression/builtin_encryption.go

+
+	hash2 := util.Sha1Hash(hash1)
+	d.SetString(fmt.Sprintf("*%X", hash2))
+	log.Infof("old password: %v new password:%v", pass, fmt.Sprintf("*%X", hash2))


Don't do that, this log will leak user's information!

tiancaiamao · 2017-05-20T13:53:48Z

privilege/privileges/privileges.go

 		log.Errorf("User [%s] password from SystemDB not like a sha1sum", user)
 		return false
 	}
+
+	// empty password
+	if len(pwd) == 0 && len(auth) == 0 {


When will len(auth) == 0 ?

when password is empty

tiancaiamao · 2017-05-20T13:54:23Z

privilege/privileges/privileges.go

+		return true
+	}
+
+	if len(pwd) == 0 || len(auth) == 0 {


Is there any test cover this change ?

no test cover this

add test case done

tiancaiamao · 2017-05-20T14:01:53Z

util/auth.go


 	"github.com/juju/errors"
 )

 // CalcPassword is the algorithm convert hashed password to auth string.
 // See https://dev.mysql.com/doc/internals/en/secure-password-authentication.html
 // SHA1( password ) XOR SHA1( "20-bytes random data from server" <concat> SHA1( SHA1( password ) ) )
-func CalcPassword(scramble, sha1pwd []byte) []byte {
-	if len(sha1pwd) == 0 {


why remove this check?

in ConnectionVerification have handled this situation

tiancaiamao · 2017-05-20T14:16:41Z

util/auth.go

 	// token = scrambleHash XOR stage1Hash
-	for i := range scramble {
-		scramble[i] ^= sha1pwd[i]


I'm confused with this function and the added auth parameter, could you explain it a bit ?

I transfer the check logic from mysql source code, you can find the logic in function check_scramble in the file sql/password.c.

The new authentication is performed in following manner: SERVER: public_seed=create_random_string() send(public_seed) CLIENT: recv(public_seed) hash_stage1=sha1("password") hash_stage2=sha1(hash_stage1) reply=xor(hash_stage1, sha1(public_seed,hash_stage2) // this three steps are done in scramble() send(reply) SERVER: recv(reply) hash_stage1=xor(reply, sha1(public_seed,hash_stage2)) candidate_hash2=sha1(hash_stage1) check(candidate_hash2==hash_stage2) // this three steps are done in check_scramble()

We need to update the comment;
And CalcPassword is actually check_scramble now? Maybe we need chose a better function name for it.

change the function name to CheckScramble

Password stored in mysql.user of TIDB is the format of SHA1(pwd), which is not compatible with MySQL. Different password format will cause user migrate data from MySQL login failed on TIDB.

tiancaiamao · 2017-05-21T14:34:24Z

bootstrap.go

@@ -382,9 +383,37 @@ func upgradeToVer9(s Session) {
 }

 func upgradeToVer10(s Session) {
-	_, err := s.Execute("UPDATE mysql.user SET `password` = old_password_upgrade(`password`)")
+	sql := "SELECT user, host, password FROM mysql.user WHERE password != ''"


s.Execute("begin")

Do we need select for update? @shenli

tiancaiamao · 2017-05-21T14:34:58Z

bootstrap.go

+	}
+
+	for _, sql := range sqls {
+		mustExecute(s, sql)
 	}


mustExecute(s, "commit")

tiancaiamao · 2017-05-21T14:44:23Z

util/auth.go

 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 	return x, nil
 }
+
+// OldPasswordUpgrade upgrade password to MySQL compatible format
+func OldPasswordUpgrade(pass string) (string, error) {


Is there any test that cover this new added function?

Add test case done

louishust

update

louishust · 2017-05-22T09:30:01Z

privilege/privileges/privileges.go

+		return true
+	}
+
+	if len(pwd) == 0 || len(auth) == 0 {


add test case done

tiancaiamao · 2017-05-22T09:50:35Z

util/auth.go

+//            candidate_hash2=sha1(hash_stage1)
+//            check(candidate_hash2==hash_stage2)
+//            // this three steps are done in check_scramble()
+func CheckScramble(scramble, hpwd, auth []byte) []byte {


Check inside it and let this function return bool, CheckXXX return []byte is strange.

Already fixed!

tiancaiamao · 2017-05-22T10:04:25Z

bootstrap.go

+	for _, sql := range sqls {
+		mustExecute(s, sql)
+	}
+


Add updateBootstrapVer(s) here.
This upgrade is not reentrant, right? So it must be success/fail along with bootstrap version change.

Rest LGTM
PTAL @shenli

Add updateBootstrapVer(s) in upgradeToVer11 is not a good choice,
cause updateBootstrapVer update tidb to currentBootstrapVersion instead of Version11,
we should update to version11, so i'd like to add the following code:

sql := fmt.Sprintf(`INSERT INTO %s.%s VALUES ("%s", "%d", "TiDB bootstrap version.") ON DUPLICATE KEY UPDATE VARIABLE_VALUE="%d"`, mysql.SystemDB, mysql.TiDBTable, tidbServerVersionVar, version11, version11) mustExecute(s, sql)

Yes, you're right!

louishust · 2017-05-22T13:54:10Z

@tiancaiamao what's wrong with the check:

license/cla — Waiting for status to be reported

last for long time this status...

tiancaiamao · 2017-05-23T06:02:21Z

LGTM
PTAL @shenli @zimulala @hanfei1991

tiancaiamao · 2017-05-25T09:28:42Z

PTAL @shenli

Conflicts: bootstrap.go

louishust · 2017-05-26T12:48:49Z

@shenli PTAL

tiancaiamao · 2017-05-31T02:18:11Z

PTAL @coocood @shenli

coocood · 2017-05-31T03:17:34Z

util/auth.go

+//            candidate_hash2=sha1(hash_stage1)
+//            check(candidate_hash2==hash_stage2)
+//            // this three steps are done in check_scramble()
+func CheckScramble(scramble, hpwd, auth []byte) bool {


Rename to CheckScrambledPassword is more clear.
And I think the first argument should be salt instead of scramble.

Yes, i will modify it.

coocood · 2017-05-31T03:25:23Z

bootstrap.go

+		user := row.Data[0].GetString()
+		host := row.Data[1].GetString()
+		pass := row.Data[2].GetString()
+		newpass, err := util.OldPasswordUpgrade(pass)


I think move OldPasswordUpgrade implementation here is better, because this function is only used here and nowhere else.

i think leave this function in util will make bootstrap concise although it is just used in bootstrap.

Semantically upgrade operation doesn't belong to util.
I think this need to be considered with higher priority.

ok， i will move the function to bootstrap.

Password stored in mysql.user of TIDB is the format of SHA1(pwd), which is not compatible with MySQL. Different password format will cause user migrate data from MySQL login failed on TIDB.

coocood · 2017-05-31T11:16:34Z

LGTM

tiancaiamao · 2017-05-31T11:25:39Z

bootstrap.go

+		return
+	}
+	r := rs[0]
+	sqls := make([]string, 1)


sqls := make([]string, 0, 1)

tiancaiamao · 2017-06-01T01:38:18Z

Good job! Thanks for your contribution! @louishust

privileges: Upgrade stored password format to Compatible with MySQL.

152ec1f

Password stored in mysql.user of TIDB is the format of SHA1(pwd), which is not compatible with MySQL. Different password format will cause user migrate data from MySQL login failed on TIDB.

tiancaiamao reviewed May 20, 2017

View reviewed changes

louishust added 2 commits May 21, 2017 15:10

privileges: Upgrade stored password format to Compatible with MySQL.

ead3915

Password stored in mysql.user of TIDB is the format of SHA1(pwd), which is not compatible with MySQL. Different password format will cause user migrate data from MySQL login failed on TIDB.

Merge branch 'master' into auth

d38ae41

tiancaiamao reviewed May 21, 2017

View reviewed changes

louishust added 3 commits May 22, 2017 16:33

postfix

418fce7

Merge remote-tracking branch 'origin/auth' into auth

4edc88d

postfix

cf43996

louishust commented May 22, 2017

View reviewed changes

privilege/privileges/privileges.go

return true

}

if len(pwd) == 0 || len(auth) == 0 {

Copy link

Contributor Author

louishust May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test case done

adjust CheckScramble function

c7a6ae9

tiancaiamao reviewed May 22, 2017

View reviewed changes

add version update for version11

26576c2

Merge branch 'master' into auth

e3a798f

louishust added 3 commits May 23, 2017 17:21

Merge remote-tracking branch 'upstream/master' into auth

f337103

Merge remote-tracking branch 'upstream/master' into auth

0241362

Merge remote-tracking branch 'upstream/master' into auth

a476363

XuHuaiyu added status/LGT1 Indicates that a PR has LGTM 1. contribution This PR is from a community contributor. labels May 24, 2017

louishust added 4 commits May 24, 2017 17:08

Merge remote-tracking branch 'upstream/master' into auth

9e8a5b0

Merge remote-tracking branch 'upstream/master' into auth

d8d4adc

Merge remote-tracking branch 'upstream/master' into auth

547170b

Merge remote-tracking branch 'upstream/master' into auth

8af304c

louishust mentioned this pull request May 26, 2017

expression:implement function password. #3275

Merged

louishust added 2 commits May 26, 2017 14:02

Merge remote-tracking branch 'upstream/master' into auth

a230c74

Merge remote-tracking branch 'upstream/master' into auth

0fa0bab

Conflicts: bootstrap.go

Merge remote-tracking branch 'upstream/master' into auth

f9bb19a

coocood reviewed May 31, 2017

View reviewed changes

louishust added 4 commits May 31, 2017 11:49

privileges: Upgrade stored password format to Compatible with MySQL.

d2d96ea

Password stored in mysql.user of TIDB is the format of SHA1(pwd), which is not compatible with MySQL. Different password format will cause user migrate data from MySQL login failed on TIDB.

Merge remote-tracking branch 'upstream/master' into auth

a93a55c

Move oldpasswordupgrade to bootstrap.go

fdc9621

Merge remote-tracking branch 'upstream/master' into auth

29396cd

coocood approved these changes May 31, 2017

View reviewed changes

tiancaiamao reviewed May 31, 2017

View reviewed changes

bootstrap.go Outdated

return

}

r := rs[0]

sqls := make([]string, 1)

Copy link

Contributor

tiancaiamao May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sqls := make([]string, 0, 1)

louishust added 2 commits May 31, 2017 22:03

postfix

67c0d39

Merge remote-tracking branch 'upstream/master' into auth

53f1a13

tiancaiamao merged commit f453b19 into pingcap:master Jun 1, 2017

louishust deleted the auth branch July 15, 2017 07:29

privileges: Upgrade stored password format to Compatible with MySQL. #3292

privileges: Upgrade stored password format to Compatible with MySQL. #3292

Conversation

louishust commented May 18, 2017

shenli commented May 20, 2017

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

louishust May 22, 2017 • edited Loading

Choose a reason for hiding this comment

louishust left a 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

louishust commented May 22, 2017

tiancaiamao commented May 23, 2017

tiancaiamao commented May 25, 2017

louishust commented May 26, 2017

tiancaiamao commented May 31, 2017

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

coocood commented May 31, 2017

Choose a reason for hiding this comment

tiancaiamao commented Jun 1, 2017

louishust May 22, 2017 •

edited

Loading