Yicong-Huang commented on code in PR #5558:
URL: https://github.com/apache/texera/pull/5558#discussion_r3741172021
##########
agent-service/src/server.ts:
##########
@@ -160,11 +162,44 @@ const agentsRouter = new Elysia({ prefix: "/agents" })
set.status = 500;
return { error: errorMessage || "Internal server error" };
})
+ // Authenticate every agent request by verifying the Bearer JWT ourselves
+ // (defense in depth — the gateway ext_authz also checks it, but this also
+ // covers direct access, e.g. bare-metal dev). The WebSocket route is guarded
+ // separately in its open() handler via the access-token query param.
+ .onBeforeHandle(({ request, set }) => {
+ const token = (request.headers.get("authorization") ??
"").replace(/^Bearer\s+/i, "").trim();
+ if (!verifyToken(token)) {
+ set.status = 401;
+ return { error: "Unauthorized" };
+ }
+ })
.get("/", () => {
const agentList = Array.from(agentStore.entries()).map(([id, agent]) =>
getAgentInfo(id, agent));
return { agents: agentList };
})
+ // Lists the models available on the LiteLLM gateway. Previously the frontend
+ // hit a LiteLLM proxy on the access-control-service; the agent service now
+ // owns this since it already holds the master key and talks to LiteLLM.
+ .get("/models", async ({ set }) => {
+ const { litellmBaseUrl } = getBackendConfig();
Review Comment:
The two proxies this PR deletes both opened with a `copilotEnabled` check
and returned 403 `{"error": "Copilot feature is disabled"}`, so
`GUI_WORKFLOW_WORKSPACE_COPILOT_ENABLED=false` used to stop chat completions
and the model list at the LLM hop. Nothing here replaces it: grepping `copilot`
now finds only `ConfigResource.scala:71`, which publishes the flag, and the
frontend's `*ngIf="copilotEnabled"`. A deployment that disables copilot still
reaches LiteLLM and bills the provider.
I'd re-gate this path in the agent-service — read the flag alongside the
other backend config and 403 from both `/models` and `createAgentInstance` —
since that keeps the switch where the traffic is. If dropping the backend gate
is intentional, it's worth stating in the description: the deleted
`LiteLLMProxyAuthSpec` covered this and nothing does now.
##########
agent-service/src/config/jwt.ts:
##########
@@ -0,0 +1,131 @@
+/**
+ * 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 { createHmac, timingSafeEqual } from "node:crypto";
+import { existsSync, readFileSync } from "node:fs";
+import { resolve } from "node:path";
+import { env } from "./env";
+import { createLogger } from "../logger";
+
+const log = createLogger("Jwt");
+
+// Token issuance lives in the Scala services (org.apache.texera.auth.JwtAuth):
+// HS256 over the UTF-8 bytes of the secret, with a required `exp` and `sub`
+// and a 30s allowed clock skew. This module mirrors the verification so the
+// agent service can validate the same tokens without a gateway.
+const CLOCK_SKEW_SECONDS = 30;
+
+// auth.conf default, used as the last-resort fallback when neither the env
+// override nor the file is available (matches AuthConfig's literal default).
+const AUTH_CONF_DEFAULT_SECRET = "8a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d";
Review Comment:
This is a verbatim second copy of `auth.conf`'s committed default, with no
pointer in either direction. The dockerfile bundles `auth.conf` into the image
and local dev probes the repo path, so this constant is unreachable in every
shipped configuration — which is what makes it risky: rotate the `auth.conf`
default and this stays stale with nothing to flag it.
I'd drop the constant and fail loudly when neither the env var nor
`auth.conf` yields a secret, rather than silently verifying against a value
that may no longer be the issuer's.
##########
bin/single-node/nginx.conf:
##########
@@ -81,6 +80,18 @@ http {
proxy_send_timeout 1d;
}
+ # Internal ext_authz subrequest: the access-control-service authorizes
+ # /api/agents by JWT. $request_uri preserves the original path so its
+ # /api/agents branch matches (same shape Envoy produces); the
+ # Authorization header and the WS access-token query ride along.
+ location = /_agent_auth {
+ internal;
+ proxy_pass http://access_control_authz/api/auth$request_uri;
Review Comment:
The Envoy policy forwards `x-user-id`/`x-user-name`/`x-user-email` via
`headersToBackend`, but nginx discards an auth subrequest's response headers
unless you capture them, and there's no `auth_request_set` here — so the
identity `checkAgentAccess` returns is dropped on the single-node path.
No consumer today (agent-service reads none of them), so nothing breaks now.
It will at #5302, where per-agent ownership needs exactly these, and it would
fail only in single-node — the hard kind to notice. Three `auth_request_set`
lines plus matching `proxy_set_header`s in the `/api/agents` block would close
the gap now.
##########
agent-service/src/config/jwt.ts:
##########
@@ -0,0 +1,131 @@
+/**
+ * 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 { createHmac, timingSafeEqual } from "node:crypto";
+import { existsSync, readFileSync } from "node:fs";
+import { resolve } from "node:path";
+import { env } from "./env";
+import { createLogger } from "../logger";
+
+const log = createLogger("Jwt");
+
+// Token issuance lives in the Scala services (org.apache.texera.auth.JwtAuth):
+// HS256 over the UTF-8 bytes of the secret, with a required `exp` and `sub`
+// and a 30s allowed clock skew. This module mirrors the verification so the
+// agent service can validate the same tokens without a gateway.
Review Comment:
The required `exp`/`sub` and the 30s skew are `jwtConsumer`'s verification
settings, not properties of issuance — worth attributing them to the side that
enforces them, since this module mirrors exactly that.
```suggestion
// Token issuance and verification live in the Scala services
// (org.apache.texera.auth.JwtAuth): HS256 over the UTF-8 bytes of the
secret.
// Verification requires `exp` and `sub` and allows 30s of clock skew. This
// module mirrors that verification so the agent service needs no gateway.
```
##########
bin/agent-service.dockerfile:
##########
@@ -35,6 +35,10 @@ RUN bun install --frozen-lockfile --production
COPY agent-service/src ./src
COPY agent-service/tsconfig.json ./
+# Shared JWT secret default (HS256) used to verify Bearer tokens. The effective
+# secret is AUTH_JWT_SECRET when set; otherwise this file's default is used.
Review Comment:
"this file" reads as the Dockerfile itself — `auth.conf` isn't named until
the `COPY` on the next line.
```suggestion
# secret is AUTH_JWT_SECRET when set; otherwise the copied auth.conf's
default.
```
##########
access-control-service/src/main/scala/org/apache/texera/service/AccessControlService.scala:
##########
@@ -32,14 +32,8 @@ import org.apache.texera.auth.{
}
import org.apache.texera.dao.SqlServer
import org.apache.texera.service.activity.UserActivityEventListener
-import org.apache.texera.service.resource.{
- AccessControlResource,
- HealthCheckResource,
- LiteLLMModelsResource,
- LiteLLMProxyResource
-}
+import org.apache.texera.service.resource.{AccessControlResource,
HealthCheckResource}
Review Comment:
Dropping `RolesAllowedDynamicFeature` is correct today — no `@RolesAllowed`
survives in this service once the proxies go. But every sibling still registers
it (`ConfigService.scala:127`, `WorkflowCompilingService.scala:110`,
`ComputingUnitManagingService.scala:95`, `TexeraWebApplication.scala:142`,
`ComputingUnitMaster.scala:172`) and each keeps a RunSpec assertion; this PR
removes access-control-service's too.
Jersey ignores an unregistered `@RolesAllowed` with no exception and no log,
and there's no startup enforcer in the tree to catch it, so the next
role-guarded endpoint added here would be silently unenforced. I'd keep the
registration and its assertion — one line, and this is the access-control
service.
##########
access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala:
##########
@@ -69,12 +70,68 @@ object AccessControlResource extends LazyLogging {
case wsapiWorkflowWebsocket() | apiExecutionsStats() |
apiExecutionsResultExport() |
pveRoute() =>
checkComputingUnitAccess(uriInfo, headers, bodyOpt)
+ case apiAgents() =>
+ checkAgentAccess(uriInfo, headers, bodyOpt)
case _ =>
logger.warn(s"No authorization logic for path: $path. Denying access.")
Response.status(Response.Status.FORBIDDEN).build()
}
}
+ // Extract the bearer token from the access-token query param, the
+ // Authorization header, or a "token" field in the body (in that order).
+ private def extractBearerToken(
Review Comment:
This is a second copy of the token-reading block still inlined in
`checkComputingUnitAccess` below — same three sources in the same order. The
two have already drifted: this one trims the query-param token, the other
doesn't.
Nothing misbehaves today, but the next fix to token parsing now has two
homes. Since this helper is the better of the two, I'd point
`checkComputingUnitAccess` at it and delete its inline block in this PR.
##########
agent-service/src/server.ts:
##########
@@ -485,6 +520,15 @@ export function buildApp() {
)
.ws(`${env.API_PREFIX}/agents/:id/react`, {
open(ws) {
+ // Browsers can't set headers on a WebSocket, so the JWT arrives as the
+ // access-token query param (same convention as the workflow WS).
+ const token = (ws.data as any).query?.["access-token"];
+ if (!verifyToken(token)) {
Review Comment:
`open()` rejects by sending an error frame, calling `ws.close()`, and
returning, but the verdict isn't recorded anywhere — so `message()` at line 554
has no flag to consult and runs on whatever socket reaches it.
Can a client that pipelines a frame right after the handshake get one
delivered between `ws.close()` and teardown here? I couldn't confirm Bun's
behavior without a live server, so treat this as a question rather than a
claim. Either way, stashing the verified user on `ws.data` in `open()` and
checking it at the top of `message()` makes the guard structural instead of
positional.
--
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]
