diff --git a/design/EP-1270-scoped-authorization.md b/design/EP-1270-scoped-authorization.md index f9fef279a..8ce1cf775 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 807508edd..e17ad642d 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 be2e010df..8a3aac073 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 ece8cb0fe..e5b6668c6 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 47e93a9e8..f0fb00cfe 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 13c104939..3d185a6a7 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()