executor: Fix authplugin handling with alter/create by dveeden · Pull Request #28468 · pingcap/tidb

dveeden · 2021-09-28T16:21:29Z

What problem does this PR solve?

Problem Summary: Fix handling of auth plugins

What is changed and how it works?

What's Changed:

ALTER USER: fix changing the auth plugin of a user with IDENTIFIED WITH ...
ALTER USER: Check for invalid auth plugins
CREATE USER: Check for invalid auth plugins
SET PASSWORD: return warning when setting a password when using socket authentication

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

Release note

Handling of authentication methods in `ALTER USER` and `CREATE USER` was improved

ti-chi-bot · 2021-09-28T16:21:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

mjonss
morgo

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.

sre-bot · 2021-09-28T16:22:09Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

dveeden · 2021-10-21T13:02:19Z

/cc @morgo

- `ALTER USER`: fix changing the auth plugin of a user with `IDETIFIED WITH ...` - `ALTER USER`: Check for invalid auth plugins - `CREATE USER`: Check for invalid auth plugins

dveeden · 2021-10-25T15:12:51Z

cc @pingcap/tidb-reviewers

morgo · 2021-10-25T19:42:06Z

/run_check_dev_2

I'm having trouble viewing the jenkins results. It LGTM, but I want to see what the error reported is.

morgo · 2021-10-26T01:49:33Z

@dveeden The failures look related:

[2021-10-25T19:38:12.044Z] FAIL: simple_test.go:893: testSuite3.TestIssue17247

[2021-10-25T19:38:12.044Z] 

[2021-10-25T19:38:12.044Z] simple_test.go:901:

[2021-10-25T19:38:12.044Z]     tk1.MustExec("ALTER USER USER() IDENTIFIED BY 'xxx'")

[2021-10-25T19:38:12.044Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:245:

[2021-10-25T19:38:12.044Z]     tk.c.Assert(err, check.IsNil, check.Commentf("sql:%s, %v, error stack %v", sql, args, errors.ErrorStack(err)))

[2021-10-25T19:38:12.044Z] ... value *errors.withStack = [executor:1524]Plugin '' is not loaded ("[executor:1524]Plugin '' is not loaded")

[2021-10-25T19:38:12.044Z] ... sql:ALTER USER USER() IDENTIFIED BY 'xxx', [], error stack [executor:1524]Plugin '' is not loaded

[2021-10-25T19:38:12.044Z] github.com/pingcap/errors.AddStack

[2021-10-25T19:38:12.044Z] 	/nfs/cache/mod/github.com/pingcap/errors@v0.11.5-0.20210425183316-da1aaba5fb63/errors.go:174

[2021-10-25T19:38:12.044Z] github.com/pingcap/errors.(*Error).GenWithStackByArgs

[2021-10-25T19:38:12.044Z] 	/nfs/cache/mod/github.com/pingcap/errors@v0.11.5-0.20210425183316-da1aaba5fb63/normalize.go:159

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/executor.(*SimpleExec).executeAlterUser

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/simple.go:939

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/executor.(*SimpleExec).Next

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/simple.go:140

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/executor.Next

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/executor.go:286

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelayExecutor

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/adapter.go:584

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelay

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/adapter.go:465

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/executor.(*ExecStmt).Exec

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/executor/adapter.go:414

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/session.runStmt

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/session/session.go:1683

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/session.(*session).ExecuteStmt

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/session/session.go:1577

[2021-10-25T19:38:12.044Z] github.com/pingcap/tidb/util/testkit.(*TestKit).Exec

[2021-10-25T19:38:12.044Z] 	/home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:198

[2021-10-25T19:38:12.044Z] --

[2021-10-25T19:38:12.044Z] FAIL: simple_test.go:357: testSuite7.TestUser

[2021-10-25T19:38:12.044Z] 

[2021-10-25T19:38:12.044Z] simple_test.go:428:

[2021-10-25T19:38:12.044Z]     result.Check(testkit.Rows(auth.EncodePassword("1")))

[2021-10-25T19:38:12.044Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:66:

[2021-10-25T19:38:12.044Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)

[2021-10-25T19:38:12.044Z] ... obtained string = "[]\n"

[2021-10-25T19:38:12.044Z] ... expected string = "[*E6CC90B878B948C35E92B003C792C46C58C4AF40]\n"

[2021-10-25T19:38:12.044Z] ... sql:SELECT authentication_string FROM mysql.User WHERE User="test1" and Host="localhost", args:[]

mjonss · 2021-10-27T13:13:45Z

/cc @mjonss

…lugin

dveeden · 2021-10-27T13:39:14Z

/run-check_dev_2

mjonss

LGTM

Two questions:

Which manual tests have you done (since the checkbox was marked)?
Are there any issue open for the duplication of error definitions (errno/errname.go vs parser/mysql/errname.go)?

dveeden · 2021-11-01T09:56:28Z

Which manual tests have you done (since the checkbox was marked)?

Changing the authentication plugin of an account:

TiDB> CREATE USER 'u1'@'%' IDENTIFIED WITH 'auth_socket';
Query OK, 0 rows affected (0.0172 sec)

TiDB> SHOW CREATE USER 'u1'@'%'\G
*************************** 1. row ***************************
CREATE USER for u1@%: CREATE USER 'u1'@'%' IDENTIFIED WITH 'auth_socket' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
1 row in set (0.0021 sec)

TiDB> ALTER USER 'u1'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'foobar';
Query OK, 0 rows affected (0.0063 sec)

TiDB> SHOW CREATE USER 'u1'@'%'\G
*************************** 1. row ***************************
CREATE USER for u1@%: CREATE USER 'u1'@'%' IDENTIFIED WITH 'mysql_native_password' AS '*9B500343BC52E2911172EB52AE5CF4847604C6E5' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT UNLOCK
1 row in set (0.0019 sec)

Using an invalid authentication plugin name:

TiDB> ALTER USER 'u1'@'%' IDENTIFIED WITH 'pumpkin_password';
ERROR: 1524 (HY000): Plugin 'pumpkin_password' is not loaded

TiDB> CREATE USER 'u2'@'%' IDENTIFIED WITH 'pumpkin_password';
ERROR: 1524 (HY000): Plugin 'pumpkin_password' is not loaded

Using SET PASSWORD on an account that uses auth_socket:

TiDB> CREATE USER 'u3'@'%' IDENTIFIED WITH 'auth_socket';
Query OK, 0 rows affected (0.0170 sec)

TiDB> SET PASSWORD FOR 'u3'@'%' = PASSWORD('foobar');
Query OK, 0 rows affected, 1 warning (0.0167 sec)
Note (code 1699): SET PASSWORD has no significance for user 'u3'@'%' as authentication plugin does not support it.

Are there any issue open for the duplication of error definitions (errno/errname.go vs parser/mysql/errname.go)?

Yes, #29026

dveeden · 2021-11-01T14:42:42Z

/assign @morgo

morgo · 2021-11-01T16:55:46Z

/merge

ti-chi-bot · 2021-11-01T16:55:49Z

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

Details

Commit hash: 7b393cc

dveeden changed the title ~~Fix authplugin handling with alter/create~~ executor: Fix authplugin handling with alter/create Sep 28, 2021

dveeden force-pushed the auth_switch_authplugin branch from 7170739 to b1ea2ac Compare September 28, 2021 16:32

dveeden mentioned this pull request Sep 30, 2021

docs: Add design doc for --initialize-secure #28482

Merged

12 tasks

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

dveeden force-pushed the auth_switch_authplugin branch from 1c2f3d8 to f5e5113 Compare October 21, 2021 10:07

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

dveeden force-pushed the auth_switch_authplugin branch 3 times, most recently from 46229d8 to 13f2c49 Compare October 21, 2021 13:02

dveeden marked this pull request as ready for review October 21, 2021 13:02

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 21, 2021

ti-chi-bot requested a review from morgo October 21, 2021 13:02

Fix changing the authplugin of a user

0da9f37

- `ALTER USER`: fix changing the auth plugin of a user with `IDETIFIED WITH ...` - `ALTER USER`: Check for invalid auth plugins - `CREATE USER`: Check for invalid auth plugins

dveeden force-pushed the auth_switch_authplugin branch from 13f2c49 to 0da9f37 Compare October 25, 2021 15:09

Merge branch 'master' into auth_switch_authplugin

b262ab3

dveeden added 2 commits October 26, 2021 08:38

Fix alter user test

9f99180

Fix handling of passwordless accounts

4f568d0

morgo approved these changes Oct 26, 2021

View reviewed changes

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

ti-chi-bot requested a review from mjonss October 27, 2021 13:13

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

Merge remote-tracking branch 'upstream/master' into auth_switch_authp…

0167d7f

…lugin

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

mjonss approved these changes Oct 27, 2021

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 27, 2021

Merge branch 'master' into auth_switch_authplugin

3ca1ba7

ti-chi-bot assigned morgo Nov 1, 2021

Merge branch 'master' into auth_switch_authplugin

7b393cc

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 1, 2021

ti-chi-bot merged commit ec7a638 into pingcap:master Nov 1, 2021

Conversation

dveeden commented Sep 28, 2021 • 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 Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented Sep 28, 2021

Uh oh!

dveeden commented Oct 21, 2021

Uh oh!

dveeden commented Oct 25, 2021

Uh oh!

morgo commented Oct 25, 2021

Uh oh!

morgo commented Oct 26, 2021

Uh oh!

mjonss commented Oct 27, 2021

Uh oh!

dveeden commented Oct 27, 2021

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

dveeden commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dveeden commented Nov 1, 2021

Uh oh!

morgo commented Nov 1, 2021

Uh oh!

ti-chi-bot commented Nov 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dveeden commented Sep 28, 2021 •

edited

Loading

ti-chi-bot commented Sep 28, 2021 •

edited

Loading

dveeden commented Nov 1, 2021 •

edited

Loading