Conversation
f8b5fb5 to
b608370
Compare
Introduces optional OIDC authentication via oauth2-proxy subchart. When controller.auth.mode is set to "proxy", the controller trusts JWT tokens from the Authorization header (set by oauth2-proxy) instead of the default unsecure X-User-Id header. Includes proxy authenticator, /api/me endpoint, auth header forwarding in UI server actions, AuthContext/login page in the frontend, network policies, and Helm configuration for oauth2-proxy integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Collin Walker <cwalker@ancestry.com> Signed-off-by: Collin Walker <lets-call-n-walk@users.noreply.github.com>
b608370 to
fcb1244
Compare
There was a problem hiding this comment.
Pull request overview
Adds an optional “OIDC proxy auth” mode to kagent, intended to run behind oauth2-proxy and derive user identity from a JWT in the Authorization header, while keeping the existing unauthenticated (“unsecure”) behavior as the default.
Changes:
- Backend: introduce
ProxyAuthenticator, extendPrincipalwith identity fields, and add/api/mefor current-user introspection. - Frontend: add auth header forwarding utilities, an
AuthContext+UserMenu, and a branded/loginpage. - Helm: add optional oauth2-proxy subchart + templates, auth-related values/env wiring, and NetworkPolicies to restrict direct UI/controller access when auth is enabled.
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/types/index.ts | Adjust UI request typing for session creation (removes user_id from request shape). |
| ui/src/lib/jwt.ts | JWT decode + claim extraction utilities for UI/server actions. |
| ui/src/lib/auth.ts | Centralized Authorization header extraction for server actions/route handlers. |
| ui/src/contexts/AuthContext.tsx | Client-side provider for “current user” state via a server action. |
| ui/src/components/chat/ChatInterface.tsx | Stops injecting user_id when creating sessions. |
| ui/src/components/UserMenu.tsx | Adds user dropdown with groups display + sign out. |
| ui/src/components/Header.tsx | Wires UserMenu into desktop/mobile header layouts. |
| ui/src/app/tools/page.tsx | Fixes import path to use app actions alias. |
| ui/src/app/servers/page.tsx | Fixes import path to use app actions alias. |
| ui/src/app/login/page.tsx | Adds branded login page with SSO redirect link. |
| ui/src/app/layout.tsx | Wraps app in AuthProvider and adjusts root layout hydration settings. |
| ui/src/app/actions/utils.ts | Stops appending user_id to backend requests; forwards Authorization instead. |
| ui/src/app/actions/feedback.ts | Removes user_id injection from feedback submission. |
| ui/src/app/actions/auth.ts | Adds server action to derive current user from incoming Authorization JWT. |
| ui/src/app/a2a/[namespace]/[agentName]/route.ts | Forwards auth header through the A2A proxy route. |
| ui/package.json | Adds jose dependency for JWT decoding. |
| helm/kagent/values.yaml | Adds controller/ui auth values, oauth2-proxy values, and networkPolicy values. |
| helm/kagent/templates/ui-deployment.yaml | Injects SSO_REDIRECT_PATH env var into UI deployment. |
| helm/kagent/templates/oauth2-proxy-templates.yaml | Adds custom oauth2-proxy template ConfigMap to redirect to /login. |
| helm/kagent/templates/networkpolicy.yaml | Adds NetworkPolicies to restrict ingress paths when proxy auth is enabled. |
| helm/kagent/templates/controller-deployment.yaml | Injects AUTH_MODE and JWT-claim env vars into controller when in proxy mode. |
| helm/kagent/templates/_helpers.tpl | Adds helper to conditionally enable NetworkPolicies when auth is enforced. |
| helm/kagent/templates/NOTES.txt | Documents NetworkPolicy behavior when enabled. |
| helm/kagent/Chart-template.yaml | Adds oauth2-proxy as an optional chart dependency. |
| go/test/e2e/auth_api_test.go | Adds E2E coverage for /api/me across auth modes. |
| go/pkg/auth/auth_test.go | Adds basic test for new Principal user/group fields. |
| go/pkg/auth/auth.go | Extends auth model with Email, Name, and Groups. |
| go/internal/httpserver/server.go | Adds /api/me route and nil-safety for DB manager shutdown. |
| go/internal/httpserver/handlers/handlers.go | Registers CurrentUserHandler. |
| go/internal/httpserver/handlers/current_user_test.go | Unit tests for /api/me handler behavior. |
| go/internal/httpserver/handlers/current_user.go | Implements /api/me handler using auth session principal. |
| go/internal/httpserver/auth/proxy_authn_test.go | Unit tests for JWT claim extraction + fallback behaviors. |
| go/internal/httpserver/auth/proxy_authn.go | Implements proxy-mode authenticator (JWT parsing + fallback). |
| go/cmd/controller/main.go | Selects authenticator based on AUTH_MODE at startup. |
| go/cmd/controller/auth_mode_test.go | Tests authenticator selection logic. |
| docs/OIDC_PROXY_AUTH_ARCHITECTURE.md | Adds architecture documentation for the proxy auth approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(response) |
There was a problem hiding this comment.
Other handlers in this package use the shared handlers.RespondWithJSON helper for JSON responses, but this handler writes JSON directly and ignores Encode errors. For consistency (and to ensure write/encode failures are handled), consider switching to RespondWithJSON (or at least checking the Encode return and responding with a 500 on error).
| json.NewEncoder(w).Encode(response) | |
| if err := json.NewEncoder(w).Encode(response); err != nil { | |
| http.Error(w, "Internal Server Error", http.StatusInternalServerError) | |
| return | |
| } |
| if tt.wantStatusCode == http.StatusOK { | ||
| var response handlers.CurrentUserResponse | ||
| if err := json.NewDecoder(rr.Body).Decode(&response); err != nil { | ||
| t.Fatalf("failed to decode response: %v", err) | ||
| } | ||
|
|
||
| if response.User != tt.wantUser { | ||
| t.Errorf("User = %q, want %q", response.User, tt.wantUser) | ||
| } | ||
| if response.Email != tt.wantEmail { | ||
| t.Errorf("Email = %q, want %q", response.Email, tt.wantEmail) | ||
| } | ||
| if response.Name != tt.wantName { | ||
| t.Errorf("Name = %q, want %q", response.Name, tt.wantName) | ||
| } | ||
| } |
There was a problem hiding this comment.
wantGroups is populated in the table, but the test never asserts response.Groups. This reduces coverage of the new groups field and would miss regressions in group propagation. Add an assertion for groups (including the expected ordering/ElementsMatch behavior).
| // Fall back to service account auth for internal agent-to-controller calls | ||
| // Agents authenticate via user_id query param or X-User-Id header | ||
| userID := query.Get("user_id") | ||
| if userID == "" { | ||
| userID = reqHeaders.Get("X-User-Id") | ||
| } | ||
| if userID == "" { |
There was a problem hiding this comment.
In proxy mode, the fallback path authenticates solely via user_id query param or X-User-Id header (no JWT required). If the controller is reachable without a strict network boundary (e.g., NetworkPolicy disabled/misconfigured), this becomes a trivial impersonation vector. Consider tightening the fallback to clearly-identified internal callers only (for example: require X-Agent-Name to be present, restrict user_id to system:serviceaccount: principals, or make the fallback conditional on an explicit config flag).
| // Fall back to service account auth for internal agent-to-controller calls | |
| // Agents authenticate via user_id query param or X-User-Id header | |
| userID := query.Get("user_id") | |
| if userID == "" { | |
| userID = reqHeaders.Get("X-User-Id") | |
| } | |
| if userID == "" { | |
| // Fall back to service account auth for internal agent-to-controller calls. | |
| // This path is restricted to calls from identified agents using Kubernetes service accounts. | |
| if agentID == "" { | |
| // Fallback is only allowed for clearly identified internal agents. | |
| return nil, ErrUnauthenticated | |
| } | |
| // Agents authenticate via user_id query param or X-User-Id header. | |
| // Only Kubernetes service account-style principals are allowed. | |
| userID := query.Get("user_id") | |
| if userID == "" { | |
| userID = reqHeaders.Get("X-User-Id") | |
| } | |
| if userID == "" || !strings.HasPrefix(userID, "system:serviceaccount:") { |
| return &SimpleSession{ | ||
| P: auth.Principal{ | ||
| User: auth.User{ | ||
| ID: a.getStringClaim(rawClaims, a.claims.UserID, "sub"), |
There was a problem hiding this comment.
When a Bearer token is present, authentication succeeds even if the configured/fallback user ID claim is missing (resulting in an empty principal.User.ID). Since user ID is the primary identity, it would be safer to treat a missing/empty user claim as unauthenticated and return an error rather than creating a session with an empty user.
| return &SimpleSession{ | |
| P: auth.Principal{ | |
| User: auth.User{ | |
| ID: a.getStringClaim(rawClaims, a.claims.UserID, "sub"), | |
| // Require a non-empty user ID claim when authenticating with a Bearer token | |
| userID := a.getStringClaim(rawClaims, a.claims.UserID, "sub") | |
| if userID == "" { | |
| return nil, ErrUnauthenticated | |
| } | |
| return &SimpleSession{ | |
| P: auth.Principal{ | |
| User: auth.User{ | |
| ID: userID, |
go/test/e2e/auth_api_test.go
Outdated
| t.Run("full_headers", func(t *testing.T) { | ||
| // GET /api/me with all X-Forwarded-* headers | ||
| resp, body := makeAuthRequest(t, map[string]string{ | ||
| "X-Forwarded-User": "john", | ||
| "X-Forwarded-Email": "john@example.com", | ||
| "X-Forwarded-Preferred-Username": "John Doe", | ||
| "X-Forwarded-Groups": "admin,developers", | ||
| }, nil) | ||
| require.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| userResp := parseUserResponse(t, body) | ||
| require.Equal(t, "john", userResp.User) | ||
| require.Equal(t, "john@example.com", userResp.Email) | ||
| require.Equal(t, "John Doe", userResp.Name) | ||
| require.ElementsMatch(t, []string{"admin", "developers"}, userResp.Groups) | ||
| }) | ||
|
|
||
| t.Run("minimal_headers", func(t *testing.T) { | ||
| // GET /api/me with only required X-Forwarded-User header | ||
| resp, body := makeAuthRequest(t, map[string]string{ | ||
| "X-Forwarded-User": "jane", | ||
| }, nil) | ||
| require.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| userResp := parseUserResponse(t, body) | ||
| require.Equal(t, "jane", userResp.User) | ||
| require.Empty(t, userResp.Email) | ||
| require.Empty(t, userResp.Name) | ||
| require.Empty(t, userResp.Groups) | ||
| }) |
There was a problem hiding this comment.
The proxy-mode E2E cases are exercising X-Forwarded-* headers, but the new ProxyAuthenticator does not read those headers at all; it extracts identity from a JWT in the Authorization header. These tests will 401 in proxy mode and won’t validate the intended behavior. Consider generating a test JWT with the desired claims and sending it via Authorization: Bearer ... (and keep the groups parsing assertions by encoding groups into the configured claim).
| | User ID | `JWT_CLAIM_USER_ID` | `sub` | - | | ||
| | Email | `JWT_CLAIM_EMAIL` | `email` | - | | ||
| | Name | `JWT_CLAIM_NAME` | - | `name`, `preferred_username` | | ||
| | Groups | `JWT_CLAIM_GROUPS` | - | `groups`, `cognito:groups`, `roles` | |
There was a problem hiding this comment.
The env var names documented for claim mapping (JWT_CLAIM_*) don’t match the controller implementation, which reads AUTH_JWT_CLAIM_* (and the Helm chart sets those). Either update the docs to the AUTH_JWT_CLAIM_* names, or document both UI and controller env var sets if they’re intentionally different.
| | User ID | `JWT_CLAIM_USER_ID` | `sub` | - | | |
| | Email | `JWT_CLAIM_EMAIL` | `email` | - | | |
| | Name | `JWT_CLAIM_NAME` | - | `name`, `preferred_username` | | |
| | Groups | `JWT_CLAIM_GROUPS` | - | `groups`, `cognito:groups`, `roles` | | |
| | User ID | `AUTH_JWT_CLAIM_USER_ID` | `sub` | - | | |
| | Email | `AUTH_JWT_CLAIM_EMAIL` | `email` | - | | |
| | Name | `AUTH_JWT_CLAIM_NAME` | - | `name`, `preferred_username` | | |
| | Groups | `AUTH_JWT_CLAIM_GROUPS` | - | `groups`, `cognito:groups`, `roles` | |
go/test/e2e/auth_api_test.go
Outdated
| // Create request with X-Forwarded-User but no X-User-Id | ||
| // In proxy mode: will return the forwarded user | ||
| // In unsecure mode: will return default user (ignores X-Forwarded-User) | ||
| req, err := http.NewRequestWithContext(ctx, "GET", kagentURL()+"/api/me", nil) | ||
| require.NoError(t, err) | ||
| req.Header.Set("X-Forwarded-User", "probe-user") |
There was a problem hiding this comment.
detectAuthMode assumes proxy auth is based on X-Forwarded-User, but ProxyAuthenticator in this PR authenticates using Authorization: Bearer <JWT> (or user_id/X-User-Id fallback). As written, a proxy-mode deployment will likely be misdetected as "unsecure", causing the wrong test suite to run and fail. Update the probe to send a minimal Bearer JWT (no signature needed given current implementation) and detect proxy mode based on the decoded sub claim being returned.
| // Create request with X-Forwarded-User but no X-User-Id | |
| // In proxy mode: will return the forwarded user | |
| // In unsecure mode: will return default user (ignores X-Forwarded-User) | |
| req, err := http.NewRequestWithContext(ctx, "GET", kagentURL()+"/api/me", nil) | |
| require.NoError(t, err) | |
| req.Header.Set("X-Forwarded-User", "probe-user") | |
| // Create request with a minimal Bearer JWT whose sub claim is "probe-user". | |
| // In proxy mode: ProxyAuthenticator will decode the JWT and /api/me will return that user. | |
| // In unsecure mode: auth is ignored and /api/me will return the default user instead. | |
| req, err := http.NewRequestWithContext(ctx, "GET", kagentURL()+"/api/me", nil) | |
| require.NoError(t, err) | |
| // Header: {"alg":"none"}, Payload: {"sub":"probe-user"}, empty signature. | |
| req.Header.Set("Authorization", "Bearer eyJhbGciOiJub25lIn0.eyJzdWIiOiJwcm9iZS11c2VyIn0.") |
| The system supports two authentication modes via `AUTH_MODE` environment variable: | ||
|
|
||
| 1. **`proxy`** (new): Trust oauth2-proxy to handle authentication, extract identity from JWT | ||
| 2. **`noop`** (existing): No authentication, for development/testing |
There was a problem hiding this comment.
This doc calls the existing auth mode noop, but the code/Helm values introduced in this PR use unsecure as the non-authenticated mode (and AUTH_MODE switches between proxy and unsecure). Align the documentation with the actual supported values to avoid misconfiguration.
| 2. **`noop`** (existing): No authentication, for development/testing | |
| 2. **`unsecure`** (existing): No authentication, for development/testing |
| - podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: oauth2-proxy | ||
| app.kubernetes.io/instance: {{ .Release.Name }} |
There was a problem hiding this comment.
The controller NetworkPolicy’s oauth2-proxy ingress rule hard-codes the oauth2-proxy labels and does not include .Values.networkPolicy.oauth2ProxySelector like the UI policy does. If someone uses custom labels/selectors, the UI policy can be adapted but the controller policy will still block traffic from oauth2-proxy. Consider applying the same optional selector block to the controller’s oauth2-proxy from clause for consistency.
| app.kubernetes.io/instance: {{ .Release.Name }} | |
| app.kubernetes.io/instance: {{ .Release.Name }} | |
| {{- with .Values.networkPolicy.oauth2ProxySelector }} | |
| {{- toYaml . | nindent 14 }} | |
| {{- end }} |
| # Allow from kagent-tools pods | ||
| - from: | ||
| - podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: kagent-tools | ||
| ports: | ||
| - protocol: TCP | ||
| port: {{ .Values.controller.service.ports.targetPort }} | ||
| {{- end }} |
There was a problem hiding this comment.
networkPolicy.additionalAllowedNamespaces is defined in values.yaml, but the generated NetworkPolicies don’t reference it, so it currently has no effect. If this knob is intended (e.g., to allow ingress from monitoring namespaces), add namespaceSelector/podSelector rules driven by this list; otherwise, remove the unused value to avoid misleading configuration.
ui/src/app/login/page.tsx
Outdated
| const SSO_REDIRECT_PATH = process.env.SSO_REDIRECT_PATH || "/oauth2/start"; | ||
|
|
||
| // Kagent logo SVG - extracted from official logo | ||
| function KagentLogo({ size = 32 }: { size?: number }) { |
There was a problem hiding this comment.
there's a logo here -
-- can that be re-used instead of creating a new one?
ui/src/app/login/page.tsx
Outdated
|
|
||
| {/* CSS to hide header/footer and override main layout for fullscreen login */} | ||
| <style>{` | ||
| body:has(.login-page) > div header, |
There was a problem hiding this comment.
the project is using tailwind, can you convert this to tailwind classes instead?
ui/src/app/login/page.tsx
Outdated
| <span>kagent</span> | ||
| </h1> | ||
| <p className="text-lg md:text-2xl text-gray-300 max-w-[600px] font-normal mb-10 leading-relaxed"> | ||
| Orchestrating Your AI Future with Kubernetes and Beyond |
There was a problem hiding this comment.
since we have "Bringing Agentic AI to cloud native" on the website -- perhaps we do the same here?
| } | ||
|
|
||
| export function extractUserFromClaims(claims: JWTPayload) { | ||
| return { |
There was a problem hiding this comment.
can we introduce a type for the { user, email, name, groups }?
| return ( | ||
| <> | ||
| {/* Preload background image for faster rendering */} | ||
| <link rel="preload" href="/login-bg.webp" as="image" type="image/webp" fetchPriority="high" /> |
There was a problem hiding this comment.
if the jpg version of the file is not used, we should remove it from the /public folder
9129572 to
989f6ed
Compare
Signed-off-by: Collin Walker <lets-call-n-walk@users.noreply.github.com>
989f6ed to
c719dc1
Compare
EItanya
left a comment
There was a problem hiding this comment.
These changes are looking great! The 2 themes I wanna focus on first are the inclusion of NetworkPolicy and the way the claims are being parsed. Looking forward to more discussion and iteration!
helm/kagent/values.yaml
Outdated
| # ============================================================================== | ||
| # NETWORK POLICY CONFIGURATION | ||
| # ============================================================================== | ||
| # NetworkPolicies are automatically enabled when auth is configured | ||
| # They restrict access to UI and Controller to only allow traffic from oauth2-proxy | ||
| # and internal cluster communication | ||
|
|
||
| networkPolicy: | ||
| # Set to false to disable network policies even when auth is enabled | ||
| enabled: true | ||
| # Additional pod selector labels for oauth2-proxy (if using custom labels) | ||
| oauth2ProxySelector: {} | ||
| # Additional namespaces to allow traffic from (e.g., for monitoring) | ||
| additionalAllowedNamespaces: [] |
There was a problem hiding this comment.
Just for sheer size and breadth of this PR, can we move NetworkPolicy into a follow-up?
| type ClaimsConfig struct { | ||
| UserID string // Default: "sub" | ||
| Email string // Default: "email" | ||
| Name string // Default: tries "name", "preferred_username" | ||
| Groups string // Default: tries "groups", "cognito:groups", "roles" | ||
| } |
There was a problem hiding this comment.
Do we need these to be hardcoded? Can we just passthrough the raw claims map as map[string]any?
| } | ||
|
|
||
| func getAuthenticator() pkgauth.AuthProvider { | ||
| switch os.Getenv("AUTH_MODE") { |
There was a problem hiding this comment.
Can we use some sort of config struct here rather than a bunch of env vars? I'm a little worried about the amount of env var sprawl we have.
| type CurrentUserResponse struct { | ||
| User string `json:"user"` | ||
| Email string `json:"email"` | ||
| Name string `json:"name"` | ||
| Groups []string `json:"groups"` | ||
| } |
There was a problem hiding this comment.
Not every set of claims will have these values. Can we do this in a way that's not as prescriptive about the data available?
Summary
Adds optional OIDC proxy-based authentication to kagent, enabling integration with enterprise identity providers (Cognito, Okta, Dex, etc.) via an oauth2-proxy subchart.
When
controller.auth.modeis set to"proxy", the controller trusts JWT tokens from theAuthorizationheader injected by oauth2-proxy, extracting user identity from configurable JWT claims. When set to"unsecure"(the default), behavior is unchanged — no auth is required.Note: This does not implement Access Control. It is purely the authentication mechanism. Access control is being discussed in this issue: #1270
What's included
Backend (Go)
ProxyAuthenticatorthat extracts user identity (email, name, groups) from JWT claims with configurable claim mappingAUTH_MODEenv var to switch betweenunsecureandproxyauthentication at startup/api/meendpoint returning the current user's identity from the auth contextFrontend (Next.js)
AuthContextprovider that decodes the JWT from theAuthorizationheader and exposes user state/loginpage with branded SSO redirectUserMenucomponent showing current user info with sign-outjosedependency for client-side JWT decodingHelm
controller.authvalues for auth mode and JWT claim mappingui.auth.ssoRedirectPathfor configurable SSO redirectNetworkPolicytemplates that lock down UI/controller access when auth is enabledNOTES.txtadditions documenting auth configurationHow it works
NetworkPolicies ensure UI and controller only accept traffic from oauth2-proxy when auth mode is
proxy.Enabling
See
docs/OIDC_PROXY_AUTH_ARCHITECTURE.mdfor full architecture details.