diff --git a/internal/config/config.go b/internal/config/config.go index 79e661efc..6b08de147 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" @@ -36,11 +37,15 @@ const ( EnvPrefix = "NGINX_AGENT" KeyDelimiter = "_" KeyValueNumber = 2 - AgentDirName = "/etc/nginx-agent/" + AgentDirName = "/etc/nginx-agent" DefaultMetricsBatchProcessor = "default_metrics" DefaultLogsBatchProcessor = "default_logs" DefaultExporter = "default" DefaultPipeline = "default" + + // 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)) @@ -83,27 +88,13 @@ 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) - // 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) - } - + // 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 @@ -142,6 +133,29 @@ 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 { + allowed := []string{AgentDirName} + for _, dir := range dirs { + re := regexp.MustCompile(regexInvalidPath) + invalidChars := re.MatchString(dir) + if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars { + slog.Warn("Ignoring invalid directory", "dir", dir) + continue + } + dir = filepath.Clean(dir) + if dir == AgentDirName { + // If the directory is the default agent directory, we skip adding it again. + continue + } + allowed = append(allowed, dir) + } + + return allowed +} + func defaultCollector(collector *Collector, config *Config) { // Always add default host metric receiver and default processor addDefaultHostMetricsReceiver(collector) @@ -909,13 +923,14 @@ func resolveClient() *Client { } func resolveCollector(allowedDirs []string) (*Collector, error) { + // 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 b8c8a11ce..8b2914519 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -163,6 +163,84 @@ func TestNormalizeFunc(t *testing.T) { assert.Equal(t, expected, result) } +func TestResolveAllowedDirectories(t *testing.T) { + tests := []struct { + name string + configuredDirs []string + expected []string + }{ + { + name: "Test 1: Empty path", + configuredDirs: []string{""}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 2: Absolute path", + configuredDirs: []string{"/etc/agent/"}, + expected: []string{"/etc/nginx-agent", "/etc/agent"}, + }, + { + name: "Test 3: Absolute paths", + configuredDirs: []string{"/etc/nginx/"}, + expected: []string{"/etc/nginx-agent", "/etc/nginx"}, + }, + { + name: "Test 4: Absolute path with multiple slashes", + configuredDirs: []string{"/etc///////////nginx-agent/"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 5: Absolute path with directory traversal", + configuredDirs: []string{"/etc/nginx/../nginx-agent"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 6: Absolute path with repeat directory traversal", + configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 7: Absolute path with control characters", + configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 8: Absolute path with invisible characters", + configuredDirs: []string{"/etc/nginx-agent/ㅤㅤㅤ/tmp/"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 9: Absolute path with escaped invisible characters", + configuredDirs: []string{"/etc/nginx-agent/\\\\ㅤ/tmp/"}, + expected: []string{"/etc/nginx-agent"}, + }, + { + name: "Test 10: Mixed paths", + configuredDirs: []string{ + "nginx-agent", + "", + "..", + "/", + "\\/", + ".", + "/etc/nginx/", + }, + expected: []string{"/etc/nginx-agent", "/etc/nginx"}, + }, + { + name: "Test 11: Relative path", + configuredDirs: []string{"nginx-agent"}, + expected: []string{"/etc/nginx-agent"}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + 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") @@ -867,89 +945,7 @@ func agentConfig() *Config { "/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/", "/usr/share/nginx/modules/", "/etc/app_protect/", }, - Collector: &Collector{ - ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml", - Exporters: Exporters{ - OtlpExporters: map[string]*OtlpExporter{ - "default": { - 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: map[string]*Batch{ - "default_logs": { - SendBatchMaxSize: DefCollectorLogsBatchProcessorSendBatchMaxSize, - SendBatchSize: DefCollectorLogsBatchProcessorSendBatchSize, - Timeout: DefCollectorLogsBatchProcessorTimeout, - }, - }, - LogsGzip: map[string]*LogsGzip{ - "default": {}, - }, - }, - Receivers: Receivers{ - OtlpReceivers: map[string]*OtlpReceiver{ - "default": { - 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", - }, - }, + Collector: createDefaultCollectorConfig(), Command: &Command{ Server: &ServerConfig{ Host: "127.0.0.1", @@ -1002,8 +998,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{ @@ -1226,3 +1222,89 @@ func createConfig() *Config { }, } } + +func createDefaultCollectorConfig() *Collector { + return &Collector{ + ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml", + Exporters: Exporters{ + OtlpExporters: map[string]*OtlpExporter{ + "default": { + 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: map[string]*Batch{ + "default_logs": { + SendBatchMaxSize: DefCollectorLogsBatchProcessorSendBatchMaxSize, + SendBatchSize: DefCollectorLogsBatchProcessorSendBatchSize, + Timeout: DefCollectorLogsBatchProcessorTimeout, + }, + }, + LogsGzip: map[string]*LogsGzip{ + "default": {}, + }, + }, + Receivers: Receivers{ + OtlpReceivers: map[string]*OtlpReceiver{ + "default": { + 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/types.go b/internal/config/types.go index b6ccebc61..e282f9f60 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -9,7 +9,9 @@ import ( "context" "errors" "fmt" + "log/slog" "path/filepath" + "regexp" "strings" "time" @@ -337,9 +339,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)) } @@ -357,9 +363,13 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error { } for _, al := range nr.AccessLogs { - if !isAllowedDir(al.FilePath, allowedDirectories) { + 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 { + 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 @@ -372,7 +382,13 @@ 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) IsCommandGrpcClientConfigured() bool { @@ -441,16 +457,33 @@ func (c *Config) NewContextWithLabels(ctx context.Context) context.Context { return metadata.NewOutgoingContext(ctx, md) } -func isAllowedDir(dir string, allowedDirs []string) bool { - if !strings.HasSuffix(dir, "/") && filepath.Ext(dir) == "" { - dir += "/" +// 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) + } + + if isFilePath { + directoryPath = filepath.Dir(directoryPath) } for _, allowedDirectory := range allowedDirs { - if strings.HasPrefix(dir, allowedDirectory) { - return true + // Check if the directoryPath starts with the allowedDirectory + // This allows for subdirectories within the allowed directories. + if strings.HasPrefix(directoryPath, allowedDirectory) { + return true, nil } } - return false + return false, nil } diff --git a/internal/config/types_test.go b/internal/config/types_test.go index f857a76d5..d41872305 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -8,86 +8,71 @@ package config import ( "testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestTypes_IsDirectoryAllowed(t *testing.T) { - config := agentConfig() - +func TestTypes_isAllowedDir(t *testing.T) { tests := []struct { name string - fileDir string + filePath string allowedDirs []string allowed bool }{ { - name: "Test 1: directory allowed", + name: "Test 1: File is in allowed directory", allowed: true, allowedDirs: []string{ - AgentDirName, - "/etc/nginx/", - "/var/log/nginx/", + "/etc/nginx", }, - fileDir: "/etc/nginx", + filePath: "/etc/nginx/nginx.conf", }, { - name: "Test 2: directory not allowed", - allowed: false, + name: "Test 2: File is in allowed directory with hyphen", + allowed: 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", + name: "Test 3: File exists and is in a subdirectory of allowed directory", allowed: 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", + name: "Test 4: File exists and is outside allowed directory", allowed: false, 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: "Test 5: File does not exist but is in allowed directory", + allowed: true, 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: "Test 6: Test File does not exist and is outside allowed directory", + allowed: false, 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) - assert.Equal(t, test.allowed, result) + result, err := isAllowedDir(test.filePath, test.allowedDirs) + require.NoError(t, err) + require.Equal(t, test.allowed, result) }) } } diff --git a/internal/watcher/file/file_watcher_service_test.go b/internal/watcher/file/file_watcher_service_test.go index 56c35415a..6af8c267f 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 }