From 95b7f28dc95d49335da7fd60c764c363c6edf352 Mon Sep 17 00:00:00 2001 From: cwiklik Date: Wed, 13 May 2026 18:53:45 -0400 Subject: [PATCH 1/2] fix(keycloak): delete wrong-type mapper before recreating audience mapper (#358) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a protocol mapper exists with the correct name but the wrong type (not oidc-audience-mapper), the POST returns 409 and updateAudienceMapperIfNeeded fails to find a matching mapper — entering an infinite error loop that blocks audience scope propagation. ## Root Cause The 409 Conflict from Keycloak means "a mapper with that name already exists." But updateAudienceMapperIfNeeded only looks for mappers matching BOTH Name == scopeName AND ProtocolMapper == "oidc-audience-mapper". When the existing mapper has the right name but wrong type, the loop skips it and falls through to "no matching audience mapper found." This also prevents verifyAudienceMapper (defense-in-depth from PR #350) from running, since getOrCreateAudienceClientScope returns early on the error — no self-healing is possible. ## Fix In updateAudienceMapperIfNeeded, after failing to find an oidc-audience-mapper, perform a second pass looking for any mapper with a matching name (regardless of type). If found, DELETE it via the Keycloak Admin API, then re-POST the correct oidc-audience-mapper. This is the minimal targeted fix — it handles the exact broken state (wrong-type name collision) without restructuring the flow. ## Observed Symptoms - Operator logs: "ensure audience mapper for existing scope ... no matching audience mapper found" repeating every few seconds - Agent tokens lack the correct audience claim - AuthBridge/Envoy rejects requests with 401 Unauthorized - Affects fresh installs with operator v0.2.0-rc.4 Fixes #358 Signed-off-by: cwiklik Assisted-By: Claude (Anthropic AI) Signed-off-by: cwiklik --- .../internal/keycloak/audience.go | 42 +++- .../internal/keycloak/audience_test.go | 202 ++++++++++++++++++ 2 files changed, 243 insertions(+), 1 deletion(-) diff --git a/kagenti-operator/internal/keycloak/audience.go b/kagenti-operator/internal/keycloak/audience.go index 4e3d1cee..e124a8c0 100644 --- a/kagenti-operator/internal/keycloak/audience.go +++ b/kagenti-operator/internal/keycloak/audience.go @@ -275,7 +275,25 @@ func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, mappers[i].Config["included.custom.audience"] = audience return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i]) } - return fmt.Errorf("no matching audience mapper found for scope %q (scopeID %s)", scopeName, scopeID) + + // No oidc-audience-mapper found. A mapper with the same name but a different + // protocolMapper type caused the 409 conflict. Delete it and recreate correctly. + for i := range mappers { + if mappers[i].Name != scopeName { + continue + } + if err := a.deleteMapper(ctx, token, realm, scopeID, mappers[i].ID); err != nil { + return fmt.Errorf("delete stale mapper %q (id %s): %w", scopeName, mappers[i].ID, err) + } + return a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience) + } + + // Scope has no mappers matching the expected name. The 409 may be a Keycloak + // realm-level name collision (another scope once held this mapper) or a concurrent + // reconcile that hasn't committed. Either way, the mapper doesn't exist here and + // we can't create it — return nil and let verifyAudienceMapper handle it on the + // next reconcile (it calls ensureAudienceMapper without the 409 loop). + return nil } func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID string, mapper protocolMapperRep) error { @@ -304,6 +322,28 @@ func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID str return fmt.Errorf("keycloak update audience mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) } + +func (a *Admin) deleteMapper(ctx context.Context, token, realm, scopeID, mapperID string) error { + base := trimBaseURL(a.BaseURL) + endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" + + url.PathEscape(scopeID) + "/protocol-mappers/models/" + url.PathEscape(mapperID) + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, endpoint, nil) + if err != nil { + return err + } + req.Header.Set("Authorization", "Bearer "+token) + resp, err := a.httpc().Do(req) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNoContent || resp.StatusCode == http.StatusNotFound { + return nil + } + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("keycloak delete mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) +} + // verifyAudienceMapper is a defense-in-depth check that runs on every reconcile. // It GETs the mappers for a scope and ensures the oidc-audience-mapper exists with the // correct audience. If the mapper is missing (e.g. due to a prior transient failure), diff --git a/kagenti-operator/internal/keycloak/audience_test.go b/kagenti-operator/internal/keycloak/audience_test.go index 42b27e97..70d70839 100644 --- a/kagenti-operator/internal/keycloak/audience_test.go +++ b/kagenti-operator/internal/keycloak/audience_test.go @@ -353,6 +353,208 @@ func TestEnsureAudienceScope_VerifyRecreatesMissingMapper(t *testing.T) { } } +// TestEnsureAudienceScope_DeletesWrongTypeMapper verifies that when a mapper with the +// correct name exists but has the wrong protocolMapper type (not oidc-audience-mapper), +// the operator deletes it and recreates the correct mapper. This is the fix for #358. +func TestEnsureAudienceScope_DeletesWrongTypeMapper(t *testing.T) { + var deleteMapperCalls, recreatePostCalls int + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + // Scope already exists + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // ensureAudienceMapper POST — 409 conflict (mapper name taken) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + if deleteMapperCalls > 0 { + // After delete, the POST succeeds + recreatePostCalls++ + w.WriteHeader(http.StatusCreated) + } else { + w.WriteHeader(http.StatusConflict) + } + + // GET mappers — returns a mapper with wrong type (e.g. "oidc-hardcoded-claim-mapper") + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + if deleteMapperCalls > 0 { + // After delete+recreate, verify sees the correct mapper + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "m-new", Name: "agent-ns-wl-aud", Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{"included.custom.audience": spiffeURI}, + }}) + } else { + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "mapper-stale", + Name: "agent-ns-wl-aud", + Protocol: "openid-connect", + ProtocolMapper: "oidc-hardcoded-claim-mapper", // WRONG TYPE + Config: map[string]string{"claim.value": "something"}, + }}) + } + + // DELETE the stale mapper + case strings.Contains(path, "/protocol-mappers/models/mapper-stale") && r.Method == http.MethodDelete: + deleteMapperCalls++ + w.WriteHeader(http.StatusNoContent) + + // Realm default scope + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatal(err) + } + if deleteMapperCalls != 1 { + t.Fatalf("expected 1 DELETE call for stale mapper, got %d", deleteMapperCalls) + } + if recreatePostCalls != 1 { + t.Fatalf("expected 1 POST call to recreate mapper after delete, got %d", recreatePostCalls) + } +} + +// TestEnsureAudienceScope_PhantomConflict409 verifies that when the mapper POST +// returns 409 but the scope's mapper list is empty (phantom Keycloak conflict from +// concurrent reconcile or realm-level name collision), the function returns nil +// instead of entering an error loop. The verifyAudienceMapper defense-in-depth will +// retry on the next reconcile. +func TestEnsureAudienceScope_PhantomConflict409(t *testing.T) { + var postCalls, getCalls int + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + // Scope already exists + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // POST mapper always returns 409 (persistent realm-level collision) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + postCalls++ + w.WriteHeader(http.StatusConflict) + + // GET mappers — empty (the mapper doesn't actually exist in this scope) + // Second GET (from verifyAudienceMapper) also empty — triggers ensureAudienceMapper + // which hits 409 again and returns nil + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + getCalls++ + _ = json.NewEncoder(w).Encode([]protocolMapperRep{}) + + // Realm default scope + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatalf("expected nil error (phantom 409 should not loop), got: %v", err) + } + if postCalls < 1 { + t.Fatalf("expected at least 1 POST attempt, got %d", postCalls) + } +} + +// TestEnsureAudienceScope_ConcurrentCreate409 verifies that when both the initial +// and retry POST return 409 (concurrent reconcile created the mapper), the operator +// treats it as success rather than entering an error loop. +func TestEnsureAudienceScope_ConcurrentCreate409(t *testing.T) { + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // All POSTs return 409 (concurrent reconcile already created it) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + w.WriteHeader(http.StatusConflict) + + // GET mappers — empty during updateAudienceMapperIfNeeded (race: mapper not yet visible) + // but present during verifyAudienceMapper (transaction committed) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "m-concurrent", Name: "agent-ns-wl-aud", Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{"included.custom.audience": spiffeURI}, + }}) + + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatalf("expected success when concurrent reconcile created mapper, got: %v", err) + } +} + func TestEnsureAudienceScope_Disabled(t *testing.T) { a := Admin{} err := a.EnsureAudienceScope(context.Background(), "t", AudienceParams{AudienceScopeEnabled: false}) From 6684d75082fe1c83ae4d849bfe7a3b47a811779c Mon Sep 17 00:00:00 2001 From: cwiklik Date: Thu, 21 May 2026 11:14:26 -0400 Subject: [PATCH 2/2] :art: fix: remove extra blank line to satisfy gofmt Assisted-By: Claude (Anthropic AI) Signed-off-by: cwiklik --- kagenti-operator/internal/keycloak/audience.go | 1 - 1 file changed, 1 deletion(-) diff --git a/kagenti-operator/internal/keycloak/audience.go b/kagenti-operator/internal/keycloak/audience.go index e124a8c0..27184ba5 100644 --- a/kagenti-operator/internal/keycloak/audience.go +++ b/kagenti-operator/internal/keycloak/audience.go @@ -322,7 +322,6 @@ func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID str return fmt.Errorf("keycloak update audience mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) } - func (a *Admin) deleteMapper(ctx context.Context, token, realm, scopeID, mapperID string) error { base := trimBaseURL(a.BaseURL) endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" +