Repository navigation
[Enhance](resource) Restrict AI resource creation by user identity - #68147
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
TPC-H: Total hot run time: 28147 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for three client-visible gaps in the new AI-resource authorization contract: malformed configuration is accepted and later misdiagnosed, supported external/JIT administrator identities are not always representable, and a configured-user denial is returned with the generic 1105/HY000 error classification.
Checkpoint conclusions:
- Goal and scope: the default wildcard preserves the prior ADMIN-only behavior, and valid stored native/domain identities take the intended AI-only exact-identity path. The three-file change is otherwise focused.
- Authorization and configuration: every supported current-version SQL creation path reaches validation on the active master, and no fail-open bypass was found. The malformed-value and JIT identity cases below make the opt-in policy unusable for supported configurations or identities.
- Lifecycle and concurrency: the immutable process-local string introduces no thread, lock, or hot-path concern; one parse on rare DDL is negligible. Ordinary failover/config deployment behavior did not establish a separate defect.
- Compatibility and persistence: forwarding preserves the exact user identity; resource construction, journaling, replay, and restore remain symmetric. There is no FE-BE protocol, storage-format, transaction, or data-write change.
- Tests and observability: the added tests cover exact allow/deny, wildcard, and non-AI branches and restore their globals, but they do not detect the protocol code, malformed value, or external/JIT cases reported inline. Standard config visibility exists, but malformed syntax has no actionable diagnosis.
- Security model: ADMIN holders are trusted by the repository threat model, so these are functional/API policy defects rather than claimed Doris vulnerabilities.
Validation was static-only under the review contract: I did not build, run tests, or edit repository source. At submission time, live CI reports CheckStyle and compile successful while FE UT and regression suites remain pending; those external results are separate from this review. No additional user focus was supplied.
| analyzeResourceType(); | ||
|
|
||
| if (resourceType == ResourceType.AI && !"*".equals(Config.ai_resource_allowed_user)) { | ||
| UserIdentity allowedUser = UserIdentity.fromString(Config.ai_resource_allowed_user); |
There was a problem hiding this comment.
[P2] Reject malformed configured identities explicitly. UserIdentity.fromString returns null for values such as root@%, so this FE starts normally and every AI CREATE later falls through to the generic caller-denied error. Because this field is not mutable, the operator cannot correct the typo without another restart. Please validate the value at configuration load, or at least detect null here and report ai_resource_allowed_user and its required syntax; add a malformed-value test.
|
|
||
| if (resourceType == ResourceType.AI && !"*".equals(Config.ai_resource_allowed_user)) { | ||
| UserIdentity allowedUser = UserIdentity.fromString(Config.ai_resource_allowed_user); | ||
| if (!ConnectContext.get().getCurrentUserIdentity().equals(allowedUser)) { |
There was a problem hiding this comment.
[P2] Support the external/JIT identities admitted by the preceding privilege check. JIT authentication builds the current identity as principal@remoteIp, but UserIdentity.fromString splits on every @. An email-style principal such as alice@example.com is therefore impossible to configure even with an exact host; principals without @ are still pinned to one client IP because equals does not apply host-pattern semantics. Please define this setting against a stable authenticated principal or use a validated quote-aware selector, or explicitly reject/document JIT identities and cover that contract in tests.
| if (resourceType == ResourceType.AI && !"*".equals(Config.ai_resource_allowed_user)) { | ||
| UserIdentity allowedUser = UserIdentity.fromString(Config.ai_resource_allowed_user); | ||
| if (!ConnectContext.get().getCurrentUserIdentity().equals(allowedUser)) { | ||
| throw new AnalysisException("Current user does not have permission to create AI resources"); |
There was a problem hiding this comment.
[P2] Preserve an access-denied SQL error here. The string-only AnalysisException constructor leaves mysqlErrorCode at ERR_UNKNOWN_ERROR, so this deliberate authorization rejection is returned as 1105/HY000, unlike the adjacent ADMIN check (1227/42000). Clients that classify permission failures will treat this as an internal error. Please use an appropriate access-denied ErrorCode (or a dedicated 5000-series code) and assert the protocol error code in the test.
There was a problem hiding this comment.
Confirmed against 182fc371e38e: if ai_resource_allowed_user is set to 'root'@'%' and another ADMIN user tries to create an AI resource, this check throws the string-only AnalysisException. That constructor defaults to ERR_UNKNOWN_ERROR, and the StmtExecutor / MysqlErrPacket path preserves it as 1105 / HY000. The existing ADMIN check in the same method reports 1227 / 42000, so clients classifying failures by error code cannot recognize this new rejection as an authorization failure.
Could we use an appropriate access-denied ErrorCode while retaining a message that explains the configured identity restriction? The caller has already passed the ADMIN check, so the message should not suggest that granting ADMIN will fix it.
Please also assert getMysqlErrorCode() in the unit test. The current assertion on the message's errCode = 2 checks Doris's internal error code, not the MySQL protocol error code. I ran CreateResourceCommandTest locally: all five tests pass, but this distinction is currently untested.
TPC-DS: Total hot run time: 154078 ms |
ClickBench: Total hot run time: 24.09 s |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 27830 ms |
TPC-DS: Total hot run time: 153711 ms |
ClickBench: Total hot run time: 23.83 s |
FE UT Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Automated review round completed against the verified exact head 1997c42 (base 04aa5a1).
No new materially distinct finding survived the complete changed-file review, two normal full-coverage scans, the dedicated risk scan, and the final sweep. I am not duplicating the existing inline findings: the malformed configured identity and external/JIT/email/exact-host matching concerns remain the applicable threads at CreateResourceInfo.java lines 82-83. The earlier access-denied MySQL error-code concern is addressed in this head with ERR_SPECIFIC_ACCESS_DENIED_ERROR, and the added test now asserts getMysqlErrorCode().
Critical checkpoint conclusions:
- Goal and call chain: the AI-only gate runs after resource-type analysis and before ResourceMgr allocation/edit-log journaling; all user SQL creation and observer-to-master forwarding paths were traced. Non-AI, Azure-guess, storage-vault, replay, and internal paths are unaffected or intentionally outside the SQL gate.
- Configuration and lifecycle: the new startup-only @ConfField follows ConfigBase loading; forwarded DDL executes on the master with the authenticated identity. No new concurrency, lifecycle, lock, persistence, FE-BE protocol, or compatibility defect was found.
- Error handling and performance: the denial preserves an access-denied protocol code; no status is silently discarded, and the extra check is limited to AI resource validation.
- Tests and isolation: the new tests restore Config and ConnectContext state in finally blocks, while TestWithFeService restores Env.accessManager per test. Coverage is validation-level; no builds or tests were run because this invocation is review-only, so conclusions are static-analysis evidence.
- User focus: no additional user-provided focus points were supplied.
Dismissing this blocking automated review at the maintainer request after reviewing the remaining findings and approving the current PR head. This dismissal does not mark the remaining findings as fixed.
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
Rstrict AI resource creation by user identity