Access Control

PoC (repro steps) to demonstrate the vulnerability prior to the fix and how the patch resolves it: 1) Prerequisites - Grafana OpenID/RBAC setup with a resource directly granted to a user (e.g., a dashboard) via a role. - Enable the query cache in the authorization service (CheckQueryCacheEnabled = true). - Have a user context that has direct access to a resource but will rely on BatchCheck for authorization decisions. 2) Reproduction steps (pre-fix behavior) - Call List to populate the ListObjectsResponse cache (which includes the full idents like folder:... or resource:...). - The List response path strips the type prefix in place (mutating the slice owned by the cache). With the fix absent, the cached full idents are downgraded to bare ids in the cache. - Call BatchCheck with an identical ListObjects request. The BatchCheck reads the mutated cache (full idents missing the prefix), leading to a mismatch in membership checks (e.g., comparing full idents against cached bare ids). - Expected: The user should have access (allowed for a directly granted resource) but BatchCheck returns not allowed due to the cache corruption. 3) Reproduction steps (post-fix behavior) - The strip helpers now return a new slice instead of mutating input, so the cached ListObjectsResponse is never mutated. - The BatchCheck reads the canonical full idents from the cache and performs membership checks correctly. - Result: The user retains access as expected, and there is no cache-poisoning effect. 4) Minimal Go-style PoC outline (adaptable to the project’s test harness) package main // Pseudo-test harness illustrating the repro flow. func PoC() { // Setup server with cache enabled server := setupOpenFGAServer(nil) setup(nil, server) server.cfg.CacheSettings.CheckQueryCacheEnabled = true ctx := newContextWithNamespace() // 1) List populates the cache _, err := server.List(ctx, &authzv1.ListRequest{ Namespace: namespace, Verb: utils.VerbList, Subject: "user:1", Group: dashboardGroup, Resource: dashboardResource, }) if err != nil { panic(err) } // 2) BatchCheck checks the same resource; should be allowed if user has direct grant res, err := server.BatchCheck(ctx, &authzv1.BatchCheckRequest{ Namespace: namespace, Subject: "user:1", Checks: []*authzv1.BatchCheckItem{{CorrelationId: "c", Verb: utils.VerbGet, Group: dashboardGroup, Resource: dashboardResource, Name: "1", Folder: ""}}, }) if err != nil { panic(err) } if !res.GetResults()["c"].GetAllowed() { panic("expected allowed after List, BatchCheck indicates not allowed due to cache mutation (PoC failed)") } } Note: The repository includes tests that demonstrate the issue and the fix, namely TestStripHelpersDoNotMutateInput and TestIntegrationListDoesNotPoisonBatchCheckCache, which serve as concrete PoC-style validation of the vulnerability and its remediation.

diff --git a/pkg/services/authz/zanzana/server/server_list.go b/pkg/services/authz/zanzana/server/server_list.go index a04957d9763f5..910bbeeb450c3 100644 --- a/pkg/services/authz/zanzana/server/server_list.go +++ b/pkg/services/authz/zanzana/server/server_list.go @@ -368,23 +368,26 @@ func getRequestHash(req *openfgav1.ListObjectsRequest) (string, error) { func typedObjects(typ string, objects []string) []string { prefix := typ + ":" + out := make([]string, len(objects)) for i := range objects { - objects[i] = strings.TrimPrefix(objects[i], prefix) + out[i] = strings.TrimPrefix(objects[i], prefix) } - return objects + return out } func genericObjects(gr string, objects []string) []string { prefix := common.TypeResourcePrefix + gr + "/" + out := make([]string, len(objects)) for i := range objects { - objects[i] = strings.TrimPrefix(objects[i], prefix) + out[i] = strings.TrimPrefix(objects[i], prefix) } - return objects + return out } func folderObject(objects []string) []string { + out := make([]string, len(objects)) for i := range objects { - objects[i] = strings.TrimPrefix(objects[i], common.TypeFolderPrefix) + out[i] = strings.TrimPrefix(objects[i], common.TypeFolderPrefix) } - return objects + return out } diff --git a/pkg/services/authz/zanzana/server/server_list_test.go b/pkg/services/authz/zanzana/server/server_list_test.go index 0b2e243959959..727372643d39d 100644 --- a/pkg/services/authz/zanzana/server/server_list_test.go +++ b/pkg/services/authz/zanzana/server/server_list_test.go @@ -444,3 +444,59 @@ func TestIntegrationServerListStreamDeadline(t *testing.T) { require.Error(t, err) }) } + +// TestStripHelpersDoNotMutateInput guards the root cause of a cache-poisoning bug: the List +// response strip helpers must return a new slice and never mutate their input, which may be the +// *openfgav1.ListObjectsResponse.Objects slice owned by the query cache. Mutating it in place +// corrupted the cached full object idents that BatchCheck relies on. +func TestStripHelpersDoNotMutateInput(t *testing.T) { + t.Run("typedObjects", func(t *testing.T) { + in := []string{"folder:abc", "folder:def"} + out := typedObjects("folder", in) + require.Equal(t, []string{"folder:abc", "folder:def"}, in, "input must not be mutated") + require.Equal(t, []string{"abc", "def"}, out) + }) + t.Run("genericObjects", func(t *testing.T) { + in := []string{"resource:dashboard.grafana.app/dashboards/x"} + out := genericObjects("dashboard.grafana.app/dashboards", in) + require.Equal(t, []string{"resource:dashboard.grafana.app/dashboards/x"}, in, "input must not be mutated") + require.Equal(t, []string{"x"}, out) + }) + t.Run("folderObject", func(t *testing.T) { + in := []string{"folder:abc", "folder:def"} + out := folderObject(in) + require.Equal(t, []string{"folder:abc", "folder:def"}, in, "input must not be mutated") + require.Equal(t, []string{"abc", "def"}, out) + }) +} + +// TestIntegrationListDoesNotPoisonBatchCheckCache reproduces the end-to-end bug: with the query +// cache enabled, a List call must not corrupt the cached ListObjects response that a subsequent +// BatchCheck (identical request) reads, so a directly-granted resource stays authorized. +func TestIntegrationListDoesNotPoisonBatchCheckCache(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + server := setupOpenFGAServer(t) + setup(t, server) + server.cfg.CacheSettings.CheckQueryCacheEnabled = true + + ctx := newContextWithNamespace() + + // 1. List populates the ListObjects query cache and strips idents for its own response. + _, err := server.List(ctx, &authzv1.ListRequest{ + Namespace: namespace, Verb: utils.VerbList, Subject: "user:1", + Group: dashboardGroup, Resource: dashboardResource, + }) + require.NoError(t, err) + + // 2. BatchCheck issues the identical ListObjects request (cache hit) and must still resolve + // user:1's direct grant on dashboard "1". + res, err := server.BatchCheck(ctx, &authzv1.BatchCheckRequest{ + Namespace: namespace, Subject: "user:1", + Checks: []*authzv1.BatchCheckItem{ + {CorrelationId: "c", Verb: utils.VerbGet, Group: dashboardGroup, Resource: dashboardResource, Name: "1", Folder: ""}, + }, + }) + require.NoError(t, err) + assert.True(t, res.GetResults()["c"].GetAllowed(), "directly-granted dashboard must stay authorized after a prior List") +}