github-actions[bot] commented on code in PR #66205:
URL: https://github.com/apache/doris/pull/66205#discussion_r3710542241
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -337,9 +337,25 @@ public class Config extends ConfigBase {
+ "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
credentials or privileges. "
+ + "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] Preserve the non-admin /api/query contract before enabling this by
default. StmtExecutionAction already authenticates every request, but this flag
also enables its separate database-ADMIN gate before the submitted SQL is
planned under the caller's identity. The existing cloud regression grants a
user only SELECT_PRIV and expects /api/query to succeed while the instance is
normal, so that test—and restricted web/CDC callers whose SQL privileges are
otherwise sufficient—now fail. Decouple this legacy authorization branch from
the authentication default (and make any replacement catalog-aware), then keep
a non-admin query regression.
##########
threat-model.md:
##########
@@ -424,6 +487,102 @@ 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`.
+
+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.
+ - `/api/backends` (`BackendsAction`) — any valid account can
+ enumerate backend host/port/liveness. Intentional: the
+ Flink/Spark connectors call it with an ordinary load account to
+ discover backends before a stream load.
Review Comment:
[P1] Migrate this FE caller before making authentication the default. The ES
open-API regression still invokes both of these endpoints through curl()
without user/pwd, and the corrected generic helper intentionally sends no
Authorization header when user is empty. ESCatalogAction now rejects those
calls under the default-true flag, so the suite's success/mappings/hits
assertions deterministically fail whenever enableEsTest=true. Pass explicit
regression credentials at these FE-only call sites; do not restore implicit
credentials in the generic helper.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/BootstrapFinishAction.java:
##########
@@ -60,7 +60,13 @@ public class BootstrapFinishAction extends
RestBaseController {
@RequestMapping(path = "/api/bootstrap", method = RequestMethod.GET)
public ResponseEntity execute(HttpServletRequest request,
HttpServletResponse response) {
- if (Config.enable_all_http_auth) {
+ String clusterIdStr = request.getParameter(CLUSTER_ID);
+ String token = request.getParameter(TOKEN);
+ // A caller that presents the cluster id and token is an FE joining or
probing this cluster;
+ // that pair is its credential and is verified below, so do not also
demand a user password
+ // from it. Callers that present neither are anonymous and must
authenticate.
+ boolean authenticatedByToken = !Strings.isNullOrEmpty(clusterIdStr) &&
!Strings.isNullOrEmpty(token);
Review Comment:
[P2] Validate the cluster credential before using it to bypass Basic
authentication. Here any two non-empty strings set authenticatedByToken, but
parsing and token/cluster comparisons run only inside the later isReady branch.
While the FE is not ready, cluster_id=garbage&token=garbage therefore skips
password/admin authentication and returns the not-ready response without ever
rejecting the invalid credential, contrary to the new threat-model contract.
Validate the pair in every readiness state, then use isReady only to select the
payload; add wrong/malformed-token cases for both states.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java:
##########
@@ -88,11 +86,12 @@ public class MetaInfoAction extends RestBaseController {
public Object getAllDatabases(
@PathVariable(value = NS_KEY) String ns,
HttpServletRequest request, HttpServletResponse response) {
- boolean checkAuth = Config.enable_all_http_auth ? true : false;
- ActionAuthorizationInfo authInfo = checkWithCookie(request, response,
checkAuth);
- if (Config.enable_all_http_auth) {
- checkAdminAuth(authInfo.userIdentity);
- }
+ // Authenticate, but do not demand global ADMIN: the per-database SHOW
check below is what
+ // authorizes this response, so a least-privilege account (for example
the user a
+ // Doris-to-Doris external catalog is configured with) can list
exactly the databases it is
+ // allowed to see. Passing false here only skips the ADMIN_OR_NODE
requirement -- a caller
Review Comment:
[P1] Keep the cloud overdue fence when removing the global ADMIN
requirement. checkWithCookie does not use this boolean only for ADMIN_OR_NODE:
in cloud mode it also gates checkInstanceOverdue for Basic requests and the
OVERDUE rejection for cookie sessions. Hard-coding false here (and on the
table/schema routes) therefore lets non-root overdue tenants enumerate metadata
even though an existing enable_all_http_auth=true deployment rejected them.
Split lifecycle validation from the global-privilege switch, and test both
credential paths in the overdue state.
--
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]
