From 153258b9d39273d53ca7ab5d1bdc09961ea9026e Mon Sep 17 00:00:00 2001 From: tegwick Date: Mon, 31 Aug 2026 23:41:29 +0200 Subject: [PATCH] Fix UserInfo canonical subject resolution Assistant: codex Assistant-Model: gpt-5.6-sol Assistant-Session: 01a058f3-8ba0-7692-a042-9a870fc3d663 --- src/internal/server/oidc/userinfo.go | 38 ++++++++++++++- src/internal/server/oidc/userinfo_test.go | 30 ++++++++++++ ...2-userinfo-canonical-subject-resolution.md | 46 +++++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 workplans/KEY-WP-0012-userinfo-canonical-subject-resolution.md diff --git a/src/internal/server/oidc/userinfo.go b/src/internal/server/oidc/userinfo.go index ee4512e..54d4bb5 100644 --- a/src/internal/server/oidc/userinfo.go +++ b/src/internal/server/oidc/userinfo.go @@ -1,6 +1,7 @@ package oidc import ( + "context" "crypto" "crypto/rsa" "crypto/sha256" @@ -52,8 +53,11 @@ func (h *UserinfoHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // 4. Look up user by sub (sub IS the username per spec §3.1). - user, err := h.Users.LookupUser(ctx, sub) + // 4. Resolve the canonical subject. Human tokens use the stable directory + // ID (the LDAP DN) as sub, while LookupUser accepts an LDAP uid. Prefer the + // scoped username hint but require it to resolve back to the signed subject; + // fall back to an ID scan for openid-only tokens and renamed users. + user, err := lookupUserBySubject(ctx, h.Users, claims, sub) if err != nil { // User referenced in token but not found → treat as invalid token. http.Error(w, `{"error":"invalid_token","description":"subject not found"}`, http.StatusUnauthorized) @@ -97,6 +101,36 @@ func (h *UserinfoHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { _ = json.NewEncoder(w).Encode(resp) } +func lookupUserBySubject( + ctx context.Context, + users domain.UserRepository, + claims map[string]interface{}, + sub string, +) (*domain.User, error) { + if username, _ := claims["preferred_username"].(string); username != "" { + if user, err := users.LookupUser(ctx, username); err == nil && user.ID == sub { + return user, nil + } + } + + // Retain compatibility with older signed tokens whose subject was the uid. + if user, err := users.LookupUser(ctx, sub); err == nil && + (user.ID == sub || user.Username == sub) { + return user, nil + } + + all, err := users.ListUsers(ctx) + if err != nil { + return nil, err + } + for i := range all { + if all[i].ID == sub { + return &all[i], nil + } + } + return nil, domain.ErrUserNotFound +} + // --------------------------------------------------------------------------- // JWT validation (stdlib only — no external JWT library) // --------------------------------------------------------------------------- diff --git a/src/internal/server/oidc/userinfo_test.go b/src/internal/server/oidc/userinfo_test.go index feaf1ca..ca372af 100644 --- a/src/internal/server/oidc/userinfo_test.go +++ b/src/internal/server/oidc/userinfo_test.go @@ -100,6 +100,36 @@ func TestUserinfoHandler_ValidToken_ReturnsClaims(t *testing.T) { } } +func TestUserinfoHandler_CanonicalSubjectResolvesViaPreferredUsername(t *testing.T) { + user := aliceUser() + users := &mockUserRepo{users: map[string]*domain.User{"alice": user}} + h, key := newUserinfoHandler(t, users) + now := time.Now() + token := buildToken(t, map[string]interface{}{ + "iss": "https://auth.netkingdom.local", + "sub": user.ID, + "preferred_username": user.Username, + "aud": "test-client", + "exp": now.Add(10 * time.Minute).Unix(), + "iat": now.Unix(), + "scope": "openid profile", + }, key) + + w := httptest.NewRecorder() + h.ServeHTTP(w, userinfoRequest(token)) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d (body: %s)", w.Code, w.Body.String()) + } + resp := decodeUserinfoClaims(t, w.Body.String()) + if resp["sub"] != user.ID { + t.Errorf("sub: expected %q, got %v", user.ID, resp["sub"]) + } + if resp["preferred_username"] != user.Username { + t.Errorf("preferred_username: expected %q, got %v", user.Username, resp["preferred_username"]) + } +} + func TestUserinfoHandler_SuspendedUserInvalidatesToken(t *testing.T) { user := aliceUser() user.Groups = append(user.Groups, "netkingdom-suspended") diff --git a/workplans/KEY-WP-0012-userinfo-canonical-subject-resolution.md b/workplans/KEY-WP-0012-userinfo-canonical-subject-resolution.md new file mode 100644 index 0000000..aa0ab44 --- /dev/null +++ b/workplans/KEY-WP-0012-userinfo-canonical-subject-resolution.md @@ -0,0 +1,46 @@ +--- +id: KEY-WP-0012 +type: workplan +title: "Repair UserInfo canonical subject resolution" +domain: infotech +repo: key-cape +status: active +owner: codex +topic_slug: userinfo-canonical-subject-resolution +created: "2026-08-31" +updated: "2026-08-31" +--- + +## Repair subject lookup + +```task +id: KEY-WP-0012-T01 +status: done +priority: high +``` + +Resolve the canonical LDAP-DN `sub` emitted by the token endpoint without +passing it to the username-only repository lookup. Preserve stable subject +semantics and verify any `preferred_username` lookup against the canonical ID. + +## Regression verification + +```task +id: KEY-WP-0012-T02 +status: done +priority: high +``` + +Cover canonical-ID, legacy username-sub, missing subject, and suspended-user +behavior. Run the KeyCape test suite and image build checks. + +## Deploy and verify OpenBao OIDC + +```task +id: KEY-WP-0012-T03 +status: progress +priority: high +``` + +Publish and deploy the corrected KeyCape image, prove `/userinfo` accepts a +fresh human access token, then resume the governed Policy Nexus bootstrap.