Check allowed dir (#1007)

Author: aphralG

internal/config/config.go

@@ -29,6 +29,7 @@ const (
 	EnvPrefix      = "NGINX_AGENT"
 	KeyDelimiter   = "_"
 	KeyValueNumber = 2
+	AgentDirName   = "/etc/nginx-agent/"
 )
 
 var viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter))
@@ -73,15 +74,20 @@ func RegisterConfigFile() error {
 func ResolveConfig() (*Config, error) {
 	// Collect allowed directories, so that paths in the config can be validated.
 	directories := viperInstance.GetStringSlice(AllowedDirectoriesKey)
-	allowedDirs := make([]string, 0)
+	allowedDirs := []string{AgentDirName}
 
 	// Check directories in allowed_directories are valid
 	for _, dir := range directories {
-		if dir != "" && filepath.IsAbs(dir) {
-			allowedDirs = append(allowedDirs, dir)
-		} else {
+		if dir == "" || !filepath.IsAbs(dir) {
 			slog.Warn("Invalid directory: ", "dir", dir)
+			continue
 		}
+
+		if !strings.HasSuffix(dir, "/") {
+			dir += "/"
+		}
+		allowedDirs = append(allowedDirs, dir)
+		slog.Info("Configured allowed directories", "allowed_directories", allowedDirs)
 	}
 
 	// Collect all parsing errors before returning the error, so the user sees all issues with config
@@ -161,7 +167,8 @@ func registerFlags() {
 
 	fs.StringSlice(AllowedDirectoriesKey,
 		DefaultAllowedDirectories(),
-		"A comma-separated list of paths that you want to grant NGINX Agent read/write access to")
+		"A comma-separated list of paths that you want to grant NGINX Agent read/write access to. Allowed "+
+			"directories are case sensitive")
 
 	fs.Duration(
 		InstanceWatcherMonitoringFrequencyKey,

internal/config/config_test.go

@@ -625,7 +625,8 @@ func getAgentConfig() *Config {
 			},
 		},
 		AllowedDirectories: []string{
-			"/etc/nginx", "/usr/local/etc/nginx", "/var/run/nginx", "/var/log/nginx", "/usr/share/nginx/modules",
+			"/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",
@@ -755,7 +756,8 @@ func createConfig() *Config {
 			},
 		},
 		AllowedDirectories: []string{
-			"/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{

internal/config/testdata/nginx-agent.conf

@@ -47,7 +47,7 @@ allowed_directories:
     - /usr/local/etc/nginx
     - /var/run/nginx
     - /usr/share/nginx/modules
-    - /var/log/nginx 
+    - /var/log/nginx
 
 command: 
     server: 

internal/config/types_test.go

@@ -0,0 +1,93 @@
+// Copyright (c) F5, Inc.
+//
+// This source code is licensed under the Apache License, Version 2.0 license found in the
+// LICENSE file in the root directory of this source tree.
+
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestTypes_IsDirectoryAllowed(t *testing.T) {
+	config := getAgentConfig()
+
+	tests := []struct {
+		name        string
+		fileDir     string
+		allowedDirs []string
+		allowed     bool
+	}{
+		{
+			name:    "Test 1: directory allowed",
+			allowed: true,
+			allowedDirs: []string{
+				AgentDirName,
+				"/etc/nginx",
+				"/var/log/nginx/",
+			},
+			fileDir: "/etc/nginx/nginx.conf",
+		},
+		{
+			name:    "Test 2: directory not allowed",
+			allowed: false,
+			allowedDirs: []string{
+				AgentDirName,
+				"/etc/nginx/",
+				"/var/log/nginx/",
+			},
+			fileDir: "/etc/nginx-test/nginx-agent.conf",
+		},
+		{
+			name:    "Test 3: directory allowed",
+			allowed: true,
+			allowedDirs: []string{
+				AgentDirName,
+				"/etc/nginx/",
+				"/var/log/nginx/",
+			},
+			fileDir: "/etc/nginx/conf.d/nginx-agent.conf",
+		},
+		{
+			name:    "Test 4: directory not allowed",
+			allowed: false,
+			allowedDirs: []string{
+				AgentDirName,
+				"/etc/nginx",
+				"/var/log/nginx",
+			},
+			fileDir: "~/test.conf",
+		},
+		{
+			name:    "Test 5: directory not allowed",
+			allowed: false,
+			allowedDirs: []string{
+				AgentDirName,
+				"/etc/nginx/",
+				"/var/log/nginx/",
+			},
+			fileDir: "//test.conf",
+		},
+		{
+			name:    "Test 6: directory allowed",
+			allowed: true,
+			allowedDirs: []string{
+				AgentDirName,
+				"/etc/nginx/",
+				"/var/log/nginx/",
+				"/",
+			},
+			fileDir: "/test.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)
+		})
+	}
+}

internal/watcher/instance/nginx_config_parser.go

@@ -10,7 +10,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"io/fs"
 	"log/slog"
 	"net"
 	"net/http"
@@ -117,6 +116,14 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
 	rootDir := filepath.Dir(instance.GetInstanceRuntime().GetConfigPath())
 
 	for _, conf := range payload.Config {
+		if !ncp.agentConfig.IsDirectoryAllowed(conf.File) {
+			slog.WarnContext(ctx, "File included in NGINX config is outside of allowed directories, "+
+				"excluding from config",
+				"file", conf.File)
+
+			continue
+		}
+
 		formatMap := make(map[string]string)
 		err := ncp.crossplaneConfigTraverse(ctx, &conf,
 			func(ctx context.Context, parent, directive *crossplane.Directive) error {
@@ -138,10 +145,8 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
 							"is disabled while applying a config; "+"log errors to file to enable error monitoring",
 							directive.Args[0]), "error_log", directive.Args[0])
 					}
-				case "root":
-					rootFiles := ncp.rootFiles(ctx, directive.Args[0])
-					nginxConfigContext.Files = append(nginxConfigContext.Files, rootFiles...)
-				case "ssl_certificate", "proxy_ssl_certificate", "ssl_client_certificate", "ssl_trusted_certificate":
+				case "ssl_certificate", "proxy_ssl_certificate", "ssl_client_certificate",
+					"ssl_trusted_certificate":
 					sslCertFile := ncp.sslCert(ctx, directive.Args[0], rootDir)
 					if !ncp.isDuplicateFile(nginxConfigContext.Files, sslCertFile) {
 						nginxConfigContext.Files = append(nginxConfigContext.Files, sslCertFile)
@@ -195,18 +200,17 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
 }
 
 func (ncp *NginxConfigParser) ignoreLog(logPath string) bool {
-	logLower := strings.ToLower(logPath)
 	ignoreLogs := []string{"off", "/dev/stderr", "/dev/stdout", "/dev/null", "stderr", "stdout"}
 
-	if strings.HasPrefix(logLower, "syslog:") || slices.Contains(ignoreLogs, logLower) {
+	if strings.HasPrefix(logPath, "syslog:") || slices.Contains(ignoreLogs, logPath) {
 		return true
 	}
 
-	if ncp.isExcludeLog(logLower) {
+	if ncp.isExcludeLog(logPath) {
 		return true
 	}
 
-	if !ncp.agentConfig.IsDirectoryAllowed(logLower) {
+	if !ncp.agentConfig.IsDirectoryAllowed(logPath) {
 		slog.Warn("Log being read is outside of allowed directories", "log_path", logPath)
 	}
 
@@ -315,39 +319,6 @@ func (ncp *NginxConfigParser) errorLogDirectiveLevel(directive *crossplane.Direc
 	return ""
 }
 
-func (ncp *NginxConfigParser) rootFiles(ctx context.Context, rootDir string) (rootFiles []*mpi.File) {
-	if !ncp.agentConfig.IsDirectoryAllowed(rootDir) {
-		slog.DebugContext(ctx, "Root directory not in allowed directories", "root_directory", rootDir)
-		return rootFiles
-	}
-
-	err := filepath.WalkDir(rootDir,
-		func(path string, d fs.DirEntry, err error) error {
-			if err != nil {
-				return err
-			}
-
-			if d.IsDir() {
-				return nil
-			}
-
-			rootFileMeta, fileMetaErr := files.FileMeta(path)
-			if fileMetaErr != nil {
-				return fileMetaErr
-			}
-
-			rootFiles = append(rootFiles, &mpi.File{FileMeta: rootFileMeta})
-
-			return nil
-		},
-	)
-	if err != nil {
-		slog.WarnContext(ctx, "Unable to walk root directory", "root_directory", rootDir)
-	}
-
-	return rootFiles
-}
-
 func (ncp *NginxConfigParser) sslCert(ctx context.Context, file, rootDir string) (sslCertFile *mpi.File) {
 	if strings.Contains(file, "$") {
 		// cannot process any filepath with variables