github-actions[bot] commented on code in PR #66205:
URL: https://github.com/apache/doris/pull/66205#discussion_r3682172501
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/controller/BaseController.java:
##########
@@ -75,6 +75,9 @@ public ActionAuthorizationInfo
checkWithCookie(HttpServletRequest request,
// If has Authorization header, check auth info
ActionAuthorizationInfo authInfo = getAuthorizationInfo(request);
UserIdentity currentUser = checkPassword(authInfo, request);
+ // Callers do privilege checks on the returned authInfo, so the
resolved identity must be
+ // carried back out. Leaving it null makes every such check throw
NPE.
+ authInfo.userIdentity = currentUser;
Review Comment:
[P1] Preserve least-privilege Doris-to-Doris catalogs when making this
identity available. `RemoteDorisRestClient` still calls the legacy
`/api/meta/namespaces/...` routes with the catalog's configured user; those
handlers now reach `checkAdminAuth(authInfo.userIdentity)` and reject every
non-admin account before their existing SHOW filters run. Thus this fixes the
NPE but not the PR's stated external-catalog use case for restricted
credentials. Route the client to the v2 privilege-filtering metadata API, or
first fix the legacy database path to sort, paginate, and return `dbNameSet`
before removing its global-ADMIN gate; add a restricted-user catalog test that
also proves unauthorized databases stay hidden.
##########
threat-model.md:
##########
@@ -424,6 +469,38 @@ provenance.
take effect; counter resets unexpectedly; wraparound. *Severity*:
**security-critical** when the configured behavior is broken
(NOT when default is unconfigured — see §4.9).
+11. **HTTP authentication + authorization on the FE HTTP surface**
+ *(maintainer, M19)*. *Condition*: default config —
+ `enable_all_http_auth` ships **on** for FE (8030) (§4.5a). Scope:
+ - **FE 8030 — every endpoint that routes through the FE auth
+ path** (the `/api/**` and `/rest/v2/**` REST actions), which
+ must require valid credentials and enforce the caller's
Review Comment:
[P1] Do not claim caller-privilege enforcement until these REST handlers
actually provide it. The default flip adds authentication, not centralized
authorization: `AddStoragePolicyAction` accepts any valid password and journals
a global policy even though `CreatePolicyCommand` requires the feature flag
plus global ADMIN; `ESCatalogAction` lets any authenticated account issue raw
mapping/search requests with the catalog's server-side credentials and no
SHOW/SELECT check; and `ImportAction` lists/reads a caller-selected external
URL without LOAD or ADMIN. Add SQL-equivalent authorization before the
operation/outbound I/O (with negative low-privilege tests), or this new
security property remains false.
##########
regression-test/plugins/plugin_curl_requester.groovy:
##########
@@ -95,6 +95,12 @@ Suite.metaClass.http_client = { String method, String url /*
param */ ->
request = new HttpPost(url)
}
+ // FE HTTP endpoints authenticate by default (enable_all_http_auth
= true).
Review Comment:
[P1] Keep default FE credentials out of this generic helper. `http_client`
is also used for BE 8040 and other non-FE URLs, so this unconditional header
sends the JDBC password to unrelated destinations and removes the ability to
make an anonymous request. The parallel `curl` fallback has the same scope
problem, then embeds the injected password in the subprocess argv and logs the
full command. Preserve empty credentials as an explicit anonymous mode and make
FE authentication opt-in (or use an FE-scoped wrapper), with redacted logging.
##########
SECURITY.md:
##########
@@ -13,3 +13,15 @@ scanners, review agents, and vulnerability triagers should
read
Findings that are out of model or by design under `threat-model.md`
should be reported with that disposition instead of being treated as
Doris vulnerabilities.
+
+Before testing the FE HTTP surface, confirm `enable_all_http_auth` is
+`true` on FE — it is the default, and it must not be turned off for
+the test run. The flag is not runtime-mutable, so `fe.conf` is
+authoritative for what a cluster is actually running. With it off, FE
Review Comment:
[P1] Check the effective config after `fe_custom.conf`, not `fe.conf` alone.
FE loads `fe.conf` and then `${custom_config_dir}/fe_custom.conf`, whose values
explicitly overwrite the main file; before this PR, `persist=true` could leave
`enable_all_http_auth=false` there. Removing `mutable=true` does not ignore or
migrate that persisted override, so an upgraded FE can have no false entry in
`fe.conf` yet still run with auth disabled. Update this baseline and every
“fe.conf is authoritative” claim to cover both files/effective runtime state,
and define how legacy persisted values are migrated.
##########
threat-model.md:
##########
@@ -424,6 +469,38 @@ provenance.
take effect; counter resets unexpectedly; wraparound. *Severity*:
**security-critical** when the configured behavior is broken
(NOT when default is unconfigured — see §4.9).
+11. **HTTP authentication + authorization on the FE HTTP surface**
+ *(maintainer, M19)*. *Condition*: default config —
+ `enable_all_http_auth` ships **on** for FE (8030) (§4.5a). Scope:
+ - **FE 8030 — every endpoint that routes through the FE auth
+ path** (the `/api/**` and `/rest/v2/**` REST actions), which
+ must require valid credentials and enforce the caller's
+ privileges before returning data or performing an action.
+
+ **BE 8040 is not in this property.** Its `enable_all_http_auth`
+ default is unchanged (off), so its `NONE`-privilege handlers stay
+ disclaimed under §4.9 and are governed by Zone-2 network
+ isolation, not by this property.
+
+ *Violation symptom*: an in-scope endpoint answers an
+ unauthenticated request, accepts invalid credentials, or returns
+ data / performs an action the authenticated caller has no
+ privilege for. *Severity*: **security-critical**. Reports of this
+ shape are `VALID`.
+
+ *Excluded from this property*: (a) **FE `/metrics`**, which is
Review Comment:
[P1] Keep the canonical carve-out list aligned with the real FE surface.
This property says the `/api/**` and `/rest/v2/**` actions authenticate, then
names an exhaustive-looking set of exceptions, but `HealthAction` deliberately
serves `/api/health` without any credential (and this PR's test asserts that
behavior). Token-only `/api/streaming/commit_offset` and
`/api/streaming/report_task_failure`, plus the token branch of
`/api/{db}/{table}/_stream_load`, are also absent. As written, triagers will
classify the intentional health response as a valid vulnerability and will
misclassify the omitted token contracts. Either gate health or document every
public/token credential path here and in the upgrade guidance.
##########
regression-test/suites/auth/test_http_api_auth.groovy:
##########
@@ -19,34 +19,27 @@ import org.apache.doris.regression.suite.ClusterOptions
import groovy.json.JsonSlurper
suite("test_http_api_auth", "docker") {
- def options = new ClusterOptions()
- options.cloudMode = false // 存算一体模式
+ def jsonSlurper = new JsonSlurper()
- docker(options) {
- // Get FE and BE HTTP addresses from cluster
+ // Helper to check JSON response code
+ def checkJsonCode = { bodyStr, expectedCode ->
+ def json = jsonSlurper.parseText(bodyStr)
+ assertEquals(expectedCode, json.code)
+ }
+
+ // ========== Test Scenario 1: enable_all_http_auth = false ==========
+ // enable_all_http_auth is not a mutable config, so the disabled path can
only be exercised
+ // by starting a cluster with it turned off in fe.conf.
+ def authOffOptions = new ClusterOptions()
+ authOffOptions.cloudMode = false // 存算一体模式
+ authOffOptions.feConfigs += ['enable_all_http_auth=false']
Review Comment:
[P2] Make this off-path test depend on the flag it configures. The block
only calls FE `/api/health`, FE `/metrics`, and BE `/api/health`; all three are
public regardless of the FE setting, so the block still passes if this startup
override is ignored or misspelled. Add an anonymous request to an ordinary FE
endpoint that the default-on block proves rejects anonymous access. The
token-only and Basic/cookie identity fixes also need focused branch coverage.
##########
regression-test/suites/auth/test_http_api_auth.groovy:
##########
@@ -78,10 +71,26 @@ suite("test_http_api_auth", "docker") {
assertEquals(200, code)
}
}
+ }
+
+ // ========== enable_all_http_auth = true (the default) ==========
+ def options = new ClusterOptions()
+ options.cloudMode = false // 存算一体模式
+
+ docker(options) {
+ // Get FE HTTP address from cluster
+ def fe = cluster.getFeByIndex(1)
+ def feHost = fe.host + ":" + fe.httpPort
- // ========== Test Scenario 2: enable_all_http_auth = true - Public
APIs ==========
+ // ========== Setup ==========
+ sql """CREATE USER IF NOT EXISTS 'test_user'@'%' IDENTIFIED BY
'test_password'"""
+ sql """GRANT SELECT_PRIV ON *.* TO 'test_user'@'%'"""
Review Comment:
[P1] Reconcile this SELECT-only setup with the `/api/backends` assertion
later in the block. `BackendsAction` calls only `executeCheckPassword` and
explicitly documents password-only access for Flink/Spark; `/api/**` is
excluded from `AuthInterceptor`, so this valid user receives code 0, not the
asserted 401/Admin error. Either add the ADMIN check to the handler (if that is
the intended new contract) or change the expectation. In its current form the
default-on scenario cannot pass when this docker suite runs.
--
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]
