*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 #54683

dveeden · 2024-07-17T08:58:48Z

What problem does this PR solve?

Issue Number: close #54660

Problem Summary:

Support a global (password_require_current) and per account (mysql.user.Password_require_current) policy for requiring the current password when changing the password.

What changed and how does it work?

To create/modify the per user policy use:

CREATE USER ... PASSWORD REQUIRE CURRENT;
CREATE USER ... PASSWORD REQUIRE CURRENT OPTIONAL;
CREATE USER ... PASSWORD REQUIRE CURRENT DEFAULT;
ALTER USER ... PASSWORD REQUIRE CURRENT;
ALTER USER ... PASSWORD REQUIRE CURRENT OPTIONAL;
ALTER USER ... PASSWORD REQUIRE CURRENT DEFAULT;

To change the system policy:

SET GLOBAL password_require_current=OFF;
SET GLOBAL password_require_current=ON;

To specify a password when changing the password:

ALTER USER ... IDENTIFIED BY 'newpwd' REPLACE 'curentpwd';

parser:

Support PASSWORD REQUIRE CURRENT
Support PASSWORD REQUIRE CURRENT OPTIONAL
Support ALTER USER...IDENTIFIED BY ... REPLACE ...
Support ALTER USER...IDENTIFIED WITH ... BY ... REPLACE ...
Support ALTER USER() IDENTIFIED BY ... REPLACE ...
Add ErrIncorrectCurrentPassword
Add ErrMissingCurrentPassword
Add ErrCurrentPasswordNotRequired

session:

Add Password_require_current column to mysql.user

sessionctx/variable:

Add password_require_current system variable

executor:

Include "PASSWORD REQUIRE CURRENT ..." in SHOW CREATE USER output
Support modifying accounts with ALTER USER ... PASSWORD REQUIRE CURRENT ...
Check current password in ALTER USER

errno:

Add ErrIncorrectCurrentPassword
Add ErrMissingCurrentPassword
Add ErrCurrentPasswordNotRequired

privilege:

Load current password policy from mysql.user table
Check current password

Questions and remarks

TestAbnormalMySQLTable has a comment that says it is testing with a mysql.user table synchronized from MySQL. However the supplied data looks like a TiDB mysql.user table instead. Is this test correct? Should this be multiple tests, with MySQL 5.7,8.0 and 8.4? Maybe 9.0? @CbcWestwolf ? TestAbnormalMySQLTable has multiple issues. #54924
Looks like there is no interface or good abstraction for authentication plugins. Now (*UserPrivilege).checkPassword() for example needs to do something different based on what authentication plugin is being used. Maybe we can make auth.CheckHashingPassword() more generic, but then we should probably change the name. Maybe an interface is best, but probably that's out of scope for this PR.
show.go has a "FIXME: the returned string is not escaped safely" comment. Maybe we should fix this outside of this PR?
This should work for mysql_native_password, tidb_sm3_password and caching_sha2_password. For other methods more testing and work might be needed.
Looks like we still have the PASSWORD() function and SET PASSWORD = PASSWORD(...) from the MySQL 5.7 era. Might be good to start the process of removing this (outside of this PR).

Documentation needed

ALTER USER docs to include:
- Add: ALTER USER...PASSWORD REQUIRE CURRENT ...
- Add: ALTER USER...IDENTIFIED BY... REPLACE ...
System variable docs
- Add: password_require_current
System table docs
- Update mysql.user docs

TODO

Check impact on audit logging for new field that contains a password
Check impact on log redact for a new field that contains a password
Check impact on BR for new column in mysql.user
Check impact on TiCDC for new column in mysql.user
Check impact on DM for new column in mysql.user

For log redaction and audit logging:

These take care of this for SET PASSWORD and ALTER USER:

pkg/parser/ast/misc.go:func (n *SetPwdStmt) SecureText() string {
pkg/parser/ast/misc.go:func (n *AlterUserStmt) SecureText() string {

Both don't really need an update but (*AlterUserStmt).SecureText() was updated anyway to make the intention clear.
The REPLACE <password> syntax is only used with SET PASSWORD and ALTER USER statements, which both already deal with passwords.

Check List

Tests

Unit test
Integration test
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

Release note

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

Support for a password policies that require the current password when changing passwords has been added.

ti-chi-bot · 2024-07-17T08:58:52Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2024-07-17T08:59:08Z

Hi @dveeden. 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.

dveeden · 2024-07-17T09:00:14Z

/test all

tiprow · 2024-07-17T09:00:36Z

@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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.

dveeden · 2024-07-17T09:12:19Z

/test all

tiprow · 2024-07-17T09:12:41Z

@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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.

dveeden · 2024-07-17T09:12:56Z

/ok-to-test

codecov · 2024-07-17T09:33:35Z

Codecov Report

Attention: Patch coverage is 77.30061% with 37 lines in your changes missing coverage. Please review.

Project coverage is 75.6721%. Comparing base (c1c74b1) to head (7fa2d96).
Report is 115 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #54683        +/-   ##
================================================
+ Coverage   72.8739%   75.6721%   +2.7982%     
================================================
  Files          1576       1609        +33     
  Lines        440775     475241     +34466     
================================================
+ Hits         321210     359625     +38415     
+ Misses        99821      93786      -6035     
- Partials      19744      21830      +2086

Flag	Coverage Δ
integration	`50.6337% <66.6666%> (?)`
unit	`71.9303% <47.1698%> (-0.9427%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.0400% <ø> (+17.0941%)`	⬆️

dveeden · 2024-07-17T10:07:51Z

/test all

dveeden · 2024-07-17T13:51:27Z

/test all

dveeden · 2024-07-17T14:06:07Z

/retest

dveeden · 2024-07-17T14:12:09Z

/test all

CbcWestwolf

Maybe more tests should be ported to cover the scenarios in password-reverification-policy and t/password_require_current.test

CbcWestwolf · 2024-07-24T10:54:54Z

pkg/session/bootstrap.go

+	if ver >= version212 {
+		return
+	}
+	doReentrantDDL(s, "ALTER TABLE mysql.user ADD COLUMN IF NOT EXISTS `Password_require_current` enum('N','Y') DEFAULT NULL AFTER `Password_reuse_time`")


Would it (add a new field into mysql.user) break any compatibility with tools like BR, DM or TiCDC?
PTAL, thanks! cc @BornChanger @D3Hunter @asddongmen

DM doesn't query this table explicitly.

pkg/parser/parser.y

pkg/privilege/privileges/cache.go

CbcWestwolf · 2024-07-24T11:40:35Z

pkg/parser/ast/misc.go

+	ByHashString  bool
+	HashString    string
+	AuthPlugin    string
+	ReplaceString string


Since a new field containing password is been added, log redact (general log or audit log) should consider this scenario. /cc @xhebox @lcwangchao

pkg/executor/simple.go

CbcWestwolf · 2024-07-24T12:30:27Z

TestAbnormalMySQLTable has a comment that says it is testing with a mysql.user table synchronized from MySQL. However the supplied data looks like a TiDB mysql.user table instead. Is this test correct? Should this be multiple tests, with MySQL 5.7,8.0 and 8.4? Maybe 9.0?

Refer to https://github.com/pingcap/tidb/pull/2722/files, I misunderstood TestAbnormalMySQLTable previously and add token_issuer into it. Let me solve these several questions in another PR, thx!

…current

dveeden · 2024-07-25T15:19:56Z

/retest

dveeden · 2024-07-26T09:07:54Z

Maybe more tests should be ported to cover the scenarios in password-reverification-policy and t/password_require_current.test

That file relies heavily on --source, which mysql-tester doesn't support yet. I've created pingcap/mysql-tester#128 to try to address that.

…current

dveeden · 2024-07-26T11:51:18Z

/retest

…ring sets withPassword to true

dveeden · 2024-07-29T06:49:28Z

/retest

dveeden · 2024-07-29T10:09:24Z

/retest

dveeden · 2024-07-29T17:45:17Z

/retest

BornChanger · 2024-07-30T07:09:08Z

pkg/session/bootstrap.go

+		Create_Tablespace_Priv   ENUM('N','Y') NOT NULL DEFAULT 'N',
+		Password_reuse_history   smallint unsigned DEFAULT NULL,
+		Password_reuse_time      smallint unsigned DEFAULT NULL,
+		Password_require_current ENUM('N','Y') DEFAULT NULL,


how about put Password_require_current under column of user_attirbutes for better cross-version system table compatibility?

I prefer to put Password_require_current as a new field in mysql.user for better compatibility with mysql's
https://dev.mysql.com/doc/mysql-security-excerpt/8.0/en/grant-tables.html#grant-tables-user-db

Yes, let's do this in the same way as MySQL unless there is a very strong reason not to do that.

it seems pr #39460 has addressed the compatibility problem here. so, i am fine with the change. but please make sure to do a cross version backup restore testing.

A backup of TiDB-with-this-PR to a vanilla TiDB returns this:

dvaneeden@dve-carbon:~$ tiup br restore full -s /tmp/b_full/ Checking updates for component br... Starting component br: /home/dvaneeden/.tiup/components/br/v8.3.0/br restore full -s /tmp/b_full/ Detail BR log in /tmp/br.log.2024-08-22T09.43.39+0200 [2024/08/22 09:43:41.949 +02:00] [INFO] [collector.go:77] ["Full Restore failed summary"] [total-ranges=0] [ranges-succeed=0] [ranges-failed=0] ####################################################################### # the target cluster is not compatible with the backup data, # you can use '--with-sys-table=false' to skip restoring system tables ####################################################################### Error: missing column in cluster data, table: user, col: Password_require_current enum('N','Y'): [BR:Restore:ErrRestoreIncompatibleSys]incompatible system table

I can make BR to succeed with this patch:

diff --git a/br/pkg/restore/snap_client/systable_restore.go b/br/pkg/restore/snap_client/systable_restore.go index a99d9925b1..7b758422d2 100644 --- a/br/pkg/restore/snap_client/systable_restore.go +++ b/br/pkg/restore/snap_client/systable_restore.go @@ -276,7 +276,7 @@ func (rc *SnapClient) replaceTemporaryTableToSystable(ctx context.Context, ti *m zap.Stringer("schema", db.Name)) // target column order may different with source cluster columnNames := make([]string, 0, len(ti.Columns)) - for _, col := range ti.Columns { + for _, col := range db.ExistingTables[tableName].Columns { columnNames = append(columnNames, utils.EncloseName(col.Name.L)) } colListStr := strings.Join(columnNames, ",") @@ -384,13 +384,19 @@ func CheckSysTableCompatibility(dom *domain.Domain, tables []*metautil.Table) er col := backupTi.Columns[i] clusterCol := clusterColMap[col.Name.L] if clusterCol == nil { - log.Error("missing column in cluster data", - zap.Stringer("table", table.Info.Name), - zap.String("col", fmt.Sprintf("%s %s", col.Name, col.FieldType.String()))) - return errors.Annotatef(berrors.ErrRestoreIncompatibleSys, - "missing column in cluster data, table: %s, col: %s %s", - table.Info.Name.O, - col.Name, col.FieldType.String()) + if col.Name.L == "password_require_current" { + log.Error("ignoring missing column in cluster data", + zap.Stringer("table", table.Info.Name), + zap.String("col", fmt.Sprintf("%s %s", col.Name, col.FieldType.String()))) + } else { + log.Error("missing column in cluster data", + zap.Stringer("table", table.Info.Name), + zap.String("col", fmt.Sprintf("%s %s", col.Name, col.FieldType.String()))) + return errors.Annotatef(berrors.ErrRestoreIncompatibleSys, + "missing column in cluster data, table: %s, col: %s %s", + table.Info.Name.O, + col.Name, col.FieldType.String()) + } } } }

This patch:

Makes the "missing column in cluster data" non-fatal for the "password_require_current" column

Uses the list of columns for the target table instead of the list of columns of the source table. Maybe this should check for columns that are present in both if we actually want to do it like this.

However the result is that the PASSWORD REQUIRE CURRENT (DEFAULT|OPTIONAL|) part of the user definition gets lost as there isn't a column for that. I think this might be acceptable but not supporting this and requiring --with-sys-table=false might be a good option as well.

Restoring a backup of TiDB v8.3.0 vanilla on TiDB-with-this-PR works:

From the logs:

[2024/08/22 10:49:10.696 +02:00] [WARN] [systable_restore.go:349] ["missing column in backup data"] [table=user] [col="Password_require_current enum('N','Y')"]

ti-chi-bot · 2024-08-10T09:22:28Z

PR needs rebase.

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.

…current

dveeden · 2024-08-21T06:56:23Z

/retest

dveeden · 2024-08-21T08:15:14Z

/retest

ti-chi-bot · 2024-09-06T08:36:01Z

PR needs rebase.

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.

Support more PASSWORD REQUIRE CURRENT options

91ccc2b

dveeden changed the title ~~Support more PASSWORD REQUIRE CURRENT options~~ *: Support more PASSWORD REQUIRE CURRENT options Jul 17, 2024

ti-chi-bot bot removed the do-not-merge/invalid-title label Jul 17, 2024

Fix formatting

5efb8f3

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. 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 Jul 17, 2024

Update executor/grant results

952c13f

Update TestBootstrap

7b0cd2e

dveeden changed the title *: Support more PASSWORD REQUIRE CURRENT options *: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 Jul 17, 2024

Update TestMonitorTheSystemTableIncremental

1ae90b7

dveeden added 2 commits July 17, 2024 16:19

Update TestFailedLoginTrackingBasic

8e445d6

Update TestBootstrapWithError

56e4782

CbcWestwolf reviewed Jul 24, 2024

View reviewed changes

dveeden added 5 commits July 25, 2024 07:28

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

2e6e352

…current

Do the same for SET PASSWORD

6def7e8

Remove unuseful line based on review

773a380

Add tests for SET PASSWORD

dfa9f53

Fix SET PASSWORD

ab4662a

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

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

11a3ea7

…current

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

dveeden added 2 commits July 26, 2024 15:39

Add more integration tests

cfb5873

Make sure UserSpec with an empty authstring but a non-empty replacest…

9d0c6a6

…ring sets withPassword to true

Try to simplify parser changes

7b60fa2

dveeden requested a review from BornChanger July 30, 2024 06:23

BornChanger reviewed Jul 30, 2024

View reviewed changes

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

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

4ee9c90

…current

Fix TestColumnTable

7fa2d96

dveeden removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 #54683

*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 #54683

dveeden commented Jul 17, 2024 •

edited

Loading

ti-chi-bot bot commented Jul 17, 2024

tiprow bot commented Jul 17, 2024

dveeden commented Jul 17, 2024

tiprow bot commented Jul 17, 2024

dveeden commented Jul 17, 2024

tiprow bot commented Jul 17, 2024

dveeden commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

dveeden commented Jul 17, 2024

dveeden commented Jul 17, 2024

dveeden commented Jul 17, 2024

dveeden commented Jul 17, 2024

CbcWestwolf left a comment

CbcWestwolf Jul 24, 2024

D3Hunter Jul 29, 2024

CbcWestwolf Jul 24, 2024

CbcWestwolf commented Jul 24, 2024

dveeden commented Jul 25, 2024

dveeden commented Jul 26, 2024 •

edited

Loading

dveeden commented Jul 26, 2024

dveeden commented Jul 29, 2024

dveeden commented Jul 29, 2024

dveeden commented Jul 29, 2024

BornChanger Jul 30, 2024 •

edited

Loading

CbcWestwolf Jul 30, 2024

dveeden Jul 30, 2024

BornChanger Jul 30, 2024

dveeden Aug 22, 2024

dveeden Aug 22, 2024

dveeden Aug 22, 2024

ti-chi-bot bot commented Aug 10, 2024

dveeden commented Aug 21, 2024

dveeden commented Aug 21, 2024

ti-chi-bot bot commented Sep 6, 2024

*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 #54683

Are you sure you want to change the base?

*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 #54683

Conversation

dveeden commented Jul 17, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Questions and remarks

Documentation needed

TODO

Check List

Release note

ti-chi-bot bot commented Jul 17, 2024

tiprow bot commented Jul 17, 2024

dveeden commented Jul 17, 2024

tiprow bot commented Jul 17, 2024

dveeden commented Jul 17, 2024

tiprow bot commented Jul 17, 2024

dveeden commented Jul 17, 2024

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

dveeden commented Jul 17, 2024

dveeden commented Jul 17, 2024

dveeden commented Jul 17, 2024

dveeden commented Jul 17, 2024

CbcWestwolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CbcWestwolf commented Jul 24, 2024

dveeden commented Jul 25, 2024

dveeden commented Jul 26, 2024 • edited Loading

dveeden commented Jul 26, 2024

dveeden commented Jul 29, 2024

dveeden commented Jul 29, 2024

dveeden commented Jul 29, 2024

BornChanger Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 10, 2024

dveeden commented Aug 21, 2024

dveeden commented Aug 21, 2024

ti-chi-bot bot commented Sep 6, 2024

dveeden commented Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading

dveeden commented Jul 26, 2024 •

edited

Loading

BornChanger Jul 30, 2024 •

edited

Loading