*: create a user using `tidb_auth_token` authentication by CbcWestwolf · Pull Request #38585 · pingcap/tidb

CbcWestwolf · 2022-10-21T01:48:23Z

What problem does this PR solve?

Issue Number: ref #38504

Problem Summary:

What is changed and how it works?

This PR supports creating and altering users with tidb_auth_token. Since the claims in the JWT differ in different scenarios, and more verified claims may be added in the future, this PR introduces a new keyword TOKEN_ISSUER, to declare an additional verified claim.

If creating the tidb_auth_token user with REQUIRE token_issuer '<issuer-abc>', then the authentication of this user requires that the JWT should have a claim "iss": "<issuer-abc>" in the payload. If not exists, the authentication fails.

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

1. `create user` using a new authentication way `tidb_auth_token`
2. add a new column `token_issuer` to `mysql.user`

ti-chi-bot · 2022-10-21T01:48:25Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bb7133
xhebox

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.

Details

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

djshow832

I'm not sure if the update of mysql.user and SQL statements may affect DP tools and other clients. Please confirm with the DP team.

djshow832 · 2022-10-26T06:53:21Z

 	tk.MustExec(dropUserSQL)
+	tk.MustExec(`CREATE USER token_user IDENTIFIED WITH 'tidb_auth_token' TOKEN_REQUIRE token_issuer 'issuer-abc'`)
+	tk.MustQuery(`SELECT plugin, token_issuer FROM mysql.user WHERE user = 'token_user'`).Check(testkit.Rows("tidb_auth_token issuer-abc"))
+	tk.MustExec(`ALTER USER token_user TOKEN_REQUIRE token_issuer 'issuer-123'`)


Is it allowed to specify token_issuer for other auth plugins in create user or alter user statements?

Yes, the specified token_issuer is stored into mysql.user for all users. But it would not affect the authentication of the other auth plugin users

I mean, if a user is identified with mysql_native_password, and the root user alters his token_issuer, this is meaningless. Should we report an error?

I think we can give a warning when create user/alter user meet such a scenario.

djshow832

I still have no idea what token_issuer is used for. It's not even specified in the spec. Can you explain it in the PR description?

djshow832

BTW, please invite @bb7133 to review because he's in the tidb-configuration-reviewers. This PR needs at least one of the tidb-configuration-reviewers to review.

djshow832 · 2022-10-28T06:56:50Z

 	dropUserSQL = `DROP USER IF EXISTS 'test1'@'localhost' ;`
 	tk.MustExec(dropUserSQL)

+	// Test create/alter user with `tidb_auth_token`


Test that alter something else of the user (such as username) when the auth plugin is tidb_auth_token. The expectation is that it doesn't need to specify the token_issuer because you just want to change the username.

Test that alter auth plugin also works.

djshow832 · 2022-10-28T06:59:13Z

 			Value: $2,
 		}
 	}
+|	"TOKEN_ISSUER" stringLit


Did you change the syntax again? I thought you have already discussed this with the PM before filing this PR. Please add this syntax in the spec and get the PM notified by @ him in the spec.

Yes, I removed the keyword TOKEN_REQUIRE. I'll notify the PM in the spec.

Co-authored-by: djshow832 <zhangming@pingcap.com>

CbcWestwolf · 2022-10-28T07:40:30Z

/cc bb7133

bb7133 · 2022-10-28T22:29:39Z

+	tk.MustQuery(`SELECT plugin, token_issuer FROM mysql.user WHERE user = 'token_user'`).Check(testkit.Rows("tidb_auth_token issuer-123"))
+	tk.MustExec(`ALTER USER token_user IDENTIFIED WITH 'tidb_auth_token'`)
+	tk.MustExec(`CREATE USER token_user1 IDENTIFIED WITH 'tidb_auth_token'`)
+	tk.MustQuery(`show warnings`).Check(testkit.RowsWithSep("|", "Warning|1105|TOKEN_ISSUER is needed for 'tidb_auth_token' user, please use 'alter user' to declare it"))


When creating a new user with tidb_auth_token plugin, if no attribute or no email is declared, TiDB server would NOT give any warning or error, since the attribute could be modified by ALTER USER statements.

This is the original specification of this feature, could you double confirm if this is expected?(Although the behavior is different, IMHO this is fine).

IMHO, I think it is expected. The warning is just for the missing token_issuer, not the email attribute, though they are both needed in the later authentication.

To be honest I think all the warning for this auth plugin is not necessary for our general users. If they indeed miss something in create user, alter user can repair. The warning is just a small reminder.

bb7133

LGTM

xhebox · 2022-10-31T03:20:59Z

/merge

ti-chi-bot · 2022-10-31T03:21:03Z

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

Details

Commit hash: 340811b

sre-bot · 2022-10-31T04:26:39Z

TiDB MergeCI notify

✅ Well Done! New fixed [2] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	✅ all 6 tests passed	32 min	Fixed
idc-jenkins-ci-tidb/integration-common-test	✅ all 17 tests passed	17 min	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 38 tests passed	22 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	9 min 44 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 41 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 33 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 21 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 14 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 52 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

Create user with tidb_auth_token

7f6f17f

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 21, 2022

Add test for create user

d21fab3

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 25, 2022

Merge branch 'master' into tidb_auth_token

28b9548

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 25, 2022

Fix UT

7be6f51

CbcWestwolf mentioned this pull request Oct 25, 2022

Support authentication using a token/JWT #38504

Closed

2 tasks

CbcWestwolf changed the title *: support tidb_auth_token authentication *: create a user uses tidb_auth_token authentication Oct 25, 2022

Support token_require and token_issuer

ca7a000

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2022

CbcWestwolf added 5 commits October 25, 2022 18:02

show create user

a1e3c5f

bazel

d559001

Fix UT

ab25f46

Add more test

10476c2

Merge branch 'master' of github.com:pingcap/tidb into tidb_auth_token

13a4b0f

CbcWestwolf marked this pull request as ready for review October 26, 2022 06:28

CbcWestwolf requested a review from a team as a code owner October 26, 2022 06:28

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

CbcWestwolf changed the title *: create a user uses tidb_auth_token authentication *: create a user using tidb_auth_token authentication Oct 26, 2022

djshow832 reviewed Oct 26, 2022

View reviewed changes

Comment thread session/bootstrap.go

djshow832 reviewed Oct 26, 2022

View reviewed changes

Update

c50c762

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022

CbcWestwolf added 2 commits October 28, 2022 12:09

Merge branch 'master' of github.com:pingcap/tidb into tidb_auth_token

c39474b

Fix build

50b54bd

CbcWestwolf requested review from djshow832 and xhebox and removed request for djshow832 and xhebox October 28, 2022 04:10

Update

a758fb6

xhebox approved these changes Oct 28, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 28, 2022

Rename

daed722

CbcWestwolf mentioned this pull request Oct 28, 2022

Add description about tidb_auth_token authentication pingcap/docs-cn#11824

Merged

13 tasks

djshow832 reviewed Oct 28, 2022

View reviewed changes

CbcWestwolf and others added 2 commits October 28, 2022 15:33

Apply suggestions from code review

b2f1d75

Co-authored-by: djshow832 <zhangming@pingcap.com>

Update

5f3294d

ti-chi-bot requested a review from bb7133 October 28, 2022 07:40

Update UT

340811b

bb7133 reviewed Oct 28, 2022

View reviewed changes

bb7133 approved these changes Oct 31, 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 Oct 31, 2022

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

Merge branch 'master' into tidb_auth_token

ffc937c

ti-chi-bot merged commit 0e23da6 into pingcap:master Oct 31, 2022

CbcWestwolf deleted the tidb_auth_token branch October 31, 2022 06:03

Conversation

CbcWestwolf commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djshow832 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djshow832 left a comment

Choose a reason for hiding this comment

Uh oh!

djshow832 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CbcWestwolf commented Oct 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

Uh oh!

xhebox commented Oct 31, 2022

Uh oh!

ti-chi-bot commented Oct 31, 2022

Uh oh!

sre-bot commented Oct 31, 2022

TiDB MergeCI notify

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CbcWestwolf commented Oct 21, 2022 •

edited

Loading

ti-chi-bot commented Oct 21, 2022 •

edited

Loading