review fixes

Signed-off-by: Collin Walker <lets-call-n-walk@users.noreply.github.com>

Author: Collin Walker

go/internal/httpserver/auth/proxy_authn.go

@@ -44,10 +44,15 @@ func (a *ProxyAuthenticator) Authenticate(ctx context.Context, reqHeaders http.H
 			return nil, ErrUnauthenticated
 		}
 
+		userID := a.getStringClaim(rawClaims, a.claims.UserID, "sub")
+		if userID == "" {
+			return nil, ErrUnauthenticated
+		}
+
 		return &SimpleSession{
 			P: auth.Principal{
 				User: auth.User{
-					ID:    a.getStringClaim(rawClaims, a.claims.UserID, "sub"),
+					ID:    userID,
 					Email: a.getStringClaim(rawClaims, a.claims.Email, "email"),
 					Name:  a.getStringClaim(rawClaims, a.claims.Name, "name", "preferred_username"),
 				},
@@ -60,7 +65,12 @@ func (a *ProxyAuthenticator) Authenticate(ctx context.Context, reqHeaders http.H
 		}, nil
 	}
 
-	// Fall back to service account auth for internal agent-to-controller calls
+	// Fall back to service account auth for internal agent-to-controller calls.
+	// Requires X-Agent-Name to identify the calling agent.
+	if agentID == "" {
+		return nil, ErrUnauthenticated
+	}
+
 	// Agents authenticate via user_id query param or X-User-Id header
 	userID := query.Get("user_id")
 	if userID == "" {

go/internal/httpserver/auth/proxy_authn_test.go

@@ -131,6 +131,13 @@ func TestProxyAuthenticator_Authenticate(t *testing.T) {
 			wantGroups: []string{},
 			wantErr:    false,
 		},
+		{
+			name: "returns error when JWT has empty sub claim",
+			claims: map[string]any{
+				"email": "user@example.com",
+			},
+			wantErr: true,
+		},
 		{
 			name: "handles single group in array",
 			claims: map[string]any{
@@ -273,37 +280,38 @@ func TestProxyAuthenticator_ServiceAccountFallback(t *testing.T) {
 		wantErr     bool
 	}{
 		{
-			name: "authenticates via user_id query param",
+			name: "authenticates via user_id query param with agent name",
 			queryParams: map[string]string{
 				"user_id": "system:serviceaccount:kagent:kebab-agent",
 			},
-			wantUserID: "system:serviceaccount:kagent:kebab-agent",
-			wantErr:    false,
-		},
-		{
-			name: "authenticates via X-User-Id header",
 			headers: map[string]string{
-				"X-User-Id": "system:serviceaccount:kagent:test-agent",
+				"X-Agent-Name": "kagent/kebab-agent",
 			},
-			wantUserID: "system:serviceaccount:kagent:test-agent",
-			wantErr:    false,
+			wantUserID:  "system:serviceaccount:kagent:kebab-agent",
+			wantAgentID: "kagent/kebab-agent",
+			wantErr:     false,
 		},
 		{
-			name: "extracts agent identity from X-Agent-Name header",
-			queryParams: map[string]string{
-				"user_id": "system:serviceaccount:kagent:kebab-agent",
-			},
+			name: "authenticates via X-User-Id header with agent name",
 			headers: map[string]string{
-				"X-Agent-Name": "kagent/kebab-agent",
+				"X-User-Id":    "system:serviceaccount:kagent:test-agent",
+				"X-Agent-Name": "kagent/test-agent",
 			},
-			wantUserID:  "system:serviceaccount:kagent:kebab-agent",
-			wantAgentID: "kagent/kebab-agent",
+			wantUserID:  "system:serviceaccount:kagent:test-agent",
+			wantAgentID: "kagent/test-agent",
 			wantErr:     false,
 		},
 		{
 			name:    "returns error when no auth method available",
 			wantErr: true,
 		},
+		{
+			name: "returns error when no X-Agent-Name header for fallback",
+			queryParams: map[string]string{
+				"user_id": "system:serviceaccount:kagent:kebab-agent",
+			},
+			wantErr: true,
+		},
 	}
 
 	for _, tt := range tests {

go/internal/httpserver/handlers/current_user.go

@@ -1,7 +1,6 @@
 package handlers
 
 import (
-	"encoding/json"
 	"net/http"
 
 	"github.com/kagent-dev/kagent/go/pkg/auth"
@@ -35,6 +34,5 @@ func (h *CurrentUserHandler) HandleGetCurrentUser(w http.ResponseWriter, r *http
 		Groups: principal.Groups,
 	}
 
-	w.Header().Set("Content-Type", "application/json")
-	json.NewEncoder(w).Encode(response)
+	RespondWithJSON(w, http.StatusOK, response)
 }

go/internal/httpserver/handlers/current_user_test.go

@@ -85,6 +85,14 @@ func TestHandleGetCurrentUser(t *testing.T) {
 				if response.Name != tt.wantName {
 					t.Errorf("Name = %q, want %q", response.Name, tt.wantName)
 				}
+				if len(response.Groups) != len(tt.wantGroups) {
+					t.Errorf("Groups length = %d, want %d", len(response.Groups), len(tt.wantGroups))
+				}
+				for i, g := range response.Groups {
+					if i < len(tt.wantGroups) && g != tt.wantGroups[i] {
+						t.Errorf("Groups[%d] = %q, want %q", i, g, tt.wantGroups[i])
+					}
+				}
 			}
 		})
 	}

go/test/e2e/auth_api_test.go

@@ -2,6 +2,7 @@ package e2e_test
 
 import (
 	"context"
+	"encoding/base64"
 	"encoding/json"
 	"io"
 	"net/http"
@@ -13,6 +14,16 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// makeTestJWT builds a minimal unsigned JWT (alg:none) with the given claims.
+// This is sufficient for proxy mode testing where the oauth2-proxy has already
+// validated the token and the backend only parses claims without verification.
+func makeTestJWT(claims map[string]any) string {
+	header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none","typ":"JWT"}`))
+	payload, _ := json.Marshal(claims)
+	payloadB64 := base64.RawURLEncoding.EncodeToString(payload)
+	return header + "." + payloadB64 + "."
+}
+
 // CurrentUserResponse mirrors the response from GET /api/me
 type CurrentUserResponse struct {
 	User   string   `json:"user"`
@@ -31,19 +42,19 @@ func kagentURL() string {
 }
 
 // detectAuthMode probes /api/me to determine if the deployment is in proxy or unsecure mode.
+// Sends a JWT Bearer token; in proxy mode the backend parses the JWT and returns the sub claim.
+// In unsecure mode the backend ignores the Bearer token and returns the default user.
 // Returns "proxy" if proxy mode, "unsecure" otherwise.
 func detectAuthMode(t *testing.T) string {
 	t.Helper()
 
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
-	// 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)
+	token := makeTestJWT(map[string]any{"sub": "probe-user"})
 	req, err := http.NewRequestWithContext(ctx, "GET", kagentURL()+"/api/me", nil)
 	require.NoError(t, err)
-	req.Header.Set("X-Forwarded-User", "probe-user")
+	req.Header.Set("Authorization", "Bearer "+token)
 
 	resp, err := http.DefaultClient.Do(req)
 	require.NoError(t, err)
@@ -173,13 +184,16 @@ func TestE2EAuthProxyMode(t *testing.T) {
 		t.Skip("Skipping proxy mode tests - deployment is in unsecure mode")
 	}
 
-	t.Run("full_headers", func(t *testing.T) {
-		// GET /api/me with all X-Forwarded-* headers
+	t.Run("full_claims", func(t *testing.T) {
+		// JWT with all standard claims
+		token := makeTestJWT(map[string]any{
+			"sub":    "john",
+			"email":  "john@example.com",
+			"name":   "John Doe",
+			"groups": []string{"admin", "developers"},
+		})
 		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",
+			"Authorization": "Bearer " + token,
 		}, nil)
 		require.Equal(t, http.StatusOK, resp.StatusCode)
 
@@ -190,10 +204,13 @@ func TestE2EAuthProxyMode(t *testing.T) {
 		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
+	t.Run("minimal_claims", func(t *testing.T) {
+		// JWT with only sub claim
+		token := makeTestJWT(map[string]any{
+			"sub": "jane",
+		})
 		resp, body := makeAuthRequest(t, map[string]string{
-			"X-Forwarded-User": "jane",
+			"Authorization": "Bearer " + token,
 		}, nil)
 		require.Equal(t, http.StatusOK, resp.StatusCode)
 
@@ -204,35 +221,56 @@ func TestE2EAuthProxyMode(t *testing.T) {
 		require.Empty(t, userResp.Groups)
 	})
 
-	t.Run("missing_required_header_returns_401", func(t *testing.T) {
-		// GET /api/me without X-Forwarded-User should return 401
+	t.Run("missing_sub_claim_returns_401", func(t *testing.T) {
+		// JWT without sub claim should return 401
+		token := makeTestJWT(map[string]any{
+			"email": "test@example.com",
+		})
 		resp, _ := makeAuthRequest(t, map[string]string{
-			"X-Forwarded-Email": "test@example.com", // email but no user
+			"Authorization": "Bearer " + token,
 		}, nil)
 		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
 	})
 
-	t.Run("group_parsing_trims_whitespace", func(t *testing.T) {
-		// Groups with whitespace around commas should be trimmed
+	t.Run("no_bearer_token_returns_401", func(t *testing.T) {
+		// No Authorization header and no agent identity should return 401
+		resp, _ := makeAuthRequest(t, nil, nil)
+		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+	})
+
+	t.Run("comma_separated_groups_string", func(t *testing.T) {
+		// Groups as comma-separated string should be parsed and trimmed
+		token := makeTestJWT(map[string]any{
+			"sub":    "user",
+			"groups": "admin, dev , qa ",
+		})
 		resp, body := makeAuthRequest(t, map[string]string{
-			"X-Forwarded-User":   "user",
-			"X-Forwarded-Groups": "admin, dev , qa ",
+			"Authorization": "Bearer " + token,
 		}, nil)
 		require.Equal(t, http.StatusOK, resp.StatusCode)
 
 		userResp := parseUserResponse(t, body)
 		require.ElementsMatch(t, []string{"admin", "dev", "qa"}, userResp.Groups)
 	})
 
-	t.Run("empty_groups_header", func(t *testing.T) {
-		// Empty groups header should result in empty groups slice
+	t.Run("agent_fallback_with_user_id", func(t *testing.T) {
+		// Agent callback: X-Agent-Name + user_id query param (no Bearer token)
 		resp, body := makeAuthRequest(t, map[string]string{
-			"X-Forwarded-User":   "user",
-			"X-Forwarded-Groups": "",
-		}, nil)
+			"X-Agent-Name": "kagent/test-agent",
+		}, map[string]string{
+			"user_id": "owner@example.com",
+		})
 		require.Equal(t, http.StatusOK, resp.StatusCode)
 
 		userResp := parseUserResponse(t, body)
-		require.Empty(t, userResp.Groups)
+		require.Equal(t, "owner@example.com", userResp.User)
+	})
+
+	t.Run("fallback_without_agent_name_returns_401", func(t *testing.T) {
+		// user_id query param without X-Agent-Name should return 401
+		resp, _ := makeAuthRequest(t, nil, map[string]string{
+			"user_id": "owner@example.com",
+		})
+		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
 	})
 }

helm/kagent/templates/networkpolicy.yaml

@@ -49,6 +49,9 @@ spec:
             matchLabels:
               app.kubernetes.io/name: oauth2-proxy
               app.kubernetes.io/instance: {{ .Release.Name }}
+              {{- with .Values.networkPolicy.oauth2ProxySelector }}
+              {{- toYaml . | nindent 14 }}
+              {{- end }}
       ports:
         - protocol: TCP
           port: {{ .Values.controller.service.ports.targetPort }}

helm/kagent/values.yaml

@@ -485,8 +485,6 @@ networkPolicy:
   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: []
 
 # ==============================================================================
 # OBSERVABILITY