mattcasters commented on code in PR #8402:
URL: https://github.com/apache/hop/pull/8402#discussion_r4024060183
##########
THREAT_MODEL.md:
##########
@@ -254,6 +309,13 @@ where strict outbound TLS verification is required (see
§9).
pipeline execution = remote code execution.
- Running Hop Server with `enable_auth=false` on an untrusted network →
unauthenticated pipeline execution + metadata mutation over the JSON API.
+- Exposing **Hop Web in mode `NONE`** to an untrusted network → an open
Review Comment:
**[suggestion]** The new §11 pattern says exposing Hop Web in mode `NONE`
yields “an open authoring UI on `/ui`; and, if
`HOP_WEB_ALLOW_UNAUTHENTICATED_SERVER_API` was enabled, unauthenticated
pipeline execution over `/hop/*` as well.” That contrast implies execution is
the flag’s problem. The GUI does not call `/hop/*` to run:
`HopGuiPipelineGraph` uses `PipelineEngineFactory` / `LocalPipelineEngine`
in-process. Mode `NONE` binds an unrestricted session
(`RapSecurityContextProvider` → `HopSecurityContext.unrestricted()`), so anyone
who can reach `/ui` can author **and** run pipelines even while `/hop/*`
returns 403. §8 already says the session is unrestricted; §11’s “authoring UI”
/ “execution over `/hop/*` as well” split understates the default-install
exposure.
**Suggestion:** Word the misuse pattern as: exposed `/ui` in `NONE` is an
unrestricted GUI that can run pipelines in-process; the flag additionally opens
the remote `/hop/*` (and `/hop/api/v1/*`) execution API.
##########
THREAT_MODEL.md:
##########
@@ -54,7 +56,8 @@ the `hop-run` CLI, or submitted to a **Hop Server** for
local/remote execution.
| Execution engine | `engine/`, `engine-beam/` | Runs pipelines/workflows the
operator authored — incl. transforms that touch files, DBs, network, and
**scripting/exec** steps. |
| Hop Server (servlet / HTTP) | `engine/` — package `org.apache.hop.www` +
`HopServerMeta` in `org.apache.hop.server`; launched by the `hop-server`
command (`org.apache.hop.www.HopServer`). *(The `org.apache.hop.server`
connection helpers `HttpUtil`/`ServerConnectionManager` — see §9 — live in
`core/`.)* | The **network trust boundary**: an embedded Jetty server whose
servlets accept pipeline/workflow run requests over HTTP (`/hop/execPipeline`,
`/hop/addPipeline`+`/hop/startExec`, `/hop/registerPackage`, …). Who may
submit/run, and with what auth, lives here (§7, §8). |
| Hop JSON API | `engine/src/main/java/org/apache/hop/www/api/` | **Part of
Hop Server**, not a separate deployable. Mounted on `/hop/api/v1/`
([`WebServer.java`](engine/src/main/java/org/apache/hop/www/WebServer.java))
and exposes `execute/sync` (run a web service), `metadata` CRUD, `plugins` and
`location` execution info. It sits inside the same `ConstraintSecurityHandler`
as every servlet, so it inherits Hop Server's Basic/JAAS auth (§8). |
-| GUI (desktop / web) | `ui/` (`hop-ui`, SWT core), `rcp/` (desktop fragment),
`rap/` (`hop-ui-rap`, RAP/RWT **web** GUI) | Authoring surface used by the
trusted operator. `rap/` is the **web GUI**, not a server. |
+| GUI (desktop / web) | `ui/` (`hop-ui`, SWT core), `rcp/` (desktop fragment),
`rap/` (`hop-ui-rap`, RAP/RWT **web** GUI) | Authoring surface used by the
trusted operator. `rap/` is the **web GUI code**, not a server — but see the
Hop Web row: the shipped web application deploys it together with Hop Server
servlets. |
+| Hop Web (deployed web application) | `assemblies/web/` (WAR +
`apache/hop-web` image), built from `rap/` + `engine/` | A **distinct
network-facing deployment**, separate from both `rap/` and `hop-server`. The
one WAR co-deploys the RAP UI on `/ui` and `/ui-dark`, `HopServerServlet` on
`/hop/*`, and `HopApiApplication` on `/hop/api/v1/*` on the same origin
([`web.xml`](assemblies/web/src/main/resources/WEB-INF/web.xml)). It does
**not** use Hop Server's `enable_auth` / `hop.pwd` Basic auth; it has its own
auth modes (§8). Docker image binds `0.0.0.0:8080`. |
Review Comment:
**[suggestion]** The new Hop Web row correctly calls the WAR a distinct
network-facing deployment (RAP on `/ui`, `HopServerServlet` on `/hop/*`,
`HopApiApplication` on `/hop/api/v1/*`, own auth modes). The sentences
immediately after the table still say in-scope is “engine + Hop Server +
plugins + connection layer + GUI glue” and “The **network-facing Hop Server**
is where an untrusted actor can appear.” §7 still scopes the in-model adversary
to “a remote actor against an exposed Hop Server.” §12 still omits Hop Web
auth/deployables even though the header revision triggers were updated. The
JSON API family row still says that API “inherits Hop Server's Basic/JAAS
auth,” which is true for standalone `hop-server` but not for the co-deployed
`/hop/api/v1/*` this row now documents. A triager can treat an exposed Hop Web
`/ui` as out of model even though §8/§11 now describe it.
**Suggestion:** Extend the in-scope / untrusted-actor sentences, §7, and §12
to name Hop Web as a second HTTP trust boundary. Qualify the JSON API row:
standalone `hop-server` uses `ConstraintSecurityHandler`; Hop Web uses
`HopBasicAuthFilter` / `HopOidcAuthFilter` / `HopServerAuthorizationFilter`.
##########
rap/src/main/java/org/apache/hop/ui/hopgui/HopWebEntryPoint.java:
##########
@@ -159,8 +160,23 @@ protected void createContents(Composite parent) {
"Hop Web security: user ''{0}'' roles={1}",
securityContext.getUsername(), securityContext.getRoleIds());
} else {
- LogChannel.UI.logDebug(
- "Hop Web security: no authenticated principal (mode NONE or
unrestricted)");
+ HopSecurityConfig.AuthMode mode = resolveAuthMode();
+ if (mode == HopSecurityConfig.AuthMode.NONE) {
+ LogChannel.UI.logDebug("Hop Web security: no authenticated principal
(mode NONE)");
+ } else {
+ // Authentication is configured but the request carries no principal.
In EXTERNAL mode
Review Comment:
**[nit]** The six-line comment restates the log message and embeds EXTERNAL
/ `web.xml` / Security-tab design rationale. The log text already explains what
to do.
**Suggestion:** Drop the block comment (or keep one line: principal-less
non-`NONE` session is unexpected; log at error so EXTERNAL-without-constraint
is visible).
##########
docs/hop-user-manual/modules/ROOT/pages/hop-gui/hop-web.adoc:
##########
@@ -249,6 +262,12 @@ To keep audit data across container restarts, mount a
volume and set the same pa
The default Hop Web docker image picks up `tomcat-users.xml` and `web.xml`
files and moves them to the correct location before Hop Web starts.
+IMPORTANT: Mode `EXTERNAL` is *delegation, not enforcement*.
+Hop itself does not challenge anyone in this mode: it only reads the
`Principal` the servlet container (or authenticating reverse proxy) put on the
request.
+The `web.xml` that ships with Hop Web contains no `<security-constraint>`, so
selecting `EXTERNAL` without supplying your own constraint over `/*` leaves
`/ui` **completely open** while the Security tab reports authentication as
enabled.
Review Comment:
**[suggestion]** Both the user manual and the threat model say that
`EXTERNAL` without a constraint leaves `/ui` open “while the Security tab
reports authentication as enabled.” The mode combo is `EXTERNAL`, but the
status line for a principal-less session is
`ConfigSecurityTab.Status.Unrestricted`: “Session is unrestricted (no AuthN).
Config mode: {0}.” Issue #8391 described this more accurately as the tab
*showing the contradiction*. The current wording overstates what the UI claims.
**Suggestion:** Say the General tab can be set to `EXTERNAL` (an
authenticated mode) while the status line still reads unrestricted / no AuthN —
that is the operator-visible contradiction, not a green “authentication
enabled” indicator.
--
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]
