*: implement auth plugin support in the extension framework

[yzhan1]

What problem does this PR solve?

Implements the extension authentication plugin support. Refer to https://github.com/pingcap/tidb/pull/53182 for design.

Issue Number: close #53181

Problem Summary:

TiDB developers can now implement new authentication plugin using the extension framework to keep proprietary code out of the core TiDB folders.

What changed and how does it work?

A new struct extension.AuthPlugin is created, where developers can implement their own authentication/authorization logic on top of TiDB. It achieves similar things to MySQL auth plugin by allowing developers implementing customized plugins.

Functions defined in extension.AuthPlugin will be executed during

1. privilege.ConnectionVerification for authentication check during logins
2. privilege.RequestVerification and privilege.DynamicRequestVerification for extra authorization checks on top of existing MySQL privilege checks

Check List

Tests

• [X] Unit test
• [ ] Integration test
• [X] Manual test (add detailed scripts or steps below)
• [ ] No need to test

  • [ ] I checked and no code files have been changed.

Side effects

• [ ] Performance regression: Consumes more CPU
• [ ] Performance regression: Consumes more Memory
• [ ] Breaking backward compatibility

Documentation

• [ ] Affects user behaviors
• [ ] Contains syntax changes
• [ ] Contains variable changes
• [ ] Contains experimental features
• [ ] Changes MySQL compatibility

Release note

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

Allow developers to implement their own authentication plugin using the extension framework.

[ti-chi-bot[bot]]

Hi @yzhan1. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tiprow[bot]]

Hi @yzhan1. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[yzhan1]

@lcwangchao @CbcWestwolf Would like to gather some early feedback from you. Thanks in advance!

[lcwangchao]

I saw that in privilege.go there are two functions called for verifying privileges of a non-current user:

1. RequestDynamicVerificationWithUser
2. RequestVerificationWithUser

Where 1 is used for checking system/admin privileges such as SUPER_USER, RESTRICTED_USER_ADMIN when performing ALTER USER/SET PASSWORD for super/admin users. 2 is used only for verifying view definer's privilege.

I think it might be too restricted to impose the same restriction on 1, that is - to not allow plugin users to be a super/admin user. Maybe we should allow this, and just call out explicitly that the verify privilege functions in the plugin will only be executed for the current user. And it's up to the plugin owner/operator to revoke unnecessary MySQL privileges.

What do you think? @lcwangchao

Do you mean if a user is created with a plugin, some privilege can only be revoked by himself? I think it is also some what restricted. I

A plugin user may have two sets of privileges:

• Privileges stored in database.
• Privileges provided by the plugin with the session context.

The real privileges of a session should be an intersection of those two sets. But in the ALTER/DROP USER statements, we can only check the first set for the target user without considering the session privilege. I think it is safer because that means "a user should not change a plugin user's privilege without admin privilege if the plugin user have a admin privilege", even if the plugin user may lost its admin privilege temporarily in some session.

[yzhan1]

I saw that in privilege.go there are two functions called for verifying privileges of a non-current user:

1. RequestDynamicVerificationWithUser
2. RequestVerificationWithUser

Where 1 is used for checking system/admin privileges such as SUPER_USER, RESTRICTED_USER_ADMIN when performing ALTER USER/SET PASSWORD for super/admin users. 2 is used only for verifying view definer's privilege. I think it might be too restricted to impose the same restriction on 1, that is - to not allow plugin users to be a super/admin user. Maybe we should allow this, and just call out explicitly that the verify privilege functions in the plugin will only be executed for the current user. And it's up to the plugin owner/operator to revoke unnecessary MySQL privileges. What do you think? @lcwangchao

Do you mean if a user is created with a plugin, some privilege can only be revoked by himself? I think it is also some what restricted. I

A plugin user may have two sets of privileges:

• Privileges stored in database.
• Privileges provided by the plugin with the session context.

The real privileges of a session should be an intersection of those two sets. But in the ALTER/DROP USER statements, we can only check the first set for the target user without considering the session privilege. I think it is safer because that means "a user should not change a plugin user's privilege without admin privilege if the plugin user have a admin privilege", even if the plugin user may lost its admin privilege temporarily in some session.

@lcwangchao Please let me clarify. I meant that for revoking/checking those special privileges for a plugin user, TiDB should only check the priv set 1 (stored in TiDB) since it lacks information about priv set 2 at the execution time, unless the user being verified is the currently connected user itself.

For example: u1 is a normal user and u2 is a plugin user. u2 has select priv on table t1. There's a view on t1 where definer is u2. When u1 wants to run a select on the view, TiDB checks the select priv on t1 for u2, where it will only be able to check u2's priv based on priv set 1. If the DB owner wants to revoke u2's select priv to reject other users from selecting the view, the owner should revoke it for both set 1 and 2 (as opposed to only set 2 for normal query scenarios where we only need to validate priv for current user).

"a user should not change a plugin user's privilege without admin privilege if the plugin user have a admin privilege"

I think by implementing what you suggested, we are not only doing this, but also not allowing an admin user to change a plugin user's privilege too. That's because inside RequestDynamicVerificationWithUser or RequestVerificationWithUser, we don't have enough information to check for privilege set 2, and the only thing we can do there seems to be rejecting the priv check if the user being checked is a plugin user.

[lcwangchao]

Maybe we can talk about some scenes. Suppose we have below user1:

• u1: A plugin user, has SELECT privilege for table t1. It also has a privilege of creating a view.
• u2: A plugin user, has SUPER privilege.
• u3: A normal user with only CREATE USER privilege.
• u4: A normal user with CREATE USER and SYSTEM_USER privilege.

And we have some sessions:

• s1: logged in with u1, but not permitted to read table t1 in this session.
• s2: logged in with u1. All privileges in u1 are permitted.

Currently, my though is:

If s1 wants to create a view create view v1 as select * from t1, it should be denied because it does not a SELECT privilege for table t1. However, if s2 wants to create this view, it should be permitted. After v1 is created, anyone who have the privilege to select v1 can select successfully even if s1 because view only check the stored privilege of u1 now.

For dropping table:

• u1 can be dropped by u2, u3, u4.
• u2 can only be dropped by u4 because it has the admin privilege. Here we do not need to check u2's privilege in session because it is not valid and not necessary.

[yzhan1]

@lcwangchao Thanks for the review!

I have addressed most of the comments and added more test cases to cover the scenario you described.

Also submitted an issue for a potential bug found along the way: https://github.com/pingcap/tidb/issues/54039. Would appreciate it if you could take a look too. Thanks!

[yzhan1]

@lcwangchao Could you please take a look at the latest diffs? Thank you!

[lcwangchao]

@bb7133 could you also take a look?

[yzhan1]

@bb7133 Could you please take a look at your convenience? Thank you!

@lcwangchao Could we mark this PR as ok-to-test so I can start fixing test issues (if any)? I recall some changes are needed for the bazel files for modified/new tests

[lcwangchao]

/ok-to-test

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 84.88372% with 26 lines in your changes missing coverage. Please review.

Project coverage is 56.6448%. Comparing base (d1854a7) to head (34facde). Report is 67 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #53494         +/-   ##
=================================================
- Coverage   74.7618%   56.6448%   -18.1170%     
=================================================
  Files          1523       1656        +133     
  Lines        361464     621501     +260037     
=================================================
+ Hits         270237     352048      +81811     
- Misses        71581     245928     +174347     
- Partials      19646      23525       +3879     

Flag	Coverage Δ
integration	37.0281% <21.5116%> (?)
unit	71.8258% <84.8837%> (-1.8340%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	52.9656% <ø> (-2.2339%)	:arrow_down:
parser	∅ <ø> (∅)
br	52.4558% <ø> (+4.5207%)	:arrow_up:

[ti-chi-bot[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133, lcwangchao

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

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [bb7133,lcwangchao]
• ~~pkg/parser/OWNERS~~ [bb7133]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot[bot]]

[LGTM Timeline notifier]

Timeline:

• 2024-06-28 07:53:49.898628596 +0000 UTC m=+965356.384117411: :ballot_box_with_check: agreed by lcwangchao.
• 2024-07-03 08:03:03.793857659 +0000 UTC m=+1397910.279346491: :ballot_box_with_check: agreed by bb7133.

[tiprow[bot]]

@yzhan1: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	34facde18812cd19fb21dd61cff0b0cd6d1f449e	link	true	/test fast_test_tiprow

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.