privilege, session, server: consistently map user login to identity (#30204) #30450

ti-srebot · 2021-12-06T16:26:11Z

cherry-pick #30204 to release-5.3
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/30450

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/tidb.git pr/30450:release-5.3-7fc6ebbda4dd

What problem does this PR solve?

Problem Summary:

TiDB server uses different rules to map a user to an identity depending on the code area. This cleans up how identity is mapped using a consistent function: MatchIdentity().

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility
The rules might be slightly different in the event that there are two 'root' users with different hosts (not a common practice, but it's possible users are doing this). This PR follows more consistent behavior with MySQL.

Documentation

Release note

The TiDB server now maps a user to an entry in the mysql.user table more consistently.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-chi-bot · 2021-12-06T16:26:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

dveeden
tiancaiamao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-srebot · 2021-12-06T16:26:14Z

/run-all-tests

ti-srebot · 2021-12-06T16:26:20Z

@morgo you're already a collaborator in bot's repo.

morgo · 2021-12-07T17:51:16Z

The merge for this was unfortunately complicated. There are a lot of changes in master that are related, so I had to merge some in that were not in the PR on master.

morgo · 2021-12-07T17:52:22Z

server/conn.go

 		case mysql.AuthCachingSha2Password:
-			resp.Auth, err = cc.authSha(ctx)
-			if err != nil {
-				return err
-			}
 		case mysql.AuthNativePassword:


This is intentional. It was removed from master, I believe because this is handled in cc.checkAuthPlugin instead. The code could be cleaned up slightly, but that's for another PR.

morgo · 2021-12-07T17:53:18Z

server/conn.go

-		if cc.ctx == nil {
-			err := cc.openSession()
-			if err != nil {
-				return err
-			}
-		}
-		userplugin, err := cc.ctx.AuthPluginForUser(&auth.UserIdentity{Username: cc.user, Hostname: cc.peerHost})
+		_, err := cc.checkAuthPlugin(ctx, resp)
 		if err != nil {
 			return err
 		}
-		if userplugin != mysql.AuthNativePassword && userplugin != "" {
-			return errNotSupportedAuthMode
-		}


These changes are also based on master, and checkAuthPlugin does these checks.

dveeden · 2021-12-17T08:24:10Z

/run-check_dev

dveeden

Tested with https://github.com/dveeden/tidb_client_test:

TiDB Client Test
Connected to:
	Release Version: v5.3.0-4-g635c0bab4
	Edition: Community
	Git Commit Hash: 635c0bab4eb54a801b0f3f12577b8b25bb0e5431
	Git Branch: release-5.3-7fc6ebbda4dd
	UTC Build Time: 2021-12-20 15:04:40
	GoVersion: go1.16.11
	Race Enabled: false
	TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
	Check Table Before Drop: false
Clients:
	/home/dvaneeden/opt/mysql/5.1.73/bin/mysql
	/home/dvaneeden/opt/mysql/5.7.31/bin/mysql
	/home/dvaneeden/opt/mysql/5.7.36/bin/mysql
	/home/dvaneeden/opt/mysql/8.0.22/bin/mysql
	/home/dvaneeden/opt/mysql/8.0.25/bin/mysql
	/home/dvaneeden/opt/mysql/8.0.26/bin/mysql
	/home/dvaneeden/opt/mysql/8.0.27/bin/mysql
-----------------------------------------------
Testing with mysql_native_password as default authentication plugin
Testing /home/dvaneeden/opt/mysql/5.1.73/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/5.7.31/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/5.7.36/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.22/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.25/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.26/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.27/bin/mysql: success=8, failures=0
Testing with caching_sha2_password as default authentication plugin
Testing /home/dvaneeden/opt/mysql/5.1.73/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/5.7.31/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/5.7.36/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.22/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.25/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.26/bin/mysql: success=8, failures=0
Testing /home/dvaneeden/opt/mysql/8.0.27/bin/mysql: success=8, failures=0

guo-shaoge · 2022-02-14T08:30:46Z

/run-check_dev
/run-mysql-test

guo-shaoge · 2022-02-14T08:35:13Z

Hi @morgo , v5.3 is about to release a new version in a few days, maybe we can continue to merge this pr. Thanks!

morgo · 2022-02-15T00:58:41Z

Hi @morgo , v5.3 is about to release a new version in a few days, maybe we can continue to merge this pr. Thanks!

Yes, it must be merged. It is release blocking according to our criteria.

morgo · 2022-02-15T00:59:36Z

/run-check_dev
/run-mysql-test

morgo · 2022-02-18T03:22:15Z

/run-mysql-test tidb-test=pr/1583

morgo · 2022-02-19T01:58:24Z

/run-mysql-test tidb-test=pr/1583

morgo · 2022-02-19T02:28:23Z

The mysql-test itself seems to be broken. I've tested the changes locally in mysql-test and it passed.

morgo · 2022-02-19T02:28:35Z

/run-mysql-test tidb-test=pr/1583

purelind · 2022-02-21T03:10:20Z

/rebuild

s3nt3 · 2022-02-21T03:15:03Z

/rebuild

purelind · 2022-02-21T03:17:20Z

/run-mysql-test

purelind · 2022-02-21T03:20:57Z

/run-mysql-test tidb-test=pr/1583

guo-shaoge · 2022-02-21T05:30:00Z

/merge

ti-chi-bot · 2022-02-21T05:30:04Z

This pull request has been accepted and is ready to merge.

Commit hash: 635c0ba

s3nt3 · 2022-02-21T05:38:40Z

/run-mysql-test tidb-test=pr/1583

s3nt3 · 2022-02-21T06:55:07Z

/rebuild

cherry pick pingcap#30204 to release-5.3

be6c044

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-chi-bot added the do-not-merge/cherry-pick-not-approved label Dec 6, 2021

ti-chi-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 6, 2021

ti-srebot mentioned this pull request Dec 6, 2021

privilege, session, server: consistently map user login to identity #30204

Merged

12 tasks

ti-chi-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Dec 6, 2021

ti-srebot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/5.3-cherry-pick labels Dec 6, 2021

ti-srebot requested review from dveeden and tiancaiamao December 6, 2021 16:26

ti-srebot assigned morgo Dec 6, 2021

Merge related changes

635c0ba

morgo reviewed Dec 7, 2021

View reviewed changes

dveeden approved these changes Dec 20, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 20, 2021

tiancaiamao approved these changes Feb 16, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 16, 2022

zhouqiang-cl added the cherry-pick-approved Cherry pick PR approved by release team. label Feb 16, 2022

ti-chi-bot removed the do-not-merge/cherry-pick-not-approved label Feb 16, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 21, 2022

Merge branch 'release-5.3' into release-5.3-7fc6ebbda4dd

e7e012b

ti-chi-bot merged commit 27ffd11 into pingcap:release-5.3 Feb 21, 2022

purelind added this to the v5.3.1 milestone Feb 24, 2022

This was referenced Feb 25, 2022

releases: add tidb 5.3.1 release notes pingcap/docs#7699

Merged

server: Combined fix for authentication issues (#29738) #30233

Closed

server: fix bug https://asktug.com/t/topic/213082/11 (#29577) #30047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

privilege, session, server: consistently map user login to identity (#30204) #30450

privilege, session, server: consistently map user login to identity (#30204) #30450

ti-srebot commented Dec 6, 2021 •

edited

Loading

ti-chi-bot commented Dec 6, 2021 •

edited

Loading

ti-srebot commented Dec 6, 2021

ti-srebot commented Dec 6, 2021

morgo commented Dec 7, 2021

morgo Dec 7, 2021

morgo Dec 7, 2021

dveeden commented Dec 17, 2021

dveeden left a comment

guo-shaoge commented Feb 14, 2022

guo-shaoge commented Feb 14, 2022

morgo commented Feb 15, 2022

morgo commented Feb 15, 2022

morgo commented Feb 18, 2022

morgo commented Feb 19, 2022

morgo commented Feb 19, 2022

morgo commented Feb 19, 2022

purelind commented Feb 21, 2022

s3nt3 commented Feb 21, 2022

purelind commented Feb 21, 2022

purelind commented Feb 21, 2022

guo-shaoge commented Feb 21, 2022

ti-chi-bot commented Feb 21, 2022

s3nt3 commented Feb 21, 2022

s3nt3 commented Feb 21, 2022

privilege, session, server: consistently map user login to identity (#30204) #30450

privilege, session, server: consistently map user login to identity (#30204) #30450

Conversation

ti-srebot commented Dec 6, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 6, 2021 • edited Loading

ti-srebot commented Dec 6, 2021

ti-srebot commented Dec 6, 2021

morgo commented Dec 7, 2021

morgo Dec 7, 2021

Choose a reason for hiding this comment

morgo Dec 7, 2021

Choose a reason for hiding this comment

dveeden commented Dec 17, 2021

dveeden left a comment

Choose a reason for hiding this comment

guo-shaoge commented Feb 14, 2022

guo-shaoge commented Feb 14, 2022

morgo commented Feb 15, 2022

morgo commented Feb 15, 2022

morgo commented Feb 18, 2022

morgo commented Feb 19, 2022

morgo commented Feb 19, 2022

morgo commented Feb 19, 2022

purelind commented Feb 21, 2022

s3nt3 commented Feb 21, 2022

purelind commented Feb 21, 2022

purelind commented Feb 21, 2022

guo-shaoge commented Feb 21, 2022

ti-chi-bot commented Feb 21, 2022

s3nt3 commented Feb 21, 2022

s3nt3 commented Feb 21, 2022

ti-srebot commented Dec 6, 2021 •

edited

Loading

ti-chi-bot commented Dec 6, 2021 •

edited

Loading