Skip to content
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

expression:implement function password. #3275

Merged
merged 12 commits into from
Jun 2, 2017
Merged

Conversation

louishust
Copy link
Contributor

No description provided.

@@ -427,6 +427,8 @@ func (v *typeInferrer) handleFuncCallExpr(x *ast.FuncCallExpr) {
tp = x.Args[0].GetType()
case ast.RowFunc:
tp = x.Args[0].GetType()
case ast.PasswordFunc:
tp = types.NewFieldType(mysql.TypeVarString)
Copy link
Member

@shenli shenli May 16, 2017

Choose a reason for hiding this comment

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

Need to set charset: chs = v.defaultCharset.

Copy link
Member

Choose a reason for hiding this comment

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

Also need to add a test case in typeinferer_test.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@shenli
Copy link
Member

shenli commented May 16, 2017

LGTM
@XuHuaiyu @tiancaiamao PTAL

@zimulala zimulala added the contribution This PR is from a community contributor. label May 16, 2017
@louishust
Copy link
Contributor Author

@XuHuaiyu @tiancaiamao PTAL

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Rest LGTM

return d, nil
}

// Two stage SHA1 hash of the password
Copy link
Contributor

Choose a reason for hiding this comment

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

add . at the end of this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/stage/stages

@tiancaiamao
Copy link
Contributor

I see this Note in the document:

This function is deprecated as of MySQL 5.7.6 and will be removed in a future MySQL release.

Should TiDB implement it? @shenli

@louishust
Copy link
Contributor Author

@tiancaiamao @shenli I think password is a useful function

@shenli
Copy link
Member

shenli commented May 16, 2017

@tiancaiamao We need this. Some DBAs still use this to generate password.

@tiancaiamao
Copy link
Contributor

But set password for 'user'@'host' = PASSWORD('xxx') will not call this function,
and the algorithm used by TiDB to generate password is not compatible with this method.
@shenli

@louishust
Copy link
Contributor Author

@tiancaiamao @shenli
It seems that the TIDB store password not in MySQL way!
So if user migrate data from MySQL using mysqldump, the user's password is changed?

@tiancaiamao
Copy link
Contributor

For some historical reasons, TiDB store password not in MySQL way.
Yes, migrate from the mysql.user talbe would have some problem.

The risk of this PR is that, if user change password in this way, he can't login any more:

update mysql.user set password=PASSWORD('xxx') where user='xxx' and host='xxx';

I'll upgrade TiDB to make the stored data compatible with MySQL, so password function
in this PR can be used.

Please wait for while, thank you for your patience! @louishust

@louishust
Copy link
Contributor Author

@tiancaiamao I'd like to upgrade TiDB to make the stored data compatible with MySQL

@tiancaiamao
Copy link
Contributor

Nice! Feel fun to have a try.

Basically, you would follow those steps:

  1. Add function upgradeToVer10 https://github.com/pingcap/tidb/blob/master/bootstrap.go#L367, which update password to the new format.
  2. Change util.EncodePassword, it should do Sha1Hash([]byte(pwd)) twice. https://github.com/pingcap/tidb/blob/master/util/auth.go#L53
    change util.CalcPassword, remove this line https://github.com/pingcap/tidb/blob/master/util/auth.go#L32
  3. Add test

@louishust
Copy link
Contributor Author

@tiancaiamao upgradeToVer10 used to upgrade the old format password stored in TIDB to MySQL format? It seems can not upgrade using simple UPDATE statement cause I have to decode the old format to []byte and then SHA1 the []byte. Add an internal function upgrade_password for the upgrade? Or some other ways?

@tiancaiamao
Copy link
Contributor

upgradeToVer10 used to upgrade the old format password stored in TIDB to MySQL format?

Yes, you're right.

It seems can not upgrade using simple UPDATE statement cause I have to decode the old format to []byte and then SHA1 the []byte.

Maybe, but you can have a try.

We don't store the raw password, the stored password in mysql.user currently is sha1(password).
it impossible to get the raw password, but... the upgrade just need to change
sha1(password) to sha1(sha1(password)), i.e. you don't need the raw password, just another sha1 on it.

Add an internal function upgrade_password for the upgrade?

Exactly.

Read the fucking source code would help, including
Auth() in session.go,
util/auth.go,
readHandshakeResponse() in server/conn.go which call Auth(),
ConnectionVerification() in privilege/privileges.go.

Here is the password check procedure (maybe miss information in detail):

  1. client connect to server
  2. server send handshake packet to client, within the packet there is a random salt
  3. client send salt+sha1(sha1(password)) to server
  4. server call Auth() function, check salt+sha1(sha1pwd) is the same.

@louishust
Copy link
Contributor Author

this issue is blocked by #3292 , after #3292 is merged,
this issue can just use the EncodePassword function in #3292

@tiancaiamao tiancaiamao removed the DNM label Jun 1, 2017
@tiancaiamao
Copy link
Contributor

Please resolve conflict.

@louishust
Copy link
Contributor Author

@tiancaiamao OK

@tiancaiamao
Copy link
Contributor

LGTM @XuHuaiyu @shenli

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 2, 2017
@shenli
Copy link
Member

shenli commented Jun 2, 2017

LGTM
@louishust Thanks!

@shenli shenli merged commit e294500 into pingcap:master Jun 2, 2017
@louishust louishust deleted the func_password branch June 2, 2017 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants