diff --git a/kagenti-operator/internal/controller/indexers_test.go b/kagenti-operator/internal/controller/indexers_test.go index f6f10991..594c2eac 100644 --- a/kagenti-operator/internal/controller/indexers_test.go +++ b/kagenti-operator/internal/controller/indexers_test.go @@ -111,8 +111,10 @@ var _ = Describe("mapWorkloadToAgentCards", func() { sbx.SetLabels(map[string]string{LabelAgentType: LabelValueAgent}) mapFn := mapWorkloadToAgentCards(indexedClient, "agents.x-k8s.io/v1alpha1", "Sandbox", logger) + Eventually(func() int { + return len(mapFn(ctx, sbx)) + }).Should(Equal(1)) requests := mapFn(ctx, sbx) - Expect(requests).To(HaveLen(1)) Expect(requests[0].Name).To(Equal("sandbox-card")) }) }) diff --git a/kagenti-operator/internal/keycloak/audience.go b/kagenti-operator/internal/keycloak/audience.go index 6989ec74..4e3d1cee 100644 --- a/kagenti-operator/internal/keycloak/audience.go +++ b/kagenti-operator/internal/keycloak/audience.go @@ -13,7 +13,6 @@ import ( "encoding/json" "fmt" "io" - "log/slog" "net/http" "net/url" "strings" @@ -66,6 +65,9 @@ func (a *Admin) EnsureAudienceScope(ctx context.Context, token string, p Audienc if err != nil { return err } + if err := a.verifyAudienceMapper(ctx, token, p.Realm, scopeID, scopeName, p.AudienceClientID); err != nil { + return fmt.Errorf("verify audience mapper for scope %q: %w", scopeName, err) + } _ = a.putRealmDefaultDefaultClientScope(ctx, token, p.Realm, scopeID) for _, plat := range p.PlatformClientIDs { plat = strings.TrimSpace(plat) @@ -87,7 +89,9 @@ func (a *Admin) getOrCreateAudienceClientScope(ctx context.Context, token, realm return "", err } if scopeID != "" { - _ = a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience) + if err := a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience); err != nil { + return "", fmt.Errorf("ensure audience mapper for existing scope %q: %w", scopeName, err) + } return scopeID, nil } @@ -105,7 +109,9 @@ func (a *Admin) getOrCreateAudienceClientScope(ctx context.Context, token, realm if scopeID == "" { return "", fmt.Errorf("create client scope %q returned empty id", scopeName) } - _ = a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience) + if err := a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience); err != nil { + return "", fmt.Errorf("ensure audience mapper for new scope %q: %w", scopeName, err) + } return scopeID, nil } @@ -219,30 +225,41 @@ func (a *Admin) ensureAudienceMapper(ctx context.Context, token, realm, scopeID, return nil } -// updateAudienceMapperIfNeeded fetches the existing mapper for the scope and updates -// its included.custom.audience if it differs from the desired value. -func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, scopeID, scopeName, audience string) error { +// listAudienceMappers fetches all protocol mappers for a client scope. +func (a *Admin) listAudienceMappers(ctx context.Context, token, realm, scopeID string) ([]protocolMapperRep, error) { base := trimBaseURL(a.BaseURL) endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" + url.PathEscape(scopeID) + "/protocol-mappers/models" req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { - return err + return nil, err } req.Header.Set("Authorization", "Bearer "+token) resp, err := a.httpc().Do(req) if err != nil { - return err + return nil, err } defer func() { _ = resp.Body.Close() }() body, _ := io.ReadAll(resp.Body) if resp.StatusCode != http.StatusOK { - return fmt.Errorf("keycloak list mappers: status %d: %s", resp.StatusCode, truncate(body, 256)) + return nil, fmt.Errorf("keycloak list mappers: status %d: %s", resp.StatusCode, truncate(body, 256)) } var mappers []protocolMapperRep if err := json.Unmarshal(body, &mappers); err != nil { - return fmt.Errorf("keycloak list mappers decode: %w", err) + return nil, fmt.Errorf("keycloak list mappers decode: %w", err) + } + return mappers, nil +} + +// updateAudienceMapperIfNeeded fetches the existing mapper for the scope and updates +// its included.custom.audience if it differs from the desired value. +// Returns an error if no matching mapper is found — this treats "no match" as a real +// failure (e.g. Keycloak race or name mismatch) rather than silently ignoring it. +func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, scopeID, scopeName, audience string) error { + mappers, err := a.listAudienceMappers(ctx, token, realm, scopeID) + if err != nil { + return err } for i := range mappers { @@ -255,12 +272,10 @@ func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, if mappers[i].Config["included.custom.audience"] == audience { return nil // already correct } - // Update the mapper with the correct audience. mappers[i].Config["included.custom.audience"] = audience return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i]) } - slog.Debug("no matching audience mapper found for scope", "scope", scopeName, "scopeID", scopeID) - return nil + return fmt.Errorf("no matching audience mapper found for scope %q (scopeID %s)", scopeName, scopeID) } func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID string, mapper protocolMapperRep) error { @@ -289,6 +304,34 @@ 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)) } +// 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), +// it re-creates it. If the audience is stale, it updates it. +// Cost: one extra GET per reconcile per audience-enabled scope; accepted tradeoff for +// catching scopes left broken by prior transient failures. +func (a *Admin) verifyAudienceMapper(ctx context.Context, token, realm, scopeID, scopeName, audience string) error { + mappers, err := a.listAudienceMappers(ctx, token, realm, scopeID) + if err != nil { + return err + } + + for i := range mappers { + if mappers[i].Name != scopeName || mappers[i].ProtocolMapper != "oidc-audience-mapper" { + continue + } + if mappers[i].Config != nil && mappers[i].Config["included.custom.audience"] == audience { + return nil + } + if mappers[i].Config == nil { + mappers[i].Config = make(map[string]string) + } + mappers[i].Config["included.custom.audience"] = audience + return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i]) + } + return a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience) +} + func (a *Admin) putRealmDefaultDefaultClientScope(ctx context.Context, token, realm, scopeID string) error { base := trimBaseURL(a.BaseURL) endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/default-default-client-scopes/" + url.PathEscape(scopeID) diff --git a/kagenti-operator/internal/keycloak/audience_test.go b/kagenti-operator/internal/keycloak/audience_test.go index 0a6f4389..42b27e97 100644 --- a/kagenti-operator/internal/keycloak/audience_test.go +++ b/kagenti-operator/internal/keycloak/audience_test.go @@ -35,6 +35,12 @@ func TestEnsureAudienceScope(t *testing.T) { case strings.Contains(path, "/client-scopes/new-scope-id/protocol-mappers/models") && r.Method == http.MethodPost: postMapperCalls++ w.WriteHeader(http.StatusCreated) + case strings.Contains(path, "/client-scopes/new-scope-id/protocol-mappers/models") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "m1", Name: "agent-ns-wl-aud", Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{"included.custom.audience": "ns/wl"}, + }}) case path == "/admin/realms/kagenti/default-default-client-scopes/new-scope-id" && r.Method == http.MethodPut: putRealmCalls++ w.WriteHeader(http.StatusNoContent) @@ -79,6 +85,7 @@ func TestEnsureAudienceScope(t *testing.T) { func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) { var getMapperCalls, putMapperCalls int var putMapperBody protocolMapperRep + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { path := r.URL.Path @@ -95,16 +102,20 @@ func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) { case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: w.WriteHeader(http.StatusConflict) - // GET mappers — returns mapper with stale audience + // GET mappers — first call returns stale, subsequent calls return corrected case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: getMapperCalls++ + aud := "ns/wl" + if putMapperCalls > 0 { + aud = spiffeURI + } _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ ID: "mapper-456", Name: "agent-ns-wl-aud", Protocol: "openid-connect", ProtocolMapper: "oidc-audience-mapper", Config: map[string]string{ - "included.custom.audience": "ns/wl", // stale short-form + "included.custom.audience": aud, "id.token.claim": "false", "access.token.claim": "true", "userinfo.token.claim": "false", @@ -134,7 +145,6 @@ func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) { t.Fatal(err) } - spiffeURI := "spiffe://example.org/ns/ns/sa/wl" err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ Realm: "kagenti", ClientName: "ns/wl", @@ -144,8 +154,8 @@ func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) { if err != nil { t.Fatal(err) } - if getMapperCalls != 1 { - t.Fatalf("expected 1 GET mapper call, got %d", getMapperCalls) + if getMapperCalls != 2 { + t.Fatalf("expected 2 GET mapper calls (update + verify), got %d", getMapperCalls) } if putMapperCalls != 1 { t.Fatalf("expected 1 PUT mapper call, got %d", putMapperCalls) @@ -221,6 +231,128 @@ func TestEnsureAudienceScope_SkipsUpdateWhenCorrect(t *testing.T) { } } +// TestEnsureAudienceScope_MapperFailurePropagated verifies that when the mapper POST +// returns a server error (e.g. 500), the error propagates to EnsureAudienceScope +// instead of being silently swallowed (regression test for #348). +func TestEnsureAudienceScope_MapperFailurePropagated(t *testing.T) { + var srv *httptest.Server + 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 does not exist yet + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{}) + + // Scope creation succeeds + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodPost: + w.Header().Set("Location", srv.URL+"/admin/realms/kagenti/client-scopes/new-scope-id") + w.WriteHeader(http.StatusCreated) + + // Mapper POST returns 500 (server error) + case strings.Contains(path, "/client-scopes/new-scope-id/protocol-mappers/models") && r.Method == http.MethodPost: + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"internal"}`)) + + 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: "spiffe://example.org/ns/ns/sa/wl", + AudienceScopeEnabled: true, + }) + if err == nil { + t.Fatal("expected error when mapper POST fails, got nil") + } + if !strings.Contains(err.Error(), "ensure audience mapper") { + t.Fatalf("expected error to contain 'ensure audience mapper', got: %s", err.Error()) + } +} + +// TestEnsureAudienceScope_VerifyRecreatesMissingMapper verifies that the defense-in-depth +// verifyAudienceMapper check detects a scope that exists without a mapper (from a prior +// failed reconcile) and re-creates the mapper. +func TestEnsureAudienceScope_VerifyRecreatesMissingMapper(t *testing.T) { + var verifyGetCalls, recreatePostCalls, verifyGetAfterRecreate 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 from prior run + 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 — mapper created (scope exists, mapper doesn't) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + recreatePostCalls++ + w.WriteHeader(http.StatusCreated) + + // GET mappers — first call (verify) returns empty (mapper missing), second returns recreated + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + verifyGetCalls++ + if recreatePostCalls > 0 { + verifyGetAfterRecreate++ + _ = 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{}) + } + + // 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 verifyGetCalls < 1 { + t.Fatalf("expected at least 1 verify GET call, got %d", verifyGetCalls) + } + if recreatePostCalls < 1 { + t.Fatalf("expected mapper to be re-created via POST, got %d calls", recreatePostCalls) + } +} + func TestEnsureAudienceScope_Disabled(t *testing.T) { a := Admin{} err := a.EnsureAudienceScope(context.Background(), "t", AudienceParams{AudienceScopeEnabled: false}) diff --git a/kagenti-operator/internal/keycloak/testidp/fake_idp.go b/kagenti-operator/internal/keycloak/testidp/fake_idp.go index 357be64a..9d399799 100644 --- a/kagenti-operator/internal/keycloak/testidp/fake_idp.go +++ b/kagenti-operator/internal/keycloak/testidp/fake_idp.go @@ -78,9 +78,10 @@ type FakeIDP struct { adminPass string tokenTTL time.Duration errorMode int - clients map[string]*ClientEntry // keyed by clientId - tokens map[string]*IssuedToken // keyed by token string - scopes map[string]scopeEntry // keyed by scope ID + clients map[string]*ClientEntry // keyed by clientId + tokens map[string]*IssuedToken // keyed by token string + scopes map[string]scopeEntry // keyed by scope ID + mappers map[string][]mapperEntry // protocol mappers keyed by scope ID nextUUID int tokenCalls int t testing.TB @@ -91,6 +92,14 @@ type scopeEntry struct { Name string } +type mapperEntry struct { + ID string `json:"id"` + Name string `json:"name"` + Protocol string `json:"protocol"` + ProtocolMapper string `json:"protocolMapper"` + Config map[string]string `json:"config"` +} + // Start creates and starts a new FakeIDP. Call Close() when done. func Start(t testing.TB, opts ...Option) *FakeIDP { t.Helper() @@ -102,6 +111,7 @@ func Start(t testing.TB, opts ...Option) *FakeIDP { clients: make(map[string]*ClientEntry), tokens: make(map[string]*IssuedToken), scopes: make(map[string]scopeEntry), + mappers: make(map[string][]mapperEntry), t: t, } for _, o := range opts { @@ -233,9 +243,13 @@ func (f *FakeIDP) handler(w http.ResponseWriter, r *http.Request) { case path == fmt.Sprintf("/admin/realms/%s/client-scopes", f.realm) && r.Method == http.MethodPost: f.handleCreateClientScope(w, r) + // Protocol mapper list (GET) + case r.Method == http.MethodGet && strings.Contains(path, "/protocol-mappers/models"): + f.handleListProtocolMappers(w, r) + // Protocol mapper create (audience mapper) case r.Method == http.MethodPost && strings.Contains(path, "/protocol-mappers/models"): - w.WriteHeader(http.StatusCreated) + f.handleCreateProtocolMapper(w, r) // Realm default scope / client default scope PUTs case r.Method == http.MethodPut && (strings.Contains(path, "/default-default-client-scopes/") || strings.Contains(path, "/default-client-scopes/")): @@ -453,6 +467,52 @@ func (f *FakeIDP) handleCreateClientScope(w http.ResponseWriter, r *http.Request w.WriteHeader(http.StatusCreated) } +func (f *FakeIDP) handleListProtocolMappers(w http.ResponseWriter, r *http.Request) { + scopeID := f.scopeIDFromMappersPath(r.URL.Path) + f.mu.Lock() + mappers := f.mappers[scopeID] + f.mu.Unlock() + if mappers == nil { + mappers = []mapperEntry{} + } + writeJSON(w, mappers) +} + +func (f *FakeIDP) handleCreateProtocolMapper(w http.ResponseWriter, r *http.Request) { + scopeID := f.scopeIDFromMappersPath(r.URL.Path) + var body struct { + Name string `json:"name"` + Protocol string `json:"protocol"` + ProtocolMapper string `json:"protocolMapper"` + Config map[string]string `json:"config"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + f.mu.Lock() + f.nextUUID++ + entry := mapperEntry{ + ID: fmt.Sprintf("mapper-%d", f.nextUUID), + Name: body.Name, + Protocol: body.Protocol, + ProtocolMapper: body.ProtocolMapper, + Config: body.Config, + } + f.mappers[scopeID] = append(f.mappers[scopeID], entry) + f.mu.Unlock() + w.WriteHeader(http.StatusCreated) +} + +func (f *FakeIDP) scopeIDFromMappersPath(path string) string { + prefix := fmt.Sprintf("/admin/realms/%s/client-scopes/", f.realm) + rest := strings.TrimPrefix(path, prefix) + if idx := strings.Index(rest, "/"); idx >= 0 { + return rest[:idx] + } + return rest +} + // issueToken creates a token and stores it. Caller must NOT hold f.mu. func (f *FakeIDP) issueToken(clientID string, ttl time.Duration) string { tok := randomHex(32)