Repository navigation
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note
|
@bb7133 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. |
|
[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 |
ti-chi-bot
Bot
added
the
size/XXL
|
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 mTLS-only internal HTTP endpoints for user reset/create/delete/role binding; implements handlers executing internal SQL with password redaction and stmt-event audit emission; wires routes, updates BUILD/test deps, adds mTLS end-to-end tests, and isolates a memory-leak test in a subprocess. ChangesUser Administration Feature
Test Isolation
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant Handler
participant InternalSession
participant AuditExtension
Client->>HTTPServer: HTTPS request (/internal/v1/users/...)
HTTPServer->>Handler: route -> ServeHTTP
Handler->>Handler: requireMTLS, parse JSON, validate
Handler->>InternalSession: ExecuteInternal(SQL statement)
InternalSession-->>Handler: result / error
Handler->>AuditExtension: auditUserAdminStmt (redacted SQL AST/event)
Handler-->>Client: HTTP Response (2xx/4xx)
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/handler/tests/http_handler_test.go (1)
437-462:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing import:
mockstorepackage is not imported.Line 439 calls
mockstore.NewMockStore()but themockstorepackage is not in the imports. This will cause a compilation error. Looking at the imports, onlyteststoreis imported fromgithubproxy.fjygbaifeng.eu.org/pingcap/tidb/pkg/store/mockstore/teststore.Additionally, lines 714-726 contain duplicate code—an identical if-block checking
parentCert == nil || parentKey == nilappears twice, with the second occurrence being unreachable dead code.🐛 Proposed fixes
For the missing import, add the mockstore package:
import ( ... + "github.com/pingcap/tidb/pkg/store/mockstore" "github.com/pingcap/tidb/pkg/store/mockstore/teststore" ... )Alternatively, use the already-imported
teststorepackage:func (ts *basicHTTPHandlerTestSuite) startServerWithConfig(t *testing.T, cfg *config.Config) { var err error - ts.store, err = mockstore.NewMockStore() + ts.store, err = teststore.NewMockStoreWithoutBootstrap() require.NoError(t, err)For the duplicate code, remove the second if-block at lines 721-726.
🤖 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/handler/tests/http_handler_test.go` around lines 437 - 462, The test fails to compile because mockstore is used but not imported and there is unreachable duplicate code: add the missing import for the mockstore package (so mockstore.NewMockStore() resolves) or replace the call with the already-imported teststore usage if you intend to use teststore; also remove the duplicated conditional block that checks parentCert == nil || parentKey == nil (the second identical if-block around the lines checking parentCert/parentKey) to eliminate dead code; update imports and delete the redundant if-block in the same test file so startServerWithConfig and related setup compile cleanly.
🧹 Nitpick comments (1)
pkg/server/tidb_library_test.go (1)
31-40: ⚡ Quick winSubprocess isolation pattern is well-implemented.
The environment variable guard prevents recursive spawning, the regex correctly targets only this test, and error handling properly propagates subprocess failures.
⏱️ Optional: Add subprocess timeout for robustness
Consider adding a timeout to prevent the parent from hanging indefinitely if the subprocess stalls:
if os.Getenv("TIDB_MEMORY_LEAK_SUBPROCESS") != "1" { // Run in a subprocess to avoid interference from other tests in this package. - cmd := exec.Command(os.Args[0], "-test.run", "^TestMemoryLeak$", "-test.v", "-test.count=1") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run", "^TestMemoryLeak$", "-test.v", "-test.count=1") cmd.Env = append(os.Environ(), "TIDB_MEMORY_LEAK_SUBPROCESS=1") output, err := cmd.CombinedOutput() if err != nil {This would require importing
contextandtime. The current implementation relies on the overall test framework timeout, which is acceptable but less explicit.🤖 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/tidb_library_test.go` around lines 31 - 40, Add a timeout to the subprocess invocation that runs TestMemoryLeak to avoid hanging: replace the exec.Command + CombinedOutput usage with a context-aware command (create a context with timeout via context.WithTimeout and use exec.CommandContext) so TestMemoryLeak’s subprocess is killed after the timeout, ensure you still set the TIDB_MEMORY_LEAK_SUBPROCESS env and capture CombinedOutput from the command before checking err (and cancel the context after use); update imports to include context and time and reference TestMemoryLeak, TIDB_MEMORY_LEAK_SUBPROCESS, exec.CommandContext, and cmd.CombinedOutput in the change.
🤖 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/handler/tests/http_handler_test.go`:
- Around line 714-726: Remove the duplicated conditional block that checks and
sets parentCert/parentKey; the second identical if (parentCert == nil ||
parentKey == nil) { template.IsCA = true; template.KeyUsage |=
x509.KeyUsageCertSign; parentCert = &template; parentKey = privateKey } is dead
code—delete the second occurrence so the logic that initializes template,
parentCert, and parentKey runs only once and avoid redundant assignments.
---
Outside diff comments:
In `@pkg/server/handler/tests/http_handler_test.go`:
- Around line 437-462: The test fails to compile because mockstore is used but
not imported and there is unreachable duplicate code: add the missing import for
the mockstore package (so mockstore.NewMockStore() resolves) or replace the call
with the already-imported teststore usage if you intend to use teststore; also
remove the duplicated conditional block that checks parentCert == nil ||
parentKey == nil (the second identical if-block around the lines checking
parentCert/parentKey) to eliminate dead code; update imports and delete the
redundant if-block in the same test file so startServerWithConfig and related
setup compile cleanly.
---
Nitpick comments:
In `@pkg/server/tidb_library_test.go`:
- Around line 31-40: Add a timeout to the subprocess invocation that runs
TestMemoryLeak to avoid hanging: replace the exec.Command + CombinedOutput usage
with a context-aware command (create a context with timeout via
context.WithTimeout and use exec.CommandContext) so TestMemoryLeak’s subprocess
is killed after the timeout, ensure you still set the
TIDB_MEMORY_LEAK_SUBPROCESS env and capture CombinedOutput from the command
before checking err (and cancel the context after use); update imports to
include context and time and reference TestMemoryLeak,
TIDB_MEMORY_LEAK_SUBPROCESS, exec.CommandContext, and cmd.CombinedOutput in the
change.
🪄 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: eaf93fba-4143-41b0-8474-7840b39de82f
📒 Files selected for processing (7)
pkg/server/handler/BUILD.bazelpkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_test.gopkg/server/handler/tests/main_test.gopkg/server/handler/user_admin_handler.gopkg/server/http_status.gopkg/server/tidb_library_test.go
|
|
|
/retest-required |
bb7133
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/server/handler/user_admin_handler.go (2)
532-534: 💤 Low valueRemove unused
buildAlterUserExpireSQLfunction.This function is defined at lines 532–534 but is never called anywhere in the codebase. Remove it to reduce dead code.
🤖 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/handler/user_admin_handler.go` around lines 532 - 534, The function buildAlterUserExpireSQL is dead code and should be removed: delete the entire buildAlterUserExpireSQL function definition from user_admin_handler.go (the ALTER USER ... PASSWORD EXPIRE helper) and then run a quick build/lint (go build/go vet or golangci-lint) to ensure no remaining references or unused-import warnings remain; if any tests or callers exist, update them to remove usage, otherwise just remove the function.
516-523: ⚖️ Poor tradeoff
requireMTLSrelies entirely on configuration checks without validating that TLS was actually negotiated.The function checks if
cfg.Security.ClusterSSLCAandcfg.Security.ClusterVerifyCNare configured, but never verifies that the connection hasreq.TLSset. If the status server's listener configuration is misconfigured or bypassed, this guard would not catch it.The comment mentions cmux may hide TLS state, but
req.TLSis successfully accessed elsewhere in this same file (clientCertCommonName,auditUserFromRequest). Adding a secondary check forreq.TLS != nilwould be a reasonable defensive improvement:if cfg != nil && cfg.Security.ClusterSSLCA != "" && len(cfg.Security.ClusterVerifyCN) > 0 && req.TLS != nil { return nil }Alternatively, if cmux truly makes
req.TLSunreliable in some cases, document that security guarantee depends entirely on listener-level TLS enforcement, so operators understand that misconfiguring the status server's TLS settings will bypass this check.🤖 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/handler/user_admin_handler.go` around lines 516 - 523, The requireMTLS function currently only checks config flags (cfg.Security.ClusterSSLCA and cfg.Security.ClusterVerifyCN) but never verifies that TLS was negotiated; update requireMTLS to also ensure the incoming *http.Request has req.TLS != nil before returning nil (i.e., only succeed when both the config fields are set and req.TLS is non-nil), referencing the requireMTLS function and the req.TLS field used elsewhere (e.g., clientCertCommonName/auditUserFromRequest) so the guard actually verifies a negotiated TLS connection; if you believe cmux makes req.TLS unreliable, instead add an explanatory comment in requireMTLS clarifying that enforcement relies on listener-level TLS and that misconfigured listeners can bypass this check.
🤖 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.
Nitpick comments:
In `@pkg/server/handler/user_admin_handler.go`:
- Around line 532-534: The function buildAlterUserExpireSQL is dead code and
should be removed: delete the entire buildAlterUserExpireSQL function definition
from user_admin_handler.go (the ALTER USER ... PASSWORD EXPIRE helper) and then
run a quick build/lint (go build/go vet or golangci-lint) to ensure no remaining
references or unused-import warnings remain; if any tests or callers exist,
update them to remove usage, otherwise just remove the function.
- Around line 516-523: The requireMTLS function currently only checks config
flags (cfg.Security.ClusterSSLCA and cfg.Security.ClusterVerifyCN) but never
verifies that TLS was negotiated; update requireMTLS to also ensure the incoming
*http.Request has req.TLS != nil before returning nil (i.e., only succeed when
both the config fields are set and req.TLS is non-nil), referencing the
requireMTLS function and the req.TLS field used elsewhere (e.g.,
clientCertCommonName/auditUserFromRequest) so the guard actually verifies a
negotiated TLS connection; if you believe cmux makes req.TLS unreliable, instead
add an explanatory comment in requireMTLS clarifying that enforcement relies on
listener-level TLS and that misconfigured listeners can bypass this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d18f61b9-6808-4b16-8216-668fee1ee129
📒 Files selected for processing (2)
pkg/server/handler/BUILD.bazelpkg/server/handler/user_admin_handler.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68502 +/- ##
================================================
- Coverage 76.3246% 74.8981% -1.4265%
================================================
Files 2041 2031 -10
Lines 562567 570640 +8073
================================================
- Hits 429377 427399 -1978
- Misses 132279 143133 +10854
+ Partials 911 108 -803
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
expxiaoli
left a comment
expxiaoli
left a comment
There was a problem hiding this comment.
I find 2 bugs. Please check.
Code Review: Internal User Admin APIsOverall the feature design is reasonable, code structure is clean, and tests cover mTLS happy/unhappy paths. However, I found several issues that should be addressed before merge, including 3 P0 security concerns. 🔴 P0 — Blocking Issues1. func requireMTLS(_ *http.Request, cfg *config.Config) error {
if cfg != nil && cfg.Security.ClusterSSLCA != "" && len(cfg.Security.ClusterVerifyCN) > 0 {
return nil
}
return errors.New("client certificate invalid or missing (mTLS failure)")
}The Suggestion: Add a defensive check ( 2. Audit log leaks real passwords on success paths For stmtText = buildCreateUserSQL(username, defaultUserHost, password) // real password
// ...
auditUserAdminStmt(req, stmtText, nil) // password goes into audit log via OriginalText()Same for stmtText := buildAlterUserPasswordSQL(username, host, payload.NewPassword) // real password
auditUserAdminStmt(req, stmtText, nil)The success paths pass the real password into audit log text. The comment says "the redact flag alone should control whether OriginalText() or SQLDigest() is used," but for a security API, passwords should never appear in audit logs — even with 3.
🟡 P1 — Important Issues4. Multi-host user ambiguity (also noted by @expxiaoli)
5.
6. if len(hosts) == 0 {
auditUserAdminStmt(req, buildDropUserSQL(auditUser, defaultUserHost), nil)
w.WriteHeader(http.StatusNoContent)
return
}Deleting a non-existent user returns 204 No Content rather than 404 Not Found. The client cannot distinguish "already deleted" from "never existed." 7. All 4 handlers use value receivers for func (h UserResetPasswordHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {Value receivers copy the struct on every call. Currently safe since all fields are pointers ( 🟢 P2 — Suggestions8. 9. Duplicate code block in 10. 11. Role grants are non-atomic — if a GRANT fails mid-loop, previously granted roles are not rolled back. Consider whether this is acceptable or needs transactional semantics. 12. Route registration without HTTP method constraints — Summary
The P0 security issues (especially #1 and #2) should be resolved before this PR lands. |
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 (3)
pkg/server/handler/user_admin_handler.go (2)
281-282:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame password leakage issue: real password in audit log.
The comment explicitly says "rebuild with real password so audit log can show it when redact=OFF" — this is the P0 security concern. Passwords should never appear in audit logs regardless of redact settings; use
auditRedactedPasswordhere as well.Proposed fix
- // username and password are valid; rebuild with real password so audit log can show it when redact=OFF - stmtText = buildCreateUserSQL(username, defaultUserHost, password) + // Keep password redacted in audit log; the real password is only used for the actual CREATE USER call + stmtText = buildCreateUserSQL(username, defaultUserHost, auditRedactedPassword)🤖 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/handler/user_admin_handler.go` around lines 281 - 282, The code currently rebuilds stmtText with the real password via buildCreateUserSQL(username, defaultUserHost, password), which leaks secrets into audit logs; instead pass the redacted token so the audit never contains the real password (use auditRedactedPassword). Update the call that assigns stmtText to use buildCreateUserSQL(username, defaultUserHost, auditRedactedPassword) and ensure any nearby comments about "rebuild with real password" are removed or adjusted accordingly.
529-536:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftmTLS check doesn't verify the client certificate per-request.
requireMTLSonly checks that the server is configured for mTLS (non-emptyClusterSSLCAandClusterVerifyCN), but it discards the*http.Requestand never inspectsreq.TLSorreq.TLS.PeerCertificates. If cmux or deployment setup allows unverified requests to reach this handler, they pass through.At minimum, when
req.TLSis available, verify thatPeerCertificatesis non-empty and that the CommonName matchesClusterVerifyCN:Proposed direction
-func requireMTLS(_ *http.Request, cfg *config.Config) error { - // cmux may hide TLS state from net/http, so this check relies on the status - // listener being configured to verify client certificate CNs. +func requireMTLS(req *http.Request, cfg *config.Config) error { if cfg != nil && cfg.Security.ClusterSSLCA != "" && len(cfg.Security.ClusterVerifyCN) > 0 { + // When TLS state is visible, verify the client certificate CN + if req.TLS != nil { + if len(req.TLS.PeerCertificates) == 0 { + return errors.New("client certificate required but not presented") + } + cn := req.TLS.PeerCertificates[0].Subject.CommonName + valid := false + for _, allowed := range cfg.Security.ClusterVerifyCN { + if cn == allowed { + valid = true + break + } + } + if !valid { + return errors.Errorf("client certificate CN %q not in allowed list", cn) + } + } + // If req.TLS is nil (cmux hides it), fall through — server-level config is trusted return nil } return errors.New("client certificate invalid or missing (mTLS failure)") }🤖 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/handler/user_admin_handler.go` around lines 529 - 536, The requireMTLS function currently only validates mTLS configuration flags; change it to also inspect the incoming *http.Request (req) TLS state: if req.TLS is non-nil, ensure req.TLS.PeerCertificates is non-empty and that at least one PeerCertificate's Subject.CommonName equals (or is contained in) cfg.Security.ClusterVerifyCN (handle multiple allowed CNs if ClusterVerifyCN is a comma/list); keep the existing cfg.Security.ClusterSSLCA and ClusterVerifyCN configuration checks as a fallback but return an error ("client certificate invalid or missing (mTLS failure)") when no TLS info or no matching CommonName is found. Ensure you reference requireMTLS, req.TLS, req.TLS.PeerCertificates, Subject.CommonName, cfg.Security.ClusterSSLCA and cfg.Security.ClusterVerifyCN when making the change.pkg/server/handler/tests/http_handler_test.go (1)
666-669:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest asserts plaintext passwords appear in audit logs — this validates a P0 security issue.
The test asserts that actual passwords (
Passw0rd!,KXMF*ivNaLWd*oNhKC+REY2+) are present in the audit SQL strings. This contradicts the expected secure behavior where passwords should be redacted. Note that the "forbidden without client cert" subtest at line 587 correctly expects******masking.If the handler is supposed to redact passwords (as indicated by P0 feedback in PR comments), these assertions should check for redaction instead:
🔒 Proposed fix to expect redacted passwords
require.Contains(t, records[0].sql, "CREATE USER") - require.Contains(t, records[0].sql, "Passw0rd!") + require.Contains(t, records[0].sql, "******") require.Contains(t, records[1].sql, "ALTER USER") - require.Contains(t, records[1].sql, "KXMF*ivNaLWd*oNhKC+REY2+") + require.Contains(t, records[1].sql, "******")🤖 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/handler/tests/http_handler_test.go` around lines 666 - 669, The test is asserting plaintext passwords appear in audit SQL (records[0].sql and records[1].sql) which contradicts the intended redaction; update the assertions in the test (http_handler_test.go) to expect the masked value "******" instead of the literal passwords ("Passw0rd!", "KXMF*ivNaLWd*oNhKC+REY2+"), and remove any other checks that assert raw passwords in the audit SQL strings so the test verifies redaction like the existing "forbidden without client cert" subtest does.
🤖 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/handler/user_admin_handler.go`:
- Around line 143-149: The current code builds stmtText with the real password
via buildAlterUserPasswordSQL and passes it to auditUserAdminStmt on both error
and success, leaking the password on success; change the success path to send a
redacted statement instead: after stmtText := buildAlterUserPasswordSQL(...,
payload.NewPassword) also create redactedStmtText :=
buildAlterUserPasswordSQL(username, defaultUserHost, "REDACTED") (or otherwise
redact the password using the same helper/parseStmtNode behavior), call
auditUserAdminStmt(req, stmtText, err) only on error, and call
auditUserAdminStmt(req, redactedStmtText, nil) on success so the real password
is never sent to audit logs.
- Around line 207-211: The handler currently returns 204 for a non-existent user
which hides "not found" from clients; in the branch inside user_admin_handler.go
where it checks if !exists (the block that calls auditUserAdminStmt(req,
buildDropUserSQL(auditUser, defaultUserHost), nil)), change the response to
return http.StatusNotFound instead of http.StatusNoContent and update the audit
call to record the not-found outcome (e.g., pass an error or a descriptive
result instead of nil) so logs reflect "user not found" while the HTTP response
is 404.
---
Outside diff comments:
In `@pkg/server/handler/tests/http_handler_test.go`:
- Around line 666-669: The test is asserting plaintext passwords appear in audit
SQL (records[0].sql and records[1].sql) which contradicts the intended
redaction; update the assertions in the test (http_handler_test.go) to expect
the masked value "******" instead of the literal passwords ("Passw0rd!",
"KXMF*ivNaLWd*oNhKC+REY2+"), and remove any other checks that assert raw
passwords in the audit SQL strings so the test verifies redaction like the
existing "forbidden without client cert" subtest does.
In `@pkg/server/handler/user_admin_handler.go`:
- Around line 281-282: The code currently rebuilds stmtText with the real
password via buildCreateUserSQL(username, defaultUserHost, password), which
leaks secrets into audit logs; instead pass the redacted token so the audit
never contains the real password (use auditRedactedPassword). Update the call
that assigns stmtText to use buildCreateUserSQL(username, defaultUserHost,
auditRedactedPassword) and ensure any nearby comments about "rebuild with real
password" are removed or adjusted accordingly.
- Around line 529-536: The requireMTLS function currently only validates mTLS
configuration flags; change it to also inspect the incoming *http.Request (req)
TLS state: if req.TLS is non-nil, ensure req.TLS.PeerCertificates is non-empty
and that at least one PeerCertificate's Subject.CommonName equals (or is
contained in) cfg.Security.ClusterVerifyCN (handle multiple allowed CNs if
ClusterVerifyCN is a comma/list); keep the existing cfg.Security.ClusterSSLCA
and ClusterVerifyCN configuration checks as a fallback but return an error
("client certificate invalid or missing (mTLS failure)") when no TLS info or no
matching CommonName is found. Ensure you reference requireMTLS, req.TLS,
req.TLS.PeerCertificates, Subject.CommonName, cfg.Security.ClusterSSLCA and
cfg.Security.ClusterVerifyCN when making the change.
🪄 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: dd3ee9d4-df4b-45c6-9e76-f8c0485ce172
📒 Files selected for processing (3)
pkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_test.gopkg/server/handler/user_admin_handler.go
1753c19 to
b4039c8
Compare
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)
pkg/server/handler/user_admin_handler.go (1)
529-535:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAuthorize from the request's client cert, not just server config.
This returns success whenever mTLS is configured, even if the current request has no TLS state or peer certificate. If these handlers are reached behind TLS termination or with
req.TLS == nil, this becomes a config flag, not authentication. Fail closed on missingreq.TLS/PeerCertificatesand verify the presented CN againstClusterVerifyCN.Proposed fix
-func requireMTLS(_ *http.Request, cfg *config.Config) error { - // cmux may hide TLS state from net/http, so this check relies on the status - // listener being configured to verify client certificate CNs. - if cfg != nil && cfg.Security.ClusterSSLCA != "" && len(cfg.Security.ClusterVerifyCN) > 0 { - return nil - } - return errors.New("client certificate invalid or missing (mTLS failure)") +func requireMTLS(req *http.Request, cfg *config.Config) error { + if cfg == nil || cfg.Security.ClusterSSLCA == "" || len(cfg.Security.ClusterVerifyCN) == 0 { + return errors.New("client certificate invalid or missing (mTLS failure)") + } + if req == nil || req.TLS == nil || len(req.TLS.PeerCertificates) == 0 { + return errors.New("client certificate invalid or missing (mTLS failure)") + } + cn := strings.TrimSpace(req.TLS.PeerCertificates[0].Subject.CommonName) + for _, allowed := range cfg.Security.ClusterVerifyCN { + if strings.TrimSpace(allowed) == cn { + return nil + } + } + return errors.New("client certificate invalid or missing (mTLS failure)") }🤖 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/handler/user_admin_handler.go` around lines 529 - 535, requireMTLS currently authorizes based only on config presence; change it to fail closed by inspecting the incoming *http.Request TLS state and peer certificates: verify req != nil && req.TLS != nil && len(req.TLS.PeerCertificates) > 0, extract the client cert CN from req.TLS.PeerCertificates[0].Subject.CommonName and check it against cfg.Security.ClusterVerifyCN (iterate allowed CNs or use a set); only return nil when a presented CN matches an allowed CN and cfg.Security.ClusterSSLCA is non-empty, otherwise return the existing "client certificate invalid or missing (mTLS failure)" error.
♻️ Duplicate comments (2)
pkg/server/handler/user_admin_handler.go (2)
207-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn 404 when
user@'%'doesn't exist.Now that delete only targets the default-host entry,
204 No Contenthides the difference between "deleted" and "never existed". Clients need a real not-found response here, and the audit event should reflect that outcome.Proposed fix
if !exists { - auditUserAdminStmt(req, buildDropUserSQL(auditUser, defaultUserHost), nil) - w.WriteHeader(http.StatusNoContent) + err := errors.New("user not found") + auditUserAdminStmt(req, buildDropUserSQL(auditUser, defaultUserHost), err) + WriteErrorWithCode(w, http.StatusNotFound, err) return }🤖 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/handler/user_admin_handler.go` around lines 207 - 210, The handler currently returns 204 when the default-host user doesn't exist; change it to return 404 and audit that the user was not found. In the branch checking exists (the if !exists block in user_admin_handler.go), call auditUserAdminStmt with an error or a "not found" result (so the audit reflects the not-found outcome for buildDropUserSQL(auditUser, defaultUserHost)), then write http.StatusNotFound instead of http.StatusNoContent and return; ensure the rest of the delete path still audits successful deletions as before.
143-149:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNever pass the raw password into
auditUserAdminStmt.
stmtTextis built frompayload.NewPasswordand reused for both success and error events, so any audit extension that readsOriginalText()still gets the secret. For password-bearing statements, the audited SQL needs to stay masked on every path.Proposed fix
- stmtText := buildAlterUserPasswordSQL(username, defaultUserHost, payload.NewPassword) + stmtText := buildAlterUserPasswordSQL(username, defaultUserHost, auditRedactedPassword) if _, err := se.ExecuteInternal(ctx, "ALTER USER %?@%? IDENTIFIED BY %?", username, defaultUserHost, payload.NewPassword); err != nil { auditUserAdminStmt(req, stmtText, err) WriteErrorWithCode(w, http.StatusInternalServerError, err) return }🤖 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/handler/user_admin_handler.go` around lines 143 - 149, The audit currently logs stmtText (built by buildAlterUserPasswordSQL using payload.NewPassword) which leaks the raw password on both success and error paths; instead, ensure auditUserAdminStmt is always passed a masked statement: after calling buildAlterUserPasswordSQL or before calling auditUserAdminStmt, create a maskedStmt that replaces the password portion (payload.NewPassword) with a constant mask (e.g., '***' or 'REDACTED') and use maskedStmt for both the error and success audit calls around se.ExecuteInternal; update references around buildAlterUserPasswordSQL, stmtText, auditUserAdminStmt and the se.ExecuteInternal call to use the masked value so the real password is never sent to the auditor.
🧹 Nitpick comments (1)
pkg/server/handler/tests/http_handler_test.go (1)
567-580: ⚡ Quick winThis subtest doesn't actually cover "no client certificate".
It calls the handler directly, never starts the HTTPS server, and never enables
ClusterVerifyCN, so this still passes even ifrequireMTLSignoresreq.TLS. Please make this an end-to-end request against a server started withClusterVerifyCNconfigured and a client created without a cert, then assert the 403 from the real TLS handshake path.🤖 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/handler/tests/http_handler_test.go` around lines 567 - 580, The subtest "forbidden without client cert" currently invokes handler.NewUserResetPasswordHandler directly and doesn't exercise TLS; modify the test to start an HTTPS server using the server certificates from artifacts (e.g., via httptest.NewUnstartedServer/StartTLS or net/http.Server with TLS) with cfg.Security.ClusterVerifyCN enabled, and then create an HTTP client whose TLSClientConfig trusts the CA but does not present a client certificate; perform a real POST to the server URL (not calling the handler directly) and assert the response is http.StatusForbidden to ensure the mTLS handshake path is exercised.
🤖 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 `@pkg/server/handler/user_admin_handler.go`:
- Around line 529-535: requireMTLS currently authorizes based only on config
presence; change it to fail closed by inspecting the incoming *http.Request TLS
state and peer certificates: verify req != nil && req.TLS != nil &&
len(req.TLS.PeerCertificates) > 0, extract the client cert CN from
req.TLS.PeerCertificates[0].Subject.CommonName and check it against
cfg.Security.ClusterVerifyCN (iterate allowed CNs or use a set); only return nil
when a presented CN matches an allowed CN and cfg.Security.ClusterSSLCA is
non-empty, otherwise return the existing "client certificate invalid or missing
(mTLS failure)" error.
---
Duplicate comments:
In `@pkg/server/handler/user_admin_handler.go`:
- Around line 207-210: The handler currently returns 204 when the default-host
user doesn't exist; change it to return 404 and audit that the user was not
found. In the branch checking exists (the if !exists block in
user_admin_handler.go), call auditUserAdminStmt with an error or a "not found"
result (so the audit reflects the not-found outcome for
buildDropUserSQL(auditUser, defaultUserHost)), then write http.StatusNotFound
instead of http.StatusNoContent and return; ensure the rest of the delete path
still audits successful deletions as before.
- Around line 143-149: The audit currently logs stmtText (built by
buildAlterUserPasswordSQL using payload.NewPassword) which leaks the raw
password on both success and error paths; instead, ensure auditUserAdminStmt is
always passed a masked statement: after calling buildAlterUserPasswordSQL or
before calling auditUserAdminStmt, create a maskedStmt that replaces the
password portion (payload.NewPassword) with a constant mask (e.g., '***' or
'REDACTED') and use maskedStmt for both the error and success audit calls around
se.ExecuteInternal; update references around buildAlterUserPasswordSQL,
stmtText, auditUserAdminStmt and the se.ExecuteInternal call to use the masked
value so the real password is never sent to the auditor.
---
Nitpick comments:
In `@pkg/server/handler/tests/http_handler_test.go`:
- Around line 567-580: The subtest "forbidden without client cert" currently
invokes handler.NewUserResetPasswordHandler directly and doesn't exercise TLS;
modify the test to start an HTTPS server using the server certificates from
artifacts (e.g., via httptest.NewUnstartedServer/StartTLS or net/http.Server
with TLS) with cfg.Security.ClusterVerifyCN enabled, and then create an HTTP
client whose TLSClientConfig trusts the CA but does not present a client
certificate; perform a real POST to the server URL (not calling the handler
directly) and assert the response is http.StatusForbidden to ensure the mTLS
handshake path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb69fda1-1f04-4f2d-89de-ff03d7eee9e3
📒 Files selected for processing (3)
pkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_test.gopkg/server/handler/user_admin_handler.go
|
/test pull-integration-realcluster-test-next-gen |
|
@bb7133: The specified target(s) for Use 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. |
b4039c8 to
39ab371
Compare
expxiaoli
left a comment
expxiaoli
left a comment
There was a problem hiding this comment.
There is a bug. Please check.
|
/test mysql-test |
|
@bb7133: The specified target(s) for Use 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. |
The ti-chi-robot bazel update incorrectly removed //pkg/session/types from the handler package deps. user_admin_handler.go explicitly imports pkg/session/types (as sessiontypes) and requires this dep.
pkg/session/types does not exist on master; the Session interface was moved to pkg/session/sessionapi in the master branch. Update user_admin_handler.go to import pkg/session/sessionapi and update BUILD.bazel to use //pkg/session/sessionapi accordingly.
999507c to
4e90176
Compare
This branch has not been deployed
4 participants
What problem does this PR solve?
Issue Number: ref #66620
Port of #66622 to
masterbranch.TiDB Cloud needs a safe way to manage temporary database users through the status API because
cloud_admincan no longer rely on direct SQL user-management privileges. This PR adds mTLS-gated user admin endpoints on the status server.What changed and how does it work?
POST /v1/internal/users— creates a new internal database user with username/password; returns201or409if user already existsPOST /internal/v1/users/{username}/reset-password— generates a new secure random password, updates the user password immediatelyPOST /internal/v1/users/{username}/roles— grants system roles to the target userDELETE /internal/v1/users/{username}— deletes the user; returns409if it is the last remaining userAll endpoints are restricted to mTLS-authenticated requests. All accesses produce structured audit records.
Cherry-picked commits from #66622 (
release-8.5) ontomaster, with conflict resolution:pkg/server: add user apiupdate reset password APISupport audit logConflict resolutions:
pkg/server/handler/BUILD.bazel: merged new deps (//pkg/extension,//pkg/parser,//pkg/parser/auth) with master's existing//pkg/domain/serverinfo; dropped//pkg/parser/model(not imported by the code)pkg/server/handler/tests/BUILD.bazel: kept master'sshard_count = 45; added//pkg/extensiondep alongside master's existing//pkg/domain/serverinfoand dxf depspkg/server/handler/tests/http_handler_test.go: kept bothdomain/serverinfoandextensionimportsCheck List
Tests
Manual test: see #66622 for detailed manual verification steps.
Release note
Summary by CodeRabbit