github-actions[bot] commented on code in PR #66746:
URL: https://github.com/apache/doris/pull/66746#discussion_r3777802235
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/MetadataGenerator.java:
##########
@@ -1015,6 +1015,13 @@ private static TFetchSchemaTableDataResult
workloadGroupPrivsMetadataResult(TSch
private static TFetchSchemaTableDataResult
queriesMetadataResult(TSchemaTableRequestParams tSchemaTableParams,
TFetchSchemaTableDataRequest parentRequest) {
+ UserIdentity currentUserIdentity = null;
+ boolean hasAdminPriv = true;
+ if (tSchemaTableParams.isSetCurrentUserIdent()) {
+ currentUserIdentity =
UserIdentity.fromThrift(tSchemaTableParams.getCurrentUserIdent());
+ hasAdminPriv = Env.getCurrentEnv().getAccessManager()
Review Comment:
[P2] Preserve session-only ADMIN roles across this RPC.
Authentication-integration mappings live in the originating
`ConnectContext.authenticatedRoles`, and `Auth.getRolesByUserWithLdap()` sees
them only with a matching thread-local context. The schema and processlist
requests carry only `TUserIdentity`, so an account whose ADMIN comes solely
from a ROLE MAPPING is evaluated as non-admin after the BE/FE or FE/FE hop,
contrary to the release note. Propagate the effective authorization context or
a trusted FE-computed decision and add integration-role ADMIN coverage.
##########
regression-test/suites/show_p0/test_show_processlist.groovy:
##########
@@ -18,6 +18,16 @@
import org.apache.doris.regression.util.Http
suite("test_show_processlist") {
+ def victimUser = "test_processlist_victim"
+ def attackerUser = "test_processlist_attacker"
+ def userPassword = "C123_567p"
+ try_sql "DROP USER '${victimUser}'"
+ try_sql "DROP USER '${attackerUser}'"
+ sql "CREATE USER '${victimUser}' IDENTIFIED BY '${userPassword}'"
+ sql "CREATE USER '${attackerUser}' IDENTIFIED BY '${userPassword}'"
Review Comment:
[P1] Please use these restricted credentials to cover the REST process-list
paths too. The existing calls at lines 42/47 go through `Http.GET`, which
hard-codes root Basic auth. In non-cloud mode `/rest/v1/session{,/all}` accepts
any valid Basic-auth user, then `SessionController.getSessionInfo()` calls
`listConnection("root", false)` and returns root sessions (including SQL text)
to that restricted caller. Please authorize/filter these endpoints using exact
caller identity plus ADMIN semantics, and add a restricted-user HTTP negative
assertion.
##########
regression-test/suites/show_p0/test_show_processlist.groovy:
##########
@@ -18,6 +18,16 @@
import org.apache.doris.regression.util.Http
suite("test_show_processlist") {
+ def victimUser = "test_processlist_victim"
+ def attackerUser = "test_processlist_attacker"
+ def userPassword = "C123_567p"
+ try_sql "DROP USER '${victimUser}'"
+ try_sql "DROP USER '${attackerUser}'"
+ sql "CREATE USER '${victimUser}' IDENTIFIED BY '${userPassword}'"
+ sql "CREATE USER '${attackerUser}' IDENTIFIED BY '${userPassword}'"
+ sql "GRANT SELECT_PRIV ON regression_test.* TO '${victimUser}'"
Review Comment:
[P1] The saved-profile parallel paths also need this ownership policy.
`/rest/v1/query_profile` exposes the global index/full profiles to any valid
non-cloud Basic user; `/api/profile/text` relies on default-disabled
`checkAdminAuth()` and even returns the latest full profile without an ID;
`SHOW QUERY PROFILE`/`SHOW LOAD PROFILE` read global saved SQL without a
privilege check; and the v2 controller uses username-only ownership while
treating literal `admin`/`root` names as privileged. Apply one
exact-owner/real-ADMIN policy across these surfaces and add two-user SQL/HTTP
tests.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/MetadataGenerator.java:
##########
@@ -1015,6 +1015,13 @@ private static TFetchSchemaTableDataResult
workloadGroupPrivsMetadataResult(TSch
private static TFetchSchemaTableDataResult
queriesMetadataResult(TSchemaTableRequestParams tSchemaTableParams,
TFetchSchemaTableDataRequest parentRequest) {
+ UserIdentity currentUserIdentity = null;
+ boolean hasAdminPriv = true;
Review Comment:
[P1] Fail closed when the caller identity is missing. During a rolling
upgrade, a new FE may schedule this schema scan on a pre-PR BE. That BE accepts
the plan but does not copy `current_user_ident` into
`TSchemaTableRequestParams` (or `TShowProcessListRequest`), so the upgraded FE
reaches this default and returns every user's rows to the restricted SQL
caller; `showProcessList()` similarly defaults to ROOT. The new identity-less
tests currently codify that fail-open behavior. Please reject/empty
identity-less authorization-sensitive requests, or add an explicit
compatibility fence and secure upgrade ordering with mixed-version tests.
##########
regression-test/suites/show_p0/test_show_processlist.groovy:
##########
@@ -42,7 +52,39 @@ suite("test_show_processlist") {
logger.info("result:${result}")
assertTrue(result[0].size() == 15)
-
+ connect(victimUser, userPassword, context.config.jdbcUrl) {
+ sql "select 1"
+ connect(attackerUser, userPassword, context.config.jdbcUrl) {
+ def attackerRows = sql """
+ SELECT User, Info
+ FROM information_schema.processlist
+ WHERE User IN ('${victimUser}', '${attackerUser}')
+ ORDER BY User
+ """
+ assertFalse(attackerRows.isEmpty())
+ assertTrue(attackerRows.every { row -> row[0] == attackerUser })
+ assertFalse(attackerRows.any { row -> row[0] == victimUser })
+ assertTrue(attackerRows.any { row ->
+ row[1] != null &&
row[1].toString().contains("information_schema.processlist")
+ })
+
+ def showRows = sql "SHOW FULL PROCESSLIST"
Review Comment:
[P1] This test needs a same-username/different-host case because local `SHOW
FULL PROCESSLIST` still filters by `ctx.getQualifiedUser()`, so distinct
`shared@host1` and `shared@host2` accounts can see each other's SQL. The same
collapsed identity boundary also survives in `KillUtils`: its query-ID and
connection-ID paths authorize when the username strings match, letting the
second account cancel the first account's query or connection. Pass the full
`UserIdentity` through both paths and cover local/all-FE SHOW plus every KILL
form.
##########
regression-test/suites/show_p0/test_show_processlist.groovy:
##########
@@ -18,6 +18,16 @@
import org.apache.doris.regression.util.Http
suite("test_show_processlist") {
+ def victimUser = "test_processlist_victim"
+ def attackerUser = "test_processlist_attacker"
+ def userPassword = "C123_567p"
+ try_sql "DROP USER '${victimUser}'"
+ try_sql "DROP USER '${attackerUser}'"
+ sql "CREATE USER '${victimUser}' IDENTIFIED BY '${userPassword}'"
+ sql "CREATE USER '${attackerUser}' IDENTIFIED BY '${userPassword}'"
+ sql "GRANT SELECT_PRIV ON regression_test.* TO '${victimUser}'"
+ sql "GRANT SELECT_PRIV ON regression_test.* TO '${attackerUser}'"
Review Comment:
[P1] Please use this restricted account to cover the live-query REST APIs
too. `/rest/v2/manager/query/current_queries` and `/kill/{id}` call
`checkAdminAuth()`, but with the supported default `enable_all_http_auth=false`
that helper is a no-op. The first returns every active query's full SQL and
IDs; the second cancels the supplied native or Arrow Flight query, including
through all-FE forwarding. Require ADMIN unconditionally or enforce exact-owner
filtering/checks, and add default-config restricted-user negative tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
