Information disclosure / Privilege isolation

Proof-of-concept (demonstrates the vulnerability prior to this fix): Assume two tenants (namespaces) share the same OrgID (e.g., OrgID = 1). Namespace A: value = "default", internal-ID for UID "shared-uid" = 1 Namespace B: value = "stacks-2", internal-ID for UID "shared-uid" = 3 Both tenants have a Team resource with UID "shared-uid" but different internal IDs. Before this patch, if the UID->internal-ID cache and the singleflight key were keyed by OrgID (or otherwise shared across tenants), resolving the same UID from both namespaces could collide in the cache and/or singleflight group. The second namespace’s lookup could return the first namespace’s internal-ID (1) instead of its own (3), leaking tenant-specific mapping information and potentially allowing cross-tenant leakage or incorrect permission decisions during permission merges. Example sequence (conceptual Go-like steps): // Pseudo-setup for two tenants sharing OrgID 1 nsA := NamespaceInfo{Value: "default", OrgID: 1} nsB := NamespaceInfo{Value: "stacks-2", OrgID: 1} // Preload two distinct UIDs with the same UID in different namespaces r, _ := newCountingResolver(nil, 0, iamObject(teamGVR.GroupVersion().WithKind("Team"), nsA.Value, "shared-uid", 1), // nsA mapping: 1 iamObject(teamGVR.GroupVersion().WithKind("Team"), nsB.Value, "shared-uid", 3), // nsB mapping: 3 ) // Resolve in Namespace A idA, errA := r.GetTeamIDByUID(context.Background(), nsA, "shared-uid") // expect idA == 1 // Resolve in Namespace B idB, errB := r.GetTeamIDByUID(context.Background(), nsB, "shared-uid") // Expected (before fix): idB == 1 due to cross-tenant cache collision // Expected (after fix): idB == 3, since the cache is now per-namespace if idA == 1 && idB == 1 { // vulnerability observed: cross-tenant leakage of internal-ID mapping }

diff --git a/pkg/services/accesscontrol/acimpl/uid_resolver_test.go b/pkg/services/accesscontrol/acimpl/uid_resolver_test.go new file mode 100644 index 0000000000000..ad4d03ce3f8a9 --- /dev/null +++ b/pkg/services/accesscontrol/acimpl/uid_resolver_test.go @@ -0,0 +1,202 @@ +package acimpl + +import ( + "context" + "strconv" + "sync" + "sync/atomic" + "testing" + "time" + + claims "github.com/grafana/authlib/types" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + dynamicfake "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" + + "github.com/grafana/grafana/pkg/apimachinery/utils" +) + +// iamObject builds an unstructured IAM resource (team or user) named by uid, carrying the +// deprecated internal ID label that getObjectID reads. +func iamObject(gvk schema.GroupVersionKind, namespace, uid string, internalID int64) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.SetGroupVersionKind(gvk) + obj.SetNamespace(namespace) + obj.SetName(uid) + obj.SetLabels(map[string]string{utils.LabelKeyDeprecatedInternalID: strconv.FormatInt(internalID, 10)}) + return obj +} + +// newCountingResolver returns a uidToIDResolver wired to a fake dynamic client (no apiserver), +// plus a per-resource "get" counter and a hook to delay each Get (used to force overlap in the +// single flight test). The configProvider is left nil — it is never reached because the dynamic +// clients are pre-populated. +func newCountingResolver(t *testing.T, getDelay time.Duration, objs ...*unstructured.Unstructured) (*uidToIDResolver, *int64) { + t.Helper() + + teamGVK := teamGVR.GroupVersion().WithKind("Team") + userGVK := userGVR.GroupVersion().WithKind("User") + + scheme := runtime.NewScheme() + scheme.AddKnownTypeWithName(teamGVK, &unstructured.Unstructured{}) + scheme.AddKnownTypeWithName(teamGVR.GroupVersion().WithKind("TeamList"), &unstructured.UnstructuredList{}) + scheme.AddKnownTypeWithName(userGVK, &unstructured.Unstructured{}) + scheme.AddKnownTypeWithName(userGVR.GroupVersion().WithKind("UserList"), &unstructured.UnstructuredList{}) + + runtimeObjs := make([]runtime.Object, 0, len(objs)) + for _, o := range objs { + runtimeObjs = append(runtimeObjs, o) + } + + fake := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme, map[schema.GroupVersionResource]string{ + teamGVR: "TeamList", + userGVR: "UserList", + }, runtimeObjs...) + + var getCount int64 + fake.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + atomic.AddInt64(&getCount, 1) + if getDelay > 0 { + time.Sleep(getDelay) + } + // Fall through to the default tracker so the object is still served. + return false, nil, nil + }) + + r := newUIDToIDResolver(nil) + r.clients[teamGVR] = fake.Resource(teamGVR) + r.clients[userGVR] = fake.Resource(userGVR) + return r, &getCount +} + +func TestUIDToIDResolver_Caching(t *testing.T) { + ns := claims.NamespaceInfo{Value: "default", OrgID: 1} + + t.Run("repeated resolves of the same UID hit the cache after the first Get", func(t *testing.T) { + r, getCount := newCountingResolver(t, 0, iamObject(teamGVR.GroupVersion().WithKind("Team"), ns.Value, "team-uid", 42)) + + for i := 0; i < 5; i++ { + id, err := r.GetTeamIDByUID(context.Background(), ns, "team-uid") + require.NoError(t, err) + require.Equal(t, int64(42), id) + } + + require.Equal(t, int64(1), atomic.LoadInt64(getCount), "only the first resolve should reach the apiserver") + }) + + t.Run("concurrent resolves of the same UID collapse into a single Get", func(t *testing.T) { + r, getCount := newCountingResolver(t, 50*time.Millisecond, iamObject(userGVR.GroupVersion().WithKind("User"), ns.Value, "user-uid", 7)) + + const goroutines = 25 + var wg sync.WaitGroup + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + id, err := r.GetUserIDByUID(context.Background(), ns, "user-uid") + require.NoError(t, err) + require.Equal(t, int64(7), id) + }() + } + wg.Wait() + + require.Equal(t, int64(1), atomic.LoadInt64(getCount), "single flight should dedupe concurrent resolves") + }) + + t.Run("team and user UIDs and namespaces are cached independently", func(t *testing.T) { + // ns2 is a different namespace that shares ns's OrgID (every cloud stacks-N namespace + // parses to OrgID 1, like "default"). Keying must use the namespace value the Get is + // scoped on, not OrgID, or these two tenants would collide on a single cache entry. + ns2 := claims.NamespaceInfo{Value: "stacks-2", OrgID: 1, StackID: 2} + r, getCount := newCountingResolver(t, 0, + iamObject(teamGVR.GroupVersion().WithKind("Team"), ns.Value, "shared-uid", 1), + iamObject(userGVR.GroupVersion().WithKind("User"), ns.Value, "shared-uid", 2), + iamObject(teamGVR.GroupVersion().WithKind("Team"), ns2.Value, "shared-uid", 3), + ) + + teamID, err := r.GetTeamIDByUID(context.Background(), ns, "shared-uid") + require.NoError(t, err) + require.Equal(t, int64(1), teamID) + + userID, err := r.GetUserIDByUID(context.Background(), ns, "shared-uid") + require.NoError(t, err) + require.Equal(t, int64(2), userID) + + teamIDOtherNS, err := r.GetTeamIDByUID(context.Background(), ns2, "shared-uid") + require.NoError(t, err) + require.Equal(t, int64(3), teamIDOtherNS) + + // Re-resolving each should be served from cache. + _, err = r.GetTeamIDByUID(context.Background(), ns, "shared-uid") + require.NoError(t, err) + _, err = r.GetUserIDByUID(context.Background(), ns, "shared-uid") + require.NoError(t, err) + _, err = r.GetTeamIDByUID(context.Background(), ns2, "shared-uid") + require.NoError(t, err) + + require.Equal(t, int64(3), atomic.LoadInt64(getCount), "each distinct key resolves exactly once") + }) + + t.Run("a caller cancelling does not fail the others sharing the flight", func(t *testing.T) { + r, getCount := newCountingResolver(t, 100*time.Millisecond, iamObject(userGVR.GroupVersion().WithKind("User"), ns.Value, "user-uid", 7)) + + const survivors = 5 + var wg sync.WaitGroup + + // One caller cancels mid-flight; it must observe its own cancellation. + cancelCtx, cancel := context.WithCancel(context.Background()) + wg.Add(1) + go func() { + defer wg.Done() + _, err := r.GetUserIDByUID(cancelCtx, ns, "user-uid") + require.ErrorIs(t, err, context.Canceled) + }() + + // The rest share the same flight and must still resolve successfully. + wg.Add(survivors) + for i := 0; i < survivors; i++ { + go func() { + defer wg.Done() + id, err := r.GetUserIDByUID(context.Background(), ns, "user-uid") + require.NoError(t, err) + require.Equal(t, int64(7), id) + }() + } + + // Cancel well before the 100ms Get completes so the cancelled caller is still in flight. + time.Sleep(20 * time.Millisecond) + cancel() + wg.Wait() + + require.Equal(t, int64(1), atomic.LoadInt64(getCount), "the shared fetch should run once despite one caller cancelling") + }) + + t.Run("failed resolves are not cached", func(t *testing.T) { + // No objects: every Get is a not-found error, so nothing should be cached. + r, getCount := newCountingResolver(t, 0) + + _, err := r.GetTeamIDByUID(context.Background(), ns, "missing-uid") + require.Error(t, err) + _, err = r.GetTeamIDByUID(context.Background(), ns, "missing-uid") + require.Error(t, err) + + require.Equal(t, int64(2), atomic.LoadInt64(getCount), "errors must not be cached") + }) + + t.Run("a zero internal ID is not cached", func(t *testing.T) { + // The object resolves but carries no usable internal ID (sentinel 0), so it must be + // re-resolved every time rather than pinning 0 for the TTL. + r, getCount := newCountingResolver(t, 0, iamObject(teamGVR.GroupVersion().WithKind("Team"), ns.Value, "team-uid", 0)) + + for i := 0; i < 3; i++ { + id, err := r.GetTeamIDByUID(context.Background(), ns, "team-uid") + require.NoError(t, err) + require.Equal(t, int64(0), id) + } + + require.Equal(t, int64(3), atomic.LoadInt64(getCount), "a zero id must not be cached") + }) +} diff --git a/pkg/services/accesscontrol/acimpl/zanzana_resolver.go b/pkg/services/accesscontrol/acimpl/zanzana_resolver.go index f75a5161bf4ec..194a7d4907e52 100644 --- a/pkg/services/accesscontrol/acimpl/zanzana_resolver.go +++ b/pkg/services/accesscontrol/acimpl/zanzana_resolver.go @@ -6,10 +6,12 @@ import ( "fmt" "strings" "sync" + "time" authzv1 "github.com/grafana/authlib/authz/proto/v1" "github.com/grafana/authlib/types" "golang.org/x/sync/errgroup" + "golang.org/x/sync/singleflight" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -17,6 +19,7 @@ import ( iamv0 "github.com/grafana/grafana/apps/iam/pkg/apis/iam/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/apiserver/restcfg" @@ -570,16 +573,32 @@ var ( userGVR = iamv0.UserResourceInfo.GroupVersionResource() ) +// UID→internal-ID mappings are immutable for an object's lifetime, so a long TTL is safe; +// the bound just caps memory and tolerates the rare deleted-then-absent UID. +const uidToIDCacheTTL = 1 * time.Hour + +// Bounds the detached singleflight fetch. The fetch runs on a context decoupled from any single +// caller (see getObjectID), so without this a hung apiserver Get would block the flight—and every +// follower waiting on it—indefinitely. +const uidToIDFetchTimeout = 10 * time.Second + type uidToIDResolver struct { mu sync.RWMutex clients map[schema.GroupVersionResource]dynamic.NamespaceableResourceInterface configProvider restcfg.RestConfigProvider + // cache stores resolved UID→internal-ID mappings; sf collapses concurrent resolutions + // of the same key into a single apiserver Get. Both teams and users resolve through here, + // and the same UIDs recur across every action during a permission merge, so this removes + // the bulk of the redundant apiserver lookups. + cache *localcache.CacheService + sf singleflight.Group } func newUIDToIDResolver(configProvider restcfg.RestConfigProvider) *uidToIDResolver { return &uidToIDResolver{ clients: make(map[schema.GroupVersionResource]dynamic.NamespaceableResourceInterface), configProvider: configProvider, + cache: localcache.New(uidToIDCacheTTL, 10*time.Minute), } } @@ -614,6 +633,54 @@ func (r *uidToIDResolver) getDynamicClient(ctx context.Context, nsInfo types.Nam } func (r *uidToIDResolver) getObjectID(ctx context.Context, nsInfo types.NamespaceInfo, gvr schema.GroupVersionResource, name string) (int64, error) { + // Key by resource type so team/user UIDs can't collide, and by the namespace value the + // fetch actually scopes the Get on (cli.Namespace(nsInfo.Value) in fetchObjectID), so + // tenants stay isolated. Don't key on OrgID: multiple namespaces can share an OrgID + // (every cloud stacks-N namespace parses to OrgID 1), which would collide their entries + // and collapse cross-tenant resolutions into a single singleflight fetch. + key := fmt.Sprintf("%s/%s/%s", gvr.Resource, nsInfo.Value, name) + if v, ok := r.cache.Get(key); ok { + return v.(int64), nil + } + + ch := r.sf.DoChan(key, func() (any, error) { + // Re-check under the flight: a concurrent resolution may have populated the cache + // while we were waiting for the singleflight slot. + if v, ok := r.cache.Get(key); ok { + return v.(int64), nil + } + // Detach from the caller's context so one caller cancelling (request timeout, client + // disconnect) does not fail the shared fetch for every other caller on this flight. + // A bounded timeout still prevents a hung Get from blocking the flight forever. + fetchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), uidToIDFetchTimeout) + defer cancel() + id, err := r.fetchObjectID(fetchCtx, nsInfo, gvr, name) + if err != nil { + return 0, err + } + // Only cache successful lookups, and skip id == 0: that is the sentinel returned when the + // deprecated internal-ID label is missing/unparseable (e.g. a dual-write window), so caching + // it would pin a bogus value for the full TTL instead of re-resolving once it's populated. + if id != 0 { + r.cache.Set(key, id, uidToIDCacheTTL) + } + return id, nil + }) + + // Wait on this caller's own context so a cancelled caller returns promptly while the + // shared fetch continues for everyone else. + select { + case <-ctx.Done(): + return 0, ctx.Err() + case res := <-ch: + if res.Err != nil { + return 0, res.Err + } + return res.Val.(int64), nil + } +} + +func (r *uidToIDResolver) fetchObjectID(ctx context.Context, nsInfo types.NamespaceInfo, gvr schema.GroupVersionResource, name string) (int64, error) { cli, err := r.getDynamicClient(ctx, nsInfo, gvr) if err != nil { return 0, err