*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 by dveeden · Pull Request #54683 · pingcap/tidb

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.

Details

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.

Details

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.

Details

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

❌ Patch coverage is 58.92857% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.4296%. Comparing base (50d73f8) to head (2796d37).
⚠️ Report is 2830 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54683         +/-   ##
=================================================
- Coverage   73.2879%   57.4296%   -15.8583%     
=================================================
  Files          1638       1836        +198     
  Lines        453742     690401     +236659     
=================================================
+ Hits         332538     396495      +63957     
- Misses       100823     266820     +165997     
- Partials      20381      27086       +6705

Flag	Coverage Δ
integration	`24.1840% <4.3750%> (?)`
unit	`72.5604% <49.3750%> (-0.1260%)`	⬇️

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

Components	Coverage Δ
dumpling	`38.4008% <ø> (∅)`
parser	`∅ <ø> (∅)`
br	`40.3910% <ø> (-6.1953%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

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.

Details

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.

Details

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.

…current

ti-chi-bot · 2024-10-29T09:33:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign benmeadowcroft, bornchanger for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

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 · 2025-09-26T15:21:36Z

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

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/check_dev	`2796d37`	link	true	`/test check-dev`
idc-jenkins-ci-tidb/check_dev_2	`2796d37`	link	true	`/test check-dev2`
pull-integration-e2e-test	`2796d37`	link	true	`/test pull-integration-e2e-test`
pull-integration-realcluster-test-next-gen	`2796d37`	link	true	`/test pull-integration-realcluster-test-next-gen`
pull-unit-test-next-gen	`2796d37`	link	true	`/test pull-unit-test-next-gen`

Full PR test history. Your PR dashboard.

Details

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.

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

dveeden added 4 commits July 25, 2024 08:26

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

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

2796d37

…current

Conversation

dveeden commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Questions and remarks

Documentation needed

TODO

Check List

Release note

Uh oh!

ti-chi-bot bot commented Jul 17, 2024

Uh oh!

tiprow bot commented Jul 17, 2024

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

tiprow bot commented Jul 17, 2024

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

tiprow bot commented Jul 17, 2024

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

codecov bot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

dveeden commented Jul 17, 2024

Uh oh!

dveeden commented Jul 25, 2024

Uh oh!

dveeden commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dveeden commented Jul 26, 2024

Uh oh!

dveeden commented Jul 29, 2024

Uh oh!

dveeden commented Jul 29, 2024

Uh oh!

dveeden commented Jul 29, 2024

Uh oh!

BornChanger Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CbcWestwolf Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

dveeden Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

BornChanger Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

dveeden Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

dveeden Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

dveeden Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Aug 10, 2024

Uh oh!

dveeden commented Aug 21, 2024

Uh oh!

dveeden commented Aug 21, 2024

Uh oh!

ti-chi-bot bot commented Sep 6, 2024

Uh oh!

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