Repository navigation
fix(cdc): repair default internal principal without relaxing authorization - #29635
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 1bc7cb587cc2a7c9c0f615fdd15bed01a289ec01 against 4.2-dev@5c4590a55d77c93cde687ab824e9238ddce4d9c1: APPROVE — no verified blocking findings.
The fix belongs at the existing internal-session producer. Both override layers are applied before normalizing only the unnamed final (account,user,role)=(0,0,0) to the catalog's reserved sys/root/moadmin principal. Explicit SQL-task definers, mismatched names and other selected roles continue through the existing binding and current-catalog authorization. This closes the common cause of CDC's task-definition read and failed-state write rejection; neither Session.isInternal nor internal-AST authorization is enabled.
The separate request-local SQL-source flag is justified: authenticated identity and statement origin are different contracts, and explicit root execution must remain distinguishable from the default internal producer. Statement copies/rewrite preserve the flag, ANALYZE carries it into its derived query, and the existing SQL-mode/diagnostic/query-ID execution flags retain their semantics. No source marker provides an authorization exemption. No new catalog lookup, worker, cache or persistent state is added; the extra work is bounded identity checks and assignments. No production performance improvement is claimed.
Independent reviewer QA passed on this exact head using a temporary overlay and the existing authenticated 3-CN fixture, with Go 1.26.4 darwin/arm64, freshly built branch-native artifacts, GOMAXPROCS=2 and serial package/test execution:
- A conditional running-to-failed UPDATE through public
ExecWithStatuschanges exactly one row; retry changes zero rows, with literal stored-state/error checks. - Explicit root with the public role is denied both SELECT and UPDATE. This challenges role enforcement with a valid catalog username, beyond denial of the synthetic
internalname. - Numeric command overrides cannot replace an explicit mismatched base username; denied writes have no side effects.
- A cancelled write has no side effects, and a subsequent call through the same executor successfully updates the row.
The probe completed with zero failures/skips (one parent and three subtests; package 15.361s, exit 0). It validates the failed-state SQL/authorization pattern, not an injected failure inside the CDC startup state machine. The author's issue evidence supplies the actual CDC RED/GREEN initial copy, incremental DML, PAUSE/RESUME and advancing-checkpoint validation, plus full frontend/CDC and retained #29399 security checks. Exact-head required CI, UT, SCA, coverage and BVT are successful. The tracked review worktree is clean and git diff --check passes.
All six changed files and their ownership/consumer paths were reviewed. The tests extend existing fixtures and cover different identity, authorization, provenance and cleanup contracts. Separate increments are implementation +30/-4 (net +26), tests +342/-2 (net +340), documentation unchanged. There are no prior human review comments/change requests on this PR left unresolved; the linked issue's evidence and declared limitations were accounted for. No subagents were used.
CI exception: Iceberg E2E Local failed while Docker was pulling MinIO: unauthorized: access to the requested resource is not authorized. dev-up-minio-local failed before E2E setup completed. No workflow/container setup changes are in this patch, and this job is not counted as a passing E2E result. The separate CI Required check is green.
Summary
Fixes #29634.
Repair the unnamed system principal produced by
frontend.InternalExecutoron 4.2-dev, without weakening the current-principal authorization introduced in #29431.CDC constructs an executor with empty session overrides and the sys account. It previously supplied numeric
(account,user,role)=(0,0,0)but the synthetic usernameinternal; the catalog's reserved user 0 isroot. The new strict authorization therefore rejected both the task-definition SELECT and its failed-state UPDATE, leaving an apparently running task with no downstream data.Change and security boundaries
(0,0,0)to the real catalog principal.Session.isInternalor bypass checks for the literal usernameinternal.UserInputflag. KeepisInternalInputunchanged, so SQL-mode staging, diagnostics and query-ID semantics are not altered. Carry the provenance through the existing statement copies and ANALYZE's derived query.Main currently does not exhibit the same failure because its sys/moadmin authorization differs; it still has the synthetic default name. This PR intentionally targets the 4.2 regression rather than deleting its security protections or changing Main.
Regression coverage
internal/rootname is not an authorization bypass.Local validation on host 50
pkg/frontendandpkg/cdctests.-racecovering identity/active-role authorization and derived execution.pkg/tests/issuesdefault-system consumer plus all existingTestIssue29399*authentication tests on the real shared 3-CN fixture.go vet, diff/format checks and changed-line SCA are run locally; GitHub CI is reported separately and is not implied by local success.Evidence is retained under
/tmp/mo-cdc-internal-identity-20261006.UTtsLUAland linked from the issue. No existing regression/TKE cluster was changed, and no runner/workflow changes are part of this patch.Validation limits
The real CDC fixture exercises normal state transitions, not an injected failed-state transition; the unfixed failed UPDATE has direct server-log evidence, and the repaired default executor's read/write authorization is covered independently. Only system-account contexts used by actual CDC are exercised through SQL; no-account-context behavior is limited to constructor tests.
The standalone launch process can take longer to stop after successful CDC activity: CN lockservice may retry an already stopped TN. The harness treats SQL resource cleanup, exact PID cleanup and graceful process shutdown as separate results, and does not declare a forced stop a graceful-shutdown PASS. This PR does not alter launch/lockservice shutdown code. No test process or listener is left running.