Skip to content

fix auth trust boundary and enforce async job ownership - #199

Merged
AjayThorve merged 6 commits into
NVIDIA-AI-Blueprints:developfrom
cdgamarose-nv:cdgamarose/security_fix
Apr 21, 2026
Merged

AjayThorve merged 6 commits into
NVIDIA-AI-Blueprints:developfrom
cdgamarose-nv:cdgamarose/security_fix

Conversation

@cdgamarose-nv

Copy link
Copy Markdown
Collaborator

Summary

This PR fixes two auth/security issues in aiq:

  1. Unverified JWT payloads were being used as trusted user identity in request-backed flows.
  2. Async job endpoints lacked object-level authorization and only checked request auth + job existence.

Changes

1. Trusted identity now comes only from verified middleware context

  • Added verified principal helpers in aiq_agent.auth:
    • get_current_principal()
    • get_verified_current_user()
  • Changed get_current_user_info() to read only from verified middleware context.
  • Renamed unsafe JWT helpers to make their trust boundary explicit:
    • decode_unverified_jwt_payload()
    • get_user_info_from_unverified_token()
  • Kept the raw decode behavior only as explicitly unverified helpers.
  • Updated request-backed chat_researcher flows to use the verified principal instead of deriving identity from raw token payloads.
  • Updated middleware internal caller labeling so raw internal JWT/cookie traffic is marked unverified_jwt, not a trusted auth type.

2. Async job object-level authorization

  • Added a separate AIQ-owned job_access table keyed by job_id.
  • Persist ownership at submission time using verified principal identity:
    • owner_auth_type
    • owner_subject
    • owner_email
  • Added shared authorization helpers to require:
    • job exists
    • job_access row exists
    • requesting principal matches stored owner
  • Applied ownership enforcement to async job endpoints:
    • status
    • stream
    • stream resume
    • cancel
    • state
    • report
  • Unauthorized or ownerless job access now returns 404.

3. WebSocket auth bridge for UI-backed chat (transitional)

  • Added a transitional WebSocket auth path in aiq_api so UI WebSocket chat can establish verified identity before the UI moves fully to HTTP/SSE.
  • Moved WebSocket auth to the actual socket route boundary rather than relying only on handler-level monkeypatching.
  • Reused the same validator chain and trusted principal shape as HTTP auth.

4. DB init and cleanup

  • Added idempotent job_access creation to both init SQL paths:
    • deploy/compose/init-db.sql
    • deploy/helm/helm-charts-k8s/aiq/files/init-db.sql
  • Added cleanup for stale job_access rows alongside job cleanup.
  • Ensured submit fails closed and rolls back partial job state if access metadata persistence fails.

5. Code quality / cleanup

  • Removed package-level lazy export workaround for aiq_api.jobs.
  • Switched callers to direct submodule imports.
  • Removed internal-only UI auth/Datadog changes from this repo so public aiq remains free of NVIDIA-internal UI overlays.

Security impact

This PR closes:

  • the request-identity trust-boundary issue where decoded JWT payloads could be mistaken for authenticated identity
  • the authenticated BOLA/IDOR-style gap on async job endpoints

Trusted authorization decisions now use verified principal identity (type + sub), and async job access is enforced per object rather than per request only.

Testing

Validated in local/dev environments with:

  • auth helper behavior for verified principal resolution
  • async job ownership persistence in job_access
  • cross-principal access denial (404) for async jobs
  • idempotent schema creation for job_access
  • WebSocket/UI chat verified-principal propagation in internal testing
  • targeted compile/test checks for touched modules

Notes

  • Deprecated unsafe JWT helper aliases remain in utils.py for compatibility, but they are no longer part of the public auth surface.
  • The WebSocket auth path is a transitional bridge until the UI fully migrates to authenticated HTTP/SSE.

@cdgamarose-nv
cdgamarose-nv marked this pull request as ready for review April 20, 2026 20:06
@greptile-apps

greptile-apps Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR closes two concrete security gaps: JWT payloads are no longer used as trusted identity (verified principal now comes exclusively from middleware ContextVar), and async job endpoints enforce per-object ownership via a new job_access table keyed by (owner_auth_type, owner_subject). A WebSocket auth bridge is added to propagate verified identity into HITL chat flows using the same validator chain as HTTP.

The previously-raised P1 concerns (blocking sync DB calls in authorize_job_access and the orphaned Dask task warning on rollback) have been addressed in this revision. One remaining open note is that the backward-compat submit_deep_research_job alias does not thread a principal through, which causes a RuntimeError for callers outside an HTTP request context when auth enforcement is enabled.

Confidence Score: 4/5

Safe to merge with two open items: the backward-compat alias runtime failure and the misleading 500 error message noted inline.

The core security fixes (verified principal, per-object job authorization, blocking-call fixes) are solid and the previously-flagged P1s are addressed. The remaining open issues are a pre-existing compat alias gap and a confusing error message — both P2 in practice.

frontends/aiq_api/src/aiq_api/jobs/submit.py (submit_deep_research_job alias), frontends/aiq_api/src/aiq_api/routes/jobs.py (500 error message)

Important Files Changed

Filename Overview
frontends/aiq_api/src/aiq_api/auth/middleware.py Extracts is_external_request, is_headless_request, extract_auth_token, and validate_token_with_validators as standalone helpers; adds user_context ContextVar manager for WebSocket auth propagation; renames internal traffic to unverified_jwt type.
frontends/aiq_api/src/aiq_api/jobs/access.py New module: per-job ownership table helpers, authorize_job_access with ownership enforcement, require_verified_principal with no-auth fallback. Blocking DB calls wrapped in run_in_executor per prior review feedback.
frontends/aiq_api/src/aiq_api/jobs/submit.py Adds principal param to submit_agent_job with rollback on access-persistence failure. Backward-compat submit_deep_research_job alias still lacks a principal parameter, which fails at runtime when called outside an HTTP request context with auth enabled.
frontends/aiq_api/src/aiq_api/routes/jobs.py All async job endpoints now call require_verified_principal() + authorize_job_access(); submit route explicitly passes principal; misleading 500 error message for non-auth failures (see inline comment).
frontends/aiq_api/src/aiq_api/websocket_reconnect.py Adds WebSocket auth bridge: configure_websocket_auth mirrors HTTP validator chain; authenticate_websocket_connection resolves identity at handshake; process_workflow_request wraps workflow invocation in user_context so ContextVar propagates to spawned tasks.
src/aiq_agent/auth/utils.py Renames unsafe JWT helpers to decode_unverified_jwt_payload/get_user_info_from_unverified_token; adds Principal model, get_current_principal, and get_verified_current_user; deprecated aliases now emit warnings.
src/aiq_agent/agents/chat_researcher/register.py Uses get_current_principal() to derive owner in _submit_deep_job but does not forward the resolved principal to submit_agent_job, causing a second internal get_current_principal() call (redundant but functionally correct while the ContextVar is still set).
frontends/aiq_api/src/aiq_api/plugin.py Adds configure_websocket_auth call alongside AuthMiddleware registration so both paths share the same validator chain; switches to direct submodule imports for EventStore and get_connection_manager.
frontends/aiq_api/src/aiq_api/jobs/init.py Replaces lazy-export workaround with explicit all listing submodule names; removes re-exports that encouraged opaque import paths.
deploy/compose/init-db.sql Adds idempotent job_access table and index for job ownership tracking; mirrors the helm chart SQL correctly.
deploy/helm/helm-charts-k8s/aiq/files/init-db.sql Parallel addition of job_access schema to the Helm deployment SQL; identical and correct.

Sequence Diagram

sequenceDiagram
    participant Client
    participant AuthMiddleware
    participant JobRoute
    participant access.py
    participant job_access DB
    participant JobStore

    Client->>AuthMiddleware: POST /v1/jobs/async/submit (Bearer token)
    AuthMiddleware->>AuthMiddleware: validate_token_with_validators()
    AuthMiddleware->>AuthMiddleware: set _current_user ContextVar (type, sub, email)
    AuthMiddleware->>JobRoute: forward request

    JobRoute->>access.py: require_verified_principal()
    access.py->>access.py: get_current_principal() from ContextVar
    access.py-->>JobRoute: Principal(type, sub, email)

    JobRoute->>JobStore: submit_job(job_id, ...)
    JobStore-->>JobRoute: ok (Dask task running)

    JobRoute->>access.py: create_job_access(job_id, principal) via run_in_executor
    access.py->>job_access DB: INSERT job_id, owner_auth_type, owner_subject
    job_access DB-->>access.py: ok
    access.py-->>JobRoute: ok

    JobRoute-->>Client: job_id + status submitted

    Client->>JobRoute: GET /v1/jobs/async/job/job_id
    JobRoute->>access.py: require_verified_principal()
    access.py-->>JobRoute: Principal(type, sub)
    JobRoute->>access.py: authorize_job_access(job_id, principal)
    access.py->>JobStore: get_job(job_id)
    JobStore-->>access.py: job
    access.py->>job_access DB: SELECT WHERE job_id
    job_access DB-->>access.py: owner_auth_type + owner_subject
    access.py->>access.py: principal_matches_access()?
    alt owner matches
        access.py-->>JobRoute: job
        JobRoute-->>Client: JobStatusResponse
    else owner mismatch or no access row
        access.py-->>JobRoute: HTTPException 404
        JobRoute-->>Client: 404 Not Found
    end
Loading

Reviews (4): Last reviewed commit: "skip ownership check when require auth i..." | Re-trigger Greptile

Comment thread frontends/aiq_api/src/aiq_api/auth/middleware.py Outdated
Comment thread frontends/aiq_api/src/aiq_api/jobs/submit.py

@AjayThorve AjayThorve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue, rest all looks good

Comment thread frontends/aiq_api/src/aiq_api/jobs/submit.py
Comment thread frontends/aiq_api/src/aiq_api/jobs/submit.py Outdated
Comment thread frontends/aiq_api/src/aiq_api/jobs/access.py

@AjayThorve AjayThorve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, works will w/ and w/o auth

@AjayThorve
AjayThorve merged commit bfbb467 into NVIDIA-AI-Blueprints:develop Apr 21, 2026
9 checks passed
taylorjordanNC pushed a commit to taylorjordanNC/rh-research that referenced this pull request May 27, 2026
…ueprints#199)

* fix auth trust boundary and enforce async job ownership

* fix old tests

* share request identity resolution for HTTP and WebSockets

* remove log and fix no auth flow

* make rollback async

* skip ownership check when require auth is false
taylorjordanNC pushed a commit to taylorjordanNC/rh-research that referenced this pull request May 27, 2026
Two upstream test gaps caught locally:

1. tests/aiq_agent/jobs/test_runner.py — auth_token is now the last
   positional arg in submit_job's job_args list (NVIDIA-AI-Blueprints#199 added it after
   data_sources). Update test to assert job_args[-2] == data_sources
   instead of [-1].

2. frontends/aiq_api/tests/test_job_access.py::TestAuthorizeJobAccess —
   authorize_job_access only enforces ownership when REQUIRE_AUTH=true,
   but the test class never set that env var. Add an autouse
   monkeypatch.setenv fixture to the class so the cross-user-denied and
   missing-access-row tests exercise the auth-enabled branch they
   expect.

Both are upstream issues caught by our long-running smoke; not
production-code changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cdgamarose-nv
cdgamarose-nv deleted the cdgamarose/security_fix branch July 6, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants