*: support username prefix for starter mode#68355
Conversation
ti-chi-bot
Bot
added
release-note-none
|
@ChangRui-Ryan 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/XL
|
Hi @ChangRui-Ryan. 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:
📝 WalkthroughWalkthroughThis PR introduces starter-mode username prefix policy enforcement and variant-aware user operations. It adds validation requiring usernames to start with a keyspace prefix, variant-aware user lookups across CREATE/ALTER/GRANT/REVOKE flows, gateway-forwarded TLS handshake parsing, variant-matching authentication, and supporting tests and BUILD wiring. ChangesStarter Username Prefix Policy
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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/server/conn.go`:
- Around line 648-667: The code currently trusts client-supplied resp.Attrs
(checked via GATEWAY_SECURECONN_ATTR_KEY) and sets cc.tlsConnState from that
JSON; instead, only accept and parse forwarded TLS state when the connection is
proven to come from a trusted gateway/channel (e.g., an authenticated upstream,
a verified proxy socket, or loopback+mutual-auth) — do not base trust solely on
deploymode.IsStarter or presence of the attr key. Update the logic around
resp.Attrs parsing to first verify the connection provenance (use existing
connection metadata on cc such as remote address or an authentication flag set
earlier, or require a signed token exchanged with the gateway), and only then
populate cc.tlsConnState; otherwise ignore resp.Attrs and enforce
tlsutil.RequireSecureTransport via the existing check that uses cc.isUnixSocket.
- Around line 952-961: The code currently calls cc.ctx.MatchIdentity before
verifying prefix/keyspace mismatches, letting a prefixed tenant like
"OTHER.user" succeed; move the prefix/keyspace rejection ahead of the exact
lookup: call the existing checkUserVariantMismatch (or equivalent validation)
first and return servererr.ErrAccessDenied when it indicates a mismatch, then
only call MatchIdentity (and, in starter mode, matchIdentityWithVariants) if the
prefix check passes; update the logic around MatchIdentity,
matchIdentityWithVariants, and checkUserVariantMismatch so prefix rejection
happens for both normal and starter-mode code paths.
🪄 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: 81d06059-fef5-4822-bb93-4c4dd3147369
📒 Files selected for processing (16)
errors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/grant.gopkg/executor/revoke.gopkg/executor/simple.gopkg/executor/test/simpletest/simple_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace_test.gopkg/keyspace/username_policy.gopkg/meta/meta.gopkg/server/conn.gopkg/server/conn_test.gopkg/server/err/error.gopkg/server/server.gopkg/util/dbterror/exeerrors/errors.go
👮 Files not reviewed due to content moderation or server errors (7)
- pkg/keyspace/username_policy.go
- pkg/keyspace/BUILD.bazel
- pkg/keyspace/keyspace_test.go
- pkg/executor/simple.go
- pkg/executor/grant.go
- pkg/executor/revoke.go
- pkg/executor/test/simpletest/simple_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68355 +/- ##
================================================
- Coverage 76.3909% 75.4023% -0.9886%
================================================
Files 2038 2021 -17
Lines 562779 566241 +3462
================================================
- Hits 429912 426959 -2953
- Misses 131949 139260 +7311
+ Partials 918 22 -896
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
from
7903720 to
eac1305
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/server/conn.go (2)
952-962:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Prefix/keyspace mismatch must be checked before exact identity lookup.
The current flow calls
cc.ctx.MatchIdentity(ctx, cc.user, host)at line 952 before checking for keyspace/prefix mismatches. If a client in keyspaceSYSTEMsupplies usernameOTHER.user, and an accountOTHER.userexists in the database, line 952 succeeds and grants access—breaking tenant isolation. The variant/mismatch validation at lines 954-961 never runs.Move the prefix/keyspace rejection ahead of the exact lookup: call
checkUserVariantMismatch(or equivalent validation) first, returnservererr.ErrAccessDeniedon mismatch, then proceed toMatchIdentity(andmatchIdentityWithVariantsin starter mode) only if the prefix check passes.🤖 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/server/conn.go` around lines 952 - 962, The tenant prefix/keyspace check must run before any exact identity lookup: call checkUserVariantMismatch (or the existing variant validation helper) immediately and return servererr.ErrAccessDenied when it reports a mismatch, then perform cc.ctx.MatchIdentity(ctx, cc.user, host); only if MatchIdentity fails and deploymode.IsStarter() is true invoke cc.matchIdentityWithVariants(ctx, host, hasPassword) as the fallback. Ensure you remove the current early MatchIdentity call and reorder logic so prefix validation occurs first, preserving existing error returns and starter-mode fallback behavior.
648-668:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Client-supplied handshake attributes should not be trusted as TLS evidence.
resp.Attrscomes from the client handshake and is fully client-controlled. Any client that knows the configuredGATEWAY_SECURECONN_ATTR_KEYcan send{"Version":...,"CipherSuite":...}to bypassrequire_secure_transportand spoof TLS state in connection info.Forwarded TLS state must be bound to a trusted, authenticated gateway channel (e.g., mutual TLS from a verified gateway, a Unix socket from localhost with access controls, or a signed/encrypted token exchanged with the gateway). Do not base trust solely on
deploymode.IsStarter()or the presence of an attribute key.🤖 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/server/conn.go` around lines 648 - 668, The code currently trusts client-supplied resp.Attrs (and GATEWAY_SECURECONN_ATTR_KEY) to set cc.tlsConnState and bypass tlsutil.RequireSecureTransport; instead, only accept forwarded TLS state when the connection is from a trusted, authenticated gateway channel or another verifiable source—do not trust deploymode.IsStarter() + attr presence alone. Change the logic around resp.Attrs and gatewaySecureConn so you only parse and assign cc.tlsConnState after: (a) the connection provenance is authenticated (e.g., mutual TLS on the gateway link, or a verified signed/encrypted token), or (b) the connection is a local Unix socket with appropriate ACLs; otherwise treat the attribute as untrusted and enforce tlsutil.RequireSecureTransport. Reference resp.Attrs, GATEWAY_SECURECONN_ATTR_KEY, deploymode.IsStarter(), cc.tlsConnState, and tlsutil.RequireSecureTransport when implementing the validation and gating.
🧹 Nitpick comments (1)
pkg/keyspace/username_policy.go (1)
54-56: 💤 Low valueConsider returning the input username instead of
nilfor consistency.
defaultUsernamePolicy.GetUsernameVariantsreturnsnil, but an empty slice[]string{}might be clearer. Similarly,GetOriginalUsernamecould return the input unchanged rather than""to avoid ambiguity (see earlier comment on interface contracts).🤖 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/keyspace/username_policy.go` around lines 54 - 56, defaultUsernamePolicy.GetUsernameVariants currently returns nil which is ambiguous; change it to return the input username as a single-element slice (i.e. []string{username}) to indicate the primary variant, and also update defaultUsernamePolicy.GetOriginalUsername to return the input username unchanged instead of an empty string so both functions consistently reflect the input value (refer to functions defaultUsernamePolicy.GetUsernameVariants and GetOriginalUsername to locate the changes).
🤖 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/keyspace/username_policy.go`:
- Around line 66-68: The ValidateUsernameFormat implementation on prefixPolicy
currently only accepts usernames with exactly one dot because it checks
len(strings.Split(username, ".")) == 2; update it to accept usernames with at
least one dot by changing the check to len(strings.Split(username, ".")) >= 2
(or confirm spec if exactly one dot is required), i.e., modify
prefixPolicy.ValidateUsernameFormat to use >= 2 to allow multi-segment usernames
like prefix.user.extra.
- Around line 24-34: The GetOriginalUsername contract is ambiguous—empty string
is used inconsistently by defaultUsernamePolicy and prefixPolicy; update the
interface and implementations so semantics are clear: change
GetOriginalUsername(username string) string to always return a non-empty
original username (return the input unchanged when no transformation applies) or
explicitly document that empty string means "no original" in the interface
comment; then update defaultUsernamePolicy.GetOriginalUsername and
prefixPolicy.GetOriginalUsername to return the input username when no
prefix/transform applies (or adjust comments if you choose documentation),
ensuring callers can rely on a preserved original value from
GetOriginalUsername.
---
Duplicate comments:
In `@pkg/server/conn.go`:
- Around line 952-962: The tenant prefix/keyspace check must run before any
exact identity lookup: call checkUserVariantMismatch (or the existing variant
validation helper) immediately and return servererr.ErrAccessDenied when it
reports a mismatch, then perform cc.ctx.MatchIdentity(ctx, cc.user, host); only
if MatchIdentity fails and deploymode.IsStarter() is true invoke
cc.matchIdentityWithVariants(ctx, host, hasPassword) as the fallback. Ensure you
remove the current early MatchIdentity call and reorder logic so prefix
validation occurs first, preserving existing error returns and starter-mode
fallback behavior.
- Around line 648-668: The code currently trusts client-supplied resp.Attrs (and
GATEWAY_SECURECONN_ATTR_KEY) to set cc.tlsConnState and bypass
tlsutil.RequireSecureTransport; instead, only accept forwarded TLS state when
the connection is from a trusted, authenticated gateway channel or another
verifiable source—do not trust deploymode.IsStarter() + attr presence alone.
Change the logic around resp.Attrs and gatewaySecureConn so you only parse and
assign cc.tlsConnState after: (a) the connection provenance is authenticated
(e.g., mutual TLS on the gateway link, or a verified signed/encrypted token), or
(b) the connection is a local Unix socket with appropriate ACLs; otherwise treat
the attribute as untrusted and enforce tlsutil.RequireSecureTransport. Reference
resp.Attrs, GATEWAY_SECURECONN_ATTR_KEY, deploymode.IsStarter(),
cc.tlsConnState, and tlsutil.RequireSecureTransport when implementing the
validation and gating.
---
Nitpick comments:
In `@pkg/keyspace/username_policy.go`:
- Around line 54-56: defaultUsernamePolicy.GetUsernameVariants currently returns
nil which is ambiguous; change it to return the input username as a
single-element slice (i.e. []string{username}) to indicate the primary variant,
and also update defaultUsernamePolicy.GetOriginalUsername to return the input
username unchanged instead of an empty string so both functions consistently
reflect the input value (refer to functions
defaultUsernamePolicy.GetUsernameVariants and GetOriginalUsername to locate the
changes).
🪄 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: 69f92d6e-b07b-4299-be35-e3b4269895bc
📒 Files selected for processing (19)
cmd/tidb-server/BUILD.bazelerrors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/grant.gopkg/executor/revoke.gopkg/executor/simple.gopkg/executor/test/simpletest/BUILD.bazelpkg/executor/test/simpletest/simple_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace_test.gopkg/keyspace/username_policy.gopkg/meta/meta.gopkg/server/BUILD.bazelpkg/server/conn.gopkg/server/conn_test.gopkg/server/err/error.gopkg/server/server.gopkg/util/dbterror/exeerrors/errors.go
✅ Files skipped from review due to trivial changes (3)
- pkg/server/err/error.go
- pkg/errno/errname.go
- pkg/errno/errcode.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/executor/revoke.go
- errors.toml
- pkg/executor/grant.go
- pkg/server/server.go
- pkg/keyspace/keyspace_test.go
- pkg/executor/test/simpletest/simple_test.go
- pkg/meta/meta.go
- pkg/server/conn_test.go
- pkg/executor/simple.go
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
from
eac1305 to
db7db72
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/executor/simple.go (2)
1783-1795:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the variant before checking
SYSTEM_USER/RESTRICTED_USER_ADMIN.In starter mode, an unprefixed
ALTER USERtarget is rewritten only after the dynamic-privilege checks run. That meansRequestDynamicVerificationWithUser(...)can evaluate protections against the unresolved name while the later update operates on the rewritten account, which can bypass the extra guard on protected users.🤖 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 1783 - 1795, The dynamic-privilege checks (calls to RequestDynamicVerificationWithUser) are performed against the unresolved user name and must run against the resolved variant to prevent bypass; move or perform the account-variant resolution (call userExistsInternalWithRetryVariants or the same resolution logic that produces the rewritten account) before the SYSTEM_USER / RESTRICTED_USER_ADMIN checks and then pass the resolved username/host into RequestDynamicVerificationWithUser (update references to spec.User passed into RequestDynamicVerificationWithUser accordingly) so the checks apply to the actual target account that will be modified.
723-733:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve revoke targets with username variants too.
REVOKE ROLE ... FROM userstill checkss.UsersthroughuserExists, so starter mode only accepts the prefixed form for the grantee whileGRANT ROLEalready accepts the unprefixed variant. This leaves half of the advertised flow broken.
🤖 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/errno/errname.go`:
- Line 867: The mapping for ErrUserPrefixMismatch currently returns a generic
transient message; update the ErrUserPrefixMismatch entry in errname.go so it
returns an actionable, contextual message indicating the username/keyspace
mismatch (e.g., "Username does not match the current keyspace; check the
username prefix or selected keyspace") instead of "An internal error occurred,
please retry." to make the auth failure deterministic and debuggable.
In `@pkg/executor/simple.go`:
- Around line 1161-1165: The code currently skips username validation when
s.IsCreateRole is true, allowing unprefixed role names; always apply the starter
username policy by removing the conditional branch and invoking
keyspace.GetUsernamePolicy().ValidateUsername(spec.User.Username)
unconditionally (or add an explicit validation call in the CREATE ROLE path) so
that both user and role creation run the same validation logic (refer to
s.IsCreateRole, keyspace.GetUsernamePolicy().ValidateUsername, and
spec.User.Username).
---
Outside diff comments:
In `@pkg/executor/simple.go`:
- Around line 1783-1795: The dynamic-privilege checks (calls to
RequestDynamicVerificationWithUser) are performed against the unresolved user
name and must run against the resolved variant to prevent bypass; move or
perform the account-variant resolution (call userExistsInternalWithRetryVariants
or the same resolution logic that produces the rewritten account) before the
SYSTEM_USER / RESTRICTED_USER_ADMIN checks and then pass the resolved
username/host into RequestDynamicVerificationWithUser (update references to
spec.User passed into RequestDynamicVerificationWithUser accordingly) so the
checks apply to the actual target account that will be modified.
🪄 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: 0cd14c7b-0820-4018-823f-2e7d9bdb3702
📒 Files selected for processing (19)
cmd/tidb-server/BUILD.bazelerrors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/grant.gopkg/executor/revoke.gopkg/executor/simple.gopkg/executor/test/simpletest/BUILD.bazelpkg/executor/test/simpletest/simple_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace_test.gopkg/keyspace/username_policy.gopkg/meta/meta.gopkg/server/BUILD.bazelpkg/server/conn.gopkg/server/conn_test.gopkg/server/err/error.gopkg/server/server.gopkg/util/dbterror/exeerrors/errors.go
✅ Files skipped from review due to trivial changes (3)
- pkg/server/err/error.go
- cmd/tidb-server/BUILD.bazel
- pkg/server/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/executor/revoke.go
- pkg/errno/errcode.go
- pkg/util/dbterror/exeerrors/errors.go
- pkg/server/server.go
- pkg/keyspace/keyspace_test.go
- pkg/executor/test/simpletest/BUILD.bazel
- pkg/executor/grant.go
- pkg/keyspace/BUILD.bazel
- pkg/meta/meta.go
- errors.toml
- pkg/executor/test/simpletest/simple_test.go
- pkg/server/conn_test.go
- pkg/server/conn.go
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
from
db7db72 to
b85671c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/keyspace/username_policy.go`:
- Around line 36-42: GetUsernamePolicy currently constructs a prefixPolicy with
GetKeyspaceNameBySettings() which may be empty and silently disable prefix
enforcement; update GetUsernamePolicy to validate the keyspace name when
deploymode.IsStarter() and fail-fast with a clear error (panic or process exit)
if GetKeyspaceNameBySettings() returns an empty string, or alternatively add a
clear comment in GetUsernamePolicy explaining that an empty keyspace
intentionally disables prefix enforcement; reference GetUsernamePolicy,
prefixPolicy, GetKeyspaceNameBySettings, and the dependent methods
ValidateUsername/GetUsernameVariants/GetOriginalUsername so the fix either
enforces non-empty keyspace at creation time or documents the deliberate
behavior.
🪄 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: 66ff8992-3998-47d2-a266-790a1bbf7b7c
📒 Files selected for processing (19)
cmd/tidb-server/BUILD.bazelerrors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/grant.gopkg/executor/revoke.gopkg/executor/simple.gopkg/executor/test/simpletest/BUILD.bazelpkg/executor/test/simpletest/simple_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace_test.gopkg/keyspace/username_policy.gopkg/meta/meta.gopkg/server/BUILD.bazelpkg/server/conn.gopkg/server/conn_test.gopkg/server/err/error.gopkg/server/server.gopkg/util/dbterror/exeerrors/errors.go
✅ Files skipped from review due to trivial changes (4)
- pkg/errno/errcode.go
- pkg/errno/errname.go
- pkg/executor/revoke.go
- pkg/server/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/tidb-server/BUILD.bazel
- pkg/executor/grant.go
- pkg/util/dbterror/exeerrors/errors.go
- pkg/server/server.go
- pkg/server/err/error.go
- errors.toml
- pkg/executor/test/simpletest/BUILD.bazel
- pkg/keyspace/keyspace_test.go
- pkg/keyspace/BUILD.bazel
- pkg/executor/test/simpletest/simple_test.go
- pkg/meta/meta.go
- pkg/executor/simple.go
- pkg/server/conn.go
- pkg/server/conn_test.go
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
from
b85671c to
99aef0e
Compare
ti-chi-bot
Bot
added
size/XXL
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 (3)
pkg/server/server.go (1)
1256-1262:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
GetStatusVars()andaddConnMetrics()to usegetTLSState().Both methods directly access
client.tlsConnand will not report TLS information for gateway-forwarded connections. SincegetTLSState()was added to abstract TLS state access (supporting both direct TLS and gateway-forwarded TLS viatlsConnState), these methods should use it for consistency:
GetStatusVars()at lines 1256-1257 inpkg/server/server.goaddConnMetrics()at line 1356 inpkg/server/conn.goWithout this change, status variables and connection metrics will be incomplete for connections using gateway-forwarded TLS.
🤖 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/server/server.go` around lines 1256 - 1262, GetStatusVars() and addConnMetrics() currently read client.tlsConn directly (e.g., the rs[pi.ID] block and the metrics code in addConnMetrics()) which misses gateway-forwarded TLS; call the existing getTLSState(client) helper instead, check its returned *tls.ConnectionState (nil-safe), and derive CipherSuite and Version from that state (using tlsutil.CipherSuiteName and tlsutil.VersionName) before populating rs[pi.ID] and the connection metrics; remove direct uses of client.tlsConn and client.tlsConn.ConnectionState() to ensure both direct TLS and forwarded TLS (tlsConnState) are handled.pkg/executor/simple.go (2)
723-724:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVariant retry missing on user lookup in
executeRevokeRole.Roles are resolved via
userExistsWithRetryVariants(line 696), but the user-existence check here still uses plainuserExists. This is asymmetric withexecuteGrantRole(line 2091), which retries variants on both roles and users. In starter mode, a client issuingREVOKE r FROM uwhere the stored row isSYSTEM.uwill fail at line 728 withErrCannotUser, and even if it didn't, the SQL at lines 739/748 would still target the unresolveduser.Username, so no rows would match.Use the variant-aware helper here as well so the user is resolved and
user.Usernameis rewritten before the DELETE statements run.🛠️ Proposed fix
for _, user := range s.Users { - exists, err := userExists(ctx, e.Ctx(), user.Username, user.Hostname) + exists, err := userExistsWithRetryVariants(ctx, e.Ctx(), &user.Username, user.Hostname) if err != nil { return errors.Trace(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/simple.go` around lines 723 - 724, The user lookup in executeRevokeRole uses userExists but must use userExistsWithRetryVariants (same approach as executeGrantRole) so stored variants (e.g., SYSTEM.u) get resolved and user.Username is rewritten before the DELETEs; replace the call to userExists(ctx, e.Ctx(), user.Username, user.Hostname) with userExistsWithRetryVariants(ctx, e.Ctx(), user.Username, user.Hostname), assign the resolved user/username back to the local user (or update user.Username) and propagate/handle the error exactly as the original path does (including ErrCannotUser) so subsequent SQL targets the resolved username.
1781-1792:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove privilege checks after username variant resolution, or repeat them against the resolved identity.
The SYSTEM_USER and RESTRICTED_USER_ADMIN privilege checks at lines 1784 and 1787 evaluate
spec.Useragainst the client-supplied username before variant resolution.userExistsInternalWithRetryVariantsat line 1792 then resolvesspec.User.Usernameto its keyspace-prefixed variant (e.g.,foo→SYSTEM.foo) by mutating the pointer. Subsequent operations use this resolved identity without re-checking privileges.This allows privilege bypass: a caller without SYSTEM_USER can supply a non-prefixed username that exists as a keyspace-prefixed variant. The initial checks pass (the non-prefixed user has no such grant), then the ALTER proceeds on the prefixed variant. Any SYSTEM_USER or RESTRICTED_USER_ADMIN grants held by the prefixed identity are silently bypassed.
Resolve variants before the privilege checks, or re-verify both dynamic privileges against the resolved identity.
🤖 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 1781 - 1792, The privilege checks for SYSTEM_USER and RESTRICTED_USER_ADMIN are performed against the caller-supplied spec.User before userExistsInternalWithRetryVariants mutates spec.User to a keyspace-prefixed variant, enabling a bypass; fix by resolving username variants first via userExistsInternalWithRetryVariants (call it before the dynamic checks) and then perform the RequestDynamicVerificationWithUser checks against the resolved spec.User (or, alternatively, re-run the checker.RequestDynamicVerificationWithUser checks using the returned resolved identity after userExistsInternalWithRetryVariants); update logic around hasSystemUserPriv, hasRestrictedUserPriv, checker.RequestDynamicVerificationWithUser, sem.IsEnabled(), and any branches that rely on spec.User so all dynamic privilege checks use the final resolved identity.
🧹 Nitpick comments (1)
pkg/executor/simple.go (1)
2465-2499: ⚡ Quick winDocument the mutating semantics of the new helpers.
Both helpers take
name *stringand silently rewrite*nameto the resolved variant on success. That side effect drives later SQL generation inexecuteRevokeRole,executeGrantRole, andexecuteAlterUser, so it is load-bearing but not at all obvious from the signatures. A short doc comment per function describing the contract (looks up the user; on a variant match, rewrites*nameto the resolved username; returns the auth plugin for the internal form) would make this much easier to follow for future readers.📝 Suggested doc comments
+// userExistsWithRetryVariants reports whether (*name, host) exists in mysql.user. +// If the direct lookup misses, it retries each variant produced by the active +// UsernamePolicy and, on a match, rewrites *name to the resolved variant so +// downstream SQL targets the correct row. func userExistsWithRetryVariants(ctx context.Context, sctx sessionctx.Context, name *string, host string) (bool, error) { @@ +// userExistsInternalWithRetryVariants behaves like userExistsWithRetryVariants +// but uses the supplied SQLExecutor (so it shares the caller's transaction) and +// additionally returns the resolved user's auth plugin. func userExistsInternalWithRetryVariants(ctx context.Context, sqlExecutor sqlexec.SQLExecutor, name *string, host string) (bool, string, error) {As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs".
🤖 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 2465 - 2499, Add short doc comments above userExistsWithRetryVariants and userExistsInternalWithRetryVariants that state their mutating contract: they look up the provided username (following username policy variants), and if a variant match is found they rewrite *name to the resolved username; userExistsInternalWithRetryVariants additionally returns the resolved auth plugin string on success; both return (exists, ...) and propagate errors. Mention that callers (e.g., executeRevokeRole, executeGrantRole, executeAlterUser) rely on this side effect for later SQL generation.
🤖 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/keyspace/username_policy.go`:
- Around line 80-85: The GetUsernameVariants implementation generates a prefixed
fallback even when the input is already in a prefixed form (e.g. OTHER.user),
which produces unintended variants like SYSTEM.OTHER.user; update
prefixPolicy.GetUsernameVariants to treat any username that already contains a
dot as an already-prefixed name and return nil (in addition to the existing
checks for empty p.userPrefix and usernames that already start with
p.userPrefix+".") so you only add p.userPrefix + "." + username for plain,
unprefixed usernames.
---
Outside diff comments:
In `@pkg/executor/simple.go`:
- Around line 723-724: The user lookup in executeRevokeRole uses userExists but
must use userExistsWithRetryVariants (same approach as executeGrantRole) so
stored variants (e.g., SYSTEM.u) get resolved and user.Username is rewritten
before the DELETEs; replace the call to userExists(ctx, e.Ctx(), user.Username,
user.Hostname) with userExistsWithRetryVariants(ctx, e.Ctx(), user.Username,
user.Hostname), assign the resolved user/username back to the local user (or
update user.Username) and propagate/handle the error exactly as the original
path does (including ErrCannotUser) so subsequent SQL targets the resolved
username.
- Around line 1781-1792: The privilege checks for SYSTEM_USER and
RESTRICTED_USER_ADMIN are performed against the caller-supplied spec.User before
userExistsInternalWithRetryVariants mutates spec.User to a keyspace-prefixed
variant, enabling a bypass; fix by resolving username variants first via
userExistsInternalWithRetryVariants (call it before the dynamic checks) and then
perform the RequestDynamicVerificationWithUser checks against the resolved
spec.User (or, alternatively, re-run the
checker.RequestDynamicVerificationWithUser checks using the returned resolved
identity after userExistsInternalWithRetryVariants); update logic around
hasSystemUserPriv, hasRestrictedUserPriv,
checker.RequestDynamicVerificationWithUser, sem.IsEnabled(), and any branches
that rely on spec.User so all dynamic privilege checks use the final resolved
identity.
In `@pkg/server/server.go`:
- Around line 1256-1262: GetStatusVars() and addConnMetrics() currently read
client.tlsConn directly (e.g., the rs[pi.ID] block and the metrics code in
addConnMetrics()) which misses gateway-forwarded TLS; call the existing
getTLSState(client) helper instead, check its returned *tls.ConnectionState
(nil-safe), and derive CipherSuite and Version from that state (using
tlsutil.CipherSuiteName and tlsutil.VersionName) before populating rs[pi.ID] and
the connection metrics; remove direct uses of client.tlsConn and
client.tlsConn.ConnectionState() to ensure both direct TLS and forwarded TLS
(tlsConnState) are handled.
---
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 2465-2499: Add short doc comments above
userExistsWithRetryVariants and userExistsInternalWithRetryVariants that state
their mutating contract: they look up the provided username (following username
policy variants), and if a variant match is found they rewrite *name to the
resolved username; userExistsInternalWithRetryVariants additionally returns the
resolved auth plugin string on success; both return (exists, ...) and propagate
errors. Mention that callers (e.g., executeRevokeRole, executeGrantRole,
executeAlterUser) rely on this side effect for later SQL generation.
🪄 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: ae196323-8b31-44b7-b7da-990474a5d712
📒 Files selected for processing (18)
errors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/grant.gopkg/executor/revoke.gopkg/executor/simple.gopkg/executor/test/simpletest/BUILD.bazelpkg/executor/test/simpletest/simple_test.gopkg/keyspace/BUILD.bazelpkg/keyspace/keyspace_test.gopkg/keyspace/username_policy.gopkg/meta/meta.gopkg/server/BUILD.bazelpkg/server/conn.gopkg/server/conn_test.gopkg/server/err/error.gopkg/server/server.gopkg/util/dbterror/exeerrors/errors.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/errno/errcode.go
- pkg/server/err/error.go
- pkg/executor/grant.go
- pkg/util/dbterror/exeerrors/errors.go
- pkg/keyspace/BUILD.bazel
- pkg/keyspace/keyspace_test.go
- pkg/server/BUILD.bazel
- pkg/executor/test/simpletest/BUILD.bazel
- pkg/executor/test/simpletest/simple_test.go
- pkg/meta/meta.go
- pkg/executor/revoke.go
- pkg/server/conn.go
- pkg/server/conn_test.go
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
3 times, most recently
from
3cbe936 to
139bd24
Compare
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
from
139bd24 to
bcbb47a
Compare
| ErrBadUser = 3162 | ||
| ErrUserAlreadyExists = 3163 | ||
| ErrInvalidJSONPathArrayCell = 3165 | ||
| ErrUserPrefixMismatch = 20003 |
There was a problem hiding this comment.
those error is define monolithic increased, maybe move to bottom
There was a problem hiding this comment.
Good catch. ErrUserPrefixMismatch is 20003, so it should be placed at the bottom to keep the error codes monotonic. I’ll move it in errcode.go, errname.go, and the server section of errors.toml. The ddl:1468 entry reuses the existing MySQL ErrUsername code, so that one stays where it is.
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Should we also modify this to userExistsWithRetryVariants?
| return nil | ||
| } | ||
|
|
||
| func (e *SimpleExec) setDefaultRoleRegular(ctx context.Context, s *ast.SetDefaultRoleStmt) error { |
There was a problem hiding this comment.
Should we also modify the userExists in this function to userExistsWithRetryVariants? Or SET DEFAULT ROLE r TO u; without username prefix will fail.
There was a problem hiding this comment.
Done. I updated the SET DEFAULT ROLE paths to use username variants for both users and roles. This covers SET DEFAULT ROLE r TO u, ALL, and NONE; the NONE path also resolves the user before deleting rows from mysql.default_roles.
| if !ok { | ||
| return errors.Trace(exeerrors.ErrPasswordFormat) | ||
| } | ||
| _, err = internalSession.GetSQLExecutor().ExecuteInternal(internalCtx, |
There was a problem hiding this comment.
This path skips the ValidateUsername, so the user can create any user with "grant auto create".
There was a problem hiding this comment.
Done. I added ValidateUsername before the grant auto-create path inserts into mysql.user, so starter mode cannot create an unprefixed user when NO_AUTO_CREATE_USER is disabled.
| } | ||
|
|
||
| func (p prefixPolicy) GetUsernameVariants(username string) []string { | ||
| if p.userPrefix == "" || strings.HasPrefix(username, p.userPrefix+".") || p.ValidateUsernameFormat(username) { |
There was a problem hiding this comment.
The username with dot might have bug here:
CREATE USER `SYSTEM.keao.yang`@'%' IDENTIFIED BY 'pwd';
ALTER USER `keao.yang`@'%' IDENTIFIED BY 'pwd2';
These sqls will fail if the prefix is SYSTEM. GetUsernameVariants will return nil because p.ValidateUsernameFormat(username) returns true. The expected behavior is to return SYSTEM.keao.space.
There was a problem hiding this comment.
Done. GetUsernameVariants now still generates the current keyspace-prefixed variant for dotted logical usernames, e.g. keao.yang -> SYSTEM.keao.yang. Wrong-prefix exact usernames are still rejected instead of falling back to an exact match.
| if resp.Capability&mysql.ClientSSL == 0 { | ||
| gatewaySecureConn := false | ||
| if deploymode.IsStarter() { | ||
| if attrKey := os.Getenv("GATEWAY_SECURECONN_ATTR_KEY"); attrKey != "" { |
There was a problem hiding this comment.
Is this change related to the "username prefix" feature? I'm a little confused 🤔 .
There was a problem hiding this comment.
Good question. This is not for forwarding the username prefix itself. The username still comes from the normal MySQL handshake field (resp.User); this change only consumes a gateway-provided connection attribute that carries the client-to-gateway TLS state.
This came from the same CSE starter/gateway login path: in starter deployments, clients connect through the gateway, and the gateway may terminate TLS before forwarding the connection to TiDB. Without this forwarded TLS state, TiDB can see the gateway-to-TiDB hop as non-TLS, so require_secure_transport=ON may reject it and the connection TLS info would be missing.
So it is related to the starter/gateway connection path from the source PR, but not directly to username-prefix matching. It is guarded by deploymode.IsStarter().
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| for _, variant := range keyspace.GetUsernamePolicy().GetUsernameVariants(cc.user) { | ||
| identity, err := cc.ctx.MatchIdentity(ctx, variant, host) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
This error might be network error, and we just ignore that?
We should distinguish user not found error with the others.
There was a problem hiding this comment.
Good catch. Fixed it to only continue on sessionapi.ErrIdentityNotFound; other errors are returned immediately, so network/internal errors won't be swallowed.
ChangRui-Ryan
force-pushed
the
changrui_cse_1877
branch
from
d64abbc to
37384d3
Compare
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
do-not-merge/hold
| logutil.Logger(ctx).Info("found user identity with variants", | ||
| zap.String("user", cc.user), | ||
| zap.String("matched-user", variant), | ||
| zap.String("host", host), | ||
| ) |
There was a problem hiding this comment.
Is this log necessary?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, tiancaiamao, YangKeao 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 |
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
pingcap#68355 added pkg/keyspace.UsernamePolicy. Use it: strip the current keyspace prefix and match the original name, matching how tidb-cse does it. Outside Starter the default policy's GetOriginalUsername returns "", so both helpers return false (strict SEM isn't reachable there anyway). TestStatement_RestrictedUserOperations now wraps the assertions in a setupStarterKeyspace helper (skipped on classic) and uses the prefixed form ("SYSTEM.cloud_admin" etc.). Two mustPass cases document the inherited behaviour: the unprefixed form and a foreign-keyspace prefix both fall through (not rejected here; the executor's variant lookup handles cross- keyspace isolation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 participants
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
In starter deployment mode, TiDB needs to isolate user and role metadata by the assigned keyspace. User accounts are stored with the keyspace name as a username prefix, for example
SYSTEM.user, but several authentication and privilege-management paths still treated the client-provided username literally.As a result, starter deployments could not consistently support connecting with an unprefixed username that should map to the current keyspace, and SQL statements such as
GRANT,REVOKE,ALTER USER, and role grants could fail to resolve existing keyspace-prefixed users. TiDB also lacked validation to prevent creating or renaming users into an unexpected keyspace prefix in starter mode.What changed and how does it work?
This PR adds a deployment-mode-aware username policy under the keyspace package. In non-starter modes, the policy keeps the existing behavior unchanged. In starter mode, the policy uses the current keyspace name as the required username prefix.
The authentication path now retries user identity matching with the current keyspace-prefixed username variant. For example, when TiDB runs in starter mode with keyspace
SYSTEM, a client login asusercan match the stored accountSYSTEM.user. If the client provides a username that already looks prefixed but does not match the assigned keyspace, TiDB returns a dedicated prefix-mismatch error instead of silently matching another tenant's account.Privilege-related statements now resolve existing users through the same username-variant logic for
GRANT,REVOKE,GRANT ROLE,REVOKE ROLE, andALTER USER, so users can be referenced consistently in starter mode while the underlying privilege tables continue to store keyspace-prefixed usernames.CREATE USERandRENAME USERvalidate new usernames and reject names that do not start with the current keyspace prefix.This PR also adds starter-only handling for forwarded gateway TLS connection state. When starter mode receives the configured gateway secure-connection attribute, TiDB treats the connection as secure for
require_secure_transportand exposes the forwarded TLS state in connection info.Tests were added for the username policy, starter-mode authentication identity matching, forwarded gateway TLS attributes, and SQL privilege/user-management behavior.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
SYSTEM.username). User creation, rename, and role management operations enforce this requirement with validation error messages.