github-actions[bot] commented on code in PR #66205:
URL: https://github.com/apache/doris/pull/66205#discussion_r3780648243
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -318,9 +318,25 @@ public class Config extends ConfigBase {
+ "BDBJE. The connection is abandoned if the clock skew is larger
than this value.")
public static long max_bdbje_clock_delta_ms = 5000; // 5s
- @ConfField(mutable = true, description = "Whether to enable "
- + "authentication for all HTTP " + "interfaces", varType =
VariableAnnotation.EXPERIMENTAL)
- public static boolean enable_all_http_auth = false;
+ @ConfField(description = "Whether to enable authentication for all HTTP
interfaces. On by default. "
+ + "While it is off, some HTTP interfaces (for example parts of the
metadata, statistics "
+ + "and admin surface) serve requests without checking the caller's
privileges, and a few "
+ + "(statistics and import endpoints) without any credentials at
all. "
+ + "This config is deliberately NOT mutable: turning authentication
off must be a recorded, "
+ + "on-disk decision in fe.conf that survives a restart, not a
runtime command. "
+ + "Upgrade note: a cluster upgrading from a version where this
defaulted to false may have "
+ + "callers that poll those interfaces anonymously (monitoring,
metrics scrapers, ops "
+ + "scripts, health probes); those callers will start getting 401
until they present "
+ + "credentials. The fix is to give them credentials -- setting
this back to false in "
+ + "fe.conf is a temporary migration aid that leaves those
interfaces unauthenticated. "
+ + "Also check fe_custom.conf when upgrading: it is read after
fe.conf and overwrites it, "
+ + "and in releases where this flag was mutable an 'ADMIN SET
FRONTEND CONFIG' with "
+ + "persist=true could have written false into it. Making the flag
non-mutable does not "
+ + "remove or migrate such a value, so a cluster can look clean in
fe.conf and still start "
+ + "with authentication off; delete the entry from fe_custom.conf
to pick up the new default. "
+ + "Security scanning and penetration testing must never be run
with this off.",
+ varType = VariableAnnotation.EXPERIMENTAL)
+ public static boolean enable_all_http_auth = true;
Review Comment:
[P1] Keep Remote-Doris row counts usable by SELECT-only catalog users. This
default activates RowCountAction's conditional global-ADMIN gate, but
RemoteDorisRestClient.getRowCount() still calls /api/rowcount and
RemoteDorisExternalTable uses it for cached estimates and synchronous ANALYZE
setup. The least-privilege account this PR now supports for /api/meta therefore
gets 401 on the later statistics path (the new _count test covers a different
SELECT-authorized endpoint). This is distinct from the resolved
metadata-listing thread: route this client through /api/{db}/{table}/_count or
make /api/rowcount authorize the requested table, and add a SELECT-only
Remote-Doris row-count/ANALYZE regression.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/StmtExecutionAction.java:
##########
@@ -90,10 +88,13 @@ public Object executeSQL(@PathVariable(value = NS_KEY)
String ns, @PathVariable(
return redirectToHttps(request);
}
+ // Authenticate only. Deliberately no privilege pre-gate here:
executeQuery hands the
Review Comment:
[P1] Preserve the identity resolved on the HTTP connection. checkWithCookie
authenticates the bare username against the request's source address and
returns a concrete host-qualified UserIdentity, but this path later forwards
only username/password to StatementSubmitter, which reconnects from 127.0.0.1
and resolves the account again. For same-named host accounts, different
passwords make a legitimate request fail; if the passwords match, the SQL runs
with the localhost account's potentially broader grants. This is distinct from
the resolved ADMIN-pre-gate thread: carry the resolved identity into statement
authorization (and add same-name/different-host Basic and cookie regressions)
instead of relying on localhost reauthentication.
##########
threat-model.md:
##########
@@ -424,6 +487,124 @@ 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* 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 establish a caller identity — valid user credentials, or
+ one of the credential forms listed under *Excluded* below —
+ before returning data or performing an action.
+
+ **Read the property title literally: this is a claim about
+ authentication, not a blanket claim about authorization.** The
+ default flip made the FE HTTP surface demand a credential; it did
+ **not** introduce a centralized privilege check. Authorization on
+ FE HTTP is per-handler, and its coverage is uneven. Handlers that
+ do run a privilege check (for example `checkAdminAuth`, or a
+ per-object `PrivPredicate.SHOW` filter) are covered by property
+ (12) below; the ones that do not are listed as known gaps there.
+ Do not cite this property as evidence that a given FE endpoint
+ enforces the caller's privileges — check the handler.
+
+ **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 a request that
+ carries no credential of any accepted form, or accepts invalid
+ credentials. *Severity*: **security-critical**. Reports of this
+ shape are `VALID`.
+
+ *Excluded from this property* — this list is meant to be
+ exhaustive; a path that belongs here and is missing is a defect in
+ this document, not a finding:
+ - (a) **FE `/metrics`**, deliberately anonymous, not gated by the
+ flag — see §4.9.
+ - (b) **FE `/api/health`** (`HealthAction`), deliberately
+ anonymous and not gated by the flag either. It returns liveness
+ plus total/online backend counts. A report that `/api/health`
+ answers an unauthenticated request is
+ `BY-DESIGN: property-disclaimed`, not a violation.
+ - (c) Clusters where the operator has set
+ `enable_all_http_auth = false` — a §4.5a knob flipped toward the
+ less-secure side, closed `OUT-OF-MODEL: non-default-build`.
+ Note that the effective value can come from `fe_custom.conf`,
+ which is read after and overwrites `fe.conf`; see §4.5a.
+ - (d) The **cluster-token authenticated** endpoints, where the
+ cluster token *is* the credential — authenticated, just not by
+ user password: FE `/api/get_small_file`; FE `/api/bootstrap`
+ when `cluster_id`+`token` are presented; FE
+ `/api/streaming/commit_offset` and
+ `/api/streaming/report_task_failure`, which accept **only** a
+ `token` header and no user credential at all; the token branch
+ of `/api/{db}/{table}/_stream_load`; and the BE handlers that
+ accept an auth token. A report that one of these serves a
+ request without a user password, while presenting a valid
+ cluster token, is `BY-DESIGN: property-disclaimed`. A report
+ that one of them accepts an *invalid* or absent token is
+ `VALID` — **with one recorded exception**: `/api/bootstrap`
+ validates the pair only on the ready path, so while the FE is
+ not ready any two non-empty strings are accepted and the caller
+ learns "not ready" and nothing else. That branch is a known,
+ deliberate asymmetry (§4.14), not a bypass of this property.
+
+12. **Per-handler authorization on the FE HTTP surface, where the
+ handler implements it** *(maintainer, M19)*. *Condition*: property
+ (11) holds and the endpoint in question runs a privilege check.
+ *Violation symptom*: a handler that performs a privilege check
+ returns data, or performs an action, that the authenticated caller
+ has no privilege for — for example a metadata listing that leaks
+ objects the caller has no `SHOW` privilege on, or an admin action
+ reachable without `ADMIN_PRIV`. *Severity*:
+ **security-critical**. Reports of this shape are `VALID`.
+
+ **Known gaps — password-only handlers, not yet covered by this
+ property.** These authenticate but perform no SQL-equivalent
+ authorization. They are recorded here so that a report against
+ them is triaged as a *known, accepted gap* rather than silently
+ treated as covered; closing them is tracked in §4.14. **This list
+ is maintained by inspection and has been wrong before — treat it
+ as the current best inventory, not as a proof of completeness. A
+ password-only handler that is missing from it is a defect in this
Review Comment:
[P1] Include the remaining password-only handlers in this inventory (or
authorize them). In particular, ClusterGuardAction lets any valid account read
implementation-defined guard details and POST a reload that defaults to every
FE and invokes the startup hook for node/time policy; it has no ADMIN check.
The list also misses enable_light_schema_change table/key state, get_load_state
for arbitrary database labels, and /api/tso. These are distinct from the
AddStoragePolicy/ES/Import instances in the earlier thread, and this paragraph
explicitly says omitted handlers remain findings. Please audit all
executeCheckPassword/checkWithCookie(..., false) actions, update both this list
and §4.14, and add the appropriate negative low-privilege tests.
##########
SECURITY.md:
##########
@@ -13,3 +13,23 @@ 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 the running FE — it is the default, and it must not be
+turned off for the test run. Confirm it against the *effective*
+runtime value, not against `fe.conf` alone: FE reads `fe.conf` and
+then `${custom_config_dir}/fe_custom.conf`, and the latter overwrites
+the former. In releases where this flag was mutable, `ADMIN SET
+FRONTEND CONFIG (...) PROPERTIES("persist" = "true")` could write
+`enable_all_http_auth=false` into `fe_custom.conf`; making the flag
+non-mutable does not remove or migrate such a value, so an upgraded
+cluster can have no `false` entry anywhere in `fe.conf` and still be
+running with authentication off. Read the value back from
+`/api/show_config` (or `ADMIN SHOW FRONTEND CONFIG`) and check both
Review Comment:
[P1] Use an FE surface for this effective-value check. /api/show_config is
registered only by the BE HTTP service; FE code itself calls that route only on
BE nodes, while ADMIN SHOW FRONTEND CONFIG reads ConfigBase directly. Calling
the stated route on FE 8030 cannot verify this flag, and calling it on BE 8040
reports BE's separate default-false value, which can misclassify an FE security
test. Please remove this alternative here and in the two threat-model
references, or replace it with the authenticated FE manager/config route.
##########
regression-test/suites/auth_p0/test_http_legacy_meta_auth.groovy:
##########
@@ -0,0 +1,134 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.junit.Assert;
+
+// Covers the LEGACY metadata controller, MetaInfoAction, served at
/api/meta/**.
+// Note the path: the sibling test_http_meta_* suites use
/rest/v2/api/meta/**, which is a
+// different class (MetaInfoActionV2). Before this suite the legacy controller
had no regression
+// coverage at all.
+//
+// What it pins down:
+// 1. The endpoints authenticate. No credential -> rejected, whatever the
privileges would be.
+// 2. They do NOT require global ADMIN. A least-privilege account gets a
successful response --
+// this is what a Doris-to-Doris external catalog (RemoteDorisRestClient)
relies on, since it
+// calls exactly these routes with the catalog's configured user.
+// 3. The response is privilege-filtered per object, and the filter is
actually applied to what
+// is returned. getAllDatabases used to compute a SHOW-filtered list and
then return the
+// unfiltered one, so a non-admin saw every database on the cluster.
+suite("test_http_legacy_meta_auth", "p0,auth,nonConcurrent") {
+ String suiteName = "test_http_legacy_meta_auth"
+ String dbName = context.config.getDbNameByFile(context.file)
+ String tableName = "${suiteName}_table"
+ String hiddenTableName = "${suiteName}_hidden_table"
+ String user = "${suiteName}_user"
+ String pwd = 'C123_567p'
+
+ try_sql("DROP USER ${user}")
+ sql """CREATE USER '${user}' IDENTIFIED BY '${pwd}'"""
+ sql """DROP TABLE IF EXISTS `${tableName}`"""
+ sql """DROP TABLE IF EXISTS `${hiddenTableName}`"""
+ sql """
+ CREATE TABLE `${tableName}` (
+ `k1` int,
+ `k2` int
+ ) ENGINE=OLAP
+ DISTRIBUTED BY random BUCKETS auto
+ PROPERTIES ('replication_num' = '1') ;
+ """
+ sql """
+ CREATE TABLE `${hiddenTableName}` (
+ `k1` int,
+ `k2` int
+ ) ENGINE=OLAP
+ DISTRIBUTED BY random BUCKETS auto
+ PROPERTIES ('replication_num' = '1') ;
+ """
+
+ try {
+ def legacyGet = { uriPath, user_name, password, check_func ->
+ httpTest {
+ if (user_name != null) {
+ basicAuthorization "${user_name}", "${password}"
+ }
+ endpoint "${context.config.feHttpAddress}"
+ uri uriPath
+ op "get"
+ check check_func
+ }
+ }
+
+ String dbsUri = "/api/meta/namespaces/default_cluster/databases"
+ String tblsUri =
"/api/meta/namespaces/default_cluster/databases/${dbName}/tables"
+
+ // 1. Authentication is required. Anonymous callers are rejected.
+ legacyGet.call(dbsUri, null, null) {
+ respCode, body ->
+ log.info("legacy databases (anonymous) respCode:${respCode}
body:${body}")
+ assertTrue(respCode == 401 || "${body}".contains("401")
+ || "${body}".contains("Unauthorized") ||
"${body}".contains("Need auth"))
+ }
+
+ // 2. A valid but non-admin account is accepted -- no global ADMIN is
demanded.
+ // 3. ... and sees nothing it has no SHOW privilege on.
+ legacyGet.call(dbsUri, user, pwd) {
+ respCode, body ->
+ log.info("legacy databases (no grants) respCode:${respCode}
body:${body}")
+ assertEquals(200, respCode)
+ assertFalse("${body}".contains("Unauthorized"))
+ assertFalse("${body}".contains("Admin_priv"))
+ assertFalse("${body}".contains("${dbName}"))
+ }
+
+ sql """grant select_priv on ${dbName}.${tableName} to ${user}"""
+
+ // The grant on one table makes the database visible...
+ legacyGet.call(dbsUri, user, pwd) {
+ respCode, body ->
+ log.info("legacy databases (after grant) respCode:${respCode}
body:${body}")
+ assertEquals(200, respCode)
+ assertTrue("${body}".contains("${dbName}"))
+ }
+
+ // ... but only the granted table inside it. The other table stays
hidden.
+ legacyGet.call(tblsUri, user, pwd) {
+ respCode, body ->
+ log.info("legacy tables (after grant) respCode:${respCode}
body:${body}")
+ assertEquals(200, respCode)
+ assertTrue("${body}".contains("${tableName}"))
+ assertFalse("${body}".contains("${hiddenTableName}"))
+ }
+
+ // The schema route authorizes per table: granted table succeeds,
ungranted one does not.
+ legacyGet.call("${tblsUri}/${tableName}/schema", user, pwd) {
+ respCode, body ->
+ log.info("legacy schema (granted) respCode:${respCode}
body:${body}")
+ assertEquals(200, respCode)
+ assertTrue("${body}".contains("k1"))
+ }
+
+ legacyGet.call("${tblsUri}/${hiddenTableName}/schema", user, pwd) {
+ respCode, body ->
+ log.info("legacy schema (ungranted) respCode:${respCode}
body:${body}")
+ assertTrue("${body}".contains("401") ||
"${body}".contains("Access denied"))
+ }
+ } finally {
Review Comment:
[P2] Preserve these tables after the run. The suite already drops both names
before creating them, while this finally block removes the fixture even when an
assertion fails. The repository regression-test standard explicitly keeps
tables after execution so the failed environment can be inspected. Please
remove the two table drops here; user cleanup can remain.
##########
SECURITY.md:
##########
@@ -13,3 +13,23 @@ 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 the running FE — it is the default, and it must not be
+turned off for the test run. Confirm it against the *effective*
+runtime value, not against `fe.conf` alone: FE reads `fe.conf` and
+then `${custom_config_dir}/fe_custom.conf`, and the latter overwrites
+the former. In releases where this flag was mutable, `ADMIN SET
Review Comment:
[P2] Correct the persistence command in this upgrade guidance. The ADMIN SET
FRONTEND CONFIG grammar has no PROPERTIES("persist" = "true") form, and
AdminSetFrontendConfigCommand only applies the mutable value in memory. The FE
path that calls ConfigBase.persistConfig is /api/_set_config?...&persist=true.
The stale fe_custom.conf risk is real, but attributing it to a nonexistent SQL
command makes the migration history and reproduction steps misleading; please
correct the repeated Config.java/threat-model wording as well.
--
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]
