From 6188526e1d2a841c633fa0f18f24a2fc00bb4607 Mon Sep 17 00:00:00 2001 From: Artur Wierzbicki Date: Sun, 17 Jul 2022 22:41:54 +0400 Subject: [PATCH] Storage: use static access rules (#52334) * Storage: use static access rules * Storage: use static access rules * Storage: add tests --- pkg/services/store/file_guardian.go | 121 ++++++++++++++++++++++++++++ pkg/services/store/http.go | 5 +- pkg/services/store/service.go | 104 ++++++++++++++++++------ pkg/services/store/service_test.go | 90 +++++++++++++++------ pkg/services/store/static_auth.go | 41 ++++++++++ pkg/services/store/tree.go | 3 +- pkg/services/store/types.go | 2 +- pkg/services/store/utils.go | 5 ++ 8 files changed, 319 insertions(+), 52 deletions(-) create mode 100644 pkg/services/store/file_guardian.go create mode 100644 pkg/services/store/static_auth.go diff --git a/pkg/services/store/file_guardian.go b/pkg/services/store/file_guardian.go new file mode 100644 index 00000000000..56a1d58c42e --- /dev/null +++ b/pkg/services/store/file_guardian.go @@ -0,0 +1,121 @@ +package store + +import ( + "context" + "strings" + + "github.com/grafana/grafana/pkg/infra/filestorage" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" +) + +const ( + ActionFilesRead = "files:read" + ActionFilesWrite = "files:write" + ActionFilesDelete = "files:delete" +) + +var ( + denyAllPathFilter = filestorage.NewDenyAllPathFilter() + allowAllPathFilter = filestorage.NewAllowAllPathFilter() +) + +func isValidAction(action string) bool { + return action == ActionFilesRead || action == ActionFilesWrite || action == ActionFilesDelete +} + +type storageAuthService interface { + newGuardian(ctx context.Context, user *models.SignedInUser, prefix string) fileGuardian +} + +type fileGuardian interface { + canView(path string) bool + canWrite(path string) bool + canDelete(path string) bool + can(action string, path string) bool + + getPathFilter(action string) filestorage.PathFilter +} + +type pathFilterFileGuardian struct { + ctx context.Context + user *models.SignedInUser + prefix string + pathFilterByAction map[string]filestorage.PathFilter + log log.Logger +} + +func (a *pathFilterFileGuardian) getPathFilter(action string) filestorage.PathFilter { + if !isValidAction(action) { + a.log.Warn("Unsupported action", "action", action) + return denyAllPathFilter + } + + if filter, ok := a.pathFilterByAction[action]; ok { + return filter + } + + return denyAllPathFilter +} + +func (a *pathFilterFileGuardian) canWrite(path string) bool { + return a.can(ActionFilesWrite, path) +} + +func (a *pathFilterFileGuardian) canView(path string) bool { + return a.can(ActionFilesRead, path) +} + +func (a *pathFilterFileGuardian) canDelete(path string) bool { + return a.can(ActionFilesDelete, path) +} + +func (a *pathFilterFileGuardian) can(action string, path string) bool { + if path == a.prefix { + path = filestorage.Delimiter + } else { + path = strings.TrimPrefix(path, a.prefix) + } + allow := false + + if !isValidAction(action) { + a.log.Warn("Unsupported action", "action", action, "path", path) + return false + } + + pathFilter, ok := a.pathFilterByAction[action] + + if !ok { + a.log.Warn("Missing path filter", "action", action, "path", path) + return false + } + + allow = pathFilter.IsAllowed(path) + if !allow { + a.log.Warn("denying", "action", action, "path", path) + } + return allow +} + +type denyAllFileGuardian struct { +} + +func (d denyAllFileGuardian) canView(path string) bool { + return d.can(ActionFilesRead, path) +} + +func (d denyAllFileGuardian) canWrite(path string) bool { + return d.can(ActionFilesWrite, path) +} + +func (d denyAllFileGuardian) canDelete(path string) bool { + return d.can(ActionFilesDelete, path) +} + +func (d denyAllFileGuardian) can(action string, path string) bool { + return false +} + +func (d denyAllFileGuardian) getPathFilter(action string) filestorage.PathFilter { + return denyAllPathFilter +} diff --git a/pkg/services/store/http.go b/pkg/services/store/http.go index 43f2f69c8c3..9f207d9a7a1 100644 --- a/pkg/services/store/http.go +++ b/pkg/services/store/http.go @@ -37,7 +37,7 @@ func ProvideHTTPService(store StorageService) HTTPStorageService { func UploadErrorToStatusCode(err error) int { switch { - case errors.Is(err, ErrUploadFeatureDisabled): + case errors.Is(err, ErrStorageNotFound): return 404 case errors.Is(err, ErrUnsupportedStorage): @@ -49,6 +49,9 @@ func UploadErrorToStatusCode(err error) int { case errors.Is(err, ErrFileAlreadyExists): return 400 + case errors.Is(err, ErrAccessDenied): + return 403 + default: return 500 } diff --git a/pkg/services/store/service.go b/pkg/services/store/service.go index 3713843f11f..50ab7da657d 100644 --- a/pkg/services/store/service.go +++ b/pkg/services/store/service.go @@ -3,7 +3,6 @@ package store import ( "context" "errors" - "fmt" "os" "path/filepath" @@ -19,13 +18,16 @@ import ( var grafanaStorageLogger = log.New("grafanaStorageLogger") -var ErrUploadFeatureDisabled = errors.New("upload feature is disabled") var ErrUnsupportedStorage = errors.New("storage does not support this operation") var ErrUploadInternalError = errors.New("upload internal error") var ErrValidationFailed = errors.New("request validation failed") var ErrFileAlreadyExists = errors.New("file exists") +var ErrStorageNotFound = errors.New("storage not found") +var ErrAccessDenied = errors.New("access denied") const RootPublicStatic = "public-static" +const RootResources = "resources" +const RootDevenv = "devenv" const MAX_UPLOAD_SIZE = 1 * 1024 * 1024 // 3MB @@ -66,9 +68,10 @@ type storageServiceConfig struct { } type standardStorageService struct { - sql *sqlstore.SQLStore - tree *nestedTree - cfg storageServiceConfig + sql *sqlstore.SQLStore + tree *nestedTree + cfg storageServiceConfig + authService storageAuthService } func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, cfg *setting.Cfg) StorageService { @@ -90,7 +93,7 @@ func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, devenv := filepath.Join(cfg.StaticRootPath, "..", "devenv") if _, err := os.Stat(devenv); !os.IsNotExist(err) { // path/to/whatever exists - s := newDiskStorage("devenv", "Development Environment", &StorageLocalDiskConfig{ + s := newDiskStorage(RootDevenv, "Development Environment", &StorageLocalDiskConfig{ Path: devenv, Roots: []string{ "/dev-dashboards/", @@ -104,7 +107,7 @@ func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, storages := make([]storageRuntime, 0) if features.IsEnabled(featuremgmt.FlagStorageLocalUpload) { storages = append(storages, - newSQLStorage("resources", + newSQLStorage(RootResources, "Resources", &StorageSQLConfig{orgId: orgId}, sql). setBuiltin(true). @@ -114,10 +117,39 @@ func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, return storages } - return newStandardStorageService(sql, globalRoots, initializeOrgStorages) + authService := newStaticStorageAuthService(func(ctx context.Context, user *models.SignedInUser, storageName string) map[string]filestorage.PathFilter { + if user == nil || !user.IsGrafanaAdmin { + return nil + } + + switch storageName { + case RootPublicStatic: + return map[string]filestorage.PathFilter{ + ActionFilesRead: allowAllPathFilter, + ActionFilesWrite: denyAllPathFilter, + ActionFilesDelete: denyAllPathFilter, + } + case RootDevenv: + return map[string]filestorage.PathFilter{ + ActionFilesRead: allowAllPathFilter, + ActionFilesWrite: denyAllPathFilter, + ActionFilesDelete: denyAllPathFilter, + } + case RootResources: + return map[string]filestorage.PathFilter{ + ActionFilesRead: allowAllPathFilter, + ActionFilesWrite: allowAllPathFilter, + ActionFilesDelete: allowAllPathFilter, + } + default: + return nil + } + }) + + return newStandardStorageService(sql, globalRoots, initializeOrgStorages, authService) } -func newStandardStorageService(sql *sqlstore.SQLStore, globalRoots []storageRuntime, initializeOrgStorages func(orgId int64) []storageRuntime) *standardStorageService { +func newStandardStorageService(sql *sqlstore.SQLStore, globalRoots []storageRuntime, initializeOrgStorages func(orgId int64) []storageRuntime, authService storageAuthService) *standardStorageService { rootsByOrgId := make(map[int64][]storageRuntime) rootsByOrgId[ac.GlobalOrgID] = globalRoots @@ -127,8 +159,9 @@ func newStandardStorageService(sql *sqlstore.SQLStore, globalRoots []storageRunt } res.init() return &standardStorageService{ - sql: sql, - tree: res, + sql: sql, + tree: res, + authService: authService, cfg: storageServiceConfig{ allowUnsanitizedSvgUpload: false, }, @@ -149,12 +182,15 @@ func getOrgId(user *models.SignedInUser) int64 { } func (s *standardStorageService) List(ctx context.Context, user *models.SignedInUser, path string) (*StorageListFrame, error) { - // apply access control here - return s.tree.ListFolder(ctx, getOrgId(user), path) + guardian := s.authService.newGuardian(ctx, user, getFirstSegment(path)) + return s.tree.ListFolder(ctx, getOrgId(user), path, guardian.getPathFilter(ActionFilesRead)) } func (s *standardStorageService) Read(ctx context.Context, user *models.SignedInUser, path string) (*filestorage.File, error) { - // TODO: permission check! + guardian := s.authService.newGuardian(ctx, user, getFirstSegment(path)) + if !guardian.canView(path) { + return nil, ErrAccessDenied + } return s.tree.GetFile(ctx, getOrgId(user), path) } @@ -171,12 +207,17 @@ type UploadRequest struct { } func (s *standardStorageService) Upload(ctx context.Context, user *models.SignedInUser, req *UploadRequest) error { - upload, storagePath := s.tree.getRoot(getOrgId(user), req.Path) - if upload == nil { - return ErrUploadFeatureDisabled + guardian := s.authService.newGuardian(ctx, user, getFirstSegment(req.Path)) + if !guardian.canWrite(req.Path) { + return ErrAccessDenied + } + + root, storagePath := s.tree.getRoot(getOrgId(user), req.Path) + if root == nil { + return ErrStorageNotFound } - if upload.Meta().ReadOnly { + if root.Meta().ReadOnly { return ErrUnsupportedStorage } @@ -195,7 +236,7 @@ func (s *standardStorageService) Upload(ctx context.Context, user *models.Signed grafanaStorageLogger.Info("uploading a file", "filetype", req.MimeType, "path", req.Path) if !req.OverwriteExistingFile { - file, err := upload.Store().Get(ctx, storagePath) + file, err := root.Store().Get(ctx, storagePath) if err != nil { grafanaStorageLogger.Error("failed while checking file existence", "err", err, "path", req.Path) return ErrUploadInternalError @@ -206,7 +247,7 @@ func (s *standardStorageService) Upload(ctx context.Context, user *models.Signed } } - if err := upload.Store().Upsert(ctx, upsertCommand); err != nil { + if err := root.Store().Upsert(ctx, upsertCommand); err != nil { grafanaStorageLogger.Error("failed while uploading the file", "err", err, "path", req.Path) return ErrUploadInternalError } @@ -215,9 +256,14 @@ func (s *standardStorageService) Upload(ctx context.Context, user *models.Signed } func (s *standardStorageService) DeleteFolder(ctx context.Context, user *models.SignedInUser, cmd *DeleteFolderCmd) error { + guardian := s.authService.newGuardian(ctx, user, getFirstSegment(cmd.Path)) + if !guardian.canDelete(cmd.Path) { + return ErrAccessDenied + } + root, storagePath := s.tree.getRoot(getOrgId(user), cmd.Path) if root == nil { - return fmt.Errorf("resources storage is not enabled") + return ErrStorageNotFound } if root.Meta().ReadOnly { @@ -227,13 +273,18 @@ func (s *standardStorageService) DeleteFolder(ctx context.Context, user *models. if storagePath == "" { storagePath = filestorage.Delimiter } - return root.Store().DeleteFolder(ctx, storagePath, &filestorage.DeleteFolderOptions{Force: true}) + return root.Store().DeleteFolder(ctx, storagePath, &filestorage.DeleteFolderOptions{Force: true, AccessFilter: guardian.getPathFilter(ActionFilesDelete)}) } func (s *standardStorageService) CreateFolder(ctx context.Context, user *models.SignedInUser, cmd *CreateFolderCmd) error { + guardian := s.authService.newGuardian(ctx, user, getFirstSegment(cmd.Path)) + if !guardian.canWrite(cmd.Path) { + return ErrAccessDenied + } + root, storagePath := s.tree.getRoot(getOrgId(user), cmd.Path) if root == nil { - return fmt.Errorf("resources storage is not enabled") + return ErrStorageNotFound } if root.Meta().ReadOnly { @@ -248,9 +299,14 @@ func (s *standardStorageService) CreateFolder(ctx context.Context, user *models. } func (s *standardStorageService) Delete(ctx context.Context, user *models.SignedInUser, path string) error { + guardian := s.authService.newGuardian(ctx, user, getFirstSegment(path)) + if !guardian.canDelete(path) { + return ErrAccessDenied + } + root, storagePath := s.tree.getRoot(getOrgId(user), path) if root == nil { - return fmt.Errorf("resources storage is not enabled") + return ErrStorageNotFound } if root.Meta().ReadOnly { diff --git a/pkg/services/store/service_test.go b/pkg/services/store/service_test.go index 9c705621512..597d6a6bf93 100644 --- a/pkg/services/store/service_test.go +++ b/pkg/services/store/service_test.go @@ -16,29 +16,41 @@ import ( ) var ( - dummyUser = &models.SignedInUser{OrgId: 1} + dummyUser = &models.SignedInUser{OrgId: 1} + allowAllAuthService = newStaticStorageAuthService(func(ctx context.Context, user *models.SignedInUser, storageName string) map[string]filestorage.PathFilter { + return map[string]filestorage.PathFilter{ + ActionFilesDelete: allowAllPathFilter, + ActionFilesWrite: allowAllPathFilter, + ActionFilesRead: allowAllPathFilter, + } + }) + denyAllAuthService = newStaticStorageAuthService(func(ctx context.Context, user *models.SignedInUser, storageName string) map[string]filestorage.PathFilter { + return map[string]filestorage.PathFilter{ + ActionFilesDelete: denyAllPathFilter, + ActionFilesWrite: denyAllPathFilter, + ActionFilesRead: denyAllPathFilter, + } + }) + publicRoot, _ = filepath.Abs("../../../public") + publicStaticFilesStorage = newDiskStorage("public", "Public static files", &StorageLocalDiskConfig{ + Path: publicRoot, + Roots: []string{ + "/testdata/", + "/img/icons/", + "/img/bg/", + "/gazetteer/", + "/maps/", + "/upload/", + }, + }).setReadOnly(true).setBuiltin(true) ) func TestListFiles(t *testing.T) { - publicRoot, err := filepath.Abs("../../../public") - require.NoError(t, err) - roots := []storageRuntime{ - newDiskStorage("public", "Public static files", &StorageLocalDiskConfig{ - Path: publicRoot, - Roots: []string{ - "/testdata/", - "/img/icons/", - "/img/bg/", - "/gazetteer/", - "/maps/", - "/upload/", - }, - }).setReadOnly(true).setBuiltin(true), - } + roots := []storageRuntime{publicStaticFilesStorage} store := newStandardStorageService(sqlstore.InitTestDB(t), roots, func(orgId int64) []storageRuntime { return make([]storageRuntime, 0) - }) + }, allowAllAuthService) frame, err := store.List(context.Background(), dummyUser, "public/testdata") require.NoError(t, err) @@ -53,22 +65,38 @@ func TestListFiles(t *testing.T) { experimental.CheckGoldenJSONFrame(t, "testdata", "public_testdata_js_libraries.golden", testDsFrame, true) } -func setupUploadStore(t *testing.T) (StorageService, *filestorage.MockFileStorage, string) { +func TestListFilesWithoutPermissions(t *testing.T) { + roots := []storageRuntime{publicStaticFilesStorage} + + store := newStandardStorageService(sqlstore.InitTestDB(t), roots, func(orgId int64) []storageRuntime { + return make([]storageRuntime, 0) + }, denyAllAuthService) + frame, err := store.List(context.Background(), dummyUser, "public/testdata") + require.NoError(t, err) + rowLen, err := frame.RowLen() + require.NoError(t, err) + require.Equal(t, 0, rowLen) +} + +func setupUploadStore(t *testing.T, authService storageAuthService) (StorageService, *filestorage.MockFileStorage, string) { t.Helper() storageName := "resources" mockStorage := &filestorage.MockFileStorage{} sqlStorage := newSQLStorage(storageName, "Testing upload", &StorageSQLConfig{orgId: 1}, sqlstore.InitTestDB(t)) sqlStorage.store = mockStorage + if authService == nil { + authService = allowAllAuthService + } store := newStandardStorageService(sqlstore.InitTestDB(t), []storageRuntime{sqlStorage}, func(orgId int64) []storageRuntime { return make([]storageRuntime, 0) - }) + }, authService) return store, mockStorage, storageName } func TestShouldUploadWhenNoFileAlreadyExists(t *testing.T) { - service, mockStorage, storageName := setupUploadStore(t) + service, mockStorage, storageName := setupUploadStore(t, nil) mockStorage.On("Get", mock.Anything, "/myFile.jpg").Return(nil, nil) mockStorage.On("Upsert", mock.Anything, mock.Anything).Return(nil) @@ -82,8 +110,20 @@ func TestShouldUploadWhenNoFileAlreadyExists(t *testing.T) { require.NoError(t, err) } +func TestShouldFailUploadWithoutAccess(t *testing.T) { + service, _, storageName := setupUploadStore(t, denyAllAuthService) + + err := service.Upload(context.Background(), dummyUser, &UploadRequest{ + EntityType: EntityTypeImage, + Contents: make([]byte, 0), + Path: storageName + "/myFile.jpg", + MimeType: "image/jpg", + }) + require.ErrorIs(t, err, ErrAccessDenied) +} + func TestShouldFailUploadWhenFileAlreadyExists(t *testing.T) { - service, mockStorage, storageName := setupUploadStore(t) + service, mockStorage, storageName := setupUploadStore(t, nil) mockStorage.On("Get", mock.Anything, "/myFile.jpg").Return(&filestorage.File{Contents: make([]byte, 0)}, nil) @@ -97,7 +137,7 @@ func TestShouldFailUploadWhenFileAlreadyExists(t *testing.T) { } func TestShouldDelegateFileDeletion(t *testing.T) { - service, mockStorage, storageName := setupUploadStore(t) + service, mockStorage, storageName := setupUploadStore(t, nil) mockStorage.On("Delete", mock.Anything, "/myFile.jpg").Return(nil) @@ -106,7 +146,7 @@ func TestShouldDelegateFileDeletion(t *testing.T) { } func TestShouldDelegateFolderCreation(t *testing.T) { - service, mockStorage, storageName := setupUploadStore(t) + service, mockStorage, storageName := setupUploadStore(t, nil) mockStorage.On("CreateFolder", mock.Anything, "/nestedFolder/mostNestedFolder").Return(nil) @@ -115,9 +155,9 @@ func TestShouldDelegateFolderCreation(t *testing.T) { } func TestShouldDelegateFolderDeletion(t *testing.T) { - service, mockStorage, storageName := setupUploadStore(t) + service, mockStorage, storageName := setupUploadStore(t, nil) - mockStorage.On("DeleteFolder", mock.Anything, "/", &filestorage.DeleteFolderOptions{Force: true}).Return(nil) + mockStorage.On("DeleteFolder", mock.Anything, "/", mock.Anything).Return(nil) err := service.DeleteFolder(context.Background(), dummyUser, &DeleteFolderCmd{ Path: storageName, diff --git a/pkg/services/store/static_auth.go b/pkg/services/store/static_auth.go new file mode 100644 index 00000000000..e97db52c487 --- /dev/null +++ b/pkg/services/store/static_auth.go @@ -0,0 +1,41 @@ +package store + +import ( + "context" + + "github.com/grafana/grafana/pkg/infra/filestorage" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" +) + +type createPathFilterByAction func(ctx context.Context, user *models.SignedInUser, storageName string) map[string]filestorage.PathFilter + +func newStaticStorageAuthService(createPathFilterByAction createPathFilterByAction) storageAuthService { + return &staticStorageAuth{ + denyAllFileGuardian: &denyAllFileGuardian{}, + createPathFilterByAction: createPathFilterByAction, + log: log.New("staticStorageAuthService"), + } +} + +type staticStorageAuth struct { + log log.Logger + denyAllFileGuardian fileGuardian + createPathFilterByAction createPathFilterByAction +} + +func (a *staticStorageAuth) newGuardian(ctx context.Context, user *models.SignedInUser, storageName string) fileGuardian { + pathFilter := a.createPathFilterByAction(ctx, user, storageName) + + if pathFilter == nil { + return a.denyAllFileGuardian + } + + return &pathFilterFileGuardian{ + ctx: ctx, + user: user, + log: a.log, + prefix: storageName, + pathFilterByAction: pathFilter, + } +} diff --git a/pkg/services/store/tree.go b/pkg/services/store/tree.go index 5748fe5adc3..a28a7689410 100644 --- a/pkg/services/store/tree.go +++ b/pkg/services/store/tree.go @@ -85,7 +85,7 @@ func (t *nestedTree) GetFile(ctx context.Context, orgId int64, path string) (*fi return root.Store().Get(ctx, path) } -func (t *nestedTree) ListFolder(ctx context.Context, orgId int64, path string) (*StorageListFrame, error) { +func (t *nestedTree) ListFolder(ctx context.Context, orgId int64, path string, accessFilter filestorage.PathFilter) (*StorageListFrame, error) { if path == "" || path == "/" { t.assureOrgIsInitialized(orgId) @@ -150,6 +150,7 @@ func (t *nestedTree) ListFolder(ctx context.Context, orgId int64, path string) ( Recursive: false, WithFolders: true, WithFiles: true, + Filter: accessFilter, }) if err != nil { diff --git a/pkg/services/store/types.go b/pkg/services/store/types.go index db9cdfa8202..c076a60e7d0 100644 --- a/pkg/services/store/types.go +++ b/pkg/services/store/types.go @@ -30,7 +30,7 @@ type WriteValueResponse struct { type storageTree interface { GetFile(ctx context.Context, orgId int64, path string) (*filestorage.File, error) - ListFolder(ctx context.Context, orgId int64, path string) (*StorageListFrame, error) + ListFolder(ctx context.Context, orgId int64, path string, accessFilter filestorage.PathFilter) (*StorageListFrame, error) } //------------------------------------------- diff --git a/pkg/services/store/utils.go b/pkg/services/store/utils.go index 70f47349f39..f7fd341ac47 100644 --- a/pkg/services/store/utils.go +++ b/pkg/services/store/utils.go @@ -28,3 +28,8 @@ func getPathAndScope(c *models.ReqContext) (string, string) { } return splitFirstSegment(path) } + +func getFirstSegment(path string) string { + firstSegment, _ := splitFirstSegment(path) + return firstSegment +}