bobbai00 commented on code in PR #9043:
URL: https://github.com/apache/texera/pull/9043#discussion_r4342613183
##########
agent-service/src/server.ts:
##########
@@ -168,6 +168,17 @@ const agentsRouter = new Elysia({ prefix: "/agents" })
set.status = ERROR_STATUS[errorMessage] ?? 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)) {
Review Comment:
**Advisory:**
This guard accepts any role, while the gateway branch requires REGULAR or
ADMIN (`AccessControlResource.scala:174`). An INACTIVE user's unexpired token
passes here: a direct `GET /api/agents` returned 200, and the same request
through nginx returned 403. The comment above names direct access as the reason
this layer exists, so have `verifyToken` (or this guard) reject other roles too.
##########
access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala:
##########
@@ -84,6 +86,8 @@ object AccessControlResource extends LazyLogging {
case wsapiWorkflowWebsocket() | apiExecutionsStats() |
apiExecutionsResultExport() |
pveRoute() =>
checkComputingUnitAccess(uriInfo, headers, bodyOpt)
+ case apiAgents() =>
Review Comment:
**Must fix:**
This resource only has GET/POST/PUT/DELETE handlers (lines 328-360), and
Envoy's ext_authz forwards the client's method. So every settings Save (`PATCH
/api/agents/:id/settings`, `agent.service.ts:1157`) gets a 405 on k8s; I
reproduced it with Envoy in front of this branch. Add an `@PATCH` handler
mirroring `authorizePut`, plus a PATCH test (nginx is fine: its subrequests are
GET).
##########
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:
**Advisory:**
This literal is a third copy of the `auth.conf` secret. When neither
`AUTH_JWT_SECRET` nor the file is found, the module logs a warning and silently
uses it (line 57). If the `auth.conf` default changes, an agent-service started
elsewhere 401s every request. The image already bundles `auth.conf`, so throw
at startup instead.
##########
access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala:
##########
@@ -47,6 +47,8 @@ object AccessControlResource extends LazyLogging {
private val apiExecutionsStats: Regex =
""".*/api/executions/[0-9]+/stats/[0-9]+.*""".r
private val apiExecutionsResultExport: Regex =
""".*/api/executions/result/export.*""".r
private val pveRoute: Regex =
"""^/?(?:auth/)?(?:api/|wsapi/)?pve(?:/.*)?$""".r
+ // Agent service: authenticate any /api/agents request (Phase 1 — see #5561).
+ private val apiAgents: Regex = """.*/api/agents.*""".r
Review Comment:
**Must fix:**
`authorize()` also backs the dynamic-routes policy (`/wsapi`, `/api/pve`),
so `auth/wsapi/x/api/agents` now returns 200 for any REGULAR user instead of
403. It does so without the Host rewrite `checkComputingUnitAccess` sets (line
264). I confirmed the 403 to 200 flip, not where the dynamic backend then
routes it. Anchor it like `pveRoute` and add a negative `/wsapi` test.
```suggestion
private val apiAgents: Regex = """^/?(?:auth/)?api/agents(?:/.*)?$""".r
```
##########
access-control-service/src/test/scala/org/apache/texera/service/resource/AgentAccessAuthSpec.scala:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.texera.service.resource
+
+import jakarta.ws.rs.core.{HttpHeaders, MultivaluedHashMap, Response, UriInfo}
+import org.apache.texera.auth.JwtAuth
+import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum
+import org.apache.texera.dao.jooq.generated.tables.pojos.User
+import org.mockito.Mockito.{mock, when}
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+import java.net.URI
+import java.util.Collections
+
+/**
+ * Exercises the `/api/agents` branch of [[AccessControlResource.authorize]] —
+ * the Phase 1 ext_authz gate that authenticates the JWT and requires a
+ * REGULAR/ADMIN role. Tokens are minted with the same [[JwtAuth]] the rest of
+ * Texera uses, so this is a real signature round-trip against auth.conf.
+ */
+class AgentAccessAuthSpec extends AnyFlatSpec with Matchers {
+
+ private def token(role: UserRoleEnum): String = {
+ val u = new User()
+ u.setUid(7)
+ u.setName("agent-user")
+ u.setEmail("[email protected]")
+ u.setGoogleId(null)
Review Comment:
**Must fix:**
This spec does not compile. `User` has no `setGoogleId` since #7055, and
`JwtAuth.jwtClaims` has no `expireInDays` parameter (line 48). That is why
`build / platform (access-control-service)` and `build / amber` are red, so the
testing section's claim does not hold yet. Deleting this line and calling
`JwtAuth.jwtClaims(u)` makes all 30 resource tests pass locally.
##########
agent-service/src/server.ts:
##########
@@ -447,6 +458,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)) {
+ sendEventToClient(ws, new WsServerErrorEvent("Unauthorized"));
+ ws.close();
Review Comment:
**Advisory:**
`ws.close()` with no code sends 1000, and the frontend only marks an agent
UNAVAILABLE when `event.code !== 1000` (`agent.service.ts:443`). Its error
handler only logs errors other than "Agent not found", so a rejected socket
leaves the panel looking connected. Both gateways refuse the upgrade before
this runs, but the frontend dev proxy sends `/react` straight here.
```suggestion
ws.close(1008, "Unauthorized");
```
##########
bin/k8s/templates/base/agent-service/agent-service-deployment.yaml:
##########
@@ -59,6 +59,10 @@ spec:
# computing unit id at request time.
- name: EXECUTION_ENDPOINT_TEMPLATE
value: http://computing-unit-{cuid}.{{
.Values.workflowComputingUnitPool.name }}-svc.{{
.Values.workflowComputingUnitPool.namespace }}.svc.cluster.local:{{
.Values.workflowComputingUnitPool.service.port }}
+ {{- range .Values.texeraEnvVars }}
+ - name: {{ .name }}
+ value: "{{ .value }}"
+ {{- end }}
Review Comment:
**Advisory:**
This injects all of `texeraEnvVars`, including `USER_SYS_ADMIN_PASSWORD`,
`USER_SYS_GOOGLE_SMTP_PASSWORD` and `USER_SYS_ORCID_CLIENT_SECRET`, but
agent-service reads only `AUTH_JWT_SECRET` from that list. Passing just that
entry keeps unrelated secrets out of the pod that runs LLM-driven tools.
```suggestion
{{- range .Values.texeraEnvVars }}
{{- if eq .name "AUTH_JWT_SECRET" }}
- name: {{ .name }}
value: "{{ .value }}"
{{- end }}
{{- end }}
```
##########
agent-service/src/server.ts:
##########
@@ -168,6 +168,17 @@ const agentsRouter = new Elysia({ prefix: "/agents" })
set.status = ERROR_STATUS[errorMessage] ?? 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 }) => {
Review Comment:
**Advisory:**
With this guard in place, the POST handler's missing-token and
`validateToken` branches (lines 199-205) and the two token entries in
`ERROR_STATUS` (lines 151-152) can no longer fire. The updated specs already
expect "Unauthorized" for those cases, and the handler now verifies the same
token up to three times. Keep `extractUserFromToken` there and drop the rest.
##########
.github/workflows/build.yml:
##########
@@ -246,7 +246,7 @@ jobs:
env:
PGPASSWORD: postgres
- name: Setup sbt launcher
- uses: sbt/setup-sbt@8feba82adc7f01ddcf8165b86f778bdb5b82cebc # v1.5.7
+ uses: sbt/setup-sbt@ecad189dd245444b9ae0a0cd5922fd938b88906f # v1.5.12
Review Comment:
**Advisory:**
This `sbt/setup-sbt` bump (commit a0791b6f9, four workflow files) is
unrelated to agent auth and duplicates open #9064. Once #9064 merges, rebase it
out so this PR's squash commit carries only the auth change.
##########
access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala:
##########
@@ -125,6 +129,60 @@ object AccessControlResource extends LazyLogging {
}
}
+ // 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(
+ uriInfo: UriInfo,
+ headers: HttpHeaders,
+ bodyOpt: Option[String]
+ ): String = {
+ val qToken = Option(uriInfo.getQueryParameters().getFirst("access-token"))
+ .map(_.trim)
+ .filter(_.nonEmpty)
+ val hToken = Option(headers.getRequestHeader("Authorization"))
+ .flatMap(_.asScala.headOption)
+ .map(_.replaceFirst("(?i)^Bearer\\s+", "")) // case-insensitive "Bearer "
+ .map(_.trim)
+ .filter(_.nonEmpty)
+ val bToken = bodyOpt.flatMap(extractTokenFromBody)
+ qToken.orElse(hToken).orElse(bToken).getOrElse("")
+ }
+
+ // Phase 1 agent authorization: authenticate the JWT and require a
+ // REGULAR/ADMIN role. Any such user may reach any agent for now; per-agent
+ // ownership is deferred (see #5302 / #5561). On success, forward the user
+ // identity headers so the agent service can trust them.
Review Comment:
**Polish:**
agent-service never reads `x-user-id`, `x-user-name` or `x-user-email`, so
"can trust them" overstates what happens today.
```suggestion
// ownership is deferred (see #5302 / #5561). On success, return the user
// identity headers for future per-agent authorization in agent-service.
```
##########
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";
+
+/** Read the `256-bit-secret` default out of auth.conf, if the file is found.
*/
+function readSecretFromAuthConf(): string | undefined {
+ const candidates = [
+ env.AUTH_CONF_PATH, // explicit override
+ "auth.conf", // bundled next to the app in the container image
+ "../common/config/src/main/resources/auth.conf", // repo layout in local
dev
+ ].filter((p): p is string => typeof p === "string" && p.length > 0);
+
+ for (const candidate of candidates) {
+ const path = resolve(process.cwd(), candidate);
+ if (!existsSync(path)) continue;
+ // The first quoted `256-bit-secret = "..."` is the literal default; the
+ // second line is the `${?AUTH_JWT_SECRET}` env override (handled below).
+ const match = readFileSync(path,
"utf-8").match(/256-bit-secret\s*=\s*"([^"]*)"/);
+ if (match) {
+ log.info({ path }, "loaded JWT secret default from auth.conf");
+ return match[1];
+ }
+ }
+ log.warn("auth.conf not found; falling back to the built-in default JWT
secret");
+ return undefined;
+}
+
+// Resolution order mirrors HOCON's `256-bit-secret = "<default>";
256-bit-secret
+// = ${?AUTH_JWT_SECRET}`, and the `.toLowerCase()` normalization AuthConfig
+// applies. ("random" is intentionally unsupported here — it cannot match
across
+// processes, so deployments share a fixed secret.)
+export const JWT_SECRET: string = (
+ env.AUTH_JWT_SECRET ||
+ readSecretFromAuthConf() ||
+ AUTH_CONF_DEFAULT_SECRET
+).toLowerCase();
+
+const SECRET_BYTES = new TextEncoder().encode(JWT_SECRET);
+
+export interface JwtUser {
+ uid: number;
+ name: string;
+ email: string;
+ role: string;
+}
+
+function decodeSegment(segment: string): unknown {
+ return JSON.parse(Buffer.from(segment, "base64url").toString("utf-8"));
+}
+
+/**
+ * Verify an HS256 JWT issued by the Scala services and return its claims, or
+ * null if the token is missing, malformed, mis-signed, expired, or uses a
+ * different algorithm. Mirrors org.apache.texera.auth.JwtParser.parseToken.
+ */
+export function verifyToken(token: string | undefined | null): JwtUser | null {
Review Comment:
**Must fix:**
This is what the PR does, but the title scopes it as `agent`, which is not a
module name. AGENTS.md asks for module scopes, and main uses `agent-service`.
The title is also 84 characters (max 75), and `(#5561)` repeats the `Closes`
link. Suggested: `feat(agent-service): authenticate requests via JWT and
gateway ext_authz`.
##########
frontend/src/app/workspace/service/agent/agent.service.ts:
##########
@@ -233,15 +234,18 @@ export class AgentService {
/**
* Build HTTP headers for agent-service requests.
- * Includes X-Agent-Workflow-Id for consistent hash routing in k8s.
+ * Includes X-Agent-Workflow-Id for consistent hash routing in k8s,
+ * and Authorization Bearer token for access control.
*/
- private agentHeaders(agentId?: string): { headers: HttpHeaders } {
+ private agentHeaders(agentId?: string, workflowId?: number): { headers:
HttpHeaders } {
let headers = new HttpHeaders();
- if (agentId) {
- const wid = this.agentStateTracking.get(agentId)?.workflowId;
- if (wid !== undefined) {
- headers = headers.set("X-Agent-Workflow-Id", String(wid));
- }
+ const token = AuthService.getAccessToken();
+ if (token) {
+ headers = headers.set("Authorization", `Bearer ${token}`);
Review Comment:
**Advisory:**
`JwtModule.forRoot` (`app.module.ts:203`) already adds this header to every
same-origin request, which is what the `createAgent` doc at line 654 says.
Setting it again duplicates the interceptor and bypasses its `skipWhenExpired`
check. The frontend change agent-service actually needed is the WebSocket
`?access-token=` at line 417, so drop the Authorization lines (the
`X-Agent-Workflow-Id` on `createAgent` is still worth keeping).
##########
bin/single-node/nginx.conf:
##########
@@ -95,7 +102,12 @@ http {
proxy_set_header X-Real-IP $remote_addr;
}
+ # LLM traffic (chat + model list) is handled by the agent-service,
which
+ # talks to LiteLLM directly; /api/agents below already routes there.
+ # Each request is first authenticated by the access-control-service
+ # (ext_authz) via the auth_request subrequest below.
Review Comment:
**Polish:**
The first two lines contradict the blocks just above: `/api/models` and
`/api/chat/` go to access-control-service, not agent-service.
```suggestion
# Each /api/agents request is first authenticated by the
# access-control-service (ext_authz) via the auth_request subrequest
below.
```
--
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]
