Repository navigation
feat(agent-service): authenticate requests via JWT and gateway ext_authz - #9043
Alwaysgaurav1 wants to merge 13 commits into
Conversation
…t_authz (apache#5561) Authorize requests reaching the agent service using JWT signature verification and gateway external authorization, providing defense in depth: 1. agent-service: - Added real HS256 signature verification in config/jwt.ts using crypto.createHmac and timingSafeEqual. - Resolves shared secret from AUTH_JWT_SECRET env or auth.conf with 30-second clock skew tolerance. - Added .onBeforeHandle guard on agentsRouter rejecting unauthenticated, expired, or invalid tokens with 401. - Added access-token query param check in WebSocket open() handler. 2. access-control-service: - Added /api/agents routing branch in AccessControlResource. - Verifies JWT validity and enforces REGULAR or ADMIN role. - Forwards trusted identity headers (x-user-id, x-user-name, x-user-email). 3. Gateways & Deployment: - bin/single-node/nginx.conf: Added auth_request /_agent_auth subrequest on /api/agents routing to access-control-service. - bin/k8s/templates/base/gateway/gateway-agent-security-policy.yaml: Added Envoy SecurityPolicy for agent-service-route. - bin/k8s/templates/base/agent-service/agent-service-deployment.yaml: Passed texeraEnvVars into agent-service container. - bin/dockerfiles/agent-service.dockerfile: Bundled auth.conf for container runtime. 4. Frontend: - Attaches Authorization Bearer token via agentHeaders() to REST requests. - Appends ?access-token= query param to WebSocket connection in startStatePolling(). - Added unit tests in agent.service.spec.ts.
github-actions
Bot
added
feature
frontend
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9043 +/- ##
============================================
+ Coverage 91.81% 91.88% +0.07%
+ Complexity 5020 5015 -5
============================================
Files 1277 1283 +6
Lines 56350 56246 -104
Branches 7360 7321 -39
============================================
- Hits 51736 51681 -55
+ Misses 2758 2734 -24
+ Partials 1856 1831 -25
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 358 | 0.219 | 26,789/41,434/41,434 us | 🔴 -11.2% / 🔴 +193.4% |
| 🟢 | bs=100 sw=10 sl=64 | 802 | 0.489 | 123,477/139,574/139,574 us | 🟢 -25.7% / 🔴 +50.9% |
| 🔴 | bs=1000 sw=10 sl=64 | 931 | 0.568 | 1,063,249/1,147,336/1,147,336 us | 🔴 +5.1% / 🔴 +35.4% |
Baseline details
Latest main 2407d79 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 358 tuples/sec | 403 tuples/sec | 968.33 tuples/sec | -11.2% | -63.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.219 MB/s | 0.246 MB/s | 0.591 MB/s | -11.0% | -62.9% |
| bs=10 sw=10 sl=64 | p50 | 26,789 us | 25,259 us | 10,646 us | +6.1% | +151.6% |
| bs=10 sw=10 sl=64 | p95 | 41,434 us | 41,144 us | 14,120 us | +0.7% | +193.4% |
| bs=10 sw=10 sl=64 | p99 | 41,434 us | 41,144 us | 16,498 us | +0.7% | +151.2% |
| bs=100 sw=10 sl=64 | throughput | 802 tuples/sec | 800 tuples/sec | 1,252 tuples/sec | +0.3% | -35.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.489 MB/s | 0.488 MB/s | 0.764 MB/s | +0.2% | -36.0% |
| bs=100 sw=10 sl=64 | p50 | 123,477 us | 119,696 us | 83,736 us | +3.2% | +47.5% |
| bs=100 sw=10 sl=64 | p95 | 139,574 us | 187,894 us | 92,464 us | -25.7% | +50.9% |
| bs=100 sw=10 sl=64 | p99 | 139,574 us | 187,894 us | 108,561 us | -25.7% | +28.6% |
| bs=1000 sw=10 sl=64 | throughput | 931 tuples/sec | 945 tuples/sec | 1,321 tuples/sec | -1.5% | -29.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.568 MB/s | 0.577 MB/s | 0.806 MB/s | -1.6% | -29.5% |
| bs=1000 sw=10 sl=64 | p50 | 1,063,249 us | 1,059,342 us | 811,260 us | +0.4% | +31.1% |
| bs=1000 sw=10 sl=64 | p95 | 1,147,336 us | 1,091,499 us | 847,636 us | +5.1% | +35.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,147,336 us | 1,091,499 us | 879,277 us | +5.1% | +30.5% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,558.50,200,128000,358,0.219,26788.63,41434.40,41434.40
1,100,10,64,20,2494.46,2000,1280000,802,0.489,123476.51,139573.57,139573.57
2,1000,10,64,20,21472.56,20000,12800000,931,0.568,1063248.68,1147336.01,1147336.01
bobbai00
left a comment
bobbai00
left a comment
There was a problem hiding this comment.
🔴 4 must-fix · 7 advisory · 2 polish: the auth design matches #5561 and works end to end on single-node; CI, k8s PATCH and the path regex need fixes.
Design & architecture (2)
server.ts:177: in-service guard accepts INACTIVE users (advisory, see inline)jwt.ts:36: hard-coded fallback copy of the JWT secret (advisory, see inline)
Correctness (4)
AccessControlResource.scala:89: no PATCH handler, so settings saves get 405 behind Envoy (must-fix, see inline)AccessControlResource.scala:51: unanchored regex authorizes unrelated dynamic-route paths (must-fix, see inline)AgentAccessAuthSpec.scala:46: spec does not compile, failing CI (must-fix, see inline)server.ts:466: rejected socket closes with the normal close code (advisory, see inline)
Simplifications (3)
agent.service.ts:244: duplicates the JwtModule interceptor header (advisory, see inline)agent-service-deployment.yaml:62: injects every texeraEnvVars secret into agent-service (advisory, see inline)server.ts:175: POST /agents token checks are now unreachable (advisory, see inline)
Conventions (2)
jwt.ts:89: title scope is not a module name, and the title is over length (must-fix, see inline)build.yml:249: unrelated sbt bump duplicates #9064 (advisory, see inline)
Polish: 2 quick touch-ups (see inline comments).
Verification trace
Ran this branch end to end on single-node (PR builds of access-control-service, agent-service and frontend, the PR's nginx.conf, a real login token). /api/agents returned 401 without a token, 403 for an INACTIVE user and 200 for REGULAR. The WebSocket upgrade was refused without a token, and an agent created from the UI built and executed a TextInput to Filter workflow. Behind a standalone Envoy ext_authz pointed at this branch's access-control-service, GET/POST/DELETE returned 200 and PATCH /api/agents/:id/settings returned 405. Unit suites: agent-service 327 pass, frontend agent.service.spec 89 pass. The access-control-service tests fail to compile, and pass 30/30 after the two-line fix noted inline.
Alwaysgaurav1
changed the title
|
Thanks for the thorough review and verification trace! I have addressed all items in commit 🔴 Must-Fix
🟡 Advisory & Simplifications
🟢 Polish
Verification:
Please take another look when you get a chance! |
|
@bobbai00 could you plss review it again. |
|
@dzueck Do you want to take a look at this PR as well? |
|
@dzueck Please check. |
bobbai00
left a comment
bobbai00
left a comment
There was a problem hiding this comment.
🔴 13 resolved · 0 open · 6 new (6 new = 4 newly introduced · 2 late catches)
All 13 findings from the last round are fixed in the code. The branch also needs main merged in: the two conflicts are adjacent import lines from #6009, and keeping both sides is enough.
Design & architecture (1)
jwt.ts:61:AUTH_JWT_SECRET=randomis used as the literal key (advisory, see inline)
Correctness (1)
server.ts:175: the guard and the POST handler parse the Authorization header differently, giving a 500 (must-fix, see inline)
Simplifications (2)
auth-api.ts:42:validateTokenhas no production caller left (advisory, see inline)agent.service.ts:255: threeagentHeaders()calls now pass empty headers (advisory, see inline)
Conventions (1)
agent.service.ts:239: the PR description still describes the first revision (must-fix, see inline)
Polish: 1 quick touch-up (see inline comments).
Verification trace
Checked each of the 13 earlier findings against fb66377 instead of the reply. Ran the agent-service suite (346 pass) and probed the app with signed tokens. Authorization: <jwt> returns 500 on POST /api/agents and 200 on GET. With extractBearerToken in the guard, both return 401. With AUTH_JWT_SECRET=RANDOM, a token signed with the string random is accepted. Merged main in a scratch copy keeping both import lines: typecheck and 357 tests pass. The red macOS frontend job is a 20 s timeout in a spec this PR does not touch.
| // 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(); |
There was a problem hiding this comment.
Must fix:
This guard strips an optional Bearer with a regex. The POST handler reads the token with extractBearerToken(...)!, which needs exactly <scheme> <token>. So Authorization: <jwt> passes here and fails there, and POST /api/agents returns 500 where main returns 401. My earlier note said those handler branches could not fire, which was wrong for this header shape. With the helper used here too, every route returns 401 and the suite passes.
| const token = (request.headers.get("authorization") ?? "").replace(/^Bearer\s+/i, "").trim(); | |
| const token = extractBearerToken(request.headers.get("authorization") ?? undefined); |
| if (!secret) { | ||
| throw new Error("JWT secret not found: neither AUTH_JWT_SECRET environment variable nor auth.conf was found."); | ||
| } | ||
| return secret.toLowerCase(); |
There was a problem hiding this comment.
Advisory:
AuthConfig maps the value random to a key generated per JVM (AuthConfig.scala:34). Here it stays the literal key. With AUTH_JWT_SECRET=random, a token signed with the string random passed verifyToken in my probe. The comment below calls the value unsupported, so I'd refuse to start with it.
| return secret.toLowerCase(); | |
| const normalized = secret.toLowerCase(); | |
| if (normalized === "random") { | |
| throw new Error('AUTH_JWT_SECRET="random" is not supported here; all services must share one fixed secret.'); | |
| } | |
| return normalized; |
| } | ||
|
|
||
| /** True only when the token is genuinely valid (signature + expiry verified). */ | ||
| export function validateToken(token: string): boolean { |
There was a problem hiding this comment.
Advisory:
validateToken lost its only production caller when the check in the POST handler was removed. Only auth-api.spec.ts references it now. I'd delete it together with its describe("validateToken") block. jwt.spec.ts already covers the same cases through verifyToken.
| private syncAgentsWithBackend(): void { | ||
| this.http | ||
| .get<ApiAgentListResponse>(`${this.AGENT_API_BASE}/agents`) | ||
| .get<ApiAgentListResponse>(`${this.AGENT_API_BASE}/agents`, this.agentHeaders()) |
There was a problem hiding this comment.
Advisory:
agentHeaders() no longer adds Authorization, so with no arguments it returns an empty header set. This call passes that. So do getAllAgents and fetchModelTypes, and the last one calls /api/models, not agent-service. Reverting the three removes about 30 changed lines. Keep the createAgent change, which sends X-Agent-Workflow-Id.
| .get<ApiAgentListResponse>(`${this.AGENT_API_BASE}/agents`, this.agentHeaders()) | |
| .get<ApiAgentListResponse>(`${this.AGENT_API_BASE}/agents`) |
| * Includes X-Agent-Workflow-Id for consistent hash routing in k8s. | ||
| */ | ||
| private agentHeaders(agentId?: string): { headers: HttpHeaders } { | ||
| private agentHeaders(agentId?: string, workflowId?: number): { headers: HttpHeaders } { |
There was a problem hiding this comment.
Must fix:
The PR description still describes the first revision, and the squash commit copies it. Four statements no longer hold. agentHeaders() does not attach Authorization. The secret has no "built-in defaults" fallback (jwt.ts:59 throws). The matcher is anchored (line 51), not .*/api/agents.*. The frontend spec has no test for the header. Please also add the PATCH handler, the 403 for other roles and the 1008 close code.
| } | ||
|
|
||
| if (userSession.isEmpty) { | ||
| return Response.status(Response.Status.UNAUTHORIZED).build() |
There was a problem hiding this comment.
Polish:
The class comment at lines 319-322 still says "403 on missing/invalid tokens". This branch returns 401 for those. I'd mention both codes there.
What changes were proposed in this PR?
This PR implements Phase 1 authorization for requests reaching the
agent-service, providing defense-in-depth across the API gateways, theaccess-control-service, and theagent-serviceitself:agent-serviceDirect JWT Verification (config/jwt.ts):node:crypto(createHmac,timingSafeEqual).AUTH_JWT_SECRET(falling back to candidateauth.conflocations and built-in defaults) with a 30-second clock skew tolerance..onBeforeHandleguard acrossagentsRouterinserver.tsthat rejects requests missing a valid Bearer token with401 Unauthorized(while preserving unauthenticated probes on/api/healthcheck)./api/agents/:id/reactWebSocket via theaccess-tokenquery parameter during theopen(ws)handshake.access-control-serviceExt-Authz Route (AccessControlResource.scala):.*/api/agents.*toAccessControlResource.authorize().parseToken()and requires aREGULARorADMINuser role.x-user-id,x-user-name,x-user-email) on HTTP 200, returning HTTP 401 for missing/malformed tokens and HTTP 403 for inactive roles.Gateway & Kubernetes Deployment:
bin/single-node/nginx.conf: Configuredupstream access_control_authz, an internal/_agent_authsubrequest location, andauth_request /_agent_auth;on/api/agents.bin/k8s/templates/base/gateway/gateway-agent-security-policy.yaml: Added an Envoy GatewaySecurityPolicyforagent-service-routepointing extAuth toaccess-control-service.bin/k8s/templates/base/agent-service/agent-service-deployment.yaml: Injected.Values.texeraEnvVars(providingAUTH_JWT_SECRET).bin/dockerfiles/agent-service.dockerfile: Bundledauth.confinto the container image.Frontend (
agent.service.ts):agentHeaders()to attachAuthorization: Bearer <token>fromAuthService.getAccessToken().?access-token=${encodeURIComponent(token)}to the WebSocket connection URL instartStatePolling().syncAgentsWithBackend,createAgent,getAllAgents,fetchModelTypes) passagentHeaders().Any related issues, documentation, discussions?
Resolves #5561
How was this PR tested?
agent-serviceUnit & Integration Tests:agent-service/src/config/jwt.spec.ts: Added 9 unit tests testing HS256 signature verification, string coercion, secret mismatches, tampering, expiration within/outside clock skew, non-HS256 algorithms, and malformed tokens.agent-service/src/server.spec.ts: Added unit tests verifying 401 rejection for unauthenticated, invalid, and expired tokens, 200 pass-through for valid tokens, and unauthenticated bypass for/api/healthcheck.agent-service/src/server.ws.spec.ts: Added unit tests verifying WebSocket disconnection withWsServerErrorEvent("Unauthorized")whenaccess-tokenis missing or invalid.tsc --noEmit) passes with 0 errors.access-control-serviceUnit Tests:AgentAccessAuthSpec.scala: Added tests covering 200 OK forREGULARandADMINroles, forwarding ofx-user-*headers, 401 on missing/malformed tokens, and 403 onINACTIVErole.Frontend Unit Tests:
agent.service.spec.ts: Added tests verifying thatAuthorizationis attached when an access token is present and omitted when unauthenticated, and that?access-token=is correctly appended to the WebSocket URL.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Antigravity 2.0 (Gemini 2.5 Pro)