*: support MySQL dual passwords — behavior (executor / privilege / tests)#68393
Open
takaidohigasi wants to merge 22 commits into
Open
*: support MySQL dual passwords — behavior (executor / privilege / tests)#68393takaidohigasi wants to merge 22 commits into
takaidohigasi wants to merge 22 commits into
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.
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
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>
Codex GPT-5.5 follow-up review found two issues with the previous "align dual-password privilege checks with MySQL" and "respect default_authentication_plugin" commits. (1) Self-service DISCARD bypass was too broad The standalone bypass only checked spec.AuthOpt == nil, not whether the statement carried OTHER per-statement options. A caller could append DISCARD OLD PASSWORD to an ALTER USER that also changed REQUIRE / RESOURCE / PASSWORD EXPIRE / COMMENT / RESOURCE GROUP and thereby bypass the standard CREATE USER privilege check for those changes. Fix: also gate the bypass on hasOtherStmtOptions = no AuthTokenOrTLS, no ResourceOptions, no PasswordOrLockOptions, no CommentOrAttribute, no ResourceGroupName option. New TestDualPasswordSelfServiceDiscard\ WithExtraOptionsStillGated covers the three problematic shapes. (2) Capability check used the raw current plugin isDualPasswordCapablePlugin received the raw mysql.user.plugin string, which could be "". The helper accepted "" as natively capable (the legacy normalization rationale), so on a deployment with default_authentication_plugin=authentication_ldap_simple, a legacy empty-plugin row resolved to LDAP everywhere except this capability gate — letting RETAIN CURRENT PASSWORD through. Fix: drop the "" case from isDualPasswordCapablePlugin and route both call sites (executeAlterUser and executeSetPwd) through effectiveAuthPlugin first. New TestDualPasswordLegacyEmptyPluginReje\ ctsLDAPDefault covers the LDAP-default legacy-row scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated by `make bazel_prepare`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
⏳ @takaidohigasi I've received your updated pull request and will continue the review. I'll update this comment when I have something to share. ℹ️ Learn more details on Pantheon AI. |
Contributor
Author
|
Addressing the remaining CodeRabbit review-body items that could not be replied inline:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/simple.go (1)
1850-1881:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlip the
ALTER USERdual-password privilege gate.This currently lets self-service dual-password changes bypass the new dynamic privilege, and lets
APPLICATION_PASSWORD_ADMINreplaceCREATE USERfor cross-user changes. The PR contract is the opposite: self-serviceRETAIN/DISCARDneedsAPPLICATION_PASSWORD_ADMIN, while cross-user dual-password stays on the normal cross-user gate.Suggested fix
- selfServiceDualPwd := alterCurrentUser && specDualPwdRequested && spec.AuthOpt == nil && !hasOtherStmtOptions + selfServiceDualPwd := alterCurrentUser && specDualPwdRequested && !hasOtherStmtOptions + if selfServiceDualPwd && !hasApplicationPasswordAdminPriv { + return plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("APPLICATION_PASSWORD_ADMIN") + } needAdminPrivCheck := !(alterCurrentUser && alterPassword) && !selfServiceDualPwd @@ - dualPwdBypass := specDualPwdRequested && hasApplicationPasswordAdminPriv - if !(hasCreateUserPriv || hasSystemSchemaPriv || dualPwdBypass) { + if !(hasCreateUserPriv || hasSystemSchemaPriv) { return plannererrors.ErrSpecificAccessDenied.GenWithStackByArgs("CREATE USER") }🤖 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/executor/simple.go` around lines 1850 - 1881, The dual-password privilege gate is inverted: dualPwdBypass currently allows APPLICATION_PASSWORD_ADMIN to substitute for CREATE USER for cross-user changes; instead require APPLICATION_PASSWORD_ADMIN only for self-service dual-password (alterCurrentUser). Update the dualPwdBypass expression in pkg/executor/simple.go (the variable named dualPwdBypass near needAdminPrivCheck / specDualPwdRequested) to include alterCurrentUser (e.g., dualPwdBypass := specDualPwdRequested && alterCurrentUser && hasApplicationPasswordAdminPriv) and adjust the surrounding comment to reflect that APPLICATION_PASSWORD_ADMIN permits self-service RETAIN/DISCARD but does not bypass the cross-user admin gate.
🧹 Nitpick comments (2)
pkg/executor/test/passwordtest/dual_password_test.go (2)
36-39: ⚡ Quick winOptional: have
authAsreturn the authenticatedTestKitto remove duplication.
authAsbuilds and authenticates a subTestKitbut discards it, returning only the error. Six call sites (Lines 195-196, 207-208, 217-218, 263-264, 444-445, 452-453) reimplement the sameNewTestKit+Auth(sha1Password(...))sequence just to keep the kit. A single helper returning(*testkit.TestKit, error)would consolidate both patterns.♻️ Sketch
// authAsTK authenticates a fresh session as user@host and returns the TestKit. func authAsTK(t *testing.T, tk *testkit.TestKit, user, host, password string) (*testkit.TestKit, error) { sub := testkit.NewTestKit(t, tk.Session().GetStore()) return sub, sub.Session().Auth(&auth.UserIdentity{Username: user, Hostname: host}, sha1Password(password), nil, nil) } // authAs keeps the error-only convenience wrapper. func authAs(t *testing.T, tk *testkit.TestKit, user, host, password string) error { _, err := authAsTK(t, tk, user, host, password) return err }🤖 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/executor/test/passwordtest/dual_password_test.go` around lines 36 - 39, Replace the current authAs helper with a two-function approach so callers that need the authenticated TestKit can receive it: add authAsTK(t *testing.T, tk *testkit.TestKit, user, host, password string) (*testkit.TestKit, error) which creates sub := testkit.NewTestKit(t, tk.Session().GetStore()) and returns sub along with sub.Session().Auth(...), and keep a convenience authAs(t *testing.T, tk *testkit.TestKit, user, host, password string) error that calls authAsTK and returns only the error; update call sites to use authAsTK when they need the TestKit.
36-39: ⚡ Quick winUpdate:
sha1Passwordhelper is present; optional cleanup for auth-kit duplicationfunc authAs(t *testing.T, tk *testkit.TestKit, user, host, password string) error { sub := testkit.NewTestKit(t, tk.Session().GetStore()) return sub.Session().Auth(&auth.UserIdentity{Username: user, Hostname: host}, sha1Password(password), nil, nil) }
sha1Passwordis defined in the samepasswordtestpackage (pkg/executor/test/passwordtest/password_management_test.go), so this file’s references compile.
- Optional: the repeated
testkit.NewTestKit(...)+Session().Auth(...)blocks elsewhere could reuse this helper to reduce duplication.🤖 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/executor/test/passwordtest/dual_password_test.go` around lines 36 - 39, The authAs helper uses sha1Password which exists in the same package, so compilation is fine; to reduce duplication replace other occurrences of testkit.NewTestKit(...).Session().Auth(...) with calls to authAs(t, tk, user, host, password). Locate duplicated auth code and swap it to use the authAs function and the existing sha1Password helper so all auth logic is centralized in authAs.
🤖 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 1715-1729: The code that builds a synthetic ast.UserSpec for
USER()/self matching currently copies user and sets userCopy.Hostname =
userCopy.AuthHostname but still keys off Username; change it to use AuthUsername
for matching (e.g., set userCopy.Username = userCopy.AuthUsername or otherwise
use AuthUsername when constructing the spec) so ALTER USER USER() and SET
PASSWORD FOR USER() operate on the authenticated account; apply the same change
wherever similar synthetic UserSpec construction occurs (the other occurrences
using s.CurrentAuth / s.CurrentDualPasswordOption) to ensure all self-matching
uses AuthUsername instead of Username.
---
Outside diff comments:
In `@pkg/executor/simple.go`:
- Around line 1850-1881: The dual-password privilege gate is inverted:
dualPwdBypass currently allows APPLICATION_PASSWORD_ADMIN to substitute for
CREATE USER for cross-user changes; instead require APPLICATION_PASSWORD_ADMIN
only for self-service dual-password (alterCurrentUser). Update the dualPwdBypass
expression in pkg/executor/simple.go (the variable named dualPwdBypass near
needAdminPrivCheck / specDualPwdRequested) to include alterCurrentUser (e.g.,
dualPwdBypass := specDualPwdRequested && alterCurrentUser &&
hasApplicationPasswordAdminPriv) and adjust the surrounding comment to reflect
that APPLICATION_PASSWORD_ADMIN permits self-service RETAIN/DISCARD but does not
bypass the cross-user admin gate.
---
Nitpick comments:
In `@pkg/executor/test/passwordtest/dual_password_test.go`:
- Around line 36-39: Replace the current authAs helper with a two-function
approach so callers that need the authenticated TestKit can receive it: add
authAsTK(t *testing.T, tk *testkit.TestKit, user, host, password string)
(*testkit.TestKit, error) which creates sub := testkit.NewTestKit(t,
tk.Session().GetStore()) and returns sub along with sub.Session().Auth(...), and
keep a convenience authAs(t *testing.T, tk *testkit.TestKit, user, host,
password string) error that calls authAsTK and returns only the error; update
call sites to use authAsTK when they need the TestKit.
- Around line 36-39: The authAs helper uses sha1Password which exists in the
same package, so compilation is fine; to reduce duplication replace other
occurrences of testkit.NewTestKit(...).Session().Auth(...) with calls to
authAs(t, tk, user, host, password). Locate duplicated auth code and swap it to
use the authAs function and the existing sha1Password helper so all auth logic
is centralized in authAs.
🪄 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: 50536d3c-49ff-45de-a277-497a1494c0b3
📒 Files selected for processing (11)
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/BUILD.bazelpkg/parser/ast/misc.gopkg/parser/ast/misc_test.gopkg/privilege/privileges/cache.gopkg/privilege/privileges/privileges.go
✅ Files skipped from review due to trivial changes (1)
- errors.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/executor/test/passwordtest/BUILD.bazel
- pkg/errno/errname.go
- pkg/privilege/privileges/privileges.go
- pkg/privilege/privileges/cache.go
…witch Code-review findings (recall pass): pingcap#2 (correctness): the dual-password secondary-password login fallback was unreachable when the primary authentication_string is empty, because the whole password-verification block was gated on `len(pwd) > 0`. After `ALTER USER u IDENTIFIED BY 'new' RETAIN CURRENT PASSWORD` followed by `ALTER USER u IDENTIFIED BY ''` (same plugin, no DISCARD), the row has an empty primary alongside a valid retained secondary, but the secondary could never authenticate. The branch now also enters when a non-empty additional_password exists, and skips the primary check for an empty primary so the secondary is still tried. pingcap#7 (altitude): the secondary-password check (`matchSecondary`) duplicated the per-plugin primary-auth switch and would drift as plugins/hash handling evolve. Extracted a single `checkPasswordForPlugin` helper used for BOTH the primary authentication_string and the retained secondary, so the two can never diverge. The historical, error-log-review-stable log lines (native "decode password string failed" warn; caching_sha2 Error) are preserved at the call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dant read Code-review findings (recall pass): #1 (security, privilege escalation): cross-user RETAIN/DISCARD let a caller with only APPLICATION_PASSWORD_ADMIN reset another account's primary password — executeSetPwd's cross-user RETAIN branch skipped the SUPER check, and executeAlterUser's dualPwdBypass substituted APPLICATION_PASSWORD_ADMIN for CREATE USER. Realigned to the MySQL 8.0 contract: self-account RETAIN/DISCARD requires APPLICATION_PASSWORD_ADMIN (CREATE USER / UPDATE-mysql also suffice); cross-account requires the normal ALTER USER / SET PASSWORD authority (CREATE USER / SUPER), for which APPLICATION_PASSWORD_ADMIN is NOT a substitute. pingcap#5 (altitude): the self-service bypass is no longer expressed via an APPLICATION_PASSWORD_ADMIN substitution into the admin gate; it is a narrow self-account-only relaxation with a documented default-deny posture (a forgotten statement-option field falls back to requiring admin authority, not to bypassing it). pingcap#3 (compat): DISCARD OLD PASSWORD is no longer gated on plugin capability — only RETAIN is. DISCARD on an LDAP/socket/token account is now a harmless no-op (matching MySQL) instead of an error. pingcap#4 (compat): DISCARD that empties user_attributes now collapses the result to NULL via NULLIF rather than storing a literal '{}', matching MySQL and the NULL-preserving behavior in deletePasswordLockingAttribute. pingcap#6 (efficiency): RETAIN no longer issues a second FOR UPDATE read for the old authentication_string — userExistsInternal now returns it from the row it already locked, and buildAdditionalPasswordEntry takes the hash directly. Removed the now-unused readAuthenticationString helper. Tests: updated dual_password.test / dual_password_test.go to the corrected privilege model and added regressions for empty-primary secondary login, DISCARD-no-op-on-LDAP, {}->NULL collapse, and cross-user APP-only denial. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ion tests Generated by \`make bazel_prepare\`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integrationtest/t/executor/dual_password.test (1)
142-142:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Incomplete cleanup - missing
dpselfadminin final DROP USER.The test creates
dpselfadminat line 64 and uses it extensively (lines 96-112), but line 142 does not drop it. This leaves the user in the database after the test completes, violating test isolation.🧹 Proposed fix to include dpselfadmin in cleanup
-drop user dpu1, dpadmin, dpaponly, dpvictim, dpvictim_ap, dpself, dpm1, dpm2, dpm3, dpcomm; +drop user dpu1, dpadmin, dpaponly, dpvictim, dpvictim_ap, dpself, dpselfadmin, dpm1, dpm2, dpm3, dpcomm;🤖 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 `@tests/integrationtest/t/executor/dual_password.test` at line 142, The cleanup DROP USER statement is missing the dpselfadmin user which leaves state behind; update the final DROP USER list in tests/integrationtest/t/executor/dual_password.test to include dpselfadmin (add "dpselfadmin" to the comma-separated users in the existing DROP USER statement) so the test removes that user during teardown and maintains isolation.
🧹 Nitpick comments (1)
tests/integrationtest/t/executor/dual_password.test (1)
8-8: ⚡ Quick winMinor: Add
dpcommto initial cleanup for full idempotency.Line 136 creates
dpcommand line 135 guards withDROP USER IF EXISTS dpcomm, but the initial cleanup at line 8 does not includedpcomm. For consistency and to ensure the test is fully idempotent on repeated runs, adddpcommto the initial drop list.♻️ Proposed fix to add dpcomm to initial cleanup
-drop user if exists dpu1, dpadmin, dpaponly, dpvictim, dpvictim_ap, dpself, dpselfadmin, dpm1, dpm2, dpm3; +drop user if exists dpu1, dpadmin, dpaponly, dpvictim, dpvictim_ap, dpself, dpselfadmin, dpm1, dpm2, dpm3, dpcomm;🤖 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 `@tests/integrationtest/t/executor/dual_password.test` at line 8, The initial cleanup DROP USER list in the test (the SQL statement beginning with "drop user if exists dpu1, dpadmin, ...") omits the dpcomm user; update that statement to include dpcomm so the test teardown is fully idempotent and matches the later guard that creates/drops dpcomm (ensure the comma-separated list now includes dpcomm among the other users).
🤖 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.
Outside diff comments:
In `@tests/integrationtest/t/executor/dual_password.test`:
- Line 142: The cleanup DROP USER statement is missing the dpselfadmin user
which leaves state behind; update the final DROP USER list in
tests/integrationtest/t/executor/dual_password.test to include dpselfadmin (add
"dpselfadmin" to the comma-separated users in the existing DROP USER statement)
so the test removes that user during teardown and maintains isolation.
---
Nitpick comments:
In `@tests/integrationtest/t/executor/dual_password.test`:
- Line 8: The initial cleanup DROP USER list in the test (the SQL statement
beginning with "drop user if exists dpu1, dpadmin, ...") omits the dpcomm user;
update that statement to include dpcomm so the test teardown is fully idempotent
and matches the later guard that creates/drops dpcomm (ensure the
comma-separated list now includes dpcomm among the other users).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2c41ca1-5855-424f-b49c-2105c2b88ba0
📒 Files selected for processing (6)
pkg/executor/simple.gopkg/executor/test/passwordtest/BUILD.bazelpkg/executor/test/passwordtest/dual_password_test.gopkg/privilege/privileges/privileges.gotests/integrationtest/r/executor/dual_password.resulttests/integrationtest/t/executor/dual_password.test
✅ Files skipped from review due to trivial changes (1)
- tests/integrationtest/r/executor/dual_password.result
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/executor/test/passwordtest/BUILD.bazel
- pkg/privilege/privileges/privileges.go
- pkg/executor/test/passwordtest/dual_password_test.go
- pkg/executor/simple.go
…d name CodeRabbit review: executeSetPwd's setPwdForSelf (added in this PR to treat an explicit `SET PASSWORD FOR '<self>'@'<host>'` as self-service) keyed on the claimed sessUser.Username instead of the authenticated AuthUsername. For a proxy/mapped login where the two differ, the self-classification was inconsistent with the row actually modified (the self path operates on sessUser.AuthUsername). Match on AuthUsername so naming the authenticated account is correctly self-service and naming the claimed alias is not. Note: the analogous ALTER USER USER() / alterCurrentUser Username handling that CodeRabbit also referenced is pre-existing upstream behavior (master simple.go: userCopy.Hostname=AuthHostname with Username kept, and spec.User.Username = user.Username) shared by the non-dual-password `ALTER USER USER() IDENTIFIED BY` form; it is left unchanged here to avoid altering established semantics outside this feature's scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…w fixes Codex GPT-5.5 re-review of the review-fix commits found two P2 regressions introduced by them: (1) Passwordless login broke for an account with a retained secondary. The empty-primary fallback condition was widened with `|| len(record.AdditionalAuthenticationString) > 0`, which made a passwordless login (empty primary AND empty client auth) ENTER the password-verification branch solely because a secondary existed, then fail both the empty-primary and secondary checks. Reverted the condition to `len(pwd) > 0 || len(authentication) > 0`: the secondary is still tried whenever the client supplies a password (len(authentication) > 0), but a true no-password login no longer enters the branch and keeps succeeding via the existing no-password success path. (2) Self SET PASSWORD ... RETAIN omitted the UPDATE-on-mysql alternative. The new self-service gate accepted CREATE USER or APPLICATION_PASSWORD_ADMIN but not the UPDATE privilege on the mysql schema, even though the adjacent comment and executeAlterUser's needAdminPrivCheck treat it as an equivalent superset. Added hasSystemSchemaPriv so a user with UPDATE on mysql.* is not wrongly denied. Regression tests: passwordless-login-with-secondary (nil client auth) and self SET PASSWORD RETAIN accepted via UPDATE on mysql.*. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated by \`make bazel_prepare\`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit review: the final DROP USER in dual_password.test omitted dpselfadmin (created and used by the self-service-with-APPLICATION_PASSWORD_ADMIN cases), leaving it behind and breaking test isolation. Added it to the cleanup list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed the outside-diff CodeRabbit finding on |
Contributor
Author
|
2 similar(identical?) issues are created...? |
Contributor
Author
|
checking review comment contents. |
…gcap#68937) Codex GPT-5.5 assessment of pingcap#68937 (P3, latent-but-real): executeAlterUser built the synthetic UserSpec for the USER() form keyed on the claimed Username (only Hostname was rewritten to AuthHostname), and the alterCurrentUser self-classification compared user.Username. For a proxy/mapped login where Username != AuthUsername this would lock/UPDATE the wrong mysql.user row (Username@AuthHostname) — not merely a privilege misclassification. The divergence is not reachable through current TiDB auth paths (MatchIdentity keeps the login username; only host is resolved), so this is latent hardening, consistent with the executeSetPwd fix in c3e2ad9. Fix (per Codex / CodeRabbit minimal suggestion): * synthetic spec: userCopy.Username = userCopy.AuthUsername * alterCurrentUser: compare user.AuthUsername == spec.User.Username * rewrite: spec.User.Username = user.AuthUsername Common case (Username == AuthUsername) is unchanged. Added TestDualPasswordAlterUserUserResolvesAuthUsername, which authenticates as dpauth then overrides the claimed Username to dplogin and asserts ALTER USER USER() IDENTIFIED BY changes dpauth, not dplogin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated by \`make bazel_prepare\`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
I am glad if we can get ok-to-test label! thanks |
Contributor
Author
Local + E2E verification (HEAD
|
| Suite | Result |
|---|---|
pkg/executor/test/passwordtest (TestDualPassword*) |
✅ ok |
pkg/privilege/privileges/... (+ ldap) |
✅ ok |
integrationtest executor/dual_password |
✅ all passed |
E2E setup
Two real servers were started and driven with the same scenario:
- MySQL 8.0.45 (Docker) on
127.0.0.1:13306 - tidb-server built from this branch (unistore) on
127.0.0.1:4000
The admin connection runs the DDL; every "login" check opens a fresh client connection as the test user, i.e. a real TCP auth handshake (SELECT current_user()), not a mocked call. The user is created with IDENTIFIED WITH mysql_native_password (a dual-password-capable plugin on both servers) so the backends are directly comparable.
Scenario (run identically on both backends)
| # | Admin action | Login check | Expected |
|---|---|---|---|
| 1 | CREATE USER e2euser IDENTIFIED WITH mysql_native_password BY 'pass1' |
connect pass1 |
success |
| 2 | ALTER USER e2euser IDENTIFIED BY 'pass2' RETAIN CURRENT PASSWORD |
connect pass2 |
success (new / primary) |
| 3 | (same state) | connect pass1 |
success (old / secondary) |
| 4 | ALTER USER e2euser DISCARD OLD PASSWORD |
connect pass2 |
success |
| 5 | (same state) | connect pass1 |
rejected (secondary removed) |
| 6 | SET PASSWORD FOR e2euser = 'pass3' RETAIN CURRENT PASSWORD |
connect pass3 |
success |
| 7 | (same state) | connect pass2 |
success (old / secondary) |
| 8 | ALTER USER e2euser IDENTIFIED BY '' RETAIN CURRENT PASSWORD |
(error check) | rejected, ER 3895 ER_CURRENT_PASSWORD_CANNOT_BE_RETAINED |
| 9 | ALTER USER e2euser IDENTIFIED WITH caching_sha2_password BY 'x' RETAIN CURRENT PASSWORD |
(error check) | rejected, ER 3894 ER_PASSWORD_CANNOT_BE_RETAINED_ON_PLUGIN_CHANGE |
Result
18/18 assertions pass (9 × 2 backends) — TiDB behaves identically to MySQL 8.0 on every case, including the dual-password login window (both passwords valid simultaneously after RETAIN), DISCARD invalidating the secondary, the same flow via SET PASSWORD, and the two RETAIN error codes.
Scope note: this e2e focuses on the wire-level authentication behavior. The APPLICATION_PASSWORD_ADMIN privilege model and cross-user vs self-service authority are covered by the Go unit / integration suites above rather than this login-focused run.
Contributor
Author
|
please let me know that I can do to realize dual password change |
Contributor
Author
|
re-reviewing with fable 5. |
…ual-password comments Close the remaining review follow-ups from the Codex GPT-5.5 pass: * Add TestDualPasswordAlterUserUserRetainAndDiscard, mirroring MySQL multiple_passwords_ddl.test Test 13: ALTER USER USER() ... RETAIN CURRENT PASSWORD / DISCARD OLD PASSWORD is gated by APPLICATION_PASSWORD_ADMIN (denied without it, no side effects) and operates on the caller's own account. * Fix the executeSetPwd cross-user comment: the check is (long-standing) SUPER only; the previous wording claimed UPDATE-on-mysql is also accepted, which misdescribed the code. MySQL's acceptance of UPDATE-on-mysql there is a pre-existing compatibility gap independent of dual passwords. * Compress the ConnectionVerification password-branch comment to the two load-bearing invariants (empty-primary reachability for the secondary fallback; passwordless logins bypass the branch). * Add a file-header scenario map in dual_password_test.go linking each test to the MySQL dual-password test case it mirrors (multiple_passwords_ddl.test / multiple_passwords.inc), so reviewers can check coverage parity at a glance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated by make bazel_prepare. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
@D3Hunter Is it possible that we can get ok-to-test label on the PR ...? |
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
1 participant
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