github-actions[bot] commented on code in PR #66205:
URL: https://github.com/apache/doris/pull/66205#discussion_r3710584605
##########
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
+ // that presents no credential at all is still rejected by
checkWithCookie.
+ checkWithCookie(request, response, false);
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.
##########
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.
+ - `POST /rest/v2/api/storage_policy` (`AddStoragePolicyAction`) —
+ any valid account can journal a global storage policy, whereas
+ the SQL equivalent (`CREATE POLICY`) requires global
+ `ADMIN_PRIV`. This is a **privilege gap, not a design choice**.
+ - `GET /rest/v2/api/es_catalog/get_mapping` and
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.
--
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]
