From 7c4ac4ece5ac5cf547072580d014a1382bc48003 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Thu, 19 Jun 2025 17:37:38 +0100 Subject: [PATCH 01/28] check for invalid characters in allowedDirectories when loading config --- internal/config/config.go | 52 +++++++++++++--------- internal/config/config_test.go | 79 ++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 20 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 85cdc6a71..8fe6ba1d9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,7 @@ import ( "log/slog" "os" "path/filepath" + "regexp" "slices" "strconv" "strings" @@ -32,11 +33,12 @@ import ( ) const ( - ConfigFileName = "nginx-agent.conf" - EnvPrefix = "NGINX_AGENT" - KeyDelimiter = "_" - KeyValueNumber = 2 - AgentDirName = "/etc/nginx-agent/" + ConfigFileName = "nginx-agent.conf" + EnvPrefix = "NGINX_AGENT" + KeyDelimiter = "_" + KeyValueNumber = 2 + AgentDirName = "/etc/nginx-agent/" + regexInvalidPath = "%[0-9*][a-fA-F]|\\\\x[0-9]*|\\.\\./" ) var viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter)) @@ -87,19 +89,7 @@ func ResolveConfig() (*Config, error) { slogger := logger.New(log.Path, log.Level) slog.SetDefault(slogger) - // Check directories in allowed_directories are valid - for _, dir := range directories { - if dir == "" || !filepath.IsAbs(dir) { - slog.Warn("Invalid directory: ", "dir", dir) - continue - } - - if !strings.HasSuffix(dir, "/") { - dir += "/" - } - allowedDirs = append(allowedDirs, dir) - } - + allowedDirs = resolveAllowedDirectories(directories) slog.Info("Configured allowed directories", "allowed_directories", allowedDirs) // Collect all parsing errors before returning the error, so the user sees all issues with config @@ -129,13 +119,35 @@ func ResolveConfig() (*Config, error) { checkCollectorConfiguration(collector, config) + slog.Info( + "Excluded files from being watched for file changes", + "exclude_files", + config.Watchers.FileWatcher.ExcludeFiles, + ) + slog.Debug("Agent config", "config", config) - slog.Info("Excluded files from being watched for file changes", "exclude_files", - config.Watchers.FileWatcher.ExcludeFiles) return config, nil } +func resolveAllowedDirectories(dirs []string) []string { + // Check directories are valid + var allowed []string + for _, dir := range dirs { + invalidChars, _ := regexp.MatchString(regexInvalidPath, dir) + if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars { + slog.Warn("Ignoring invalid directory", "dir", dir) + continue + } + dir = filepath.Clean(dir) + if !strings.HasSuffix(dir, "/") { + dir += "/" + } + allowed = append(allowed, dir) + } + return allowed +} + func checkCollectorConfiguration(collector *Collector, config *Config) { if isOTelExporterConfigured(collector) && config.IsGrpcClientConfigured() && config.IsAuthConfigured() && config.IsTLSConfigured() { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6ec1f7775..ffa804606 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -158,6 +158,85 @@ func TestNormalizeFunc(t *testing.T) { assert.Equal(t, expected, result) } +func TestResolveAllowedDirectories(t *testing.T) { + tests := []struct { + name string + configuredDirs []string + expected []string + }{ + { + name: "Empty path", + configuredDirs: []string{""}, + expected: []string(nil), + }, + { + name: "Absolute path", + configuredDirs: []string{"/etc/nginx-agent"}, + expected: []string{"/etc/nginx-agent/"}, + }, + { + name: "Absolute paths", + configuredDirs: []string{"/etc/nginx-agent", "/etc/nginx/"}, + expected: []string{"/etc/nginx-agent/", "/etc/nginx/"}, + }, + { + name: "Mixed paths", + configuredDirs: []string{ + "nginx-agent", + "", + "..", + "/", + "\\/", + ".", + "/etc/nginx/", + }, + expected: []string{"/etc/nginx/"}, + }, + { + name: "Absolute path", + configuredDirs: []string{"/etc/nginx-agent"}, + expected: []string{"/etc/nginx-agent/"}, + }, + { + name: "Relative path", + configuredDirs: []string{"nginx-agent"}, + expected: []string(nil), + }, + { + name: "Absolute path with directory traversal", + configuredDirs: []string{"/etc/nginx/../nginx-agent"}, + expected: []string(nil), + }, + { + name: "Absolute path with repeat directory traversal", + configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"}, + expected: []string(nil), + }, + { + name: "Absolute path with unicode characters", + configuredDirs: []string{"/etc/nginx-agent/%2e%2e/tmp/"}, + expected: []string(nil), + }, + { + name: "Absolute path with control characters", + configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"}, + expected: []string(nil), + }, + { + name: "Absolute path with escaped control characters", + configuredDirs: []string{"/etc/nginx-agent/\\\\x08/tmp/"}, + expected: []string(nil), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + allowed := make([]string, 0, len(test.configuredDirs)) + allowed = resolveAllowedDirectories(test.configuredDirs) + assert.Equal(t, test.expected, allowed) + }) + } +} + func TestResolveLog(t *testing.T) { viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter)) viperInstance.Set(LogLevelKey, "error") From d64f804095f62974e90e6a9e84ac1a52cd6ce9ae Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 24 Jun 2025 17:02:24 +0100 Subject: [PATCH 02/28] add sanitising of allowed directories when loading configuration --- go.mod | 2 +- internal/config/config.go | 36 +++-- internal/config/config_test.go | 252 +++++++++++++++++---------------- internal/config/defaults.go | 2 +- internal/config/types.go | 35 ++++- internal/config/types_test.go | 118 +++++++++------ 6 files changed, 267 insertions(+), 178 deletions(-) diff --git a/go.mod b/go.mod index 434b15a13..de2d1ce13 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/nginx/agent/v3 go 1.23.7 -toolchain go1.23.10 +toolchain go1.24.4 require ( buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.4-20250130201111-63bb56e20495.1 diff --git a/internal/config/config.go b/internal/config/config.go index 8fe6ba1d9..170b2a5a2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -33,12 +33,15 @@ import ( ) const ( - ConfigFileName = "nginx-agent.conf" - EnvPrefix = "NGINX_AGENT" - KeyDelimiter = "_" - KeyValueNumber = 2 - AgentDirName = "/etc/nginx-agent/" - regexInvalidPath = "%[0-9*][a-fA-F]|\\\\x[0-9]*|\\.\\./" + ConfigFileName = "nginx-agent.conf" + EnvPrefix = "NGINX_AGENT" + KeyDelimiter = "_" + KeyValueNumber = 2 + AgentDirName = "/etc/nginx-agent" + + // Regular expression to match invalid characters in paths. + // It matches whitespace, control characters, non-printable characters, and specific Unicode characters. + regexInvalidPath = "\\s|[[:cntrl:]]|[[:space:]]|[[^:print:]]|ㅤ|\\.\\.|\\*" ) var viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter)) @@ -81,15 +84,14 @@ func RegisterConfigFile() error { } func ResolveConfig() (*Config, error) { - // Collect allowed directories, so that paths in the config can be validated. - directories := viperInstance.GetStringSlice(AllowedDirectoriesKey) - allowedDirs := []string{AgentDirName} log := resolveLog() slogger := logger.New(log.Path, log.Level) slog.SetDefault(slogger) - allowedDirs = resolveAllowedDirectories(directories) + // Collect allowed directories, so that paths in the config can be validated. + directories := viperInstance.GetStringSlice(AllowedDirectoriesKey) + allowedDirs := resolveAllowedDirectories(directories) slog.Info("Configured allowed directories", "allowed_directories", allowedDirs) // Collect all parsing errors before returning the error, so the user sees all issues with config @@ -130,9 +132,12 @@ func ResolveConfig() (*Config, error) { return config, nil } +// resolveAllowedDirectories checks if the provided directories are valid and returns a slice of cleaned absolute paths. +// It ignores empty paths, paths that are not absolute, and paths containing invalid characters. +// Invalid paths are logged as warnings. func resolveAllowedDirectories(dirs []string) []string { - // Check directories are valid var allowed []string + allowed = append(allowed, AgentDirName) for _, dir := range dirs { invalidChars, _ := regexp.MatchString(regexInvalidPath, dir) if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars { @@ -140,8 +145,9 @@ func resolveAllowedDirectories(dirs []string) []string { continue } dir = filepath.Clean(dir) - if !strings.HasSuffix(dir, "/") { - dir += "/" + if dir == AgentDirName { + slog.Warn("Ignoring reserved directory", "dir", dir) + continue } allowed = append(allowed, dir) } @@ -756,13 +762,15 @@ func resolveClient() *Client { } func resolveCollector(allowedDirs []string) (*Collector, error) { - var receivers Receivers + // Collect receiver configurations + var receivers Receivers err := resolveMapStructure(CollectorReceiversKey, &receivers) if err != nil { return nil, fmt.Errorf("unmarshal collector receivers config: %w", err) } + // Collect exporter configurations exporters, err := resolveExporters() if err != nil { return nil, fmt.Errorf("unmarshal collector exporters config: %w", err) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ffa804606..ee81e8c70 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -167,65 +167,65 @@ func TestResolveAllowedDirectories(t *testing.T) { { name: "Empty path", configuredDirs: []string{""}, - expected: []string(nil), + expected: []string{"/etc/nginx-agent"}, }, { name: "Absolute path", - configuredDirs: []string{"/etc/nginx-agent"}, - expected: []string{"/etc/nginx-agent/"}, + configuredDirs: []string{"/etc/agent/"}, + expected: []string{"/etc/nginx-agent", "/etc/agent"}, }, { name: "Absolute paths", - configuredDirs: []string{"/etc/nginx-agent", "/etc/nginx/"}, - expected: []string{"/etc/nginx-agent/", "/etc/nginx/"}, + configuredDirs: []string{"/etc/nginx/"}, + expected: []string{"/etc/nginx-agent", "/etc/nginx"}, }, { - name: "Mixed paths", - configuredDirs: []string{ - "nginx-agent", - "", - "..", - "/", - "\\/", - ".", - "/etc/nginx/", - }, - expected: []string{"/etc/nginx/"}, - }, - { - name: "Absolute path", - configuredDirs: []string{"/etc/nginx-agent"}, - expected: []string{"/etc/nginx-agent/"}, - }, - { - name: "Relative path", - configuredDirs: []string{"nginx-agent"}, - expected: []string(nil), + name: "Absolute path with multiple slashes", + configuredDirs: []string{"/etc///////////nginx-agent/"}, + expected: []string{"/etc/nginx-agent"}, }, { name: "Absolute path with directory traversal", configuredDirs: []string{"/etc/nginx/../nginx-agent"}, - expected: []string(nil), + expected: []string{"/etc/nginx-agent"}, }, { name: "Absolute path with repeat directory traversal", configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"}, - expected: []string(nil), - }, - { - name: "Absolute path with unicode characters", - configuredDirs: []string{"/etc/nginx-agent/%2e%2e/tmp/"}, - expected: []string(nil), + expected: []string{"/etc/nginx-agent"}, }, { name: "Absolute path with control characters", configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"}, - expected: []string(nil), + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Absolute path with invisible characters", + configuredDirs: []string{"/etc/nginx-agent/ㅤㅤㅤ/tmp/"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Absolute path with escaped invisible characters", + configuredDirs: []string{"/etc/nginx-agent/\\\\ㅤ/tmp/"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Mixed paths", + configuredDirs: []string{ + "nginx-agent", + "", + "..", + "/", + "\\/", + ".", + "/etc/nginx/", + }, + expected: []string{"/etc/nginx-agent", "/etc/nginx"}, }, { - name: "Absolute path with escaped control characters", - configuredDirs: []string{"/etc/nginx-agent/\\\\x08/tmp/"}, - expected: []string(nil), + name: "Relative path", + configuredDirs: []string{"nginx-agent"}, + expected: []string{"/etc/nginx-agent"}, }, } for _, test := range tests { @@ -843,7 +843,7 @@ func agentConfig() *Config { return &Config{ UUID: "", Version: "", - Path: "", + Path: "testdata/agent.conf", Log: &Log{}, Client: &Client{ HTTP: &HTTP{ @@ -868,87 +868,14 @@ func agentConfig() *Config { }, }, AllowedDirectories: []string{ - "/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/", - "/usr/share/nginx/modules/", - }, - Collector: &Collector{ - ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml", - Exporters: Exporters{ - OtlpExporters: []OtlpExporter{ - { - Server: &ServerConfig{ - Host: "127.0.0.1", - Port: 1234, - Type: Grpc, - }, - TLS: &TLSConfig{ - Cert: "/path/to/server-cert.pem", - Key: "/path/to/server-cert.pem", - Ca: "/path/to/server-cert.pem", - SkipVerify: true, - ServerName: "remote-saas-server", - }, - }, - }, - }, - Processors: Processors{ - Batch: &Batch{ - SendBatchMaxSize: DefCollectorBatchProcessorSendBatchMaxSize, - SendBatchSize: DefCollectorBatchProcessorSendBatchSize, - Timeout: DefCollectorBatchProcessorTimeout, - }, - }, - Receivers: Receivers{ - OtlpReceivers: []OtlpReceiver{ - { - Server: &ServerConfig{ - Host: "localhost", - Port: 4317, - Type: Grpc, - }, - Auth: &AuthConfig{ - Token: "even-secreter-token", - }, - OtlpTLSConfig: &OtlpTLSConfig{ - GenerateSelfSignedCert: false, - Cert: "/path/to/server-cert.pem", - Key: "/path/to/server-cert.pem", - Ca: "/path/to/server-cert.pem", - SkipVerify: true, - ServerName: "local-data-plane-server", - }, - }, - }, - NginxReceivers: []NginxReceiver{ - { - InstanceID: "cd7b8911-c2c5-4daf-b311-dbead151d938", - StubStatus: APIDetails{ - URL: "http://localhost:4321/status", - Listen: "", - }, - AccessLogs: []AccessLog{ - { - LogFormat: accessLogFormat, - FilePath: "/var/log/nginx/access-custom.conf", - }, - }, - }, - }, - }, - Extensions: Extensions{ - Health: &Health{ - Server: &ServerConfig{ - Host: "localhost", - Port: 1337, - }, - Path: "/", - }, - }, - Log: &Log{ - Level: "INFO", - Path: "/var/log/nginx-agent/opentelemetry-collector-agent.log", - }, + "/etc/nginx", + "/etc/nginx-agent", + "/usr/local/etc/nginx", + "/var/run/nginx", + "/var/log/nginx", + "/usr/share/nginx/modules", }, + Collector: createDefaultCollectorConfig(), Command: &Command{ Server: &ServerConfig{ Host: "127.0.0.1", @@ -1001,8 +928,12 @@ func createConfig() *Config { }, }, AllowedDirectories: []string{ - "/etc/nginx-agent/", "/etc/nginx/", "/usr/local/etc/nginx/", "/var/run/nginx/", - "/usr/share/nginx/modules/", "/var/log/nginx/", + "/etc/nginx-agent", + "/etc/nginx", + "/usr/local/etc/nginx", + "/var/run/nginx", + "/usr/share/nginx/modules", + "/var/log/nginx", }, DataPlaneConfig: &DataPlaneConfig{ Nginx: &NginxDataPlaneConfig{ @@ -1184,3 +1115,84 @@ func createConfig() *Config { }, } } + +func createDefaultCollectorConfig() *Collector { + return &Collector{ + ConfigPath: "/etc/nginx-agent/testdata/nginx-agent-otelcol.yaml", + Exporters: Exporters{ + OtlpExporters: []OtlpExporter{ + { + Server: &ServerConfig{ + Host: "127.0.0.1", + Port: 1234, + Type: Grpc, + }, + TLS: &TLSConfig{ + Cert: "/path/to/server-cert.pem", + Key: "/path/to/server-cert.pem", + Ca: "/path/to/server-cert.pem", + SkipVerify: true, + ServerName: "remote-saas-server", + }, + }, + }, + }, + Processors: Processors{ + Batch: &Batch{ + SendBatchMaxSize: DefCollectorBatchProcessorSendBatchMaxSize, + SendBatchSize: DefCollectorBatchProcessorSendBatchSize, + Timeout: DefCollectorBatchProcessorTimeout, + }, + }, + Receivers: Receivers{ + OtlpReceivers: []OtlpReceiver{ + { + Server: &ServerConfig{ + Host: "localhost", + Port: 4317, + Type: Grpc, + }, + Auth: &AuthConfig{ + Token: "even-secreter-token", + }, + OtlpTLSConfig: &OtlpTLSConfig{ + GenerateSelfSignedCert: false, + Cert: "/path/to/server-cert.pem", + Key: "/path/to/server-cert.pem", + Ca: "/path/to/server-cert.pem", + SkipVerify: true, + ServerName: "local-data-plane-server", + }, + }, + }, + NginxReceivers: []NginxReceiver{ + { + InstanceID: "cd7b8911-c2c5-4daf-b311-dbead151d938", + StubStatus: APIDetails{ + URL: "http://localhost:4321/status", + Listen: "", + }, + AccessLogs: []AccessLog{ + { + LogFormat: accessLogFormat, + FilePath: "/var/log/nginx/access-custom.conf", + }, + }, + }, + }, + }, + Extensions: Extensions{ + Health: &Health{ + Server: &ServerConfig{ + Host: "localhost", + Port: 1337, + }, + Path: "/", + }, + }, + Log: &Log{ + Level: "INFO", + Path: "/var/log/nginx-agent/opentelemetry-collector-agent.log", + }, + } +} diff --git a/internal/config/defaults.go b/internal/config/defaults.go index ddeee7a3b..77ceb2f1a 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -54,7 +54,7 @@ const ( DefFileWatcherMonitoringFrequency = 5 * time.Second // Collector defaults - DefCollectorConfigPath = "/etc/nginx-agent/opentelemetry-collector-agent.yaml" + DefCollectorConfigPath = "/opt/homebrew/etc/nginx-agent/opentelemetry-collector-agent.yaml" DefCollectorLogLevel = "INFO" DefCollectorLogPath = "/var/log/nginx-agent/opentelemetry-collector-agent.log" DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem" diff --git a/internal/config/types.go b/internal/config/types.go index 8017987f1..051e13a77 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -8,7 +8,10 @@ package config import ( "errors" "fmt" + "log/slog" + "os" "path/filepath" + "regexp" "strings" "time" @@ -411,9 +414,37 @@ func (c *Config) AreReceiversConfigured() bool { len(c.Collector.Receivers.TcplogReceivers) > 0 } -func isAllowedDir(dir string, allowedDirs []string) 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 does not exist, it logs a warning and returns false. +// It also checks if the path is a file, in which case it checks the directory of the file. +func isAllowedDir(path string, allowedDirs []string) bool { + if len(allowedDirs) == 0 { + slog.Warn("No allowed directories configured") + return false + } + + directoryPath := path + isFilePath, err := regexp.MatchString(`\.(\w+)$`, directoryPath) + if err != nil { + slog.Error("Error matching path", "path", directoryPath, "error", err) + return false + } + + if isFilePath { + directoryPath = filepath.Dir(directoryPath) + slog.Debug("File path detected, checking parent directory is allowed", "path", directoryPath) + } + + _, err = os.Stat(directoryPath) + if err != nil { + slog.Warn("Path error", "path", path, "error", err) + } + for _, allowedDirectory := range allowedDirs { - if strings.HasPrefix(dir, allowedDirectory) { + // Check if the directoryPath starts with the allowedDirectory + // This allows for subdirectories within the allowed directories. + if strings.HasPrefix(directoryPath, allowedDirectory) { return true } } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 46e2c7f61..735acd6fc 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -6,87 +6,125 @@ package config import ( + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" ) -func TestTypes_IsDirectoryAllowed(t *testing.T) { - config := agentConfig() +func TestTypes_isAllowedDir(t *testing.T) { tests := []struct { name string - fileDir string + filePath string + dirExists bool + fileExists bool allowedDirs []string allowed bool }{ { - name: "Test 1: directory allowed", - allowed: true, + name: "File exists and is in allowed directory", + allowed: true, + dirExists: true, + fileExists: true, allowedDirs: []string{ - AgentDirName, "/etc/nginx", - "/var/log/nginx/", }, - fileDir: "/etc/nginx/nginx.conf", + filePath: "/etc/nginx/nginx.conf", }, { - name: "Test 2: directory not allowed", - allowed: false, + name: "File exists and is in allowed directory with hyphen", + allowed: true, + dirExists: true, + fileExists: true, allowedDirs: []string{ - AgentDirName, - "/etc/nginx/", - "/var/log/nginx/", + "/etc/nginx-agent", }, - fileDir: "/etc/nginx-test/nginx-agent.conf", + filePath: "/etc/nginx-agent/nginx.conf", }, { - name: "Test 3: directory allowed", - allowed: true, + name: "File exists and is in a subdirectory of allowed directory", + allowed: true, + dirExists: true, + fileExists: true, allowedDirs: []string{ - AgentDirName, - "/etc/nginx/", - "/var/log/nginx/", + "/etc/nginx", }, - fileDir: "/etc/nginx/conf.d/nginx-agent.conf", + filePath: "/etc/nginx/conf.d/nginx.conf", }, { - name: "Test 4: directory not allowed", - allowed: false, + name: "File exists and is outside allowed directory", + allowed: false, + dirExists: true, + fileExists: true, allowedDirs: []string{ - AgentDirName, "/etc/nginx", - "/var/log/nginx", }, - fileDir: "~/test.conf", + filePath: "/etc/test/nginx.conf", }, { - name: "Test 5: directory not allowed", - allowed: false, + name: "File does not exist but is in allowed directory", + allowed: true, + dirExists: true, + fileExists: false, allowedDirs: []string{ - AgentDirName, - "/etc/nginx/", - "/var/log/nginx/", + "/etc/nginx", }, - fileDir: "//test.conf", + filePath: "/etc/nginx/idontexist.conf", }, { - name: "Test 6: directory allowed", - allowed: true, + name: "File does not exist and is outside allowed directory", + allowed: false, + dirExists: false, + fileExists: true, allowedDirs: []string{ - AgentDirName, - "/etc/nginx/", - "/var/log/nginx/", - "/", + "/etc/nginx", }, - fileDir: "/test.conf", + filePath: "/not-nginx-test/idontexist.conf", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - config.AllowedDirectories = test.allowedDirs - result := config.IsDirectoryAllowed(test.fileDir) + if test.dirExists { + // Create the temporary directory for testing + tmpDir, err := os.MkdirTemp("", "test-allowed-dir") + defer func() { + err := os.RemoveAll(tmpDir) + if err != nil { + t.Fatalf("Failed to remove temp directory: %v", err) + } + }() + if err != nil { + t.Fatalf("Failed to create temp directory: %v", err) + } + + // Prepend the temporary directory to the allowed directories for testing + for i, dir := range test.allowedDirs { + if dir != "" && dir[0] != '/' { + test.allowedDirs[i] = tmpDir + "/" + dir + } else { + test.allowedDirs[i] = tmpDir + dir + } + } + + // Prepend the temporary directory to the fileDir for testing + test.filePath = tmpDir + test.filePath + + // Create the parent directories + if err := os.MkdirAll(filepath.Dir(test.filePath), 0755); err != nil { + t.Fatalf("Failed to create directory for file: %v", err) + } + + // Create the test file if it should exist + if test.fileExists { + if _, err := os.Create(test.filePath); err != nil { + t.Fatalf("Failed to create test file: %v", err) + } + } + } + result := isAllowedDir(test.filePath, test.allowedDirs) assert.Equal(t, test.allowed, result) }) } From 88d04e2668f070741ac5c05eabb625ec1558ea3e Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 24 Jun 2025 17:04:41 +0100 Subject: [PATCH 03/28] remove log --- internal/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 170b2a5a2..8ff22aedc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -146,7 +146,7 @@ func resolveAllowedDirectories(dirs []string) []string { } dir = filepath.Clean(dir) if dir == AgentDirName { - slog.Warn("Ignoring reserved directory", "dir", dir) + // If the directory is the default agent directory, we skip adding it again. continue } allowed = append(allowed, dir) From 5c41b2ae52e7d457c23a4a229029c5a4decebb39 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 25 Jun 2025 11:23:23 +0100 Subject: [PATCH 04/28] fix lint issues --- internal/config/config.go | 12 ++++---- internal/config/config_test.go | 3 +- internal/config/types_test.go | 55 +++++++++++++++++----------------- 3 files changed, 36 insertions(+), 34 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 8ff22aedc..fb7b6eeaa 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -84,7 +84,6 @@ func RegisterConfigFile() error { } func ResolveConfig() (*Config, error) { - log := resolveLog() slogger := logger.New(log.Path, log.Level) slog.SetDefault(slogger) @@ -136,10 +135,14 @@ func ResolveConfig() (*Config, error) { // It ignores empty paths, paths that are not absolute, and paths containing invalid characters. // Invalid paths are logged as warnings. func resolveAllowedDirectories(dirs []string) []string { - var allowed []string - allowed = append(allowed, AgentDirName) + allowed := []string{AgentDirName} for _, dir := range dirs { - invalidChars, _ := regexp.MatchString(regexInvalidPath, dir) + re, err := regexp.Compile(regexInvalidPath) + if err != nil { + slog.Error("Failed to compile regex for invalid path characters", "error", err) + continue + } + invalidChars := re.MatchString(dir) if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars { slog.Warn("Ignoring invalid directory", "dir", dir) continue @@ -762,7 +765,6 @@ func resolveClient() *Client { } func resolveCollector(allowedDirs []string) (*Collector, error) { - // Collect receiver configurations var receivers Receivers err := resolveMapStructure(CollectorReceiversKey, &receivers) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ee81e8c70..b4be5c661 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -230,8 +230,7 @@ func TestResolveAllowedDirectories(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - allowed := make([]string, 0, len(test.configuredDirs)) - allowed = resolveAllowedDirectories(test.configuredDirs) + allowed := resolveAllowedDirectories(test.configuredDirs) assert.Equal(t, test.expected, allowed) }) } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 735acd6fc..e705a5d3a 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -13,14 +13,15 @@ import ( "github.com/stretchr/testify/assert" ) +// nolint: nestif, gocognit func TestTypes_isAllowedDir(t *testing.T) { tests := []struct { name string filePath string + allowedDirs []string dirExists bool fileExists bool - allowedDirs []string allowed bool }{ { @@ -89,39 +90,24 @@ func TestTypes_isAllowedDir(t *testing.T) { t.Run(test.name, func(t *testing.T) { if test.dirExists { // Create the temporary directory for testing - tmpDir, err := os.MkdirTemp("", "test-allowed-dir") + tmpDir, err := createTempDir(t, "test-allowed-dir") defer func() { - err := os.RemoveAll(tmpDir) - if err != nil { - t.Fatalf("Failed to remove temp directory: %v", err) + rmErr := os.RemoveAll(tmpDir) + if rmErr != nil { + t.Log(rmErr) } }() - if err != nil { - t.Fatalf("Failed to create temp directory: %v", err) - } - - // Prepend the temporary directory to the allowed directories for testing - for i, dir := range test.allowedDirs { - if dir != "" && dir[0] != '/' { - test.allowedDirs[i] = tmpDir + "/" + dir - } else { - test.allowedDirs[i] = tmpDir + dir - } - } - - // Prepend the temporary directory to the fileDir for testing - test.filePath = tmpDir + test.filePath - - // Create the parent directories - if err := os.MkdirAll(filepath.Dir(test.filePath), 0755); err != nil { - t.Fatalf("Failed to create directory for file: %v", err) - } // Create the test file if it should exist if test.fileExists { - if _, err := os.Create(test.filePath); err != nil { - t.Fatalf("Failed to create test file: %v", err) + // Prepend the temporary directory to the fileDir for testing + test.filePath = tmpDir + test.filePath + + // Create the parent directories + if err = os.MkdirAll(filepath.Dir(test.filePath), 0755); err != nil { + t.Fatalf("Failed to create directory for file: %v", err) } + createTempFile(t, test.filePath) } } result := isAllowedDir(test.filePath, test.allowedDirs) @@ -129,3 +115,18 @@ func TestTypes_isAllowedDir(t *testing.T) { }) } } + +func createTempFile(t *testing.T, path string) { + if _, err := os.Create(path); err != nil { + t.Fatalf("Failed to create test file: %v", err) + } +} + +func createTempDir(t *testing.T, prefix string) (string, error) { + tmpDir, err := os.MkdirTemp("", prefix) + if err != nil { + t.Fatalf("Failed to create temp directory: %v", err) + return "", err + } + return tmpDir, nil +} From dd1f2f23ab0e6fe9e4fd089cd6e871be2d1b694c Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 25 Jun 2025 14:23:48 +0100 Subject: [PATCH 05/28] fix test --- internal/config/config.go | 7 +--- internal/config/types_test.go | 79 ++++++----------------------------- 2 files changed, 14 insertions(+), 72 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index fb7b6eeaa..c7cb1e50a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -137,11 +137,7 @@ func ResolveConfig() (*Config, error) { func resolveAllowedDirectories(dirs []string) []string { allowed := []string{AgentDirName} for _, dir := range dirs { - re, err := regexp.Compile(regexInvalidPath) - if err != nil { - slog.Error("Failed to compile regex for invalid path characters", "error", err) - continue - } + re := regexp.MustCompile(regexInvalidPath) invalidChars := re.MatchString(dir) if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars { slog.Warn("Ignoring invalid directory", "dir", dir) @@ -154,6 +150,7 @@ func resolveAllowedDirectories(dirs []string) []string { } allowed = append(allowed, dir) } + return allowed } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index e705a5d3a..97d53555b 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -6,79 +6,61 @@ package config import ( - "os" - "path/filepath" "testing" "github.com/stretchr/testify/assert" ) -// nolint: nestif, gocognit func TestTypes_isAllowedDir(t *testing.T) { - tests := []struct { name string filePath string allowedDirs []string - dirExists bool - fileExists bool allowed bool }{ { - name: "File exists and is in allowed directory", - allowed: true, - dirExists: true, - fileExists: true, + name: "File is in allowed directory", + allowed: true, allowedDirs: []string{ "/etc/nginx", }, filePath: "/etc/nginx/nginx.conf", }, { - name: "File exists and is in allowed directory with hyphen", - allowed: true, - dirExists: true, - fileExists: true, + name: "File is in allowed directory with hyphen", + allowed: true, allowedDirs: []string{ "/etc/nginx-agent", }, filePath: "/etc/nginx-agent/nginx.conf", }, { - name: "File exists and is in a subdirectory of allowed directory", - allowed: true, - dirExists: true, - fileExists: true, + name: "File exists and is in a subdirectory of allowed directory", + allowed: true, allowedDirs: []string{ "/etc/nginx", }, filePath: "/etc/nginx/conf.d/nginx.conf", }, { - name: "File exists and is outside allowed directory", - allowed: false, - dirExists: true, - fileExists: true, + name: "File exists and is outside allowed directory", + allowed: false, allowedDirs: []string{ "/etc/nginx", }, filePath: "/etc/test/nginx.conf", }, { - name: "File does not exist but is in allowed directory", - allowed: true, - dirExists: true, - fileExists: false, + name: "File does not exist but is in allowed directory", + allowed: true, allowedDirs: []string{ "/etc/nginx", }, filePath: "/etc/nginx/idontexist.conf", }, { - name: "File does not exist and is outside allowed directory", - allowed: false, - dirExists: false, - fileExists: true, + name: "File does not exist and is outside allowed directory", + allowed: false, allowedDirs: []string{ "/etc/nginx", }, @@ -88,45 +70,8 @@ func TestTypes_isAllowedDir(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if test.dirExists { - // Create the temporary directory for testing - tmpDir, err := createTempDir(t, "test-allowed-dir") - defer func() { - rmErr := os.RemoveAll(tmpDir) - if rmErr != nil { - t.Log(rmErr) - } - }() - - // Create the test file if it should exist - if test.fileExists { - // Prepend the temporary directory to the fileDir for testing - test.filePath = tmpDir + test.filePath - - // Create the parent directories - if err = os.MkdirAll(filepath.Dir(test.filePath), 0755); err != nil { - t.Fatalf("Failed to create directory for file: %v", err) - } - createTempFile(t, test.filePath) - } - } result := isAllowedDir(test.filePath, test.allowedDirs) assert.Equal(t, test.allowed, result) }) } } - -func createTempFile(t *testing.T, path string) { - if _, err := os.Create(path); err != nil { - t.Fatalf("Failed to create test file: %v", err) - } -} - -func createTempDir(t *testing.T, prefix string) (string, error) { - tmpDir, err := os.MkdirTemp("", prefix) - if err != nil { - t.Fatalf("Failed to create temp directory: %v", err) - return "", err - } - return tmpDir, nil -} From 8c99d038182200c8a74c0be2bce1434bfe0053f5 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 25 Jun 2025 15:13:10 +0100 Subject: [PATCH 06/28] add check for symlinks, disallow all symlinks --- go.mod | 2 +- internal/config/types.go | 14 +++++++++++++- internal/config/types_test.go | 31 +++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index de2d1ce13..434b15a13 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/nginx/agent/v3 go 1.23.7 -toolchain go1.24.4 +toolchain go1.23.10 require ( buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.4-20250130201111-63bb56e20495.1 diff --git a/internal/config/types.go b/internal/config/types.go index 051e13a77..4ca1482c0 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -436,11 +436,23 @@ func isAllowedDir(path string, allowedDirs []string) bool { slog.Debug("File path detected, checking parent directory is allowed", "path", directoryPath) } - _, err = os.Stat(directoryPath) + fInfo, err := os.Stat(directoryPath) if err != nil { slog.Warn("Path error", "path", path, "error", err) } + if fInfo != nil { + //check if it contains a symlink + lInfo, err := os.Lstat(directoryPath) + if err != nil { + slog.Warn("Lstat error", "path", path, "error", err) + } + if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { + slog.Warn("Path is a symlink, not allowed", "path", path) + return false + } + } + for _, allowedDirectory := range allowedDirs { // Check if the directoryPath starts with the allowedDirectory // This allows for subdirectories within the allowed directories. diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 97d53555b..29aecdca8 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -6,6 +6,8 @@ package config import ( + "github.com/stretchr/testify/require" + "os" "testing" "github.com/stretchr/testify/assert" @@ -74,4 +76,33 @@ func TestTypes_isAllowedDir(t *testing.T) { assert.Equal(t, test.allowed, result) }) } + + t.Run("Symlink in allowed directory", func(t *testing.T) { + allowedDirs := []string{"/etc/nginx"} + filePath := "file.conf" + symlinkPath := "file_link" + + // Create a temp directory for the symlink + tempDir, err := os.MkdirTemp("", "symlink_test") + assert.NoError(t, err) + defer os.RemoveAll(tempDir) // Clean up the temp directory after the test + + // Ensure the temp directory is in the allowedDirs + allowedDirs = append(allowedDirs, tempDir) + + //create filePath + filePath = tempDir + "/" + filePath + defer os.RemoveAll(filePath) + err = os.WriteFile(filePath, []byte("test content"), 0644) + require.NoError(t, err) + + // Create a symlink for testing + symlinkPath = tempDir + "/" + symlinkPath + defer os.Remove(symlinkPath) + err = os.Symlink(filePath, symlinkPath) + assert.NoError(t, err) + + result := isAllowedDir(symlinkPath, allowedDirs) + assert.False(t, result, "Symlink in allowed directory should return false") + }) } From cb04ec06a875d8e01030604441e9163a965fc6eb Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 25 Jun 2025 15:45:08 +0100 Subject: [PATCH 07/28] fix lint --- internal/config/types.go | 29 +++++++++++++++++++---------- internal/config/types_test.go | 12 ++++++------ 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 4ca1482c0..e92e8f3ee 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -436,19 +436,14 @@ func isAllowedDir(path string, allowedDirs []string) bool { slog.Debug("File path detected, checking parent directory is allowed", "path", directoryPath) } - fInfo, err := os.Stat(directoryPath) - if err != nil { - slog.Warn("Path error", "path", path, "error", err) + fInfo, statErr := os.Stat(directoryPath) + if statErr != nil { + slog.Warn("Stat: Path error", "path", path, "error", statErr) } if fInfo != nil { - //check if it contains a symlink - lInfo, err := os.Lstat(directoryPath) - if err != nil { - slog.Warn("Lstat error", "path", path, "error", err) - } - if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { - slog.Warn("Path is a symlink, not allowed", "path", path) + if isSymlink(directoryPath) { + slog.Warn("Path is a symlink, skipping allowed directory check", "path", directoryPath) return false } } @@ -463,3 +458,17 @@ func isAllowedDir(path string, allowedDirs []string) bool { return false } + +func isSymlink(path string) bool { + // check if it contains a symlink + lInfo, err := os.Lstat(path) + if err != nil { + slog.Warn("Lstat error", "path", path, "error", err) + return false + } + if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { + slog.Warn("Path is a symlink", "path", path) + return true + } + return false +} diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 29aecdca8..ae7212c66 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -6,10 +6,11 @@ package config import ( - "github.com/stretchr/testify/require" "os" "testing" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) @@ -84,25 +85,24 @@ func TestTypes_isAllowedDir(t *testing.T) { // Create a temp directory for the symlink tempDir, err := os.MkdirTemp("", "symlink_test") - assert.NoError(t, err) + require.NoError(t, err) defer os.RemoveAll(tempDir) // Clean up the temp directory after the test // Ensure the temp directory is in the allowedDirs allowedDirs = append(allowedDirs, tempDir) - //create filePath filePath = tempDir + "/" + filePath defer os.RemoveAll(filePath) - err = os.WriteFile(filePath, []byte("test content"), 0644) + err = os.WriteFile(filePath, []byte("test content"), 0o600) require.NoError(t, err) // Create a symlink for testing symlinkPath = tempDir + "/" + symlinkPath defer os.Remove(symlinkPath) err = os.Symlink(filePath, symlinkPath) - assert.NoError(t, err) + require.NoError(t, err) result := isAllowedDir(symlinkPath, allowedDirs) - assert.False(t, result, "Symlink in allowed directory should return false") + require.False(t, result, "Symlink in allowed directory should return false") }) } From faf003a9ca69aa3d202af03596470af8b4c47197 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 25 Jun 2025 15:47:31 +0100 Subject: [PATCH 08/28] fix lint --- internal/config/types.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/config/types.go b/internal/config/types.go index e92e8f3ee..5b608033c 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -468,7 +468,9 @@ func isSymlink(path string) bool { } if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { slog.Warn("Path is a symlink", "path", path) + return true } + return false } From be3e2301bf93bb9cf5d2a48e161b5d33486c98c9 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 25 Jun 2025 15:54:10 +0100 Subject: [PATCH 09/28] fix lint --- internal/config/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/types.go b/internal/config/types.go index 5b608033c..cabeafe0b 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -468,7 +468,7 @@ func isSymlink(path string) bool { } if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { slog.Warn("Path is a symlink", "path", path) - + return true } From 195808f63c4fdf4efa0f00aebef5a433888b7ddf Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Thu, 26 Jun 2025 14:21:20 +0100 Subject: [PATCH 10/28] fix default paths --- internal/config/defaults.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 77ceb2f1a..ddeee7a3b 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -54,7 +54,7 @@ const ( DefFileWatcherMonitoringFrequency = 5 * time.Second // Collector defaults - DefCollectorConfigPath = "/opt/homebrew/etc/nginx-agent/opentelemetry-collector-agent.yaml" + DefCollectorConfigPath = "/etc/nginx-agent/opentelemetry-collector-agent.yaml" DefCollectorLogLevel = "INFO" DefCollectorLogPath = "/var/log/nginx-agent/opentelemetry-collector-agent.log" DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem" From a13d9fea5541819d45b8ccfe73b3d847efa3a3f9 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 2 Jul 2025 15:45:21 +0100 Subject: [PATCH 11/28] update comment --- internal/config/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/types.go b/internal/config/types.go index cabeafe0b..086216ba3 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -416,7 +416,7 @@ func (c *Config) AreReceiversConfigured() 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 does not exist, it logs a warning and returns false. +// 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 directory of the file. func isAllowedDir(path string, allowedDirs []string) bool { if len(allowedDirs) == 0 { From 38fdf246a922267c490d3a6eadd282f272671f90 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Fri, 4 Jul 2025 12:36:58 +0100 Subject: [PATCH 12/28] fix default collector config test --- internal/config/config_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2fef7c80b..6e9490898 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1144,6 +1144,7 @@ func createDefaultCollectorConfig() *Collector { SendBatchSize: DefCollectorBatchProcessorSendBatchSize, Timeout: DefCollectorBatchProcessorTimeout, }, + LogsGzip: &LogsGzip{}, }, Receivers: Receivers{ OtlpReceivers: []OtlpReceiver{ From 9183a0736c46c5297384bea7918d04d318fd2249 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 9 Jul 2025 15:03:43 +0100 Subject: [PATCH 13/28] return bool + error from isAllowedDirs --- internal/config/types.go | 36 +++++++++++++++++++++++------------ internal/config/types_test.go | 6 ++++-- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 0466bdbd0..27493506d 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -324,9 +324,13 @@ type ( func (col *Collector) Validate(allowedDirectories []string) error { var err error - cleaned := filepath.Clean(col.ConfigPath) + cleanedConfPath := filepath.Clean(col.ConfigPath) - if !isAllowedDir(cleaned, allowedDirectories) { + allowed, err := isAllowedDir(cleanedConfPath, allowedDirectories) + if err != nil { + return err + } + if !allowed { err = errors.Join(err, fmt.Errorf("collector path %s not allowed", col.ConfigPath)) } @@ -344,9 +348,13 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error { } for _, al := range nr.AccessLogs { - if !isAllowedDir(al.FilePath, allowedDirectories) { + allowed, err := isAllowedDir(al.FilePath, allowedDirectories) + if err != nil { err = errors.Join(err, fmt.Errorf("invalid nginx receiver access log path: %s", al.FilePath)) } + if !allowed { + err = errors.Join(err, fmt.Errorf("nginx receiver access log path %s not allowed", al.FilePath)) + } if len(al.FilePath) != 0 { // The log format's double quotes must be escaped so that @@ -359,7 +367,12 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error { } func (c *Config) IsDirectoryAllowed(directory string) bool { - return isAllowedDir(directory, c.AllowedDirectories) + allow, err := isAllowedDir(directory, c.AllowedDirectories) + if err != nil { + slog.Warn("Unable to determine if directory is allowed", "error", err) + return false + } + return allow } func (c *Config) IsGrpcClientConfigured() bool { @@ -421,17 +434,17 @@ func (c *Config) AreReceiversConfigured() bool { // 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 directory of the file. -func isAllowedDir(path string, allowedDirs []string) bool { +func isAllowedDir(path string, allowedDirs []string) (bool, error) { if len(allowedDirs) == 0 { slog.Warn("No allowed directories configured") - return false + return false, errors.New("no allowed directories configured") } directoryPath := path isFilePath, err := regexp.MatchString(`\.(\w+)$`, directoryPath) if err != nil { slog.Error("Error matching path", "path", directoryPath, "error", err) - return false + return false, errors.New("error matching path" + directoryPath) } if isFilePath { @@ -441,13 +454,12 @@ func isAllowedDir(path string, allowedDirs []string) bool { fInfo, statErr := os.Stat(directoryPath) if statErr != nil { - slog.Warn("Stat: Path error", "path", path, "error", statErr) + slog.Debug("Stat: Path error", "path", path, "error", statErr) } if fInfo != nil { if isSymlink(directoryPath) { - slog.Warn("Path is a symlink, skipping allowed directory check", "path", directoryPath) - return false + return false, errors.New("path is a symlink, skipping " + directoryPath) } } @@ -455,11 +467,11 @@ func isAllowedDir(path string, allowedDirs []string) bool { // Check if the directoryPath starts with the allowedDirectory // This allows for subdirectories within the allowed directories. if strings.HasPrefix(directoryPath, allowedDirectory) { - return true + return true, nil } } - return false + return false, nil } func isSymlink(path string) bool { diff --git a/internal/config/types_test.go b/internal/config/types_test.go index ae7212c66..ad1ecb2bd 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -73,7 +73,8 @@ func TestTypes_isAllowedDir(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - result := isAllowedDir(test.filePath, test.allowedDirs) + result, err := isAllowedDir(test.filePath, test.allowedDirs) + require.NoError(t, err) assert.Equal(t, test.allowed, result) }) } @@ -102,7 +103,8 @@ func TestTypes_isAllowedDir(t *testing.T) { err = os.Symlink(filePath, symlinkPath) require.NoError(t, err) - result := isAllowedDir(symlinkPath, allowedDirs) + result, err := isAllowedDir(symlinkPath, allowedDirs) + require.Error(t, err) require.False(t, result, "Symlink in allowed directory should return false") }) } From b8b714b027018cb5d497487c5664111d71f9c340 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 9 Jul 2025 15:11:00 +0100 Subject: [PATCH 14/28] fix lint --- internal/config/types.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 27493506d..cfddde410 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -348,8 +348,8 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error { } for _, al := range nr.AccessLogs { - allowed, err := isAllowedDir(al.FilePath, allowedDirectories) - if err != nil { + allowed, allowedError := isAllowedDir(al.FilePath, allowedDirectories) + if allowedError != nil { err = errors.Join(err, fmt.Errorf("invalid nginx receiver access log path: %s", al.FilePath)) } if !allowed { @@ -372,6 +372,7 @@ func (c *Config) IsDirectoryAllowed(directory string) bool { slog.Warn("Unable to determine if directory is allowed", "error", err) return false } + return allow } From d5c4bfa48b086f420a2f7c33cca423cc48776a37 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 9 Jul 2025 15:17:24 +0100 Subject: [PATCH 15/28] rename tests --- internal/config/config_test.go | 20 ++++++++++---------- internal/config/types_test.go | 14 +++++++------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6e9490898..91da4111f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -170,47 +170,47 @@ func TestResolveAllowedDirectories(t *testing.T) { expected: []string{"/etc/nginx-agent"}, }, { - name: "Absolute path", + name: "Test 1: Absolute path", configuredDirs: []string{"/etc/agent/"}, expected: []string{"/etc/nginx-agent", "/etc/agent"}, }, { - name: "Absolute paths", + name: "Test 2: Absolute paths", configuredDirs: []string{"/etc/nginx/"}, expected: []string{"/etc/nginx-agent", "/etc/nginx"}, }, { - name: "Absolute path with multiple slashes", + name: "Test 3: Absolute path with multiple slashes", configuredDirs: []string{"/etc///////////nginx-agent/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Absolute path with directory traversal", + name: "Test 4: Absolute path with directory traversal", configuredDirs: []string{"/etc/nginx/../nginx-agent"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Absolute path with repeat directory traversal", + name: "Test 5: Absolute path with repeat directory traversal", configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Absolute path with control characters", + name: "Test 6: Absolute path with control characters", configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Absolute path with invisible characters", + name: "Test 7: Absolute path with invisible characters", configuredDirs: []string{"/etc/nginx-agent/ㅤㅤㅤ/tmp/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Absolute path with escaped invisible characters", + name: "Test 8: Absolute path with escaped invisible characters", configuredDirs: []string{"/etc/nginx-agent/\\\\ㅤ/tmp/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Mixed paths", + name: "Test 9: Mixed paths", configuredDirs: []string{ "nginx-agent", "", @@ -223,7 +223,7 @@ func TestResolveAllowedDirectories(t *testing.T) { expected: []string{"/etc/nginx-agent", "/etc/nginx"}, }, { - name: "Relative path", + name: "Test 10: Relative path", configuredDirs: []string{"nginx-agent"}, expected: []string{"/etc/nginx-agent"}, }, diff --git a/internal/config/types_test.go b/internal/config/types_test.go index ad1ecb2bd..a9e3e0bab 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -22,7 +22,7 @@ func TestTypes_isAllowedDir(t *testing.T) { allowed bool }{ { - name: "File is in allowed directory", + name: "Test 1:File is in allowed directory", allowed: true, allowedDirs: []string{ "/etc/nginx", @@ -30,7 +30,7 @@ func TestTypes_isAllowedDir(t *testing.T) { filePath: "/etc/nginx/nginx.conf", }, { - name: "File is in allowed directory with hyphen", + name: "Test 2: File is in allowed directory with hyphen", allowed: true, allowedDirs: []string{ "/etc/nginx-agent", @@ -38,7 +38,7 @@ func TestTypes_isAllowedDir(t *testing.T) { filePath: "/etc/nginx-agent/nginx.conf", }, { - name: "File exists and is in a subdirectory of allowed directory", + name: "Test 3: File exists and is in a subdirectory of allowed directory", allowed: true, allowedDirs: []string{ "/etc/nginx", @@ -46,7 +46,7 @@ func TestTypes_isAllowedDir(t *testing.T) { filePath: "/etc/nginx/conf.d/nginx.conf", }, { - name: "File exists and is outside allowed directory", + name: "Test 4: File exists and is outside allowed directory", allowed: false, allowedDirs: []string{ "/etc/nginx", @@ -54,7 +54,7 @@ func TestTypes_isAllowedDir(t *testing.T) { filePath: "/etc/test/nginx.conf", }, { - name: "File does not exist but is in allowed directory", + name: "Test 5: File does not exist but is in allowed directory", allowed: true, allowedDirs: []string{ "/etc/nginx", @@ -62,7 +62,7 @@ func TestTypes_isAllowedDir(t *testing.T) { filePath: "/etc/nginx/idontexist.conf", }, { - name: "File does not exist and is outside allowed directory", + name: "Test 6: Test File does not exist and is outside allowed directory", allowed: false, allowedDirs: []string{ "/etc/nginx", @@ -79,7 +79,7 @@ func TestTypes_isAllowedDir(t *testing.T) { }) } - t.Run("Symlink in allowed directory", func(t *testing.T) { + t.Run("Test 7: Symlink in allowed directory", func(t *testing.T) { allowedDirs := []string{"/etc/nginx"} filePath := "file.conf" symlinkPath := "file_link" From a017437896cd79f8cdf2e33fb7ad6545bd4b8cda Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Mon, 14 Jul 2025 14:05:16 +0100 Subject: [PATCH 16/28] remove needless logs --- internal/config/types.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index cfddde410..cccc0cce3 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -437,14 +437,12 @@ func (c *Config) AreReceiversConfigured() bool { // It also checks if the path is a file, in which case it checks the directory of the file. func isAllowedDir(path string, allowedDirs []string) (bool, error) { if len(allowedDirs) == 0 { - slog.Warn("No allowed directories configured") return false, errors.New("no allowed directories configured") } directoryPath := path isFilePath, err := regexp.MatchString(`\.(\w+)$`, directoryPath) if err != nil { - slog.Error("Error matching path", "path", directoryPath, "error", err) return false, errors.New("error matching path" + directoryPath) } From dfbdecd0b0374b42aea8df776734a35007cb0dfc Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Mon, 14 Jul 2025 15:06:28 +0100 Subject: [PATCH 17/28] fix lint warning --- internal/config/types_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/config/types_test.go b/internal/config/types_test.go index a9e3e0bab..db610d336 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -85,8 +85,7 @@ func TestTypes_isAllowedDir(t *testing.T) { symlinkPath := "file_link" // Create a temp directory for the symlink - tempDir, err := os.MkdirTemp("", "symlink_test") - require.NoError(t, err) + tempDir := t.TempDir() defer os.RemoveAll(tempDir) // Clean up the temp directory after the test // Ensure the temp directory is in the allowedDirs @@ -94,7 +93,7 @@ func TestTypes_isAllowedDir(t *testing.T) { filePath = tempDir + "/" + filePath defer os.RemoveAll(filePath) - err = os.WriteFile(filePath, []byte("test content"), 0o600) + err := os.WriteFile(filePath, []byte("test content"), 0o600) require.NoError(t, err) // Create a symlink for testing From 4e9451f0ae73d41a66f7ed10223d879a7fea6097 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 10:22:25 +0100 Subject: [PATCH 18/28] refactor to remove unessessary logs --- internal/config/types.go | 15 ++------------- internal/config/types_test.go | 2 +- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 072186021..cb7fc8ebe 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -466,14 +466,9 @@ func isAllowedDir(path string, allowedDirs []string) (bool, error) { if isFilePath { directoryPath = filepath.Dir(directoryPath) - slog.Debug("File path detected, checking parent directory is allowed", "path", directoryPath) - } - - fInfo, statErr := os.Stat(directoryPath) - if statErr != nil { - slog.Debug("Stat: Path error", "path", path, "error", statErr) } + fInfo, _ := os.Stat(directoryPath) if fInfo != nil { if isSymlink(directoryPath) { return false, errors.New("path is a symlink, skipping " + directoryPath) @@ -493,14 +488,8 @@ func isAllowedDir(path string, allowedDirs []string) (bool, error) { func isSymlink(path string) bool { // check if it contains a symlink - lInfo, err := os.Lstat(path) - if err != nil { - slog.Warn("Lstat error", "path", path, "error", err) - return false - } + lInfo, _ := os.Lstat(path) if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { - slog.Warn("Path is a symlink", "path", path) - return true } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index db610d336..dff086b0d 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -103,7 +103,7 @@ func TestTypes_isAllowedDir(t *testing.T) { require.NoError(t, err) result, err := isAllowedDir(symlinkPath, allowedDirs) - require.Error(t, err) + require.False(t, result, "Symlink in allowed directory should return false") }) } From 6b4d18f7cafa9d2327983da9fcf17766d2f8c5b6 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 10:25:43 +0100 Subject: [PATCH 19/28] fix lint --- internal/config/types_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/config/types_test.go b/internal/config/types_test.go index dff086b0d..410d89759 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -103,7 +103,8 @@ func TestTypes_isAllowedDir(t *testing.T) { require.NoError(t, err) result, err := isAllowedDir(symlinkPath, allowedDirs) - + require.NoError(t, err) + require.False(t, result, "Symlink in allowed directory should return false") }) } From 99a0a79c65e9e2efd3d7ada5a0db248fbfb36ef8 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 10:27:36 +0100 Subject: [PATCH 20/28] fix test names --- internal/config/config_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3a03dff4a..27adc12e8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -165,52 +165,52 @@ func TestResolveAllowedDirectories(t *testing.T) { expected []string }{ { - name: "Empty path", + name: "Test 1: Empty path", configuredDirs: []string{""}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 1: Absolute path", + name: "Test 2: Absolute path", configuredDirs: []string{"/etc/agent/"}, expected: []string{"/etc/nginx-agent", "/etc/agent"}, }, { - name: "Test 2: Absolute paths", + name: "Test 3: Absolute paths", configuredDirs: []string{"/etc/nginx/"}, expected: []string{"/etc/nginx-agent", "/etc/nginx"}, }, { - name: "Test 3: Absolute path with multiple slashes", + name: "Test 4: Absolute path with multiple slashes", configuredDirs: []string{"/etc///////////nginx-agent/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 4: Absolute path with directory traversal", + name: "Test 5: Absolute path with directory traversal", configuredDirs: []string{"/etc/nginx/../nginx-agent"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 5: Absolute path with repeat directory traversal", + name: "Test 6: Absolute path with repeat directory traversal", configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 6: Absolute path with control characters", + name: "Test 7: Absolute path with control characters", configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 7: Absolute path with invisible characters", + name: "Test 8: Absolute path with invisible characters", configuredDirs: []string{"/etc/nginx-agent/ㅤㅤㅤ/tmp/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 8: Absolute path with escaped invisible characters", + name: "Test 9: Absolute path with escaped invisible characters", configuredDirs: []string{"/etc/nginx-agent/\\\\ㅤ/tmp/"}, expected: []string{"/etc/nginx-agent"}, }, { - name: "Test 9: Mixed paths", + name: "Test 10: Mixed paths", configuredDirs: []string{ "nginx-agent", "", @@ -223,7 +223,7 @@ func TestResolveAllowedDirectories(t *testing.T) { expected: []string{"/etc/nginx-agent", "/etc/nginx"}, }, { - name: "Test 10: Relative path", + name: "Test 11: Relative path", configuredDirs: []string{"nginx-agent"}, expected: []string{"/etc/nginx-agent"}, }, From 5ede483e9410c7359c60fb3b9f372691526d8ff1 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 11:07:55 +0100 Subject: [PATCH 21/28] fix test --- internal/config/types_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 410d89759..29332f1f8 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -78,8 +78,10 @@ func TestTypes_isAllowedDir(t *testing.T) { assert.Equal(t, test.allowed, result) }) } +} - t.Run("Test 7: Symlink in allowed directory", func(t *testing.T) { +func TestTypes_isAllowedDirWithSymlink(t *testing.T) { + t.Run("Test 1: Symlink in allowed directory is not allowed", func(t *testing.T) { allowedDirs := []string{"/etc/nginx"} filePath := "file.conf" symlinkPath := "file_link" @@ -103,8 +105,7 @@ func TestTypes_isAllowedDir(t *testing.T) { require.NoError(t, err) result, err := isAllowedDir(symlinkPath, allowedDirs) - require.NoError(t, err) - + require.Error(t, err) require.False(t, result, "Symlink in allowed directory should return false") }) } From b8f30dbb6f578d5c65aa0590ac1cb4fd18c9f75e Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 15:11:48 +0100 Subject: [PATCH 22/28] add unit test for isSymlink --- internal/config/types_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 29332f1f8..b0121aec7 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -109,3 +109,23 @@ func TestTypes_isAllowedDirWithSymlink(t *testing.T) { require.False(t, result, "Symlink in allowed directory should return false") }) } + +func TestTypes_isSymlink(t *testing.T) { + // create temp dir + tempDir := t.TempDir() + tempConf := tempDir + "test.conf" + defer os.RemoveAll(tempDir) + + t.Run("Test 1: File is not a symlink", func(t *testing.T) { + filePath := tempConf + err := os.WriteFile(filePath, []byte("test content"), 0o600) + require.NoError(t, err) + assert.False(t, isSymlink(filePath), "File is not a symlink") + }) + t.Run("Test 2: File is a symlink", func(t *testing.T) { + filePath := tempDir + "test_conf_link" + err := os.Symlink(tempConf, filePath) + require.NoError(t, err) + assert.True(t, isSymlink(filePath), "File is a symlink") + }) +} From 73939b3a20f2fd29e2c83024f4401262b1ed603c Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 15:28:18 +0100 Subject: [PATCH 23/28] use require in place of assert --- internal/config/types_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/config/types_test.go b/internal/config/types_test.go index b0121aec7..031fc3d94 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -10,8 +10,6 @@ import ( "testing" "github.com/stretchr/testify/require" - - "github.com/stretchr/testify/assert" ) func TestTypes_isAllowedDir(t *testing.T) { @@ -75,7 +73,7 @@ func TestTypes_isAllowedDir(t *testing.T) { t.Run(test.name, func(t *testing.T) { result, err := isAllowedDir(test.filePath, test.allowedDirs) require.NoError(t, err) - assert.Equal(t, test.allowed, result) + require.Equal(t, test.allowed, result) }) } } @@ -120,12 +118,12 @@ func TestTypes_isSymlink(t *testing.T) { filePath := tempConf err := os.WriteFile(filePath, []byte("test content"), 0o600) require.NoError(t, err) - assert.False(t, isSymlink(filePath), "File is not a symlink") + require.False(t, isSymlink(filePath), "File is not a symlink") }) t.Run("Test 2: File is a symlink", func(t *testing.T) { filePath := tempDir + "test_conf_link" err := os.Symlink(tempConf, filePath) require.NoError(t, err) - assert.True(t, isSymlink(filePath), "File is a symlink") + require.True(t, isSymlink(filePath), "File is a symlink") }) } From 7fcbfe1a44d881842075d5216768625393998462 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 15:45:54 +0100 Subject: [PATCH 24/28] update regex --- internal/config/types.go | 4 ++-- internal/config/types_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index cb7fc8ebe..cd94ee822 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -452,14 +452,14 @@ func (c *Config) AreReceiversConfigured() 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 directory of the file. +// 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 - isFilePath, err := regexp.MatchString(`\.(\w+)$`, directoryPath) + isFilePath, err := regexp.MatchString(`/(\w+)\.(\w+)$`, directoryPath) if err != nil { return false, errors.New("error matching path" + directoryPath) } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 031fc3d94..8f63164b9 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -20,7 +20,7 @@ func TestTypes_isAllowedDir(t *testing.T) { allowed bool }{ { - name: "Test 1:File is in allowed directory", + name: "Test 1: File is in allowed directory", allowed: true, allowedDirs: []string{ "/etc/nginx", From 279dcdd98d37bdf137f2d01db02212590804399e Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 15 Jul 2025 15:48:38 +0100 Subject: [PATCH 25/28] update comment --- internal/config/types.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/types.go b/internal/config/types.go index cd94ee822..079b843a1 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -459,6 +459,7 @@ func isAllowedDir(path string, allowedDirs []string) (bool, error) { } 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) From c00d955251e7916d819b3beb507cabf0d1348002 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Mon, 21 Jul 2025 10:29:15 +0100 Subject: [PATCH 26/28] fix unit test --- internal/config/config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e9e0fd35c..f6cb6f9a0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -929,8 +929,8 @@ func createConfig() *Config { }, }, AllowedDirectories: []string{ - "/etc/nginx-agent/", "/etc/nginx/", "/usr/local/etc/nginx/", "/var/run/nginx/", - "/usr/share/nginx/modules/", "/var/log/nginx/", + "/etc/nginx-agent", "/etc/nginx", "/usr/local/etc/nginx", "/var/run/nginx", + "/usr/share/nginx/modules", "/var/log/nginx", }, DataPlaneConfig: &DataPlaneConfig{ Nginx: &NginxDataPlaneConfig{ From 2d3e6e8578ec149ca7110ac7095cfa0f5c63f413 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Tue, 22 Jul 2025 11:37:19 +0100 Subject: [PATCH 27/28] Remove symlink check --- internal/config/types.go | 18 ------- internal/config/types_test.go | 51 ------------------- .../watcher/file/file_watcher_service_test.go | 2 +- 3 files changed, 1 insertion(+), 70 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 76241e173..b1fb8d5cd 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -10,7 +10,6 @@ import ( "errors" "fmt" "log/slog" - "os" "path/filepath" "regexp" "strings" @@ -484,13 +483,6 @@ func isAllowedDir(path string, allowedDirs []string) (bool, error) { directoryPath = filepath.Dir(directoryPath) } - fInfo, _ := os.Stat(directoryPath) - if fInfo != nil { - if isSymlink(directoryPath) { - return false, errors.New("path is a symlink, skipping " + directoryPath) - } - } - for _, allowedDirectory := range allowedDirs { // Check if the directoryPath starts with the allowedDirectory // This allows for subdirectories within the allowed directories. @@ -501,13 +493,3 @@ func isAllowedDir(path string, allowedDirs []string) (bool, error) { return false, nil } - -func isSymlink(path string) bool { - // check if it contains a symlink - lInfo, _ := os.Lstat(path) - if lInfo != nil && lInfo.Mode()&os.ModeSymlink != 0 { - return true - } - - return false -} diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 8f63164b9..d41872305 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -6,7 +6,6 @@ package config import ( - "os" "testing" "github.com/stretchr/testify/require" @@ -77,53 +76,3 @@ func TestTypes_isAllowedDir(t *testing.T) { }) } } - -func TestTypes_isAllowedDirWithSymlink(t *testing.T) { - t.Run("Test 1: Symlink in allowed directory is not allowed", func(t *testing.T) { - allowedDirs := []string{"/etc/nginx"} - filePath := "file.conf" - symlinkPath := "file_link" - - // Create a temp directory for the symlink - tempDir := t.TempDir() - defer os.RemoveAll(tempDir) // Clean up the temp directory after the test - - // Ensure the temp directory is in the allowedDirs - allowedDirs = append(allowedDirs, tempDir) - - filePath = tempDir + "/" + filePath - defer os.RemoveAll(filePath) - err := os.WriteFile(filePath, []byte("test content"), 0o600) - require.NoError(t, err) - - // Create a symlink for testing - symlinkPath = tempDir + "/" + symlinkPath - defer os.Remove(symlinkPath) - err = os.Symlink(filePath, symlinkPath) - require.NoError(t, err) - - result, err := isAllowedDir(symlinkPath, allowedDirs) - require.Error(t, err) - require.False(t, result, "Symlink in allowed directory should return false") - }) -} - -func TestTypes_isSymlink(t *testing.T) { - // create temp dir - tempDir := t.TempDir() - tempConf := tempDir + "test.conf" - defer os.RemoveAll(tempDir) - - t.Run("Test 1: File is not a symlink", func(t *testing.T) { - filePath := tempConf - err := os.WriteFile(filePath, []byte("test content"), 0o600) - require.NoError(t, err) - require.False(t, isSymlink(filePath), "File is not a symlink") - }) - t.Run("Test 2: File is a symlink", func(t *testing.T) { - filePath := tempDir + "test_conf_link" - err := os.Symlink(tempConf, filePath) - require.NoError(t, err) - require.True(t, isSymlink(filePath), "File is a symlink") - }) -} diff --git a/internal/watcher/file/file_watcher_service_test.go b/internal/watcher/file/file_watcher_service_test.go index 3177c7226..a353241d5 100644 --- a/internal/watcher/file/file_watcher_service_test.go +++ b/internal/watcher/file/file_watcher_service_test.go @@ -266,7 +266,7 @@ func TestFileWatcherService_Watch(t *testing.T) { select { case <-channel: - t.Fatalf("Expected file to be skipped") + t.Fatalf("Expected file to be skipped: %v", skippableFile.Name()) case <-time.After(150 * time.Millisecond): return } From c362abf350d1e2467837dd636df3bbb16e687fa9 Mon Sep 17 00:00:00 2001 From: Sean Breen Date: Wed, 23 Jul 2025 11:16:24 +0100 Subject: [PATCH 28/28] fix unit test after merging main --- internal/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index e5ab1365d..6b08de147 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -37,7 +37,7 @@ const ( EnvPrefix = "NGINX_AGENT" KeyDelimiter = "_" KeyValueNumber = 2 - AgentDirName = "/etc/nginx-agent/" + AgentDirName = "/etc/nginx-agent" DefaultMetricsBatchProcessor = "default_metrics" DefaultLogsBatchProcessor = "default_logs" DefaultExporter = "default"