diff --git a/internal/config/config_test.go b/internal/config/config_test.go index cce67b7ff..f55afee23 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1037,8 +1037,8 @@ func agentConfig() *Config { }, }, AllowedDirectories: []string{ - "/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/", - "/usr/share/nginx/modules/", "/etc/app_protect/", + "/etc/nginx", "/etc/nginx-agent", "/usr/local/etc/nginx", "/var/run/nginx", "/var/log/nginx", + "/usr/share/nginx/modules", "/etc/app_protect", }, Collector: createDefaultCollectorConfig(), Command: &Command{ diff --git a/internal/config/types.go b/internal/config/types.go index e1831dfae..a5d07a210 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -9,9 +9,8 @@ import ( "context" "errors" "fmt" - "log/slog" "path/filepath" - "regexp" + "slices" "strings" "time" @@ -356,10 +355,7 @@ func (col *Collector) Validate(allowedDirectories []string) error { var err error cleanedConfPath := filepath.Clean(col.ConfigPath) - allowed, err := isAllowedDir(cleanedConfPath, allowedDirectories) - if err != nil { - return err - } + allowed := isAllowedDir(cleanedConfPath, allowedDirectories) if !allowed { err = errors.Join(err, fmt.Errorf("collector path %s not allowed", col.ConfigPath)) } @@ -378,10 +374,7 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error { } for _, al := range nr.AccessLogs { - allowed, allowedError := isAllowedDir(al.FilePath, allowedDirectories) - if allowedError != nil { - err = errors.Join(err, fmt.Errorf("invalid nginx receiver access log path: %s", al.FilePath)) - } + allowed := isAllowedDir(al.FilePath, allowedDirectories) if !allowed { err = errors.Join(err, fmt.Errorf("nginx receiver access log path %s not allowed", al.FilePath)) } @@ -396,13 +389,9 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error { return err } -func (c *Config) IsDirectoryAllowed(directory string) bool { - allow, err := isAllowedDir(directory, c.AllowedDirectories) - if err != nil { - slog.Warn("Unable to determine if directory is allowed", "error", err) - return false - } - +// IsAllowedDirectory checks if the given path is in the list of allowed directories. +func (c *Config) IsDirectoryAllowed(path string) bool { + allow := isAllowedDir(path, c.AllowedDirectories) return allow } @@ -480,32 +469,19 @@ func (c *Config) IsCommandServerProxyConfigured() bool { } // isAllowedDir checks if the given path is in the list of allowed directories. -// It returns true if the path is allowed, false otherwise. -// If the path is allowed but does not exist, it also logs a warning. -// It also checks if the path is a file, in which case it checks the parent directory of the file. -func isAllowedDir(path string, allowedDirs []string) (bool, error) { - if len(allowedDirs) == 0 { - return false, errors.New("no allowed directories configured") - } - - directoryPath := path - // Check if the path is a file, regex matches when end of string is /. - isFilePath, err := regexp.MatchString(`/(\w+)\.(\w+)$`, directoryPath) - if err != nil { - return false, errors.New("error matching path" + directoryPath) - } +// It recursively checks the parent directories of the path, until it finds a match or reaches the root directory. +func isAllowedDir(path string, allowedDirs []string) bool { + return checkDirIsAllowed(filepath.Clean(path), allowedDirs) +} - if isFilePath { - directoryPath = filepath.Dir(directoryPath) +func checkDirIsAllowed(path string, allowedDirs []string) bool { + if slices.Contains(allowedDirs, path) { + return true } - for _, allowedDirectory := range allowedDirs { - // Check if the directoryPath starts with the allowedDirectory - // This allows for subdirectories within the allowed directories. - if strings.HasPrefix(directoryPath, allowedDirectory) { - return true, nil - } + if path == "/" || !strings.HasPrefix(path, "/") { // root directory reached with no match, path is not allowed + return false } - return false, nil + return checkDirIsAllowed(filepath.Dir(path), allowedDirs) } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index d41872305..d2802ac31 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -66,12 +66,69 @@ func TestTypes_isAllowedDir(t *testing.T) { }, filePath: "/not-nginx-test/idontexist.conf", }, + { + name: "Test 7: Prefix match not allowed", + allowed: false, + allowedDirs: []string{ + "/etc/nginx", + }, + filePath: "/etc/nginx-test/nginx.conf", + }, + { + name: "Test 8: Empty allowed directories", + allowed: false, + allowedDirs: []string{ + "", + }, + filePath: "/etc/nginx/nginx.conf", + }, + { + name: "Test 9: Multiple allowed directories, file in one of them", + allowed: true, + allowedDirs: []string{ + "/etc/nginx", + "/usr/local/nginx", + }, + filePath: "/usr/local/nginx/nginx.conf", + }, + { + name: "Test 10: Multiple allowed directories, file not in any of them", + allowed: false, + allowedDirs: []string{ + "/etc/nginx", + "/usr/local/nginx", + }, + filePath: "/opt/nginx/nginx.conf", + }, + { + name: "Test 11: Path is root directory, not in allowed directories", + allowed: false, + allowedDirs: []string{ + "/etc/nginx", + }, + filePath: "/", + }, + { + name: "Test 12: File is in directory nested 5 levels deep within allowed directory", + allowed: true, + allowedDirs: []string{ + "/etc/nginx", + }, + filePath: "/etc/nginx/level1/level2/level3/level4/nginx.conf", + }, + { + name: "Test 13: Root directory is allowed, file in subdirectory", + allowed: true, + allowedDirs: []string{ + "/", + }, + filePath: "/etc/nginx/nginx.conf", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - result, err := isAllowedDir(test.filePath, test.allowedDirs) - require.NoError(t, err) + result := isAllowedDir(test.filePath, test.allowedDirs) require.Equal(t, test.allowed, result) }) } diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 5b0593f28..541530b48 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -74,7 +74,7 @@ func NewNginxConfigParser(agentConfig *config.Config) *NginxConfigParser { } func (ncp *NginxConfigParser) Parse(ctx context.Context, instance *mpi.Instance) (*model.NginxConfigContext, error) { - configPath := instance.GetInstanceRuntime().GetConfigPath() + configPath, _ := filepath.Abs(instance.GetInstanceRuntime().GetConfigPath()) if !ncp.agentConfig.IsDirectoryAllowed(configPath) { return nil, fmt.Errorf("config path %s is not in allowed directories", configPath) diff --git a/internal/datasource/config/nginx_config_parser_benchmark_test.go b/internal/datasource/config/nginx_config_parser_benchmark_test.go index bfd3e2651..ed0dc2f3e 100644 --- a/internal/datasource/config/nginx_config_parser_benchmark_test.go +++ b/internal/datasource/config/nginx_config_parser_benchmark_test.go @@ -35,8 +35,9 @@ func BenchmarkNginxConfigParser_Parse(b *testing.B) { for _, configFilePath := range configFilePaths { func(configFilePath string) { b.Run(configFilePath, func(bb *testing.B) { + absPath, _ := filepath.Abs(configFilePath) agentConfig.AllowedDirectories = []string{ - filepath.Dir(configFilePath), + filepath.Dir(absPath), } nginxConfigParser := NewNginxConfigParser( diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 6492e8c47..ca12f820f 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -344,6 +344,12 @@ func (fms *FileManagerService) DetermineFileActions( } fileContents[fileName] = fileContent + // Allowed directories could have been modified since file was created, + // so we should check before marking for deletion + if !fms.agentConfig.IsDirectoryAllowed(fileName) { + return nil, nil, fmt.Errorf("error deleting file %s: file not in allowed directories", fileName) + } + fileDiff[fileName] = &model.FileCache{ File: manifestFile, Action: model.Delete, diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index 5a2644080..a867c02f5 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -462,7 +462,7 @@ func TestFileManagerService_Rollback(t *testing.T) { func TestFileManagerService_DetermineFileActions(t *testing.T) { ctx := context.Background() - tempDir := os.TempDir() + tempDir := filepath.Clean(os.TempDir()) deleteTestFile := helpers.CreateFileWithErrorCheck(t, tempDir, "nginx_delete.conf") defer helpers.RemoveFileWithErrorCheck(t, deleteTestFile.Name()) @@ -498,9 +498,11 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { expectedCache map[string]*model.FileCache expectedContent map[string][]byte name string + allowedDirs []string }{ { - name: "Test 1: Add, Update & Delete Files", + name: "Test 1: Add, Update & Delete Files", + allowedDirs: []string{tempDir}, modifiedFiles: map[string]*model.FileCache{ addTestFileName: { File: &mpi.File{ @@ -563,7 +565,8 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { expectedError: nil, }, { - name: "Test 2: Files same as on disk", + name: "Test 2: Files same as on disk", + allowedDirs: []string{tempDir}, modifiedFiles: map[string]*model.FileCache{ addTestFile.Name(): { File: &mpi.File{ @@ -598,6 +601,7 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { }, { name: "Test 3: File being deleted already doesn't exist", + allowedDirs: []string{tempDir}, modifiedFiles: make(map[string]*model.FileCache), currentFiles: map[string]*mpi.File{ "/unknown/file.conf": { @@ -620,6 +624,7 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { fakeFileServiceClient := &v1fakes.FakeFileServiceClient{} fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{}) + fileManagerService.agentConfig.AllowedDirectories = test.allowedDirs fileManagerService.agentConfig.LibDir = manifestDirPath fileManagerService.manifestFilePath = manifestFilePath @@ -794,6 +799,7 @@ func TestFileManagerService_UpdateManifestFile(t *testing.T) { fakeFileServiceClient := &v1fakes.FakeFileServiceClient{} fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{}) + fileManagerService.agentConfig.AllowedDirectories = []string{"manifestDirPath"} fileManagerService.agentConfig.LibDir = manifestDirPath fileManagerService.manifestFilePath = file.Name() diff --git a/test/types/config.go b/test/types/config.go index 73d7e15ac..f544b8926 100644 --- a/test/types/config.go +++ b/test/types/config.go @@ -62,7 +62,7 @@ func AgentConfig() *config.Config { Multiplier: commonMultiplier, }, }, - AllowedDirectories: []string{"/tmp/"}, + AllowedDirectories: []string{"/tmp"}, Collector: &config.Collector{ ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml", Exporters: config.Exporters{