From 325f16b3ae62c99b036dd2ba5aa7958bade9fcfa Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Fri, 1 May 2026 17:30:27 -0400 Subject: [PATCH 1/7] fix: read keycloak-admin-secret from kagenti-system namespace This commit addresses security issue #320 by changing the client registration controller to read Keycloak admin credentials from the kagenti-system namespace instead of from each agent namespace. Security improvement: - Operator now reads keycloak-admin-secret from kagenti-system only - Agent namespaces no longer have access to Keycloak admin credentials - Prevents compromised agent namespace from gaining realm admin access Changes: - Add operatorNamespace constant set to "kagenti-system" - Update secret read to use operatorNamespace instead of agent namespace - Update comments to clarify secret location and security model The operator already has ClusterRole permissions for cluster-wide secret access, so no RBAC changes are needed. **Deployment sequence:** This PR should be merged AFTER kagenti/kagenti#1422 is merged and deployed to ensure the secret exists in kagenti-system before the operator tries to read it. Closes: #320 Related: kagenti/kagenti#1422 Signed-off-by: Alan Cha --- .../controller/clientregistration_controller.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/kagenti-operator/internal/controller/clientregistration_controller.go b/kagenti-operator/internal/controller/clientregistration_controller.go index 66aa1818..bbcbe24c 100644 --- a/kagenti-operator/internal/controller/clientregistration_controller.go +++ b/kagenti-operator/internal/controller/clientregistration_controller.go @@ -35,10 +35,13 @@ import ( "github.com/kagenti/operator/internal/keycloak" ) -// Well-known namespace resources (same contract as kagenti-webhook injector). +// Well-known namespace resources. const ( authbridgeConfigConfigMap = "authbridge-config" keycloakAdminSecret = "keycloak-admin-secret" + // operatorNamespace is where the operator and keycloak-admin-secret are deployed. + // The operator reads Keycloak admin credentials from this namespace, not from agent namespaces. + operatorNamespace = "kagenti-system" // LabelClientRegistrationInject: when not "true", the operator registers the OAuth client and sets // AnnotationKeycloakClientSecretName. Value "true" opts the workload into the legacy webhook @@ -55,8 +58,9 @@ const ( // never reference a missing Secret; the webhook mounts the Secret for injected sidecars that use shared-data. type ClientRegistrationReconciler struct { client.Client - // APIReader reads authbridge-config and keycloak-admin-secret from the API server. Those objects - // are not in the manager's ConfigMap cache (see cmd/main.go cache.ByObject for ConfigMap). + // APIReader reads authbridge-config from agent namespaces and keycloak-admin-secret from + // kagenti-system namespace from the API server. Those objects are not in the manager's + // ConfigMap cache (see cmd/main.go cache.ByObject for ConfigMap). APIReader client.Reader Scheme *runtime.Scheme @@ -163,10 +167,13 @@ func (r *ClientRegistrationReconciler) reconcileOne( return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } + // Read keycloak-admin-secret from the operator namespace (kagenti-system), not from agent namespace. + // This prevents Keycloak admin credentials from being replicated to every agent namespace, + // which would be a security risk if an agent namespace is compromised. adminSecret := &corev1.Secret{} - if err := r.uncachedReader().Get(ctx, types.NamespacedName{Namespace: ns, Name: keycloakAdminSecret}, adminSecret); err != nil { + if err := r.uncachedReader().Get(ctx, types.NamespacedName{Namespace: operatorNamespace, Name: keycloakAdminSecret}, adminSecret); err != nil { if apierrors.IsNotFound(err) { - logger.Info("waiting for keycloak-admin-secret", "namespace", ns) + logger.Info("waiting for keycloak-admin-secret", "namespace", operatorNamespace) return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } return ctrl.Result{}, err From 62c6659f9886c0c502ee4dfecfca6325796bac3e Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sat, 2 May 2026 14:29:40 -0400 Subject: [PATCH 2/7] refactor: dynamically detect operator namespace instead of hardcoding Updated to address PR feedback - instead of hardcoding 'kagenti-system', the operator now dynamically detects its own namespace from the service account mount. Changes: - Add OperatorNamespace field to ClientRegistrationReconciler struct - Add getOperatorNamespace() helper function that reads from /var/run/secrets/kubernetes.io/serviceaccount/namespace - Falls back to 'kagenti-system' if detection fails - Log the detected namespace at startup for visibility This makes the code more flexible for downstream deployments that may use different namespace conventions. Signed-off-by: Alan Cha --- kagenti-operator/cmd/main.go | 22 +++++++++++++++++++ .../clientregistration_controller.go | 15 +++++++------ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/kagenti-operator/cmd/main.go b/kagenti-operator/cmd/main.go index d17cdf75..a0fc610e 100644 --- a/kagenti-operator/cmd/main.go +++ b/kagenti-operator/cmd/main.go @@ -68,6 +68,24 @@ func init() { // +kubebuilder:scaffold:scheme } +// getOperatorNamespace returns the namespace the operator is running in. +// It reads from the service account namespace file, falling back to a default. +func getOperatorNamespace() string { + // Read namespace from service account mount + nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + if err != nil { + setupLog.Info("Could not read operator namespace from service account, using default", + "error", err, "default", "kagenti-system") + return "kagenti-system" + } + ns := strings.TrimSpace(string(nsBytes)) + if ns == "" { + setupLog.Info("Operator namespace is empty, using default", "default", "kagenti-system") + return "kagenti-system" + } + return ns +} + // nolint:gocyclo func main() { var metricsAddr string @@ -381,10 +399,14 @@ func main() { } if enableOperatorClientRegistration { + operatorNS := getOperatorNamespace() + setupLog.Info("Client registration controller will read keycloak-admin-secret from operator namespace", + "namespace", operatorNS) if err = (&controller.ClientRegistrationReconciler{ Client: mgr.GetClient(), APIReader: mgr.GetAPIReader(), Scheme: mgr.GetScheme(), + OperatorNamespace: operatorNS, SpireTrustDomain: spireTrustDomain, KeycloakAdminTokenCache: &keycloak.CachedAdminTokenProvider{}, }).SetupWithManager(mgr); err != nil { diff --git a/kagenti-operator/internal/controller/clientregistration_controller.go b/kagenti-operator/internal/controller/clientregistration_controller.go index bbcbe24c..3c274f4b 100644 --- a/kagenti-operator/internal/controller/clientregistration_controller.go +++ b/kagenti-operator/internal/controller/clientregistration_controller.go @@ -39,9 +39,6 @@ import ( const ( authbridgeConfigConfigMap = "authbridge-config" keycloakAdminSecret = "keycloak-admin-secret" - // operatorNamespace is where the operator and keycloak-admin-secret are deployed. - // The operator reads Keycloak admin credentials from this namespace, not from agent namespaces. - operatorNamespace = "kagenti-system" // LabelClientRegistrationInject: when not "true", the operator registers the OAuth client and sets // AnnotationKeycloakClientSecretName. Value "true" opts the workload into the legacy webhook @@ -59,11 +56,15 @@ const ( type ClientRegistrationReconciler struct { client.Client // APIReader reads authbridge-config from agent namespaces and keycloak-admin-secret from - // kagenti-system namespace from the API server. Those objects are not in the manager's + // the operator's namespace from the API server. Those objects are not in the manager's // ConfigMap cache (see cmd/main.go cache.ByObject for ConfigMap). APIReader client.Reader Scheme *runtime.Scheme + // OperatorNamespace is the namespace where the operator is running and where keycloak-admin-secret + // is located. This is dynamically detected from the operator's service account. + OperatorNamespace string + SpireTrustDomain string // KeycloakAdminTokenCache caches admin password-grant tokens by Keycloak URL and credentials to // avoid a token request on every reconcile. If nil, PasswordGrantToken is used without caching. @@ -167,13 +168,13 @@ func (r *ClientRegistrationReconciler) reconcileOne( return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } - // Read keycloak-admin-secret from the operator namespace (kagenti-system), not from agent namespace. + // Read keycloak-admin-secret from the operator's namespace, not from agent namespace. // This prevents Keycloak admin credentials from being replicated to every agent namespace, // which would be a security risk if an agent namespace is compromised. adminSecret := &corev1.Secret{} - if err := r.uncachedReader().Get(ctx, types.NamespacedName{Namespace: operatorNamespace, Name: keycloakAdminSecret}, adminSecret); err != nil { + if err := r.uncachedReader().Get(ctx, types.NamespacedName{Namespace: r.OperatorNamespace, Name: keycloakAdminSecret}, adminSecret); err != nil { if apierrors.IsNotFound(err) { - logger.Info("waiting for keycloak-admin-secret", "namespace", operatorNamespace) + logger.Info("waiting for keycloak-admin-secret", "namespace", r.OperatorNamespace) return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } return ctrl.Result{}, err From 514fa71984db81cb8cca8f013e47a164c2c84198 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sat, 2 May 2026 14:37:35 -0400 Subject: [PATCH 3/7] refactor: use POD_NAMESPACE env var for simpler namespace detection Simplified getOperatorNamespace() based on feedback - now uses the standard POD_NAMESPACE environment variable pattern (via downward API) instead of reading from the service account mount file. Changes: - Updated getOperatorNamespace() to read from POD_NAMESPACE env var - Added POD_NAMESPACE to manager deployment using downward API - Follows the same pattern already used in agentcard-signer - Much simpler and more maintainable The deployment now injects POD_NAMESPACE via: valueFrom: fieldRef: fieldPath: metadata.namespace Signed-off-by: Alan Cha --- .../templates/manager/manager.yaml | 10 ++++++++-- kagenti-operator/cmd/main.go | 19 ++++++------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/charts/kagenti-operator/templates/manager/manager.yaml b/charts/kagenti-operator/templates/manager/manager.yaml index 787b1b27..afe0ac7f 100644 --- a/charts/kagenti-operator/templates/manager/manager.yaml +++ b/charts/kagenti-operator/templates/manager/manager.yaml @@ -65,13 +65,19 @@ spec: - {{ .Values.controllerManager.container.cmd }} image: {{ .Values.controllerManager.container.image.repository }}:{{ .Values.controllerManager.container.image.tag }} imagePullPolicy: {{ .Values.controllerManager.container.image.pullPolicy | default "IfNotPresent" }} - {{- if .Values.controllerManager.container.env }} env: + # POD_NAMESPACE is used by the client registration controller to locate + # keycloak-admin-secret in the operator's namespace + - name: POD_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + {{- if .Values.controllerManager.container.env }} {{- range $key, $value := .Values.controllerManager.container.env }} - name: {{ $key }} value: {{ $value | quote }} {{- end }} - {{- end }} + {{- end }} livenessProbe: {{- toYaml .Values.controllerManager.container.livenessProbe | nindent 12 }} readinessProbe: diff --git a/kagenti-operator/cmd/main.go b/kagenti-operator/cmd/main.go index a0fc610e..a45758f4 100644 --- a/kagenti-operator/cmd/main.go +++ b/kagenti-operator/cmd/main.go @@ -69,21 +69,14 @@ func init() { } // getOperatorNamespace returns the namespace the operator is running in. -// It reads from the service account namespace file, falling back to a default. +// Reads from POD_NAMESPACE environment variable (set via downward API in deployment), +// falling back to kagenti-system if not set. func getOperatorNamespace() string { - // Read namespace from service account mount - nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") - if err != nil { - setupLog.Info("Could not read operator namespace from service account, using default", - "error", err, "default", "kagenti-system") - return "kagenti-system" - } - ns := strings.TrimSpace(string(nsBytes)) - if ns == "" { - setupLog.Info("Operator namespace is empty, using default", "default", "kagenti-system") - return "kagenti-system" + if ns := os.Getenv("POD_NAMESPACE"); ns != "" { + return ns } - return ns + setupLog.Info("POD_NAMESPACE not set, using default", "default", "kagenti-system") + return "kagenti-system" } // nolint:gocyclo From f5df832f5f555ac721c1d699bf5fb6f4c0c44308 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sat, 2 May 2026 15:32:34 -0400 Subject: [PATCH 4/7] refactor: simplify client registration flags - remove sidecar support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR feedback - simplify confusing dual-flag setup now that the client-registration sidecar is being sunset. Changes: - Remove --enable-operator-client-registration flag (was defaulted to false) - --enable-client-registration now controls operator-based registration only - Webhook no longer injects client-registration sidecar (pass false) - Updated flag description to clarify it enables operator-based registration Before (confusing): --enable-client-registration=true (global toggle) --enable-operator-client-registration=false (operator vs sidecar) After (simplified): --enable-client-registration=true → operator-based registration --enable-client-registration=false → no registration The legacy sidecar path is removed entirely. Signed-off-by: Alan Cha --- kagenti-operator/cmd/main.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/kagenti-operator/cmd/main.go b/kagenti-operator/cmd/main.go index a45758f4..5c36b4c3 100644 --- a/kagenti-operator/cmd/main.go +++ b/kagenti-operator/cmd/main.go @@ -97,7 +97,6 @@ func main() { var requireA2ASignature bool var signatureAuditMode bool var enforceNetworkPolicies bool - var enableOperatorClientRegistration bool var enableMLflow bool var spireTrustDomain string @@ -125,7 +124,7 @@ func main() { flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics and webhook servers") flag.BoolVar(&enableClientRegistration, "enable-client-registration", true, - "If set, AuthBridge webhook will register tool clients in Keycloak") + "Enable operator-based Keycloak client registration for agent/tool workloads") flag.StringVar(&configPath, "config-path", "/etc/kagenti/config.yaml", "Path to platform config file") flag.StringVar(&featureGatesPath, "feature-gates-path", "/etc/kagenti/feature-gates/feature-gates.yaml", "Path to feature gates config file") @@ -135,9 +134,6 @@ func main() { "When true, log signature verification failures but don't block (use for rollout)") flag.BoolVar(&enforceNetworkPolicies, "enforce-network-policies", false, "Create NetworkPolicies to restrict traffic for agents with unverified signatures") - flag.BoolVar(&enableOperatorClientRegistration, "enable-operator-client-registration", false, - "Reconcile Keycloak client registration for agent/tool workloads unless "+ - "kagenti.io/client-registration-inject=true (legacy sidecar)") flag.BoolVar(&enableMLflow, "enable-mlflow", false, "Enable MLflow experiment tracking integration") @@ -391,7 +387,7 @@ func main() { setupLog.Info("MLflow experiment tracking controller enabled") } - if enableOperatorClientRegistration { + if enableClientRegistration { operatorNS := getOperatorNamespace() setupLog.Info("Client registration controller will read keycloak-admin-secret from operator namespace", "namespace", operatorNS) @@ -406,7 +402,7 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "ClientRegistration") os.Exit(1) } - setupLog.Info("Operator-managed client registration controller enabled") + setupLog.Info("Operator-based client registration controller enabled") } if controller.TektonConfigCRDExists(mgr.GetConfig()) { @@ -429,10 +425,12 @@ func main() { // AuthBridge sidecar injection webhook if authBridgeWebhooksEnabled() { + // Pass false to disable legacy client-registration sidecar injection. + // Client registration is now handled by the operator controller. podMutator := injector.NewPodMutator( mgr.GetClient(), mgr.GetAPIReader(), - enableClientRegistration, + false, // disableLegacyClientRegistrationSidecar configLoader.Get, featureGateLoader.Get, ) From a4602c3aef00d30e45b7a9acce1b40a80c445609 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sun, 3 May 2026 12:03:04 -0400 Subject: [PATCH 5/7] test: fix unit tests for operator namespace change Update tests to create keycloak-admin-secret in the operator namespace instead of the agent namespace, matching the controller changes. Changes: - Add clientRegistrationTestOperatorNS constant for operator namespace - Set OperatorNamespace field when creating test reconcilers - Create keycloak-admin-secret in operator namespace instead of agent NS All tests now pass: - TestClientRegistrationReconciler_Reconcile: PASS - All subtests pass including happy path test Signed-off-by: Alan Cha --- .../clientregistration_controller_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/kagenti-operator/internal/controller/clientregistration_controller_test.go b/kagenti-operator/internal/controller/clientregistration_controller_test.go index 9acb183f..c63026ae 100644 --- a/kagenti-operator/internal/controller/clientregistration_controller_test.go +++ b/kagenti-operator/internal/controller/clientregistration_controller_test.go @@ -24,6 +24,7 @@ import ( const ( clientRegistrationTestNamespace = "test-ns" + clientRegistrationTestOperatorNS = "operator-ns" clientRegistrationTestDeploymentName = "my-dep" ) @@ -322,7 +323,11 @@ func TestClientRegistrationReconciler_Reconcile(t *testing.T) { t.Run(tc.name, func(t *testing.T) { scheme := clientRegistrationTestScheme(t) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.objs...).Build() - r := &ClientRegistrationReconciler{Client: c, Scheme: scheme} + r := &ClientRegistrationReconciler{ + Client: c, + Scheme: scheme, + OperatorNamespace: clientRegistrationTestOperatorNS, + } res, err := r.Reconcile(ctx, req) if err != nil { t.Fatalf("Reconcile: %v", err) @@ -350,9 +355,13 @@ func TestClientRegistrationReconciler_Reconcile(t *testing.T) { clusterFeatureGatesConfigMap(true), dep, authbridgeConfigMapForTest(clientRegistrationTestNamespace, srv.URL), - keycloakAdminSecretForTest(clientRegistrationTestNamespace), + keycloakAdminSecretForTest(clientRegistrationTestOperatorNS), ).Build() - r := &ClientRegistrationReconciler{Client: c, Scheme: scheme} + r := &ClientRegistrationReconciler{ + Client: c, + Scheme: scheme, + OperatorNamespace: clientRegistrationTestOperatorNS, + } res, err := r.Reconcile(ctx, req) if err != nil || res != (ctrl.Result{}) { t.Fatalf("got (%v, %v), want (zero Result, nil)", res, err) From eccbb0f8aec705e5e824468fcb5abd146284a646 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sun, 3 May 2026 12:09:00 -0400 Subject: [PATCH 6/7] docs: clarify test comments for operator namespace behavior Add comments to make it explicit that tests verify operator namespace behavior, not agent namespace behavior. This clarifies that: - Admin secret should be in operator namespace (not agent namespace) - Tests verify correct behavior when secret is missing/present in operator NS - Agent namespaces should NOT have admin credentials (security improvement) No functional changes, only comment improvements for clarity. Signed-off-by: Alan Cha --- .../controller/clientregistration_controller_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/kagenti-operator/internal/controller/clientregistration_controller_test.go b/kagenti-operator/internal/controller/clientregistration_controller_test.go index c63026ae..eb8fbce5 100644 --- a/kagenti-operator/internal/controller/clientregistration_controller_test.go +++ b/kagenti-operator/internal/controller/clientregistration_controller_test.go @@ -197,6 +197,10 @@ func authbridgeConfigMapForTest(ns, keycloakURL string) *corev1.ConfigMap { } } +// keycloakAdminSecretForTest creates a test keycloak-admin-secret. +// The secret should be created in the operator namespace (clientRegistrationTestOperatorNS), +// NOT in agent namespaces. This matches the production behavior where admin credentials +// are restricted to the operator namespace for security. func keycloakAdminSecretForTest(ns string) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -309,11 +313,13 @@ func TestClientRegistrationReconciler_Reconcile(t *testing.T) { wantRequeue: requeue, }, { - name: "missing keycloak admin secret waits with requeue", + name: "missing keycloak admin secret in operator namespace waits with requeue", objs: []client.Object{ clusterFeatureGatesConfigMap(true), testDeploymentForClientReg(), authbridgeConfigMapForTest(clientRegistrationTestNamespace, "https://keycloak.example"), + // Note: keycloak-admin-secret intentionally NOT created in operator namespace + // to test the missing secret path }, wantRequeue: requeue, }, @@ -355,6 +361,7 @@ func TestClientRegistrationReconciler_Reconcile(t *testing.T) { clusterFeatureGatesConfigMap(true), dep, authbridgeConfigMapForTest(clientRegistrationTestNamespace, srv.URL), + // keycloak-admin-secret created in OPERATOR namespace (not agent namespace) keycloakAdminSecretForTest(clientRegistrationTestOperatorNS), ).Build() r := &ClientRegistrationReconciler{ From 9c25259ca70e4c35a9835176fe5b7ce39bc450e2 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sun, 3 May 2026 12:13:15 -0400 Subject: [PATCH 7/7] refactor: use 'operator-managed' instead of 'operator-based' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use consistent terminology with the rest of the codebase. The term 'operator-managed' is already used throughout the operator: - precedence.go: 'operator-managed client registration is default' - pod_mutator.go: 'operator-managed Keycloak client credentials' - keycloak_client_credentials.go: 'operator-managed Keycloak client' - clientregistration_controller_test.go: 'operator-managed registration' Changed: - Flag description: operator-based → operator-managed - Log message: Operator-based → Operator-managed This aligns with established terminology in the codebase. Signed-off-by: Alan Cha --- kagenti-operator/cmd/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kagenti-operator/cmd/main.go b/kagenti-operator/cmd/main.go index 5c36b4c3..ebf4feb0 100644 --- a/kagenti-operator/cmd/main.go +++ b/kagenti-operator/cmd/main.go @@ -124,7 +124,7 @@ func main() { flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics and webhook servers") flag.BoolVar(&enableClientRegistration, "enable-client-registration", true, - "Enable operator-based Keycloak client registration for agent/tool workloads") + "Enable operator-managed Keycloak client registration for agent/tool workloads") flag.StringVar(&configPath, "config-path", "/etc/kagenti/config.yaml", "Path to platform config file") flag.StringVar(&featureGatesPath, "feature-gates-path", "/etc/kagenti/feature-gates/feature-gates.yaml", "Path to feature gates config file") @@ -402,7 +402,7 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "ClientRegistration") os.Exit(1) } - setupLog.Info("Operator-based client registration controller enabled") + setupLog.Info("Operator-managed client registration controller enabled") } if controller.TektonConfigCRDExists(mgr.GetConfig()) {