From 58daa44c63d2b7493324cfbef92aa3622d143a91 Mon Sep 17 00:00:00 2001 From: waveywaves <11972233+waveywaves@users.noreply.github.com> Date: Tue, 11 Aug 2026 23:14:23 +0530 Subject: [PATCH 1/2] Scope analysis identity to its run namespace Signed-off-by: waveywaves <11972233+waveywaves@users.noreply.github.com> --- .ai/spec/what/sandbox-execution.md | 7 +- config/rbac/role.yaml | 14 ++ controller/agenticrun/analysis_identity.go | 125 ++++++++++++++ .../agenticrun/analysis_identity_test.go | 153 ++++++++++++++++++ controller/agenticrun/handlers.go | 19 ++- controller/agenticrun/reconciler.go | 17 +- controller/agenticrun/reconciler_test.go | 12 +- docs/rbac.md | 18 ++- 8 files changed, 347 insertions(+), 18 deletions(-) create mode 100644 controller/agenticrun/analysis_identity.go create mode 100644 controller/agenticrun/analysis_identity_test.go diff --git a/.ai/spec/what/sandbox-execution.md b/.ai/spec/what/sandbox-execution.md index 3f31fcf7..766a8f70 100644 --- a/.ai/spec/what/sandbox-execution.md +++ b/.ai/spec/what/sandbox-execution.md @@ -52,7 +52,8 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 19b. [PLANNED: OLS-3594] **OLSConfig / classic-operator handoff** for introspection and connection details is deferred with 19a (includes OLS-3572). 19c. [PLANNED: OLS-3697] **RHOKP endpoint and CA** are included in the handoff ConfigMap (`lightspeed-sandbox-config`) when OKP is enabled. The RHOKP CA cert volume/mount is part of the base `sandbox-pod-spec` PodSpec; the `rhokp-endpoint` key provides the HTTPS URL. Sandbox pods can query OKP documentation directly via the standalone RHOKP Service. 20. **Sandbox observability patch**: Immediately after creating a claim (or Pod in `bare-pod` mode), the controller MUST patch `AgenticRun.status.steps..sandbox` with the resource name and operator namespace so consoles/CLIs can tail logs before the sandbox is ready. In `bare-pod` mode, `status.steps..sandbox.claimName` MUST be set to the Pod name (same field used for SandboxClaim names in `sandbox-claim` mode). -21. **Execution RBAC materialization**: When the approved remediation option includes RBAC requests, before execution the controller MUST create a **per-run ServiceAccount** named `ls-exec-{run-namespace}-{run-name}` in the operator namespace (truncated to 63 chars), then create namespace-scoped `Role`+`RoleBinding` pairs in each target namespace and `ClusterRole`+`ClusterRoleBinding` for cluster-scoped rules, binding subjects to **this per-run SA** (not the shared `lightspeed-agent` SA). The per-run SA MUST NOT carry an owner reference — cross-namespace owner refs are unsupported by Kubernetes GC (the SA lives in the operator namespace while the AgenticRun may be in a different namespace). Cleanup is handled explicitly by `cleanupExecutionRBAC` via the AgenticRun's finalizer. Idempotent create MUST tolerate existing objects (`AlreadyExists` is a no-op). Only execution sandbox pods use this SA — analysis and verification pods continue using `lightspeed-agent`. The operator's `cluster-admin` privilege (external prerequisite) ensures no RBAC escalation issues when creating arbitrary Roles/SAs. See the workspace-level agentic security spec at `ols/.ai/spec/what/agentic-security.md` rules 7-15 for full specification. +20a. **Analysis identity isolation**: Before analysis, the controller MUST create a UID-qualified per-run ServiceAccount in the operator namespace. A RoleBinding to the built-in `view` ClusterRole MUST grant that identity read access only in the AgenticRun namespace; `spec.targetNamespaces` MUST NOT widen analysis access because the AgenticRun API does not carry a verified creator identity. A supplemental Role MAY grant only `pods/log` get. The identity MUST receive no ClusterRoleBinding, Secret permission, or namespace-wide Result write permission. A future batch sandbox MUST use a separately enforced, expected-result-name write path rather than widening this identity. It MUST be deleted after analysis and on failure, suspension, terminal reconciliation, or AgenticRun deletion. +21. **Execution RBAC materialization**: When the approved remediation option includes RBAC requests, before execution the controller MUST create a **per-run ServiceAccount** named `ls-exec-{run-namespace}-{run-name}` in the operator namespace (truncated to 63 chars), then create namespace-scoped `Role`+`RoleBinding` pairs in each target namespace and `ClusterRole`+`ClusterRoleBinding` for cluster-scoped rules, binding subjects to **this per-run SA** (not the shared `lightspeed-agent` SA). The per-run SA MUST NOT carry an owner reference — cross-namespace owner refs are unsupported by Kubernetes GC (the SA lives in the operator namespace while the AgenticRun may be in a different namespace). Cleanup is handled explicitly by `cleanupExecutionRBAC` via the AgenticRun's finalizer. Idempotent create MUST tolerate existing objects (`AlreadyExists` is a no-op). Only execution sandbox pods use this SA — analysis uses its own per-run identity, while verification continues using `lightspeed-agent`. The operator's `cluster-admin` privilege (external prerequisite) ensures no RBAC escalation issues when creating arbitrary Roles/SAs. See the workspace-level agentic security spec at `ols/.ai/spec/what/agentic-security.md` rules 7-15 for full specification. 21a. **Read RBAC multi-binding resolution**: The shared `lightspeed-agent` SA may have **multiple** `ClusterRoleBinding`s granting read access (e.g. `cluster-reader` and `cluster-monitoring-view`). When granting read permissions to a per-run execution SA, the controller MUST discover **all** `ClusterRoleBinding`s where `lightspeed-agent` is a subject and add the per-run SA to every one of them — not just the first match. Discovery results MUST be cached for the lifetime of the process (the infrastructure CRBs are static). On cleanup, the per-run SA MUST be removed from all discovered bindings. [OLS-3712] 21b. **Execution outcome override**: When the execution agent reports `success=false` but **all mutating actions succeeded**, the controller MUST NOT hard-fail the run. Instead it MUST override the execution outcome to `Succeeded` and proceed to the verification step (or trust-mode completion if verification is absent). The controller determines mutation success by inspecting `ExecutionAction` entries: observation action types (`pre-check`, `post-check`, `verification`, `check`, `wait`) are excluded; all remaining action types are considered mutations. The run MUST hard-fail only when (a) no mutating actions exist (agent never attempted the fix), or (b) any mutating action has `outcome=Failed`. [OLS-3558] 22. **RBAC subjects namespace**: RoleBindings MUST reference the **per-run service account** in the **operator namespace** (where sandbox pods run), even when roles live in target namespaces. @@ -96,9 +97,9 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 34. **[OLS-3066] No endpoint construction**: With the batch execution model, the operator does not construct agent HTTP URLs. There is no HTTP communication between operator and sandbox. Input is delivered via ConfigMap mount (rule 7); output is delivered via Result CR creation (rule 8). 35. **Sandbox release**: In `bare-pod` mode, `Release` deletes the Pod by name. In `sandbox-claim` mode, `Release` deletes both the SandboxClaim and its SandboxTemplate (same name). Both paths are idempotent — NotFound is treated as success. 36. **PodSpecBuilder**: `PodSpecBuilder` takes a base `*corev1.PodSpec` (from the config cache) and overlays agent-specific configuration: LLM env vars, credential mounts, skills volumes, MCP config, required secrets, input ConfigMap volume mount [OLS-3066], SA token mounting, security context. It produces a single typed `corev1.PodSpec`. In `bare-pod` mode, this PodSpec is used directly to create a Pod. In `sandbox-claim` mode, it is converted to an unstructured map and embedded in a `SandboxTemplate`. [OLS-3066] HTTP readiness/liveness probes are no longer set (see rule 30). -36a. **SA token mounting**: `PodSpecBuilder` MUST set `AutomountServiceAccountToken=true` for **all** steps. The correct ServiceAccount is already resolved upstream (per-run SA for execution, shared `lightspeed-agent` for analysis/verification/escalation), and the pod needs the projected token to authenticate `oc`/`kubectl` commands against the cluster API regardless of step. +36a. **SA token mounting**: `PodSpecBuilder` MUST set `AutomountServiceAccountToken=true` for **all** steps. The correct ServiceAccount is already resolved upstream (per-run SA for analysis and execution, shared `lightspeed-agent` for verification/escalation), and the pod needs the projected token to authenticate `oc`/`kubectl` commands against the cluster API regardless of step. 37. **RBAC**: The operator's ClusterRole MUST include Pod create/delete/get/list/watch verbs (bare-pod mode) AND SandboxClaim, SandboxTemplate, Sandbox resource verbs (sandbox-claim mode). -38. **Bootstrap**: The operator MUST unconditionally create the `lightspeed-agent` ServiceAccount in the operator namespace at startup (idempotent). This SA is used by analysis/verification/escalation steps — execution uses per-run SAs per rule 21. The `lightspeed-agent` SA MUST NOT have any execution-level Roles or ClusterRoles bound to it. No base `SandboxTemplate` is created at bootstrap — templates are built per-run by `SandboxManager`. +38. **Bootstrap**: The operator MUST unconditionally create the `lightspeed-agent` ServiceAccount in the operator namespace at startup (idempotent). This SA is used by verification/escalation steps — analysis and execution use separate per-run SAs per rules 20a and 21. The `lightspeed-agent` SA MUST NOT have any execution-level Roles or ClusterRoles bound to it. No base `SandboxTemplate` is created at bootstrap — templates are built per-run by `SandboxManager`. ## Configuration Surface diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 67dbfa84..f142b31d 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -30,6 +30,12 @@ rules: - get - list - watch +- apiGroups: + - "" + resources: + - pods/log + verbs: + - get - apiGroups: - "" resources: @@ -134,6 +140,14 @@ rules: - get - list - update +- apiGroups: + - rbac.authorization.k8s.io + resourceNames: + - view + resources: + - clusterroles + verbs: + - bind - apiGroups: - rbac.authorization.k8s.io resources: diff --git a/controller/agenticrun/analysis_identity.go b/controller/agenticrun/analysis_identity.go new file mode 100644 index 00000000..81173c34 --- /dev/null +++ b/controller/agenticrun/analysis_identity.go @@ -0,0 +1,125 @@ +package agenticrun + +import ( + "context" + "crypto/sha256" + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" +) + +const ( + analysisViewClusterRole = "view" + + ErrCreateAnalysisSA = "create analysis SA" + ErrCreateAnalysisViewBinding = "create analysis view RoleBinding" + ErrCreateAnalysisLogRole = "create analysis log Role" + ErrCreateAnalysisLogBinding = "create analysis log RoleBinding" + ErrDeleteAnalysisViewBinding = "delete analysis view RoleBinding" + ErrDeleteAnalysisLogBinding = "delete analysis log RoleBinding" + ErrDeleteAnalysisLogRole = "delete analysis log Role" + ErrDeleteAnalysisSA = "delete analysis SA" +) + +func analysisIdentityName(run *agenticv1alpha1.AgenticRun) string { + digest := fmt.Sprintf("%x", sha256.Sum256([]byte(run.Namespace+"\x00"+run.Name+"\x00"+string(run.UID))))[:8] + prefix := strings.TrimRight(fmt.Sprintf("ls-analysis-%s-%s", run.Namespace, run.Name), "-._") + const maxPrefixLength = 47 // 47 + '-' + 8 leaves room for RoleBinding suffixes. + if len(prefix) > maxPrefixLength { + prefix = strings.TrimRight(prefix[:maxPrefixLength], "-._") + } + return prefix + "-" + digest +} + +// ensureAnalysisIdentity creates a per-run ServiceAccount in the operator +// namespace. It can read only the AgenticRun namespace through the built-in +// view ClusterRole, plus failed-step pod logs. +func ensureAnalysisIdentity(ctx context.Context, c client.Client, run *agenticv1alpha1.AgenticRun, operatorNS string) (string, error) { + name := analysisIdentityName(run) + labels := rbacLabels(run.Name, "analysis-identity") + subjects := []rbacv1.Subject{{ + Kind: rbacv1.ServiceAccountKind, + Name: name, + Namespace: operatorNS, + }} + + resources := []struct { + object client.Object + errMsg string + }{ + { + object: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: operatorNS, Labels: labels}, + AutomountServiceAccountToken: ptr.To(false), + }, + errMsg: ErrCreateAnalysisSA, + }, + { + object: &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name + "-view", Namespace: run.Namespace, Labels: labels}, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: analysisViewClusterRole, + }, + Subjects: subjects, + }, + errMsg: ErrCreateAnalysisViewBinding, + }, + { + object: &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: run.Namespace, Labels: labels}, + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods/log"}, Verbs: []string{"get"}}, + }, + }, + errMsg: ErrCreateAnalysisLogRole, + }, + { + object: &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name + "-logs", Namespace: run.Namespace, Labels: labels}, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: name, + }, + Subjects: subjects, + }, + errMsg: ErrCreateAnalysisLogBinding, + }, + } + + for _, resource := range resources { + if err := c.Create(ctx, resource.object); err != nil && !apierrors.IsAlreadyExists(err) { + return "", fmt.Errorf("%s %s: %w", resource.errMsg, name, err) + } + } + return name, nil +} + +func cleanupAnalysisIdentity(ctx context.Context, c client.Client, run *agenticv1alpha1.AgenticRun, operatorNS string) error { + name := analysisIdentityName(run) + resources := []struct { + object client.Object + errMsg string + }{ + {&rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: name + "-view", Namespace: run.Namespace}}, ErrDeleteAnalysisViewBinding}, + {&rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: name + "-logs", Namespace: run.Namespace}}, ErrDeleteAnalysisLogBinding}, + {&rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: run.Namespace}}, ErrDeleteAnalysisLogRole}, + {&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: operatorNS}}, ErrDeleteAnalysisSA}, + } + for _, resource := range resources { + if err := deleteIfExists(ctx, c, resource.object); err != nil { + return fmt.Errorf("%s %s: %w", resource.errMsg, name, err) + } + } + return nil +} diff --git a/controller/agenticrun/analysis_identity_test.go b/controller/agenticrun/analysis_identity_test.go new file mode 100644 index 00000000..d8450908 --- /dev/null +++ b/controller/agenticrun/analysis_identity_test.go @@ -0,0 +1,153 @@ +package agenticrun + +import ( + "context" + "errors" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" +) + +func TestEnsureAnalysisIdentityScopesReadAccessToRunNamespace(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "diagnose", Namespace: "team-a", UID: types.UID("run-uid")}, + Spec: agenticv1alpha1.AgenticRunSpec{TargetNamespaces: []string{"team-b"}}, + } + fc := fake.NewClientBuilder().WithScheme(testScheme()).Build() + + name, err := ensureAnalysisIdentity(context.Background(), fc, run, "operator-ns") + if err != nil { + t.Fatalf("ensureAnalysisIdentity: %v", err) + } + if len(name) > 63 || !strings.HasPrefix(name, "ls-analysis-team-a-diagnose-") { + t.Fatalf("unexpected identity name %q", name) + } + // A repeated reconcile is idempotent. + if _, err := ensureAnalysisIdentity(context.Background(), fc, run, "operator-ns"); err != nil { + t.Fatalf("second ensureAnalysisIdentity: %v", err) + } + + var sa corev1.ServiceAccount + if err := fc.Get(context.Background(), client.ObjectKey{Name: name, Namespace: "operator-ns"}, &sa); err != nil { + t.Fatalf("get ServiceAccount: %v", err) + } + if len(sa.OwnerReferences) != 0 { + t.Fatal("cross-namespace ServiceAccount must not have an owner reference") + } + if sa.AutomountServiceAccountToken == nil || *sa.AutomountServiceAccountToken { + t.Fatal("ServiceAccount should disable implicit token mounts") + } + + var viewBinding rbacv1.RoleBinding + if err := fc.Get(context.Background(), client.ObjectKey{Name: name + "-view", Namespace: "team-a"}, &viewBinding); err != nil { + t.Fatalf("get view RoleBinding: %v", err) + } + if viewBinding.RoleRef.Kind != "ClusterRole" || viewBinding.RoleRef.Name != analysisViewClusterRole { + t.Fatalf("unexpected view roleRef: %#v", viewBinding.RoleRef) + } + if len(viewBinding.Subjects) != 1 || viewBinding.Subjects[0].Name != name || viewBinding.Subjects[0].Namespace != "operator-ns" { + t.Fatalf("unexpected view subjects: %#v", viewBinding.Subjects) + } + + var logRole rbacv1.Role + if err := fc.Get(context.Background(), client.ObjectKey{Name: name, Namespace: "team-a"}, &logRole); err != nil { + t.Fatalf("get log Role: %v", err) + } + if len(logRole.Rules) != 1 || len(logRole.Rules[0].Resources) != 1 || logRole.Rules[0].Resources[0] != "pods/log" { + t.Fatalf("supplemental analysis permissions must contain only pods/log: %#v", logRole.Rules) + } + + var otherBindings rbacv1.RoleBindingList + if err := fc.List(context.Background(), &otherBindings, client.InNamespace("team-b")); err != nil { + t.Fatalf("list other namespace bindings: %v", err) + } + if len(otherBindings.Items) != 0 { + t.Fatalf("targetNamespaces must not widen analysis access: %#v", otherBindings.Items) + } + + if err := cleanupAnalysisIdentity(context.Background(), fc, run, "operator-ns"); err != nil { + t.Fatalf("cleanupAnalysisIdentity: %v", err) + } + if err := fc.Get(context.Background(), client.ObjectKey{Name: name, Namespace: "operator-ns"}, &sa); !apierrors.IsNotFound(err) { + t.Fatalf("ServiceAccount still exists after cleanup: %v", err) + } +} + +func TestAnalysisUsesScopedIdentityAndCleansItUp(t *testing.T) { + run := testAgenticRun() + run.UID = types.UID("analysis-run-uid") + agent := newTestAgentCaller() + objects := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objects...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build() + r := &AgenticRunReconciler{Client: fc, Agent: agent, Namespace: "operator-ns"} + + if _, err := reconcileOnce(r, run.Name); err != nil { + t.Fatalf("reconcile analysis: %v", err) + } + want := analysisIdentityName(run) + if agent.analysisServiceAccount != want { + t.Fatalf("analysis ServiceAccount = %q, want %q", agent.analysisServiceAccount, want) + } + var sa corev1.ServiceAccount + if err := fc.Get(context.Background(), client.ObjectKey{Name: want, Namespace: "operator-ns"}, &sa); err != nil { + t.Fatalf("analysis ServiceAccount should remain until completion is observed: %v", err) + } + if _, err := reconcileOnce(r, run.Name); err != nil { + t.Fatalf("reconcile post-analysis cleanup: %v", err) + } + if err := fc.Get(context.Background(), client.ObjectKey{Name: want, Namespace: "operator-ns"}, &sa); !apierrors.IsNotFound(err) { + t.Fatalf("analysis ServiceAccount was not cleaned up: %v", err) + } +} + +func TestAnalysisIdentityNameIncludesRunUID(t *testing.T) { + first := &agenticv1alpha1.AgenticRun{ObjectMeta: metav1.ObjectMeta{Name: strings.Repeat("long-name-", 10), Namespace: "team", UID: types.UID("first")}} + second := first.DeepCopy() + second.UID = types.UID("second") + + if analysisIdentityName(first) == analysisIdentityName(second) { + t.Fatal("recreated runs must not share an analysis identity") + } + if len(analysisIdentityName(first)+"-logs") > 63 { + t.Fatalf("derived log binding name is too long: %q", analysisIdentityName(first)+"-logs") + } +} + +func TestAnalysisIdentityCleanupFailureRequeues(t *testing.T) { + run := testAgenticRun() + run.UID = types.UID("analysis-run-uid") + objects := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objects...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*rbacv1.RoleBinding); ok && strings.HasPrefix(obj.GetName(), "ls-analysis-") { + return errors.New("transient delete failure") + } + return c.Delete(ctx, obj, opts...) + }, + }).Build() + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "operator-ns"} + + if _, err := reconcileOnce(r, run.Name); err != nil { + t.Fatalf("reconcile analysis: %v", err) + } + result, err := reconcileOnce(r, run.Name) + if err != nil { + t.Fatalf("reconcile cleanup: %v", err) + } + if result.RequeueAfter != rbacCleanupRequeueAfter { + t.Fatalf("cleanup requeue = %v, want %v", result.RequeueAfter, rbacCleanupRequeueAfter) + } +} diff --git a/controller/agenticrun/handlers.go b/controller/agenticrun/handlers.go index 5b4f501e..ff181d12 100644 --- a/controller/agenticrun/handlers.go +++ b/controller/agenticrun/handlers.go @@ -17,6 +17,7 @@ import ( const ( ErrUpdateToAnalyzing = "update to Analyzing" + ErrEnsureAnalysisIdentity = "ensure analysis identity" ErrCreateAnalysisResult = "create analysis result" ErrUpdateAfterAnalysis = "update after analysis" ErrUpdateToAnalyzingRevision = "update to Analyzing (revision)" @@ -99,7 +100,11 @@ func (r *AgenticRunReconciler) handleAnalysis( r.Audit.EmitAgenticRunReceived(spanCtx, run) } - analysisResult, err := r.Agent.Analyze(spanCtx, run, resolved.Analysis, run.Spec.Request, defaultSandboxSA) + analysisSA, err := ensureAnalysisIdentity(spanCtx, r.Client, run, r.Namespace) + if err != nil { + return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionAnalyzed, fmt.Errorf("%s: %w", ErrEnsureAnalysisIdentity, err)) + } + analysisResult, err := r.Agent.Analyze(spanCtx, run, resolved.Analysis, run.Spec.Request, analysisSA) if err != nil { return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionAnalyzed, err) } @@ -182,7 +187,11 @@ func (r *AgenticRunReconciler) handleRevision( revisionSuffix := buildRevisionContext(run) requestWithRevision := run.Spec.Request + "\n\n" + revisionSuffix - analysisResult, err := r.Agent.Analyze(spanCtx, run, resolved.Analysis, requestWithRevision, defaultSandboxSA) + analysisSA, err := ensureAnalysisIdentity(spanCtx, r.Client, run, r.Namespace) + if err != nil { + return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionAnalyzed, fmt.Errorf("%s: %w", ErrEnsureAnalysisIdentity, err)) + } + analysisResult, err := r.Agent.Analyze(spanCtx, run, resolved.Analysis, requestWithRevision, analysisSA) if err != nil { return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionAnalyzed, err) } @@ -580,6 +589,9 @@ func (r *AgenticRunReconciler) handleFailed( log.Error(err, "RBAC cleanup on failure") } } + if err := cleanupAnalysisIdentity(ctx, r.Client, run, r.Namespace); err != nil { + log.Error(err, "analysis identity cleanup on failure") + } return ctrl.Result{}, nil } @@ -603,6 +615,9 @@ func (r *AgenticRunReconciler) handleSuspension( log.Error(err, "best-effort RBAC cleanup during suspension") } } + if err := cleanupAnalysisIdentity(ctx, r.Client, run, r.Namespace); err != nil { + log.Error(err, "best-effort analysis identity cleanup during suspension") + } if isTerminal(phase) { return ctrl.Result{}, nil diff --git a/controller/agenticrun/reconciler.go b/controller/agenticrun/reconciler.go index 3828d933..c4b4b93f 100644 --- a/controller/agenticrun/reconciler.go +++ b/controller/agenticrun/reconciler.go @@ -57,7 +57,9 @@ type AgenticRunReconciler struct { // +kubebuilder:rbac:groups=agentic.openshift.io,resources=escalationresults,verbs=get;list;watch;create // +kubebuilder:rbac:groups=agentic.openshift.io,resources=analysisresults/status;executionresults/status;verificationresults/status;escalationresults/status,verbs=get;patch;update // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;create;delete +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles,resourceNames=view,verbs=bind // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;create;update;delete +// +kubebuilder:rbac:groups="",resources=pods/log,verbs=get // +kubebuilder:rbac:groups=agentic.openshift.io,resources=agenticolsconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch @@ -102,6 +104,15 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) phase := agenticv1alpha1.DerivePhase(run.Status.Conditions) + // Keep the per-run identity while analysis is active. All later phases must + // revoke it before progressing; retry transient cleanup failures. + if phase != agenticv1alpha1.AgenticRunPhasePending && phase != agenticv1alpha1.AgenticRunPhaseAnalyzing { + if err := cleanupAnalysisIdentity(ctx, r.Client, &run, r.Namespace); err != nil { + log.Error(err, "analysis identity cleanup failed, will retry") + return ctrl.Result{RequeueAfter: rbacCleanupRequeueAfter}, nil + } + } + // --- Terminal phases (before suspension guard so audit cleanup always runs) --- switch phase { case agenticv1alpha1.AgenticRunPhaseNoActionRequired: @@ -350,13 +361,17 @@ func (r *AgenticRunReconciler) handleRBACCleanup(ctx context.Context, run *agent if attempts < rbacMaxCleanupAttempts { sandboxErr := r.Agent.ReleaseSandboxes(ctx, run) rbacErr := cleanupExecutionRBAC(ctx, r.Client, run, r.Namespace) - if sandboxErr != nil || rbacErr != nil { + analysisIdentityErr := cleanupAnalysisIdentity(ctx, r.Client, run, r.Namespace) + if sandboxErr != nil || rbacErr != nil || analysisIdentityErr != nil { if sandboxErr != nil { log.Error(sandboxErr, "sandbox release failed, will retry", "attempt", attempts+1, "max", rbacMaxCleanupAttempts) } if rbacErr != nil { log.Error(rbacErr, "RBAC cleanup failed, will retry", "attempt", attempts+1, "max", rbacMaxCleanupAttempts) } + if analysisIdentityErr != nil { + log.Error(analysisIdentityErr, "analysis identity cleanup failed, will retry", "attempt", attempts+1, "max", rbacMaxCleanupAttempts) + } original := run.DeepCopy() if run.Annotations == nil { run.Annotations = make(map[string]string) diff --git a/controller/agenticrun/reconciler_test.go b/controller/agenticrun/reconciler_test.go index 44067410..5582b1b2 100644 --- a/controller/agenticrun/reconciler_test.go +++ b/controller/agenticrun/reconciler_test.go @@ -30,10 +30,11 @@ type testAgentCaller struct { verifyErr error escalateErr error - analyzeResult *AnalysisOutput - executeResult *ExecutionOutput - verifyResult *VerificationOutput - escalateResult *EscalationOutput + analyzeResult *AnalysisOutput + executeResult *ExecutionOutput + verifyResult *VerificationOutput + escalateResult *EscalationOutput + analysisServiceAccount string } func newTestAgentCaller() *testAgentCaller { @@ -45,7 +46,8 @@ func newTestAgentCaller() *testAgentCaller { return &testAgentCaller{analyzeResult: a, executeResult: e, verifyResult: v, escalateResult: esc} } -func (ta *testAgentCaller) Analyze(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ resolvedStep, _ string, _ string) (*AnalysisOutput, error) { +func (ta *testAgentCaller) Analyze(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ resolvedStep, _ string, serviceAccount string) (*AnalysisOutput, error) { + ta.analysisServiceAccount = serviceAccount if ta.analyzeErr != nil { return nil, ta.analyzeErr } diff --git a/docs/rbac.md b/docs/rbac.md index 95bb9a49..c18767ed 100644 --- a/docs/rbac.md +++ b/docs/rbac.md @@ -5,7 +5,7 @@ The agentic operator uses a layered RBAC model: - **Operator RBAC** — what the operator itself can do (static, deployed with the operator) - **External prerequisites** — admin-created permissions the operator depends on but does **not** create itself. These must be applied as a post-install step (see sections 1 and 2 below): - - **Agent read RBAC** — what sandbox pods can read (admin prerequisite, all phases) + - **Shared agent read RBAC** — what verification and escalation sandboxes can read (admin prerequisite) - **Operator escalation privilege** — allows the operator to create Roles with arbitrary content ## Operator RBAC (static) @@ -28,9 +28,9 @@ Key permissions: These must be created by a **platform admin** before the operator and agents can function correctly. The operator does not create them — it assumes they exist. -### 1. Agent ServiceAccount and read access (all phases) +### 1. Shared agent ServiceAccount and read access -**Why:** The agent pod runs as the `lightspeed-agent` ServiceAccount (referenced in `SandboxTemplate.spec.podTemplate.spec.serviceAccountName`). This SA is the runtime identity for all k8s API calls the agent makes (e.g. `kubectl get pods`, `kubectl patch deployment`). It must exist (or pods fail to start) and must have read permissions bound to it (or agents can't inspect cluster state to diagnose problems). +**Why:** Verification and escalation pods run as the `lightspeed-agent` ServiceAccount. It must exist and have read permissions. Analysis and execution use separate per-run identities described below. **What:** A ServiceAccount + ClusterRole + ClusterRoleBinding granting read permissions. @@ -70,7 +70,7 @@ subjects: **Note:** The ServiceAccount is typically included in the SandboxTemplate YAML (see `test/agent/sandboxtemplate/sandboxtemplate.yaml`). -**Scope decision:** Cluster-wide read is shown above. For tighter security, use per-namespace Roles binding only to `targetNamespaces` the AgenticRun references — but this requires dynamic admin action per namespace. +**Scope decision:** Cluster-wide read remains a prerequisite only for shared verification and escalation. Analysis uses a dynamic namespace-scoped identity and never trusts `targetNamespaces` to widen access. ### 2. Operator escalation privilege @@ -106,6 +106,10 @@ subjects: **Production alternative:** Scope the ClusterRole to only the resources and verbs agents are expected to request (e.g. `deployments patch`, `configmaps get/update` in specific API groups), rather than `"*"` on everything. +## Dynamic analysis identity (per-AgenticRun) + +Before analysis, the operator creates a UID-qualified ServiceAccount in its own namespace and binds it to the built-in `view` ClusterRole only in the AgenticRun namespace. A supplemental Role grants only `pods/log` get. The identity has no ClusterRoleBinding, Secret access, Result write permission, or access to other entries in `spec.targetNamespaces`. It is deleted after analysis and by terminal/finalizer cleanup. + ## Dynamic execution RBAC (per-AgenticRun) Created by the operator during the execution phase, deleted on terminal state or AgenticRun deletion. @@ -123,15 +127,15 @@ Lifecycle: - **Created**: just before execution agent call (`ensureExecutionRBAC`) - **Deleted**: immediately after execution completes (before verification starts). Retries on failure via requeue. Also cleaned up on AgenticRun deletion (finalizer), escalation, or system failure as fallback. -> **Resolved: per-run SA isolation.** Each AgenticRun in execution phase gets its own ServiceAccount (`ls-exec-{namespace}-{name}`) in the operator namespace. Execution RBAC binds to this per-run SA, not the shared `lightspeed-agent`. The per-run SA is explicitly deleted after execution completes (before verification). This eliminates cross-run permission bleed — concurrent AgenticRuns cannot share write RBAC. Analysis and verification continue using the shared `lightspeed-agent` SA (read-only). The operator's `cluster-admin` privilege (external prerequisite) allows it to create SAs and Roles with arbitrary content without escalation issues. +> **Resolved: per-run SA isolation.** Each AgenticRun gets separate analysis and execution ServiceAccounts in the operator namespace. Analysis is read-only and namespace-scoped; execution receives only approved remediation RBAC. Both identities are explicitly deleted when their phase ends. Verification and escalation continue using the shared `lightspeed-agent` SA. ## Agent RBAC per phase -The sandbox SA is `lightspeed-agent` (from `SandboxTemplate.spec.podTemplate.spec.serviceAccountName`). +The sandbox ServiceAccount is selected per phase: | Phase | SA | Read access | Write access | Notes | |-------|-----|-------------|--------------|-------| -| Analysis | `lightspeed-agent` | Admin-created ClusterRole (pods, deployments, events, logs, etc.) | None | Agent inspects cluster to diagnose; no mutations | +| Analysis | `ls-analysis-{ns}-{name}-{uid-hash}` (per-run) | Built-in `view` plus `pods/log`, only in the AgenticRun namespace | None | `targetNamespaces` cannot widen access | | Execution | `ls-exec-{ns}-{name}` (per-run) | Inherited from bound Roles | `ls-exec-*` Roles (operator-created) | Agent mutates cluster per remediation plan; isolated SA per AgenticRun | | Verification | `lightspeed-agent` | Admin-created read access | None | Per-proposal SA deleted after execution; verification has read only | | Escalation | `lightspeed-agent` | Admin-created read access | None | Agent re-analyzes failure; no mutations | From 3734b5a02d464736a091d2fcc540e2deaa53e2d5 Mon Sep 17 00:00:00 2001 From: waveywaves <11972233+waveywaves@users.noreply.github.com> Date: Tue, 11 Aug 2026 23:22:01 +0530 Subject: [PATCH 2/2] Clarify phase identity contracts Signed-off-by: waveywaves <11972233+waveywaves@users.noreply.github.com> --- .ai/spec/what/sandbox-execution.md | 6 +++--- docs/rbac.md | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.ai/spec/what/sandbox-execution.md b/.ai/spec/what/sandbox-execution.md index 766a8f70..2b192437 100644 --- a/.ai/spec/what/sandbox-execution.md +++ b/.ai/spec/what/sandbox-execution.md @@ -13,7 +13,7 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 7. **[OLS-3066] Input delivery — ConfigMap**: For each step invocation, the operator MUST create a namespaced `ConfigMap` in the operator namespace with owner reference to the `AgenticRun` (`controller: true`, `blockOwnerDeletion: true`). Name pattern `ls-input-{step}-{run}` truncated to 63 chars. The ConfigMap MUST contain four keys: `query` (rendered prompt text from the step template), `output-schema` (JSON schema computed by `outputSchemaForStep`), `context` (JSON object with targetNamespaces, previousAttempts, approvedOption, executionResult as applicable), and `result-template` (pre-filled Result CR JSON — see rule 7a). The ConfigMap MUST be mounted read-only into the sandbox pod at `/input/`. Creation MUST be idempotent (`AlreadyExists` = no-op). 7a. **[OLS-3066] Result template**: The `result-template` key MUST contain a JSON object with complete `apiVersion`, `kind`, `metadata` (name, namespace, labels, ownerReferences including AgenticRun UID), and `spec` (agenticRunName, retryIndex for execution/verification). The operator pre-computes all metadata — the sandbox only fills in `status` fields. CR naming MUST use the existing `resultCRName` convention. Owner references MUST use the current AgenticRun UID. 8. **[OLS-3066] Output delivery — Result CR via `oc`**: The sandbox MUST create the Result CR in two steps: (a) `oc create -f ` using the pre-filled template with `spec` fields, then (b) `oc patch --type=merge --subresource=status` with the agent output in `status` fields (options, diagnosis, actionRequired, actionsTaken, checks, conditions, failureReason as applicable per step). The Result CR `status.conditions` MUST include a `Completed` condition set to `True` as part of the status patch — this is the operator's readiness signal (see rule 8b). On agent failure, the sandbox MUST still create the Result CR with `status.failureReason` populated and exit 0 (the sandbox succeeded; the agent failed). On sandbox failure (cannot read input, `oc create` fails, etc.), the sandbox MUST write an error message to `/dev/termination-log` (max 4096 bytes) and exit non-zero. -8a. **[OLS-3066] Sandbox RBAC for Result CRs**: The sandbox ServiceAccount (`lightspeed-agent` for analysis/verification/escalation, per-run SA for execution) MUST have `create` and `patch` (with `status` subresource) permissions on `AnalysisResult`, `ExecutionResult`, `VerificationResult`, and `EscalationResult` in the AgenticRun namespace. +8a. **[OLS-3066] Sandbox RBAC for Result CRs**: Verification, escalation, and execution sandbox identities MUST have the narrow Result permissions required by their phases in the AgenticRun namespace. The per-run analysis identity MUST NOT receive Result write permission. Before asynchronous analysis is enabled, its Result delivery MUST use a separately enforced expected-result-name publisher path as required by rule 20a. 8b. **[OLS-3066] Result CR readiness signal**: The operator MUST only process a Result CR when its `status.conditions` includes `Completed=True`. A Result CR without this condition indicates the sandbox has called `oc create` but has not yet patched the status — the operator MUST wait for the status update (which triggers another `Owns()` watch event). This guards against the race between `oc create` and `oc patch --subresource=status`. 9. **[OLS-3066] Watch-driven async**: The controller MUST use watch-based event delivery instead of synchronous polling. `SetupWithManager` MUST `Owns()` Pods (bare-pod mode), SandboxClaims (sandbox-claim mode), ConfigMaps, and all Result CR types (AnalysisResult, ExecutionResult, VerificationResult, EscalationResult). Pod watches are for **failure detection only** (Pod `Failed`, `ImagePullBackOff`). Result CR watches are for **completion detection** (Result CR created with `Completed` condition). Every in-progress step MUST return `RequeueAfter(30s)` as a safety net for missed watch events. 10. **Output schema selection**: The `output-schema` key in the input ConfigMap MUST be the step-specific JSON schema computed by the operator: analysis schema depends on `spec.analysisOutput.mode`, whether execution/verification steps exist in the run, and optional injected `components` sub-schema from `spec.analysisOutput.schema`; other steps use fixed schemas for their response shapes. @@ -97,9 +97,9 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 34. **[OLS-3066] No endpoint construction**: With the batch execution model, the operator does not construct agent HTTP URLs. There is no HTTP communication between operator and sandbox. Input is delivered via ConfigMap mount (rule 7); output is delivered via Result CR creation (rule 8). 35. **Sandbox release**: In `bare-pod` mode, `Release` deletes the Pod by name. In `sandbox-claim` mode, `Release` deletes both the SandboxClaim and its SandboxTemplate (same name). Both paths are idempotent — NotFound is treated as success. 36. **PodSpecBuilder**: `PodSpecBuilder` takes a base `*corev1.PodSpec` (from the config cache) and overlays agent-specific configuration: LLM env vars, credential mounts, skills volumes, MCP config, required secrets, input ConfigMap volume mount [OLS-3066], SA token mounting, security context. It produces a single typed `corev1.PodSpec`. In `bare-pod` mode, this PodSpec is used directly to create a Pod. In `sandbox-claim` mode, it is converted to an unstructured map and embedded in a `SandboxTemplate`. [OLS-3066] HTTP readiness/liveness probes are no longer set (see rule 30). -36a. **SA token mounting**: `PodSpecBuilder` MUST set `AutomountServiceAccountToken=true` for **all** steps. The correct ServiceAccount is already resolved upstream (per-run SA for analysis and execution, shared `lightspeed-agent` for verification/escalation), and the pod needs the projected token to authenticate `oc`/`kubectl` commands against the cluster API regardless of step. +36a. **SA token mounting**: `PodSpecBuilder` MUST set `AutomountServiceAccountToken=true` for **all** steps. The correct ServiceAccount is already resolved upstream: analysis uses its per-run SA; execution uses a per-run SA only when requested RBAC rules are materialized and otherwise uses `lightspeed-agent`; verification and escalation use `lightspeed-agent`. The pod needs the projected token to authenticate `oc`/`kubectl` commands against the cluster API regardless of step. 37. **RBAC**: The operator's ClusterRole MUST include Pod create/delete/get/list/watch verbs (bare-pod mode) AND SandboxClaim, SandboxTemplate, Sandbox resource verbs (sandbox-claim mode). -38. **Bootstrap**: The operator MUST unconditionally create the `lightspeed-agent` ServiceAccount in the operator namespace at startup (idempotent). This SA is used by verification/escalation steps — analysis and execution use separate per-run SAs per rules 20a and 21. The `lightspeed-agent` SA MUST NOT have any execution-level Roles or ClusterRoles bound to it. No base `SandboxTemplate` is created at bootstrap — templates are built per-run by `SandboxManager`. +38. **Bootstrap**: The operator MUST unconditionally create the `lightspeed-agent` ServiceAccount in the operator namespace at startup (idempotent). This SA is used by verification and escalation, plus execution when no requested RBAC rules require a per-run SA. Analysis always uses its per-run SA; execution with requested RBAC uses a separate per-run SA per rules 20a and 21. The `lightspeed-agent` SA MUST NOT have any execution-level Roles or ClusterRoles bound to it. No base `SandboxTemplate` is created at bootstrap — templates are built per-run by `SandboxManager`. ## Configuration Surface diff --git a/docs/rbac.md b/docs/rbac.md index c18767ed..977f706a 100644 --- a/docs/rbac.md +++ b/docs/rbac.md @@ -108,11 +108,11 @@ subjects: ## Dynamic analysis identity (per-AgenticRun) -Before analysis, the operator creates a UID-qualified ServiceAccount in its own namespace and binds it to the built-in `view` ClusterRole only in the AgenticRun namespace. A supplemental Role grants only `pods/log` get. The identity has no ClusterRoleBinding, Secret access, Result write permission, or access to other entries in `spec.targetNamespaces`. It is deleted after analysis and by terminal/finalizer cleanup. +Before analysis, the operator creates a UID-qualified ServiceAccount in its own namespace and binds it to the built-in `view` ClusterRole only in the AgenticRun namespace. A supplemental Role grants only `pods/log` get. The identity has no ClusterRoleBinding, Secret access, Result write permission, or access to other entries in `spec.targetNamespaces`. It remains while analysis is active and is deleted after analysis, on suspension, and by terminal/finalizer cleanup. Transient cleanup failures requeue reconciliation. ## Dynamic execution RBAC (per-AgenticRun) -Created by the operator during the execution phase, deleted on terminal state or AgenticRun deletion. +Created by the operator during execution only when approved RBAC is requested, then deleted on terminal state or AgenticRun deletion. | Resource | Name pattern | Scope | Content | |----------|-------------|-------|---------| @@ -127,7 +127,7 @@ Lifecycle: - **Created**: just before execution agent call (`ensureExecutionRBAC`) - **Deleted**: immediately after execution completes (before verification starts). Retries on failure via requeue. Also cleaned up on AgenticRun deletion (finalizer), escalation, or system failure as fallback. -> **Resolved: per-run SA isolation.** Each AgenticRun gets separate analysis and execution ServiceAccounts in the operator namespace. Analysis is read-only and namespace-scoped; execution receives only approved remediation RBAC. Both identities are explicitly deleted when their phase ends. Verification and escalation continue using the shared `lightspeed-agent` SA. +> **Resolved: per-run SA isolation.** Each AgenticRun gets a per-run analysis ServiceAccount. Execution gets a separate per-run ServiceAccount only when approved remediation RBAC is requested; otherwise it uses the shared `lightspeed-agent` identity. Analysis is read-only and namespace-scoped, while privileged execution receives only approved remediation RBAC. Per-run identities are explicitly deleted when their phases end. Verification and escalation continue using `lightspeed-agent`. ## Agent RBAC per phase @@ -136,7 +136,7 @@ The sandbox ServiceAccount is selected per phase: | Phase | SA | Read access | Write access | Notes | |-------|-----|-------------|--------------|-------| | Analysis | `ls-analysis-{ns}-{name}-{uid-hash}` (per-run) | Built-in `view` plus `pods/log`, only in the AgenticRun namespace | None | `targetNamespaces` cannot widen access | -| Execution | `ls-exec-{ns}-{name}` (per-run) | Inherited from bound Roles | `ls-exec-*` Roles (operator-created) | Agent mutates cluster per remediation plan; isolated SA per AgenticRun | +| Execution | `ls-exec-{ns}-{name}` when RBAC is requested; otherwise `lightspeed-agent` | Inherited from bound Roles or shared read access | `ls-exec-*` Roles only when requested | Privileged execution is isolated per AgenticRun | | Verification | `lightspeed-agent` | Admin-created read access | None | Per-proposal SA deleted after execution; verification has read only | | Escalation | `lightspeed-agent` | Admin-created read access | None | Agent re-analyzes failure; no mutations |