Authorization bypass / Access control (Cross-tenant isolation violation)

Proof-of-concept (high level, controlled test environment): 1) Run a two-tenant setup with namespaces A and B. 2) Create a resource in namespace B. 3) Use a test service running in namespace A with an authenticated user identity for namespace A. 4) Invoke an internal delegated RPC (for example GetBlob or ListManagedObjects) targeting the resource in namespace B using the A-namespace identity. 5) Observe that, prior to this fix, the RPC might allow access to B's resource or leak information because no cross-namespace authorization was enforced. 6) After applying the fix in 12.4.0, the server should respond with 401 when no user and 403 for a cross-namespace request, blocking access. Modify tests to assert status codes accordingly.

diff --git a/pkg/registry/apis/provisioning/webhooks/pullrequest/render_test.go b/pkg/registry/apis/provisioning/webhooks/pullrequest/render_test.go index c4c454aa7fa9e..1d2f3e2cd9247 100644 --- a/pkg/registry/apis/provisioning/webhooks/pullrequest/render_test.go +++ b/pkg/registry/apis/provisioning/webhooks/pullrequest/render_test.go @@ -8,11 +8,13 @@ import ( "path/filepath" "testing" + authlib "github.com/grafana/authlib/types" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" @@ -292,3 +294,54 @@ func TestScreenshotRenderer_RenderScreenshot(t *testing.T) { }) } } + +// PutBlob on the unified storage server requires a user in ctx; a refactor +// that drops the ctx between the renderer boundary and PutBlob would 401 +// every PR screenshot in production. Identity is stamped two callers +// upstream (jobs/driver.go), so this test only verifies the renderer +// doesn't strip it. +func TestScreenshotRenderer_PropagatesIdentity(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + tmpFile, cleanup := setupTempFile(t) + t.Cleanup(cleanup) + + render := rendering.NewMockService(ctrl) + render.EXPECT().Render(gomock.Any(), rendering.RenderPNG, gomock.Any()). + Return(&rendering.RenderResult{FilePath: tmpFile}, nil) + + user := &identity.StaticRequester{ + Type: authlib.TypeUser, + Login: "worker", + UserID: 1, + UserUID: "u1", + Namespace: "test-ns", + } + inCtx := authlib.WithAuthInfo(context.Background(), user) + + // Assert outside the mock callback: require.* inside MatchedBy calls + // runtime.Goexit on failure and deadlocks the test waiting on a PutBlob + // that never returns. + var capturedCtx context.Context + blobstore := NewMockBlobStoreClient(t) + blobstore.On("PutBlob", mock.MatchedBy(func(callCtx context.Context) bool { + capturedCtx = callCtx + return true + }), mock.Anything).Return(&resourcepb.PutBlobResponse{Uid: "blob-uid"}, nil) + + renderer := NewScreenshotRenderer(render, blobstore) + _, err := renderer.RenderScreenshot(inCtx, provisioning.ResourceRepositoryInfo{ + Name: "test-repo", + Namespace: "test-ns", + }, "test", nil) + require.NoError(t, err) + + require.NotNil(t, capturedCtx, "PutBlob was never called") + got, ok := authlib.AuthInfoFrom(capturedCtx) + require.True(t, ok, "PutBlob received ctx with no AuthInfo -- identity was dropped by the renderer") + require.NotNil(t, got) + require.Equal(t, "test-ns", got.GetNamespace(), "AuthInfo namespace must survive the renderer") + // GetIdentifier returns the raw UID; GetUID is the type-prefixed form. + require.Equal(t, user.UserUID, got.GetIdentifier(), "AuthInfo UID must survive the renderer") +} diff --git a/pkg/storage/unified/proto/blob.proto b/pkg/storage/unified/proto/blob.proto index 9b58e09a71332..85b665a39e68a 100644 --- a/pkg/storage/unified/proto/blob.proto +++ b/pkg/storage/unified/proto/blob.proto @@ -18,8 +18,9 @@ message PutBlobRequest { HTTP = 1; } - // The resource that will use this blob - // NOTE: the name may not yet exist, but group+resource are required + // The resource this blob attaches to. The resource MUST already exist; + // the caller must be authorized to update it. PutBlob is not a staging + // primitive -- create the resource first, then attach a blob to it. ResourceKey resource = 1; // How to upload diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 9dd401e1cd807..636853e4df4ec 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -1934,7 +1934,34 @@ func (s *server) GetStats(ctx context.Context, req *resourcepb.ResourceStatsRequ return s.search.GetStats(ctx, req) } +// requireUserNamespace is a cross-tenant safety net for delegated-only +// RPCs. It does not replace resource-level access.Check; it only catches +// the case where a caller authenticated for one namespace asks for data +// in another. +func requireUserNamespace(ctx context.Context, namespace string) *resourcepb.ErrorResult { + user, ok := claims.AuthInfoFrom(ctx) + if !ok || user == nil { + return &resourcepb.ErrorResult{ + Message: "no user found in context", + Code: http.StatusUnauthorized, + } + } + if !claims.NamespaceMatches(user.GetNamespace(), namespace) { + return &resourcepb.ErrorResult{ + Message: "namespace mismatch", + Code: http.StatusForbidden, + } + } + return nil +} + +// ListManagedObjects implements ManagedObjectIndexServer. +// NOTE: Internal RPC -- callers are responsible for authorizing the originating user request. +// Do not route end-user traffic here directly. func (s *server) ListManagedObjects(ctx context.Context, req *resourcepb.ListManagedObjectsRequest) (*resourcepb.ListManagedObjectsResponse, error) { + if errRes := requireUserNamespace(ctx, req.Namespace); errRes != nil { + return &resourcepb.ListManagedObjectsResponse{Error: errRes}, nil + } if s.search == nil { return nil, fmt.Errorf("search index not configured") } @@ -1942,7 +1969,13 @@ func (s *server) ListManagedObjects(ctx context.Context, req *resourcepb.ListMan return s.search.ListManagedObjects(ctx, req) } +// CountManagedObjects implements ManagedObjectIndexServer. +// NOTE: Internal RPC -- callers are responsible for authorizing the originating user request. +// Do not route end-user traffic here directly. func (s *server) CountManagedObjects(ctx context.Context, req *resourcepb.CountManagedObjectsRequest) (*resourcepb.CountManagedObjectsResponse, error) { + if errRes := requireUserNamespace(ctx, req.Namespace); errRes != nil { + return &resourcepb.CountManagedObjectsResponse{Error: errRes}, nil + } if s.search == nil { return nil, fmt.Errorf("search index not configured") } @@ -1955,8 +1988,16 @@ func (s *server) IsHealthy(ctx context.Context, req *resourcepb.HealthCheckReque return s.diagnostics.IsHealthy(ctx, req) //nolint:staticcheck } -// GetBlob implements BlobStore. +// PutBlob implements BlobStore. +// NOTE: Internal RPC -- callers are responsible for authorizing the originating user request. +// Do not route end-user traffic here directly. func (s *server) PutBlob(ctx context.Context, req *resourcepb.PutBlobRequest) (*resourcepb.PutBlobResponse, error) { + if req.Resource == nil { + return &resourcepb.PutBlobResponse{Error: &resourcepb.ErrorResult{ + Message: "missing resource key", + Code: http.StatusBadRequest, + }}, nil + } if s.blob == nil { return &resourcepb.PutBlobResponse{Error: &resourcepb.ErrorResult{ Message: "blob store not configured", @@ -1964,6 +2005,45 @@ func (s *server) PutBlob(ctx context.Context, req *resourcepb.PutBlobRequest) (* }}, nil } + user, ok := claims.AuthInfoFrom(ctx) + if !ok || user == nil { + return &resourcepb.PutBlobResponse{Error: &resourcepb.ErrorResult{ + Message: "no user found in context", + Code: http.StatusUnauthorized, + }}, nil + } + + // Load the parent both to enforce existence (see proto) and to get its + // folder for access.Check. + parent := s.backend.ReadResource(ctx, &resourcepb.ReadRequest{Key: req.Resource}) + switch { + case parent == nil: + return &resourcepb.PutBlobResponse{Error: &resourcepb.ErrorResult{ + Message: "parent resource not found", + Code: http.StatusNotFound, + }}, nil + case parent.Error != nil: + // Surface backend status as-is; collapsing to 404 would hide + // transient 5xx as "not found". + return &resourcepb.PutBlobResponse{Error: parent.Error}, nil + } + + a, err := s.access.Check(ctx, user, claims.CheckRequest{ + Verb: utils.VerbUpdate, + Group: req.Resource.Group, + Resource: req.Resource.Resource, + Namespace: req.Resource.Namespace, + Name: req.Resource.Name, + }, parent.Folder) + if err != nil { + return &resourcepb.PutBlobResponse{Error: AsErrorResult(err)}, nil + } + if !a.Allowed { + return &resourcepb.PutBlobResponse{Error: &resourcepb.ErrorResult{ + Code: http.StatusForbidden, + }}, nil + } + rsp, err := s.blob.PutResourceBlob(ctx, req) if err != nil { rsp.Error = AsErrorResult(err) @@ -2029,7 +2109,18 @@ func (s *server) getPartialObject(ctx context.Context, key *resourcepb.ResourceK } // GetBlob implements BlobStore. +// NOTE: Internal RPC -- callers are responsible for authorizing the originating user request. +// Do not route end-user traffic here directly. func (s *server) GetBlob(ctx context.Context, req *resourcepb.GetBlobRequest) (*resourcepb.GetBlobResponse, error) { + if req.Resource == nil { + return &resourcepb.GetBlobResponse{Error: &resourcepb.ErrorResult{ + Message: "missing resource key", + Code: http.StatusBadRequest, + }}, nil + } + if errRes := requireUserNamespace(ctx, req.Resource.Namespace); errRes != nil { + return &resourcepb.GetBlobResponse{Error: errRes}, nil + } if s.blob == nil { return &resourcepb.GetBlobResponse{Error: &resourcepb.ErrorResult{ Message: "blob store not configured", @@ -2113,7 +2204,13 @@ func (s *server) runInQueue(ctx context.Context, tenantID string, runnable func( } } +// RebuildIndexes implements ResourceIndexServer. +// NOTE: Internal RPC -- callers are responsible for authorizing the originating user request. +// Do not route end-user traffic here directly. func (s *server) RebuildIndexes(ctx context.Context, req *resourcepb.RebuildIndexesRequest) (*resourcepb.RebuildIndexesResponse, error) { + if errRes := requireUserNamespace(ctx, req.Namespace); errRes != nil { + return &resourcepb.RebuildIndexesResponse{Error: errRes}, nil + } if s.search == nil { return nil, fmt.Errorf("search index not configured") } diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index 6e604d2b3980f..098451d791603 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -1985,3 +1985,267 @@ func TestFolderDeletePermissionChecks(t *testing.T) { require.NotEqual(t, folderName, capturedFolder) }) } + +// stubBlobSupport records whether PutResourceBlob was reached so tests can +// assert that the authz gate short-circuits or delegates. +type stubBlobSupport struct { + putReached bool +} + +func (s *stubBlobSupport) SupportsSignedURLs() bool { return false } + +func (s *stubBlobSupport) PutResourceBlob(_ context.Context, _ *resourcepb.PutBlobRequest) (*resourcepb.PutBlobResponse, error) { + s.putReached = true + return &resourcepb.PutBlobResponse{Uid: "blob-uid"}, nil +} + +func (s *stubBlobSupport) GetResourceBlob(_ context.Context, _ *resourcepb.ResourceKey, _ *utils.BlobInfo, _ bool) (*resourcepb.GetBlobResponse, error) { + return &resourcepb.GetBlobResponse{}, nil +} + +// errorOnReadResourceBackend lets tests inject an arbitrary ReadResource +// error to exercise PutBlob's failure passthrough. +type errorOnReadResourceBackend struct { + StorageBackend + readErr *resourcepb.ErrorResult +} + +func (b *errorOnReadResourceBackend) ReadResource(_ context.Context, _ *resourcepb.ReadRequest) *BackendReadResponse { + return &BackendReadResponse{Error: b.readErr} +} + +// Embedding the StorageBackend interface doesn't promote Stop; without +// this override srv.Stop can't reach the real backend's goroutines and +// goleak flags them. +func (b *errorOnReadResourceBackend) Stop(ctx context.Context) error { + if s, ok := b.StorageBackend.(ResourceServerStopper); ok { + return s.Stop(ctx) + } + return nil +} + +// newBlobAuthzTestServer is the shared fixture for the PutBlob and +// namespace-gate tests. backendWrap is optional and wraps the real KV +// backend so tests can inject ReadResource failures. +func newBlobAuthzTestServer(t *testing.T, backendWrap func(StorageBackend) StorageBackend) (*server, *callbackAccessClient, *stubBlobSupport) { + t.Helper() + db, err := badger.Open(badger.DefaultOptions("").WithInMemory(true).WithLogger(nil)) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + var store StorageBackend + store, err = NewKVStorageBackend(KVBackendOptions{KvStore: NewBadgerKV(db)}) + require.NoError(t, err) + if backendWrap != nil { + store = backendWrap(store) + } + + ac := &callbackAccessClient{fn: func(authlib.CheckRequest, string) (authlib.CheckResponse, error) { return allow() }} + blob := &stubBlobSupport{} + + srv, err := NewResourceServer(ResourceServerOptions{ + Backend: store, + AccessClient: ac, + Blob: BlobConfig{Backend: blob}, + }) + require.NoError(t, err) + t.Cleanup(func() { + stopCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _ = srv.Stop(stopCtx) + }) + return srv, ac, blob +} + +func ctxWithUserInNs(ns string) context.Context { + return authlib.WithAuthInfo(context.Background(), &identity.StaticRequester{ + Type: authlib.TypeUser, + UserID: 1, + UserUID: "u1", + Namespace: ns, + }) +} + +func TestPutBlobPermissionChecks(t *testing.T) { + const ( + group = "playlist.grafana.app" + resource = "playlists" + namespace = "default" + name = "test-resource" + ) + key := &resourcepb.ResourceKey{Group: group, Resource: resource, Namespace: namespace, Name: name} + value := []byte(`{"apiVersion":"playlist.grafana.app/v0alpha1","kind":"Playlist","metadata":{"name":"` + name + `","uid":"test-uid","namespace":"` + namespace + `"},"spec":{"title":"t","interval":"5m","items":[]}}`) + ctxWithUser := ctxWithUserInNs(namespace) + + // seedParent makes ReadResource inside PutBlob succeed. ac is left in + // allow() so callers can flip it before the PutBlob under test. + seedParent := func(t *testing.T, srv *server, ac *callbackAccessClient) { + t.Helper() + ac.fn = func(authlib.CheckRequest, string) (authlib.CheckResponse, error) { return allow() } + rsp, err := srv.Create(ctxWithUser, &resourcepb.CreateRequest{Key: key, Value: value}) + require.NoError(t, err) + require.Nil(t, rsp.Error) + } + + t.Run("rejects when no user in context", func(t *testing.T) { + srv, _, blob := newBlobAuthzTestServer(t, nil) + rsp, err := srv.PutBlob(context.Background(), &resourcepb.PutBlobRequest{Resource: key}) + require.NoError(t, err) + require.Equal(t, int32(http.StatusUnauthorized), rsp.Error.Code) + require.False(t, blob.putReached) + }) + + t.Run("returns 404 when parent resource does not exist", func(t *testing.T) { + srv, _, blob := newBlobAuthzTestServer(t, nil) + rsp, err := srv.PutBlob(ctxWithUser, &resourcepb.PutBlobRequest{Resource: key}) + require.NoError(t, err) + require.Equal(t, int32(http.StatusNotFound), rsp.Error.Code) + require.False(t, blob.putReached) + }) + + t.Run("rejects with 403 whe ... [truncated]