*: support MySQL dual passwords — behavior (executor / privilege / tests)#68393
Merged
ti-chi-bot[bot] merged 39 commits intoJul 9, 2026
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note
|
@takaidohigasi I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/XXL
|
Hi @takaidohigasi. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Hi @takaidohigasi. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MySQL 8.0 dual-password (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD) support: new error codes, parser/secure-text tweaks, executor validation and storage, privilege gating and auth fallback to a retained secondary, plus unit and integration tests. ChangesDual-Password Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant Executor
participant Privilege
participant MySQLUser as "mysql.user"
Client->>Parser: send ALTER/SET PASSWORD (with RETAIN/DISCARD)
Parser->>Executor: AST (UserSpec with dual flags)
Executor->>Privilege: check CREATE USER / APPLICATION_PASSWORD_ADMIN as needed
Executor->>MySQLUser: SELECT ... FOR UPDATE (capture authentication_string)
Executor->>MySQLUser: UPDATE user_attributes (json merge/remove additional_password)
Executor->>Client: return success or executor error (3878/3894/3895)
Client->>Executor: connect/authenticate
Executor->>Privilege: ConnectionVerification -> checkPasswordForPlugin(primary)
alt primary fails and AdditionalAuthenticationString present
Privilege->>Executor: verify retained secondary (success -> log rate-limited info)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merged
13 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/parser/parser.y (1)
14409-14435:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSupport dual-password clauses on the
ALTER USER USER()branch.The new
AlterUserSpecListpath can parseRETAIN CURRENT PASSWORD/DISCARD OLD PASSWORD, but the dedicatedALTER USER USER() IDENTIFIED BY ...alternative still cannot. That leaves the self-service current-user syntax unable to use this feature even though the rest of the grammar now supports it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/parser/parser.y` around lines 14409 - 14435, The "ALTER USER USER() IDENTIFIED BY AuthString" alternative currently only sets IfExists and CurrentAuth on the returned *ast.AlterUserStmt and therefore doesn't accept the dual-password clauses parsed by AlterUserSpecList; update that alternative to include the PasswordOrLockOptions production (the same production used in the other ALTER USER branch) and assign the parsed value to the AlterUserStmt.PasswordOrLockOptions field so the self-service form supports RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD (keep using ast.AuthOption for CurrentAuth and ast.AlterUserStmt as before).
🧹 Nitpick comments (2)
pkg/parser/parser_test.go (1)
5260-5269: ⚡ Quick winAdd CREATE USER dual-password parser coverage to match grammar scope.
This block covers ALTER USER / SET PASSWORD well, but not CREATE USER RETAIN/DISCARD parsing. Adding a couple of CREATE USER table cases here would close the parser-contract gap for this feature set.
Proposed test additions
{"SET PASSWORD FOR 'u1'@'%' = 'new' RETAIN CURRENT PASSWORD", true, "SET PASSWORD FOR `u1`@`%`='new' RETAIN CURRENT PASSWORD"}, + {"CREATE USER 'u1'@'%' IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD", true, "CREATE USER `u1`@`%` IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD"}, + {"CREATE USER 'u1'@'%' DISCARD OLD PASSWORD", true, "CREATE USER `u1`@`%` DISCARD OLD PASSWORD"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/parser/parser_test.go` around lines 5260 - 5269, The test suite in parser_test.go adds coverage for ALTER USER and SET PASSWORD dual-password forms but omits CREATE USER RETAIN/DISCARD cases; add new test entries mirroring the existing ALTER/SET lines but using CREATE USER syntax (e.g. "CREATE USER 'u1'@'%' RETAIN CURRENT PASSWORD" and "CREATE USER 'u1'@'%' DISCARD OLD PASSWORD", plus multi-user variants) so the parser's CREATE USER handling for RETAIN/DISCARD is exercised; insert these new cases alongside the existing Dual password block so they assert the expected normalized output strings like the ALTER/SET examples.pkg/privilege/privileges/privileges.go (1)
735-741: Consider a metric or rate-limited log for secondary-password hits.This
Infolog will fire on every successful fallback login, so a partially rotated service with high connection churn can flood logs. A counter plus sampled/rate-limited logging would preserve the signal without creating noisy auth logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/privilege/privileges/privileges.go` around lines 735 - 741, When secondaryAccepted is true, instead of unconditionally emitting logutil.BgLogger().Info for every fallback login, increment a dedicated counter metric (e.g., metricSecondaryPasswordFallback.Inc()) and replace the immediate Info call with a rate-limited or sampled log path so high-connection churn cannot flood logs; keep the same context fields (authUser, authHost, record.AuthPlugin) but only emit the Info message when the rate limiter/sampler allows it (or periodically), while the counter always records each hit. Ensure you modify the code around the secondaryAccepted check and the existing logutil.BgLogger().Info call so the counter and rate-limiting logic are applied together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/executor/simple.go`:
- Around line 1814-1819: After merging statement-level flags into each UserSpec,
detect when both specRetainCurrentPassword and specDiscardOldPassword are true
for the same user (i.e., specDualPwdRequested after OR-ing results from
dualPasswordOption and plOptions) and return an explicit error rejecting the
combined RETAIN+DISCARD for that user; update the same check in the other merge
location (the second occurrence using
specRetainCurrentPassword/specDiscardOldPassword) so neither path silently
prefers RETAIN—validate immediately after computing specDualPwdRequested and
fail with a clear message if true.
- Around line 1873-1882: The privilege-check branches in the dual-password logic
are inverted: the alterCurrentUser path (variable alterCurrentUser) currently
requires CREATE USER or APPLICATION_PASSWORD_ADMIN while the cross-user path
only requires CREATE USER; reverse the checks so that when alterCurrentUser is
true you only validate the normal self-password authority (i.e., do NOT require
APPLICATION_PASSWORD_ADMIN), and when alterCurrentUser is false require both
CREATE USER and APPLICATION_PASSWORD_ADMIN; update the conditional using
hasCreateUserPriv and hasApplicationPasswordAdminPriv and return
plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs(...) with the
appropriate message ("CREATE USER" for self vs "CREATE USER or
APPLICATION_PASSWORD_ADMIN" for cross-user) accordingly. Also apply the same fix
to the analogous block around the other occurrence referenced (lines
~2739-2750).
- Around line 1893-1895: The plugin-change checks compare raw stored values and
misclassify empty plugin strings as a change; normalize any empty plugin to
"mysql_native_password" before performing comparisons. Update the checks that
use spec.AuthOpt.AuthPlugin and currentAuthPlugin (e.g., the block returning
ErrPasswordCannotBeRetainedOnPluginChange) to first map "" to
"mysql_native_password" (either inline or via the existing normalization helper
used elsewhere), and apply the same normalization to the other occurrences
mentioned (around the other comparison sites) so both sides use the normalized
plugin name when deciding whether a plugin change occurred.
In `@pkg/parser/ast/misc.go`:
- Around line 1432-1435: SetPwdStmt.SecureText currently always injects n.User
into the formatted string which can be nil and produce "<nil>" in redacted SQL;
update SecureText to check n.User for nil and produce the correct text when nil
(e.g. omit "for user %s" or use the current-user form consistent with Restore)
while preserving the RetainCurrentPassword branch; locate the SecureText
implementation for SetPwdStmt and adjust both branches (when
n.RetainCurrentPassword is true and false) to conditionally include "for user
<name>" only when n.User != nil (using the same nil-handling semantics as
Restore).
---
Outside diff comments:
In `@pkg/parser/parser.y`:
- Around line 14409-14435: The "ALTER USER USER() IDENTIFIED BY AuthString"
alternative currently only sets IfExists and CurrentAuth on the returned
*ast.AlterUserStmt and therefore doesn't accept the dual-password clauses parsed
by AlterUserSpecList; update that alternative to include the
PasswordOrLockOptions production (the same production used in the other ALTER
USER branch) and assign the parsed value to the
AlterUserStmt.PasswordOrLockOptions field so the self-service form supports
RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD (keep using ast.AuthOption for
CurrentAuth and ast.AlterUserStmt as before).
---
Nitpick comments:
In `@pkg/parser/parser_test.go`:
- Around line 5260-5269: The test suite in parser_test.go adds coverage for
ALTER USER and SET PASSWORD dual-password forms but omits CREATE USER
RETAIN/DISCARD cases; add new test entries mirroring the existing ALTER/SET
lines but using CREATE USER syntax (e.g. "CREATE USER 'u1'@'%' RETAIN CURRENT
PASSWORD" and "CREATE USER 'u1'@'%' DISCARD OLD PASSWORD", plus multi-user
variants) so the parser's CREATE USER handling for RETAIN/DISCARD is exercised;
insert these new cases alongside the existing Dual password block so they assert
the expected normalized output strings like the ALTER/SET examples.
In `@pkg/privilege/privileges/privileges.go`:
- Around line 735-741: When secondaryAccepted is true, instead of
unconditionally emitting logutil.BgLogger().Info for every fallback login,
increment a dedicated counter metric (e.g.,
metricSecondaryPasswordFallback.Inc()) and replace the immediate Info call with
a rate-limited or sampled log path so high-connection churn cannot flood logs;
keep the same context fields (authUser, authHost, record.AuthPlugin) but only
emit the Info message when the rate limiter/sampler allows it (or periodically),
while the counter always records each hit. Ensure you modify the code around the
secondaryAccepted check and the existing logutil.BgLogger().Info call so the
counter and rate-limiting logic are applied together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e968ad10-3415-4ed6-a234-f28c1a9235db
📒 Files selected for processing (19)
errors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/simple.gopkg/executor/test/passwordtest/BUILD.bazelpkg/executor/test/passwordtest/dual_password_test.gopkg/parser/ast/misc.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.gopkg/util/dbterror/exeerrors/errors.gotests/integrationtest/r/executor/dual_password.resulttests/integrationtest/r/executor/executor.resulttests/integrationtest/t/executor/dual_password.test
takaidohigasi
added a commit
to takaidohigasi/tidb
that referenced
this pull request
May 17, 2026
Fixes for D3Hunter's review on pingcap#68028: [Blocker] Dual-password clauses are parsed but ignored by execution - Add executor stubs that reject RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD with ER_NOT_SUPPORTED_YET in executeAlterUser and executeSetPwd. Full execution / privilege / storage logic lives in the follow-up PR pingcap#68393. - Expose exeerrors.ErrNotSupportedYet for callers (was previously only available via dbterror.ClassExecutor.NewStd inline). - New TestDualPasswordParserOnlyStub in passwordtest to pin the stub's behavior. [Major pingcap#2] CREATE USER accepted dual-password clauses outside MySQL grammar - Drop the CreateUserSpec/CreateUserSpecList non-terminals and route CREATE USER back through the original UserSpec/UserSpecList chain. CREATE USER + RETAIN/DISCARD is now a parser-level syntax error, matching MySQL. [Major pingcap#3] ALTER USER over-accepted invalid RETAIN combinations - Introduce AuthOptionWithPassword (the BY-form subset of AuthOption) and restructure AlterUserSpec to bind RETAIN only to that subset. Reject at parse time: ALTER USER u IDENTIFIED WITH plugin AS '<hash>' RETAIN ... ALTER USER u IDENTIFIED WITH plugin RETAIN ... ALTER USER u RETAIN CURRENT PASSWORD (no auth) ALTER USER u IDENTIFIED BY '...' DISCARD OLD PASSWORD - DISCARD becomes its own AlterUserSpec variant (no auth-option coexists with it). [Major pingcap#4] Grammar label/documentation contradiction - Drop the misleading 'unsupported dual password option' label on the deleted CreateUserSpec; rewrite UserSpec/AlterUserSpec labels to state the contract accurately. [Major pingcap#5] Generic *PasswordOrLockOption type was overloaded - Introduce dedicated ast.DualPasswordOption and ast.DualPasswordOptionType (with DualPasswordRetainCurrent / DualPasswordDiscardOld). UserSpec.DualPasswordOption now uses the dedicated type. Remove the RetainCurrentPassword and DiscardOldPassword iota entries from PasswordOrLockOption. [Minor pingcap#6] ALTER USER USER() branch - USER() still does not route through AlterUserSpecList; dual-password on USER() is now a parse-time syntax error (covered by negative test). [Minor pingcap#7] Parser tests had no negative coverage - Add explicit ok=false rows for CREATE USER + RETAIN, the AS-hash + RETAIN form, bare RETAIN with no auth, plain BY + DISCARD, bare-plugin + RETAIN, and ALTER USER USER() + RETAIN. [Minor pingcap#8] CREATE/ALTER user-spec grammar duplication - Now naturally eliminated because CREATE USER reuses UserSpec. Behavior PR (pingcap#68393) needs to: - Remove the executor stubs added here. - Adopt the new ast.DualPasswordOption / DualPasswordOptionType symbols (the iota entries it currently consumes are gone). Ref pingcap#60587
takaidohigasi
commented
May 17, 2026
| ErrTableWithoutPrimaryKey = 3750 | ||
| // Dual-password (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD) — match MySQL 8.0 | ||
| // error numbers from mysql/mysql-server share/messages_to_clients.txt. | ||
| ErrSecondPasswordCannotBeEmpty = 3878 |
Contributor
Author
There was a problem hiding this comment.
takaidohigasi
commented
May 17, 2026
| // Dual-password (RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD) — match MySQL 8.0 | ||
| // error numbers from mysql/mysql-server share/messages_to_clients.txt. | ||
| ErrSecondPasswordCannotBeEmpty = 3878 | ||
| ErrPasswordCannotBeRetainedOnPluginChange = 3894 |
Contributor
Author
There was a problem hiding this comment.
takaidohigasi
commented
May 17, 2026
| // error numbers from mysql/mysql-server share/messages_to_clients.txt. | ||
| ErrSecondPasswordCannotBeEmpty = 3878 | ||
| ErrPasswordCannotBeRetainedOnPluginChange = 3894 | ||
| ErrCurrentPasswordCannotBeRetained = 3895 |
Contributor
Author
There was a problem hiding this comment.
takaidohigasi
commented
May 17, 2026
| // Dual-password errors — text matches MySQL 8.0 | ||
| // share/messages_to_clients.txt (length-bounded %-.64s is a TiDB convention | ||
| // for user/host identifiers). | ||
| ErrSecondPasswordCannotBeEmpty: mysql.Message("Empty password can not be retained as second password for user '%-.64s'@'%-.64s'.", nil), |
Contributor
Author
There was a problem hiding this comment.
takaidohigasi
commented
May 17, 2026
| // share/messages_to_clients.txt (length-bounded %-.64s is a TiDB convention | ||
| // for user/host identifiers). | ||
| ErrSecondPasswordCannotBeEmpty: mysql.Message("Empty password can not be retained as second password for user '%-.64s'@'%-.64s'.", nil), | ||
| ErrPasswordCannotBeRetainedOnPluginChange: mysql.Message("Current password can not be retained for user '%-.64s'@'%-.64s' because authentication plugin is being changed.", nil), |
Contributor
Author
There was a problem hiding this comment.
takaidohigasi
commented
May 17, 2026
| // for user/host identifiers). | ||
| ErrSecondPasswordCannotBeEmpty: mysql.Message("Empty password can not be retained as second password for user '%-.64s'@'%-.64s'.", nil), | ||
| ErrPasswordCannotBeRetainedOnPluginChange: mysql.Message("Current password can not be retained for user '%-.64s'@'%-.64s' because authentication plugin is being changed.", nil), | ||
| ErrCurrentPasswordCannotBeRetained: mysql.Message("Current password can not be retained for user '%-.64s'@'%-.64s' because new password is empty.", nil), |
Contributor
Author
There was a problem hiding this comment.
takaidohigasi
marked this pull request as draft
June 3, 2026 07:05
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
first half of the PR (parser change) was merged, so I will rebase the changes and review the contents again. |
…D PASSWORD) This is the behavior PR (follow-up to pingcap#68028, which added parser-only support). Implements the full MySQL 8.0 dual-password feature on top of the AST shape that landed in pingcap#68028. * Privilege cache: UserRecord.AdditionalAuthenticationString, decoded from mysql.user.user_attributes '$.additional_password'. * Auth: ConnectionVerification falls back to the secondary password for mysql_native_password / caching_sha2_password / tidb_sm3_password when the primary check fails. LDAP / socket / token plugins are skipped. * Privilege: APPLICATION_PASSWORD_ADMIN dynamic privilege; required when RETAIN/DISCARD is applied to another user's account (self-service is always allowed, whether the statement uses CURRENT_USER() or names the same user explicitly). * Executor (ALTER USER): on RETAIN, move the current authentication_string into user_attributes.$.additional_password via JSON_MERGE_PATCH and install the new hash as primary; on DISCARD, JSON_REMOVE the secondary. Reject RETAIN when the new password is empty, when the plugin is being changed, or when the current plugin doesn't support secondary passwords. Silently discard any existing secondary when the plugin changes without RETAIN (matching MySQL). * Executor (CREATE USER): reject RETAIN / DISCARD per spec — MySQL does not allow dual passwords on user creation. * Executor (SET PASSWORD): full RETAIN support for both the named-user and current-user forms, with the same privilege gating as ALTER USER. * Executor (USER() form): propagate CurrentDualPasswordOption from the AlterUserStmt into the synthetic UserSpec so the per-spec loop only needs to inspect spec.DualPasswordOption. Covers both `ALTER USER USER() IDENTIFIED BY '...' RETAIN CURRENT PASSWORD` and the standalone `ALTER USER USER() DISCARD OLD PASSWORD`. * SecureText / SecurityString: surface RETAIN/DISCARD verbatim so the redacted output still indicates the statement targets the secondary slot. * Tests: dual_password_test.go covers self-service RETAIN/DISCARD, cross-user gating with APPLICATION_PASSWORD_ADMIN, login fallback to the secondary password, plugin-change handling, SHOW CREATE USER redaction, chained RETAIN, DROP USER cleanup, RENAME USER preservation, and the corruption fallback when the stored secondary hash is unreadable. Also adds tests/integrationtest/t/executor/dual_password.test for the SQL-level round trip. Closes pingcap#60587 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
takaidohigasi
force-pushed
the
feature/dual-password-behavior
branch
from
June 3, 2026 08:26
7eb5088 to
52c5978
Compare
CodeRabbit review noted the privilege checks were inverted: * self-service was denied unless the caller held CREATE USER or APPLICATION_PASSWORD_ADMIN — should require no extra privilege. * cross-user accepted only CREATE USER — should also accept APPLICATION_PASSWORD_ADMIN as an alternative. This matches MySQL 8.0: https://dev.mysql.com/doc/refman/8.0/en/password-management.html#password-management-dual-password Changes: * executeAlterUser: relax the outer ALTER USER privilege check to allow APPLICATION_PASSWORD_ADMIN as a substitute for CREATE USER when the statement carries RETAIN/DISCARD. The inner gate is reduced to the plugin-capability check; self-service no longer hits a privilege guard. * executeSetPwd: cross-user SET PASSWORD ... RETAIN now accepts CREATE USER or APPLICATION_PASSWORD_ADMIN; self-service requires no extra privilege. * Tests: rewrite TestDualPasswordCrossUserRequiresCreateUser to cover the three cases (CREATE USER, APPLICATION_PASSWORD_ADMIN only, neither); simplify TestDualPasswordSetPasswordSelfByExplicitName to assert that self-service succeeds without any grant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hange checks CodeRabbit review flagged that the plugin-change comparisons used raw mysql.user.plugin values. Legacy rows can have an empty plugin column (resolved at auth time as mysql_native_password), and an explicit IDENTIFIED WITH mysql_native_password ... RETAIN CURRENT PASSWORD against such a row was misclassified as a plugin switch — wrongly rejecting RETAIN, pruning password history, and dropping additional_password. Added effectiveAuthPlugin() helper that maps "" -> mysql_native_password and applied it at the three plugin-equality comparison sites in executeAlterUser. Added TestDualPasswordLegacyEmptyPluginAcceptsNative as a regression guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit review flagged that SecureText unconditionally formatted "set password for user %s" with n.User, which can be nil for the current-user form (`SET PASSWORD = '...'`) — leaking "<nil>" into redacted SQL text. Mirror Restore's nil-handling: omit the "for user ..." segment when n.User is nil. Added TestSetPwdStmtSecureText covering the four combinations of (nil/named user) × (with/without RETAIN). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit review noted that the unsampled Info log for retained-password
fallback logins can flood logs on a partially-rotated high-churn service.
Wrap the log call in logutil.SampleLoggerFactory(time.Minute, 1, ...) so
the operator-facing signal ("which accounts have finished rotating") is
preserved but rate-limited to once per minute per process. Auth events
themselves remain recorded separately via the slow-log / connect-log
pipeline.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bump pkg/executor/test/passwordtest shard_count 25 -> 26 for TestDualPasswordLegacyEmptyPluginAcceptsNative; add pkg/parser/auth dep to pkg/parser/ast/BUILD.bazel for TestSetPwdStmtSecureText. Generated by \`make bazel_prepare\`. Also fix a one-character struct-field alignment in the synthetic UserSpec construction in executor.executeAlterUser that nogo's gofmt check flagged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…in checks Codex GPT-5.5 review (P2) flagged that effectiveAuthPlugin hard-coded the empty-plugin fallback to mysql_native_password, ignoring the default_authentication_plugin session variable that the privilege cache uses to resolve legacy mysql.user rows (see pkg/privilege/privileges/cache.go:1012-1020). On a deployment with default_authentication_plugin = caching_sha2_password, an explicit IDENTIFIED WITH mysql_native_password ... RETAIN CURRENT PASSWORD against a legacy empty-plugin row IS a plugin switch and must be rejected. The previous helper accepted it as a no-op, incorrectly preserving secondary-password and password-history state. * effectiveAuthPlugin now takes the resolved defaultPlugin and falls back to mysql_native_password only when the sysvar is empty/unreadable, matching the cache's behavior. * executeAlterUser resolves default_authentication_plugin once per statement and threads it through the three comparison sites. * New TestDualPasswordLegacyEmptyPluginHonorsDefaultPlugin sets the default to caching_sha2_password, creates a legacy row, and verifies that an explicit native plugin with RETAIN is now rejected as the plugin switch it actually is. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/retest |
Contributor
Author
|
/test check-dev2 |
Contributor
Author
|
test passed |
tiancaiamao
approved these changes
Jul 8, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Contributor
|
/hold hold for a while, I only reviewed half of the changes in the PR, will review rest part soon |
ti-chi-bot
Bot
added
the
do-not-merge/hold
D3Hunter
reviewed
Jul 8, 2026
D3Hunter
left a comment
D3Hunter
left a comment
Contributor
There was a problem hiding this comment.
rest lgtm, you can unhold after fix below comments
Co-authored-by: D3Hunter <jujj603@gmail.com>
Contributor
Author
|
(I will add doc changes for the dual password) |
takaidohigasi
marked this pull request as draft
July 8, 2026 20:36
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
working for adding additional change to rename |
7f548a4 (review suggestion applied via the GitHub UI) renamed the UserRecord field only at its definition, leaving the two usages — decodeUserTableRow's write and ConnectionVerification's secondary-password read — referencing the old AdditionalAuthenticationString name, so the branch did not compile (the check_dev_2 failure). Rename the usages and the doc comment, and gofmt the realigned struct. Co-Authored-By: Claude <noreply@anthropic.com>
takaidohigasi
marked this pull request as ready for review
July 8, 2026 20:41
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
10 tasks
Contributor
Author
|
green again, thanks all so much. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, tiancaiamao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Contributor
Author
|
thanks so much!! 👏 |
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #60587
Problem Summary:
This PR is the behavior half of the MySQL 8.0 dual-password feature, split from #68028 per @D3Hunter's review request for easier review.
How to review
pkg/executor/simple.go(executeAlterUser→executeSetPwd) →pkg/privilege/privileges/privileges.go(ConnectionVerificationsecondary-password fallback) → tests.shard_countbumps); squash-merge is expected.pkg/executor/test/passwordtest/dual_password_test.gomaps each test to the scenario it mirrors in MySQL'smysql-test/suite/auth_sec/t/multiple_passwords_ddl.test/include/multiple_passwords.inc.mysql/mysql-servershare/messages_to_clients.txt).REPLACE '<current>'clause /PASSWORD REQUIRE CURRENTenforcement is not implemented in TiDB; cross-userSET PASSWORDin TiDB requiresSUPER(MySQL also acceptsUPDATEon themysqlschema).What changed and how does it work?
Storage
mysql.user.User_attributesJSON column under key$.additional_password— same shape MySQL uses. No schema migration needed.Login fallback
privilege.UserPrivileges.ConnectionVerificationnow retries against the secondary hash when the primary check fails, formysql_native_password,caching_sha2_password, andtidb_sm3_password. LDAP / socket / token plugins are explicitly excluded (no secondary support). A corrupt retained hash degrades to primary-only authentication (does not lock the account out).Privilege
APPLICATION_PASSWORD_ADMIN— required for self-serviceRETAIN CURRENT PASSWORD/DISCARD OLD PASSWORD. Cross-user retain/discard is gated by the existingCREATE USERprivilege, matching MySQL.Executor
executeAlterUserappliesRETAIN/DISCARDperUserSpec(matching MySQL's per-spec grammar).RETAINcaptures the pre-changeauthentication_stringand writes it touser_attributes.$.additional_passwordviaJSON_MERGE_PATCH.DISCARD(and a plugin change withoutRETAIN) silently remove the secondary viaJSON_REMOVE. The merge + remove are emitted as a singleJSON_REMOVE(JSON_MERGE_PATCH(...))expression so behavior does not depend on MySQL's left-to-right same-row-assignment semantics.executeCreateUserrejectsRETAIN/DISCARDinCREATE USER(per MySQL).RETAIN, plugin change +RETAIN, non-password-plugin +RETAIN, empty current primary +RETAIN.executeSetPwdhonors the newRetainCurrentPasswordflag onSetPwdStmtand applies the same self-service / cross-user privilege gating.Error codes (MySQL-compatible, verified against
mysql/mysql-server/share/messages_to_clients.txt)ErrSecondPasswordCannotBeEmpty(3878) — "Empty password can not be retained as second password..."ErrPasswordCannotBeRetainedOnPluginChange(3894) — "Current password can not be retained ... because authentication plugin is being changed."ErrCurrentPasswordCannotBeRetained(3895) — "Current password can not be retained ... because new password is empty."SHOW CREATE USER— does not expose the secondary password. Behavior matches MySQL 8.0 (verified via end-to-end parity test against MySQL 8.0.45).Compatibility with TiCDC, DM, BR — unaffected.
mysql.userrow changes (includingadditional_password) are filtered as system-schema DML, andALTER USER/SET PASSWORDare not TiDB DDL Jobs so they never enter changefeed/binlog streams. BR restoresUser_attributesJSON verbatim. The feature can be used to rotate the TiDB-side credential of these tools with zero downtime.Check List
Tests
pkg/executor/test/passwordtest/dual_password_test.go— 16 tests covering self-service, cross-user (with/withoutCREATE USERandAPPLICATION_PASSWORD_ADMIN), empty-password rejection, plugin-change rejection, plugin-change silently drops secondary,CREATE USERrejection,caching_sha2_passwordstorage,SET PASSWORD ... RETAIN, chained RETAIN, ALTER-without-RETAIN preserves secondary, RENAME USER preserves, DROP USER removes, multi-user ALTER,SHOW CREATE USERhides secondary, COMMENT + DISCARD atomic.tests/integrationtest/t/executor/dual_password.test— 41 cases covering storage shape, error codes (1105 / 3878 / 3894 / 3895 / 1227),SHOW CREATE USERoutput, cross-user vs self-service privilege enforcement, RETAIN/DISCARD combined withCOMMENT.bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true— 1406 targets, exit 0.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes
Tests