From afc30a3cf2bef8da21612bd642d138e7b16e6876 Mon Sep 17 00:00:00 2001 From: Michael Costello Date: Sun, 4 Oct 2026 18:14:26 -0400 Subject: [PATCH] fix: Register conversation subresource permissions Milo's OpenFGA provider now authorizes subresources on their own, with no fallback to the parent resource's permissions. Conversations serve two subresources and declared neither, so every transcript load and every turn sent through the conversations apiserver failed as "not registered". A read-only production probe confirmed it for conversations/messages.get. Key changes: - declare conversations/messages (get) and conversations/sendmessage (create) on the Conversation ProtectedResource; these are the verbs the stock RequestInfoFactory resolves for the only methods served, GET and POST - grant messages.get in conversation-viewer beside conversations.get, and sendmessage.create in conversation-creator beside conversations.create; the user role inherits both - extract the v1alpha1 storage map into v1alpha1Storage so tests can read it - add iam_test.go: it probes every method on every subresource in the storage map through the assembled server (only 405 counts as unserved), resolves each with the stock RequestInfoFactory, and requires the permission to be declared and granted by every role that grants the base verb; a second test requires every role permission to be declared Without the fix the test fails for both subresources (undeclared, ungranted); with it, both pass. Granting an undeclared permission fails the second test. Hazard: with subresource authorization off, the provider marks any role holding a subresource permission invalid as a whole, so these roles would grant nothing on a Milo running without the flag. Fixes #115 --- config/milo/iam/resources/conversations.yaml | 8 + .../milo/iam/roles/conversation-creator.yaml | 7 +- .../milo/iam/roles/conversation-viewer.yaml | 1 + internal/apiserver/apiserver.go | 21 +- internal/apiserver/iam_test.go | 184 ++++++++++++++++++ 5 files changed, 209 insertions(+), 12 deletions(-) create mode 100644 internal/apiserver/iam_test.go diff --git a/config/milo/iam/resources/conversations.yaml b/config/milo/iam/resources/conversations.yaml index cde44ec..49ec1ba 100644 --- a/config/milo/iam/resources/conversations.yaml +++ b/config/milo/iam/resources/conversations.yaml @@ -35,6 +35,14 @@ spec: - update - patch - delete + # Checked with no fallback to the base permissions above. + subresources: + - name: messages + permissions: + - get + - name: sendmessage + permissions: + - create # Conversations are scoped to a project: the SAR carries the project name as # the request namespace (internal/tenant treats the namespace as # authoritative). Organization is listed so an org-level grant is inherited diff --git a/config/milo/iam/roles/conversation-creator.yaml b/config/milo/iam/roles/conversation-creator.yaml index 3c14d8e..ede214f 100644 --- a/config/milo/iam/roles/conversation-creator.yaml +++ b/config/milo/iam/roles/conversation-creator.yaml @@ -1,6 +1,6 @@ -# The grant that makes Patch usable. This single permission is what the -# assistant's SubjectAccessReview asks for on every request; a subject without -# it gets a 403 before the agent loop is ever entered. +# The grant that makes Patch usable. These permissions are what A2A and +# sendmessage requests ask for; a subject without them gets a 403 before the +# agent loop is ever entered. apiVersion: iam.miloapis.com/v1alpha1 kind: Role metadata: @@ -13,3 +13,4 @@ spec: launchStage: Beta includedPermissions: - assistant.miloapis.com/conversations.create + - assistant.miloapis.com/conversations/sendmessage.create diff --git a/config/milo/iam/roles/conversation-viewer.yaml b/config/milo/iam/roles/conversation-viewer.yaml index b916cec..f3fc22a 100644 --- a/config/milo/iam/roles/conversation-viewer.yaml +++ b/config/milo/iam/roles/conversation-viewer.yaml @@ -23,6 +23,7 @@ spec: includedPermissions: - assistant.miloapis.com/conversations.get - assistant.miloapis.com/conversations.list + - assistant.miloapis.com/conversations/messages.get - assistant.miloapis.com/capabilitygapreports.list - assistant.miloapis.com/capabilitygaps.list - assistant.miloapis.com/assistantendpoints.get diff --git a/internal/apiserver/apiserver.go b/internal/apiserver/apiserver.go index 5f05102..3384306 100644 --- a/internal/apiserver/apiserver.go +++ b/internal/apiserver/apiserver.go @@ -113,15 +113,7 @@ func (c completedConfig) New() (*ConversationServer, error) { apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(v1alpha1.GroupName, Scheme, metav1.ParameterCodec, Codecs) - v1alpha1Storage := map[string]rest.Storage{ - "conversations": conversation.NewConversationREST(c.ExtraConfig.Conversations), - "conversations/messages": conversation.NewMessagesREST(c.ExtraConfig.Conversations), - "conversations/sendmessage": conversation.NewSendMessageREST(c.ExtraConfig.Runner), - "capabilitygapreports": capabilitygapreport.NewCapabilityGapReportREST(c.ExtraConfig.GapReports), - "capabilitygaps": capabilitygapreport.NewCapabilityGapREST(c.ExtraConfig.GapReports), - "assistantendpoints": endpoint.NewAssistantEndpointREST(c.ExtraConfig.PublicBaseURL), - } - apiGroupInfo.VersionedResourcesStorageMap["v1alpha1"] = v1alpha1Storage + apiGroupInfo.VersionedResourcesStorageMap["v1alpha1"] = v1alpha1Storage(c.ExtraConfig) if err := s.GenericAPIServer.InstallAPIGroup(&apiGroupInfo); err != nil { return nil, err @@ -130,3 +122,14 @@ func (c completedConfig) New() (*ConversationServer, error) { klog.Info("conversations apiserver initialized") return s, nil } + +func v1alpha1Storage(extra *ExtraConfig) map[string]rest.Storage { + return map[string]rest.Storage{ + "conversations": conversation.NewConversationREST(extra.Conversations), + "conversations/messages": conversation.NewMessagesREST(extra.Conversations), + "conversations/sendmessage": conversation.NewSendMessageREST(extra.Runner), + "capabilitygapreports": capabilitygapreport.NewCapabilityGapReportREST(extra.GapReports), + "capabilitygaps": capabilitygapreport.NewCapabilityGapREST(extra.GapReports), + "assistantendpoints": endpoint.NewAssistantEndpointREST(extra.PublicBaseURL), + } +} diff --git a/internal/apiserver/iam_test.go b/internal/apiserver/iam_test.go new file mode 100644 index 0000000..a77e8d2 --- /dev/null +++ b/internal/apiserver/iam_test.go @@ -0,0 +1,184 @@ +package apiserver + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apiserver/pkg/endpoints/request" + "sigs.k8s.io/yaml" + + "github.com/milo-os/assistant/pkg/apis/assistant/v1alpha1" +) + +const iamDir = "../../config/milo/iam" + +type protectedResource struct { + Spec struct { + ServiceRef struct { + Name string `json:"name"` + } `json:"serviceRef"` + Plural string `json:"plural"` + Permissions []string `json:"permissions"` + Subresources []struct { + Name string `json:"name"` + Permissions []string `json:"permissions"` + } `json:"subresources"` + } `json:"spec"` +} + +type role struct { + Metadata struct { + Name string `json:"name"` + } `json:"metadata"` + Spec struct { + IncludedPermissions []string `json:"includedPermissions"` + } `json:"spec"` +} + +func readYAML(t *testing.T, path string, into any) { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + if err := yaml.Unmarshal(data, into); err != nil { + t.Fatalf("parse %s: %v", path, err) + } +} + +func globYAML(t *testing.T, dir string) []string { + t.Helper() + files, err := filepath.Glob(filepath.Join(iamDir, dir, "*.yaml")) + if err != nil { + t.Fatal(err) + } + var out []string + for _, f := range files { + if filepath.Base(f) != "kustomization.yaml" { + out = append(out, f) + } + } + if len(out) == 0 { + t.Fatalf("no manifests under %s/%s", iamDir, dir) + } + return out +} + +func declaredPermissions(t *testing.T) sets.Set[string] { + t.Helper() + declared := sets.New[string]() + for _, f := range globYAML(t, "resources") { + var pr protectedResource + readYAML(t, f, &pr) + base := pr.Spec.ServiceRef.Name + "/" + pr.Spec.Plural + for _, verb := range pr.Spec.Permissions { + declared.Insert(base + "." + verb) + } + for _, sub := range pr.Spec.Subresources { + for _, verb := range sub.Permissions { + declared.Insert(base + "/" + sub.Name + "." + verb) + } + } + } + return declared +} + +func roles(t *testing.T) []role { + t.Helper() + var out []role + for _, f := range globYAML(t, "roles") { + var r role + readYAML(t, f, &r) + out = append(out, r) + } + return out +} + +func servedSubresourceRequests(t *testing.T) []*http.Request { + t.Helper() + srv, _ := newTestAPI(t) + var subresources []string + for path := range v1alpha1Storage(&ExtraConfig{}) { + if strings.Contains(path, "/") { + subresources = append(subresources, path) + } + } + if len(subresources) == 0 { + t.Fatal("the storage map serves no subresources") + } + sort.Strings(subresources) + + var served []*http.Request + for _, path := range subresources { + resource, sub, _ := strings.Cut(path, "/") + url := "/apis/" + v1alpha1.GroupName + "/v1alpha1/namespaces/demo/" + resource + "/ctx-a/" + sub + routed := 0 + for _, method := range []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} { + // Only 405 means unrouted; any other answer reached the storage. + if code, _ := do(t, srv, method, url, "application/json", "{}"); code == http.StatusMethodNotAllowed { + continue + } + served = append(served, httptest.NewRequest(method, url, nil)) + routed++ + } + if routed == 0 { + t.Fatalf("%s is in the storage map but no method reaches it", path) + } + } + return served +} + +// A subresource permission belongs in every role granting the base verb. +func TestSubresourcePermissionsAreDeclaredAndGranted(t *testing.T) { + declared := declaredPermissions(t) + allRoles := roles(t) + resolver := &request.RequestInfoFactory{ + APIPrefixes: sets.NewString("api", "apis"), + GrouplessAPIPrefixes: sets.NewString("api"), + } + + for _, req := range servedSubresourceRequests(t) { + t.Run(req.Method+" "+req.URL.Path, func(t *testing.T) { + info, err := resolver.NewRequestInfo(req) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if info.Subresource == "" { + t.Fatalf("resolved %+v, want a subresource", info) + } + base := info.APIGroup + "/" + info.Resource + "." + info.Verb + permission := info.APIGroup + "/" + info.Resource + "/" + info.Subresource + "." + info.Verb + if !declared.Has(permission) { + t.Errorf("Milo asks for %s, which no ProtectedResource declares", permission) + } + granted := false + for _, r := range allRoles { + has := sets.New(r.Spec.IncludedPermissions...) + if has.Has(base) && !has.Has(permission) { + t.Errorf("%s grants %s but not %s", r.Metadata.Name, base, permission) + } + granted = granted || has.Has(permission) + } + if !granted { + t.Errorf("no role grants %s", permission) + } + }) + } +} + +func TestRolesGrantOnlyDeclaredPermissions(t *testing.T) { + declared := declaredPermissions(t) + for _, r := range roles(t) { + for _, permission := range r.Spec.IncludedPermissions { + if !declared.Has(permission) { + t.Errorf("%s grants %s, which no ProtectedResource declares", r.Metadata.Name, permission) + } + } + } +}