From 565c21c968a1efc79d50541809636114cd816c59 Mon Sep 17 00:00:00 2001 From: Cody Hartsook Date: Thu, 17 Sep 2026 11:21:31 -0700 Subject: [PATCH] refactor(core): replace update helpers with CRUD verbs GetForUpdate implied locking that never happened, while SaveUpdate relied on every caller pairing it with an earlier authorization check. Replace them with the canonical Get and Update operations, with Update authorizing the object identity it writes. ModelConfig keeps its owned-Secret workflow local and preauthorizes update before any side effect, then performs the separately authorized read. Document the operation ordering and cover denied writes without retaining the unrelated matcher and Testify churn. Signed-off-by: Cody Hartsook --- design/EP-1270-scoped-authorization.md | 8 ++- go/core/internal/grpcserver/agenttemplate.go | 4 +- go/core/internal/service/kubecrud/service.go | 40 ++++++------- .../internal/service/kubecrud/service_test.go | 45 ++++++++++---- go/core/internal/service/model/service.go | 7 ++- .../internal/service/model/service_test.go | 60 ++++++++++++------- 6 files changed, 99 insertions(+), 65 deletions(-) diff --git a/design/EP-1270-scoped-authorization.md b/design/EP-1270-scoped-authorization.md index f9fef279a8..8ce1cf7754 100644 --- a/design/EP-1270-scoped-authorization.md +++ b/design/EP-1270-scoped-authorization.md @@ -48,11 +48,13 @@ Unsupported or invalid authorization decisions fail closed. ## Resource identity -Authorization uses identity derived from stored or validated resource data. A request reference identifies what to load; it is not trusted evidence about the resource itself. +Authorization uses namespace and name, the only attributes in scope. Each decision is made before the operation it authorizes. -- Reads and deletes are decided from the stored resource. +- Reads and deletes are decided from their references before the read, so denial does not reveal whether the resource exists. - Creates are decided from the validated proposed resource. -- Updates are decided from the stored resource and preserve its namespace and name. +- Updates authorize get before the read and update before the write. Generic updates use the identity of the loaded resource. + +A service that writes through its own client authorizes update from the request reference before its first side effect. ## Collection behavior diff --git a/go/core/internal/grpcserver/agenttemplate.go b/go/core/internal/grpcserver/agenttemplate.go index 807508edd3..e17ad642df 100644 --- a/go/core/internal/grpcserver/agenttemplate.go +++ b/go/core/internal/grpcserver/agenttemplate.go @@ -82,13 +82,13 @@ func (s *agentTemplateServer) UpdateAgentTemplate(ctx context.Context, request * if err := s.decodeResource(request.GetRef(), request.GetResource(), incoming); err != nil { return nil, err } - existing, err := s.service.GetForUpdate(ctx, ref) + existing, err := s.service.Get(ctx, ref) if err != nil { return nil, err } existing.Spec = *incoming.Spec.DeepCopy() existing.Labels = maps.Clone(incoming.Labels) - result, err := s.service.SaveUpdate(ctx, existing) + result, err := s.service.Update(ctx, existing) if err != nil { return nil, err } diff --git a/go/core/internal/service/kubecrud/service.go b/go/core/internal/service/kubecrud/service.go index be2e010df6..8a3aac0732 100644 --- a/go/core/internal/service/kubecrud/service.go +++ b/go/core/internal/service/kubecrud/service.go @@ -78,10 +78,7 @@ func (s *Service[T, L]) List(ctx context.Context, namespace string) ([]T, error) func (s *Service[T, L]) Get(ctx context.Context, ref types.NamespacedName) (T, error) { var zero T - if err := s.validateRef(ref); err != nil { - return zero, err - } - if err := s.authorize(ctx, auth.VerbGet, ref); err != nil { + if err := s.Authorize(ctx, auth.VerbGet, ref); err != nil { return zero, err } return s.get(ctx, ref) @@ -113,21 +110,16 @@ func (s *Service[T, L]) Create(ctx context.Context, object T) (T, error) { return object, nil } -// GetForUpdate authorizes an update and loads the live object that owns metadata and status. -func (s *Service[T, L]) GetForUpdate(ctx context.Context, ref types.NamespacedName) (T, error) { +// Update persists an object read from the cluster, authorized by the identity it is about to write. +func (s *Service[T, L]) Update(ctx context.Context, object T) (T, error) { var zero T - if err := s.validateRef(ref); err != nil { - return zero, err + if object == zero { + return zero, serviceerrors.NewInvalidArgument(s.resource+" resource is required", nil) } - if err := s.authorize(ctx, auth.VerbUpdate, ref); err != nil { + ref := types.NamespacedName{Namespace: object.GetNamespace(), Name: object.GetName()} + if err := s.Authorize(ctx, auth.VerbUpdate, ref); err != nil { return zero, err } - return s.get(ctx, ref) -} - -// SaveUpdate persists an object returned by GetForUpdate after its spec is changed. -func (s *Service[T, L]) SaveUpdate(ctx context.Context, object T) (T, error) { - var zero T if err := s.client.Update(ctx, object); err != nil { if apierrors.IsInvalid(err) { return zero, serviceerrors.NewInvalidArgument("Invalid "+s.resource, err) @@ -138,10 +130,7 @@ func (s *Service[T, L]) SaveUpdate(ctx context.Context, object T) (T, error) { } func (s *Service[T, L]) Delete(ctx context.Context, ref types.NamespacedName) error { - if err := s.validateRef(ref); err != nil { - return err - } - if err := s.authorize(ctx, auth.VerbDelete, ref); err != nil { + if err := s.Authorize(ctx, auth.VerbDelete, ref); err != nil { return err } object, err := s.get(ctx, ref) @@ -178,7 +167,14 @@ func (s *Service[T, L]) get(ctx context.Context, ref types.NamespacedName) (T, e return object, nil } -// authorize decides a single operation before any read, so a denial never depends on the object existing. +// Authorize validates a reference and checks one operation without accessing Kubernetes. +func (s *Service[T, L]) Authorize(ctx context.Context, verb auth.Verb, ref types.NamespacedName) error { + if err := s.validateRef(ref); err != nil { + return err + } + return s.authorize(ctx, verb, ref) +} + func (s *Service[T, L]) authorize(ctx context.Context, verb auth.Verb, ref types.NamespacedName) error { session, ok := auth.AuthSessionFrom(ctx) if !ok || session == nil { @@ -198,10 +194,8 @@ func (s *Service[T, L]) validateRef(ref types.NamespacedName) error { return nil } +// validateNewRef rejects a ref Kubernetes would not accept; an empty value is not a valid subdomain either. func (s *Service[T, L]) validateNewRef(ref types.NamespacedName) error { - if err := s.validateRef(ref); err != nil { - return err - } if len(utilvalidation.IsDNS1123Subdomain(ref.Namespace)) > 0 { return serviceerrors.NewInvalidArgument("namespace must be a valid DNS subdomain", nil) } diff --git a/go/core/internal/service/kubecrud/service_test.go b/go/core/internal/service/kubecrud/service_test.go index ece8cb0fe5..e5b6668c6a 100644 --- a/go/core/internal/service/kubecrud/service_test.go +++ b/go/core/internal/service/kubecrud/service_test.go @@ -88,20 +88,20 @@ func TestServiceFiltersBeforeSortingAndUsesTrustedAttributes(t *testing.T) { t.Fatalf("Create() error = %v", err) } mutableRef := types.NamespacedName{Namespace: "team", Name: "mutable"} - mutable, err := service.GetForUpdate(ctx, mutableRef) + mutable, err := service.Get(ctx, mutableRef) if err != nil { - t.Fatalf("GetForUpdate() error = %v", err) + t.Fatalf("Get() error = %v", err) } mutable.Spec.Description = "updated" - if _, err := service.SaveUpdate(ctx, mutable); err != nil { - t.Fatalf("SaveUpdate() error = %v", err) + if _, err := service.Update(ctx, mutable); err != nil { + t.Fatalf("Update() error = %v", err) } if err := service.Delete(ctx, types.NamespacedName{Namespace: "team", Name: "b"}); err != nil { t.Fatalf("Delete() error = %v", err) } - wantVerbs := []auth.Verb{auth.VerbGet, auth.VerbCreate, auth.VerbUpdate, auth.VerbDelete} - wantNames := []string{"a", "created", "mutable", "b"} + wantVerbs := []auth.Verb{auth.VerbGet, auth.VerbCreate, auth.VerbGet, auth.VerbUpdate, auth.VerbDelete} + wantNames := []string{"a", "created", "mutable", "mutable", "b"} if len(authorizer.checkCalls) != len(wantVerbs) { t.Fatalf("Check() calls = %d, want %d", len(authorizer.checkCalls), len(wantVerbs)) } @@ -171,10 +171,11 @@ func TestServiceRejectsInvalidScope(t *testing.T) { } } -// readRecordingClient counts the reads that reach Kubernetes. +// readRecordingClient counts the reads and updates that reach Kubernetes. type readRecordingClient struct { client.Client - gets int + gets int + updates int } func (c *readRecordingClient) Get(ctx context.Context, key client.ObjectKey, object client.Object, options ...client.GetOption) error { @@ -182,6 +183,11 @@ func (c *readRecordingClient) Get(ctx context.Context, key client.ObjectKey, obj return c.Client.Get(ctx, key, object, options...) } +func (c *readRecordingClient) Update(ctx context.Context, object client.Object, options ...client.UpdateOption) error { + c.updates++ + return c.Client.Update(ctx, object, options...) +} + // A denied caller must not be able to tell an existing object from a missing one. func TestDeniedSingleResourceOperationsDoNotRevealExistence(t *testing.T) { scheme := runtime.NewScheme() @@ -197,10 +203,6 @@ func TestDeniedSingleResourceOperationsDoNotRevealExistence(t *testing.T) { _, err := s.Get(ctx, ref) return err }, - "GetForUpdate": func(s *kubecrud.Service[*v1alpha3.AgentTemplate, *v1alpha3.AgentTemplateList], ctx context.Context, ref types.NamespacedName) error { - _, err := s.GetForUpdate(ctx, ref) - return err - }, "Delete": func(s *kubecrud.Service[*v1alpha3.AgentTemplate, *v1alpha3.AgentTemplateList], ctx context.Context, ref types.NamespacedName) error { return s.Delete(ctx, ref) }, @@ -227,6 +229,25 @@ func TestDeniedSingleResourceOperationsDoNotRevealExistence(t *testing.T) { } } +func TestDeniedUpdateDoesNotWrite(t *testing.T) { + scheme := runtime.NewScheme() + if err := v1alpha3.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + kubeClient := &readRecordingClient{Client: fake.NewClientBuilder().WithScheme(scheme).Build()} + authorizer := &recordingAuthorizer{checkErr: errors.New("denied")} + service := kubecrud.NewService(kubeClient, authorizer, &v1alpha3.AgentTemplate{}, &v1alpha3.AgentTemplateList{}, "AgentTemplate") + ctx := auth.AuthSessionTo(t.Context(), testSession{}) + + _, err := service.Update(ctx, &v1alpha3.AgentTemplate{ObjectMeta: metav1.ObjectMeta{Namespace: "team", Name: "existing"}}) + if !serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied) { + t.Fatalf("Update() error = %v, want permission denied", err) + } + if kubeClient.updates != 0 { + t.Fatalf("Update() denied the caller but wrote Kubernetes %d times", kubeClient.updates) + } +} + // A cluster-wide list must order same-named objects deterministically. func TestListOrdersAcrossNamespaces(t *testing.T) { scheme := runtime.NewScheme() diff --git a/go/core/internal/service/model/service.go b/go/core/internal/service/model/service.go index 47e93a9e82..f0fb00cfe9 100644 --- a/go/core/internal/service/model/service.go +++ b/go/core/internal/service/model/service.go @@ -145,9 +145,12 @@ func (s *Service) createOwnedSecrets(ctx context.Context, modelConfig *v1alpha3. } // Update keeps its own write because the owned Secrets must land between the -// authorized read and the retrying ModelConfig write. +// authorized read and the retrying ModelConfig write; the update is authorized first. func (s *Service) Update(ctx context.Context, request UpdateRequest) (*v1alpha3.ModelConfig, error) { - modelConfig, err := s.modelConfigs.GetForUpdate(ctx, request.Ref) + if err := s.modelConfigs.Authorize(ctx, auth.VerbUpdate, request.Ref); err != nil { + return nil, err + } + modelConfig, err := s.modelConfigs.Get(ctx, request.Ref) if err != nil { return nil, err } diff --git a/go/core/internal/service/model/service_test.go b/go/core/internal/service/model/service_test.go index 13c104939f..3d185a6a72 100644 --- a/go/core/internal/service/model/service_test.go +++ b/go/core/internal/service/model/service_test.go @@ -335,27 +335,6 @@ func TestServiceCRUDAndValidation(t *testing.T) { assert.Error(t, err) }) - t.Run("update permission denied before write", func(t *testing.T) { - config := &v1alpha3.ModelConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, - Spec: v1alpha3.ModelConfigSpec{Model: "original", Provider: v1alpha3.ModelProviderOpenAI}, - } - authorizer := &recordingAuthorizer{denyCheck: 1} - service, kubeClient, ctx := newService(authorizer, config) - - _, err := service.Update(ctx, model.UpdateRequest{ - Ref: types.NamespacedName{Namespace: "default", Name: "cfg"}, - Spec: v1alpha3.ModelConfigSpec{Model: "updated", Provider: v1alpha3.ModelProviderOpenAI}, - }) - require.Error(t, err) - assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied)) - require.Len(t, authorizer.checkCalls, 1) - - stored := &v1alpha3.ModelConfig{} - require.NoError(t, kubeClient.Get(ctx, ctrlclient.ObjectKey{Namespace: "default", Name: "cfg"}, stored)) - assert.Equal(t, "original", stored.Spec.Model) - }) - t.Run("denied collection is empty and denied item is rejected", func(t *testing.T) { service, _, ctx := newService(denyAuthorizer{}, &v1alpha3.ModelConfig{ ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, @@ -443,8 +422,8 @@ func TestModelConfigCRUDUsesTrustedAttributes(t *testing.T) { t.Fatalf("Delete() error = %v", err) } - wantVerbs := []pkgauth.Verb{pkgauth.VerbGet, pkgauth.VerbCreate, pkgauth.VerbUpdate, pkgauth.VerbDelete} - wantNames := []string{"existing", "created", "existing", "existing"} + wantVerbs := []pkgauth.Verb{pkgauth.VerbGet, pkgauth.VerbCreate, pkgauth.VerbUpdate, pkgauth.VerbGet, pkgauth.VerbDelete} + wantNames := []string{"existing", "created", "existing", "existing", "existing"} require.Len(t, authorizer.checkCalls, len(wantVerbs)) for index, call := range authorizer.checkCalls { assert.Equal(t, wantVerbs[index], call.verb) @@ -454,6 +433,41 @@ func TestModelConfigCRUDUsesTrustedAttributes(t *testing.T) { } } +// A caller who may read but not update must not have its API key written on the way to the denial. +func TestDeniedUpdateWritesNothing(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, v1alpha3.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + ref := types.NamespacedName{Namespace: "team", Name: "existing"} + kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&v1alpha3.ModelConfig{ + ObjectMeta: metav1.ObjectMeta{Namespace: ref.Namespace, Name: ref.Name}, + Spec: v1alpha3.ModelConfigSpec{Model: "old", Provider: v1alpha3.ModelProviderOpenAI}, + }).Build() + authorizer := &recordingAuthorizer{scope: apiauthorization.AuthorizationScope{Kind: apiauthorization.ScopeAll}, denyCheck: 1} + service := model.NewService(kubeClient, authorizer, "default") + ctx := pkgauth.AuthSessionTo(context.Background(), &authimpl.SimpleSession{P: pkgauth.Principal{User: pkgauth.User{ID: "test-user"}}}) + + apiKey := "api-key-value" + _, err := service.Update(ctx, model.UpdateRequest{ + Ref: ref, + APIKey: &apiKey, + Spec: v1alpha3.ModelConfigSpec{Model: "updated", Provider: v1alpha3.ModelProviderOpenAI}, + }) + require.Error(t, err) + assert.Truef(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied), "got %v", err) + + // The update decision is the only one reached: a read before it would record a get first. + assert.Equal(t, []authorizationCall{ + {verb: pkgauth.VerbUpdate, resource: pkgauth.Resource{Type: "ModelConfig", Namespace: ref.Namespace, Name: ref.Name}}, + }, authorizer.checkCalls) + + err = kubeClient.Get(ctx, ref, &corev1.Secret{}) + assert.True(t, apierrors.IsNotFound(err), "denied update wrote its API key secret: %v", err) + stored := &v1alpha3.ModelConfig{} + require.NoError(t, kubeClient.Get(ctx, ref, stored)) + assert.Equal(t, "old", stored.Spec.Model) +} + // A create that cannot finish its Secrets must not leave the ModelConfig behind. func TestCreateRollsBackWhenSecretWriteFails(t *testing.T) { scheme := runtime.NewScheme()