chore: add file permission check

Signed-off-by: Arjun Raja Yogidas <[email protected]>

Author: coderbirju

.github/workflows/ci.yaml

@@ -105,17 +105,19 @@ jobs:
         run: |
           sudo ls /etc/cni/net.d
           sudo rm /etc/cni/net.d/87-podman-bridge.conflist
-      - name: Start finch-daemon
-        run: sudo bin/finch-daemon  --debug --socket-owner $UID &
-      - name: Run e2e test
-        run: sudo make test-e2e
-      - name: Clean up Daemon socket
-        run: sudo rm /var/run/finch.sock && sudo rm /run/finch.pid
       - name: Verify Rego file presence
         run: ls -l ${{ github.workspace }}/docs/sample-rego-policies/default.rego
       - name: Set Rego file path
         run: echo "REGO_FILE_PATH=${{ github.workspace }}/docs/sample-rego-policies/default.rego" >> $GITHUB_ENV
       - name: Start finch-daemon with opa Authz
-        run: sudo bin/finch-daemon --debug --enable-middleware --rego-file ${{ github.workspace }}/docs/sample-rego-policies/default.rego --socket-owner $UID &
+        run: sudo bin/finch-daemon --debug --enable-middleware --rego-file ${{ github.workspace }}/docs/sample-rego-policies/default.rego --skip-rego-perm-check --socket-owner $UID --socket-addr /run/finch.sock --pidfile /run/finch.pid &
       - name: Run opa e2e tests
         run: sudo -E make test-e2e-opa
+      - name: Clean up Daemon socket
+        run: sudo rm /run/finch.sock && sudo rm /run/finch.pid
+      - name: Start finch-daemon
+        run: sudo bin/finch-daemon  --debug --socket-owner $UID &
+      - name: Run e2e test
+        run: sudo make test-e2e
+      - name: Clean up Daemon socket
+        run: sudo rm /var/run/finch.sock && sudo rm /run/finch.pid

Makefile

@@ -117,9 +117,10 @@ test-e2e: linux
 .PHONY: test-e2e-opa
 test-e2e-opa: linux
 	DOCKER_HOST="unix:///run/finch.sock" \
-	DOCKER_API_VERSION="v1.43" \
+	DOCKER_API_VERSION="v1.41" \
 	MIDDLEWARE_E2E=1 \
-	TEST_E2E=1 \
+	TEST_E2E=0 \
+	DAEMON_ROOT="$(BIN)/finch-daemon" \
 	$(GINKGO) $(GFLAGS) ./e2e/...
 
 .PHONY: licenses
@@ -134,4 +135,4 @@ coverage: linux
 .PHONY: release
 release: linux
 	@echo "$@"
-	@$(FINCH_DAEMON_PROJECT_ROOT)/scripts/create-releases.sh $(RELEASE_TAG)
+	@$(FINCH_DAEMON_PROJECT_ROOT)/scripts/create-releases.sh $(RELEASE_TAG)

api/router/router.go

@@ -135,20 +135,26 @@ func CreateRegoMiddleware(regoFilePath string) (func(next http.Handler) http.Han
 				Path:   r.URL.Path,
 			}
 
-			fmt.Printf("Evaluating policy rules for API request with Method = %s and Path = %s \n", input.Method, input.Path)
+			fmt.Printf("[OPA Debug] Input being evaluated: Method=%s, Path=%s\n", input.Method, input.Path)
+			fmt.Printf("[OPA Debug] Query being executed: %s\n", query)
+
 			rs, err := preppedQuery.Eval(r.Context(), rego.EvalInput(input))
 			if err != nil {
+				fmt.Printf("[OPA Error] Policy evaluation failed: %v\n", err)
 				response.SendErrorResponse(w, http.StatusInternalServerError, errInput)
 				return
 			}
 
+			fmt.Printf("[OPA Debug] Evaluation results: %+v\n", rs)
+			fmt.Printf("[OPA Debug] Number of results: %d\n", len(rs))
+
 			if !rs.Allowed() {
-				// need to log evaluation result in order to mitigate Repudiation threat
-				fmt.Printf("Evaluation result: failed, method %s not allowed for path %s \n", r.Method, r.URL.Path)
+				fmt.Printf("[OPA Denied] Request denied: Method=%s, Path=%s\n", r.Method, r.URL.Path)
 				response.SendErrorResponse(w, http.StatusForbidden,
 					fmt.Errorf("method %s not allowed for path %s", r.Method, r.URL.Path))
 				return
 			}
+			fmt.Printf("[OPA Allowed] Request allowed: Method=%s, Path=%s\n", r.Method, r.URL.Path)
 			newReq := r.WithContext(r.Context())
 			next.ServeHTTP(w, newReq)
 		})

cmd/finch-daemon/main.go

@@ -43,15 +43,16 @@ const (
 )
 
 type DaemonOptions struct {
-	debug            bool
-	socketAddr       string
-	socketOwner      int
-	debugAddress     string
-	configPath       string
-	pidFile          string
-	regoFilePath     string
-	enableMiddleware bool
-	regoFileLock     *flock.Flock
+	debug             bool
+	socketAddr        string
+	socketOwner       int
+	debugAddress      string
+	configPath        string
+	pidFile           string
+	regoFilePath      string
+	enableMiddleware  bool
+	skipRegoPermCheck bool
+	regoFileLock      *flock.Flock
 }
 
 var options = new(DaemonOptions)
@@ -72,6 +73,8 @@ func main() {
 	rootCmd.Flags().StringVar(&options.pidFile, "pidfile", defaultPidFile, "pid file location")
 	rootCmd.Flags().StringVar(&options.regoFilePath, "rego-file", "", "Rego Policy Path")
 	rootCmd.Flags().BoolVar(&options.enableMiddleware, "enable-middleware", false, "turn on middleware for allowlisting")
+	rootCmd.Flags().BoolVar(&options.skipRegoPermCheck, "skip-rego-perm-check", false, "skip the rego file permission check (allows permissions more permissive than 0600)")
+
 	if err := rootCmd.Execute(); err != nil {
 		log.Printf("got error: %v", err)
 		log.Fatal(err)
@@ -228,7 +231,7 @@ func newRouter(options *DaemonOptions, logger *flog.Logrus) (http.Handler, error
 
 	var regoFilePath string
 	if options.enableMiddleware {
-		regoFilePath, err = sanitizeRegoFile(options)
+		regoFilePath, err = sanitizeRegoFile(options, logger)
 		if err != nil {
 			return nil, err
 		}

cmd/finch-daemon/router_utils.go

@@ -14,7 +14,6 @@ import (
 	"github.com/containerd/containerd/v2/pkg/namespaces"
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/config"
-	"github.com/gofrs/flock"
 	toml "github.com/pelletier/go-toml/v2"
 	"github.com/runfinch/finch-daemon/api/router"
 	"github.com/runfinch/finch-daemon/internal/backend"
@@ -97,7 +96,7 @@ func createContainerdClient(conf *config.Config) (*backend.ContainerdClientWrapp
 // sanitizeRegoFile validates and prepares the Rego policy file for use.
 // It checks validates the file, acquires a file lock,
 // and sets rego file to be read-only.
-func sanitizeRegoFile(options *DaemonOptions) (string, error) {
+func sanitizeRegoFile(options *DaemonOptions, logger *flog.Logrus) (string, error) {
 	if options.regoFilePath != "" {
 		if !options.enableMiddleware {
 			return "", fmt.Errorf("rego file path was provided without the --enable-middleware flag, please provide the --enable-middleware flag") // todo, can we default to setting this flag ourselves is this better UX?
@@ -106,30 +105,25 @@ func sanitizeRegoFile(options *DaemonOptions) (string, error) {
 		if err := checkRegoFileValidity(options.regoFilePath); err != nil {
 			return "", err
 		}
+
+		if !options.skipRegoPermCheck {
+			fileInfo, err := os.Stat(options.regoFilePath)
+			if err != nil {
+				return "", fmt.Errorf("error checking rego file permissions: %v", err)
+			}
+
+			if fileInfo.Mode().Perm() > 0600 {
+				return "", fmt.Errorf("rego file permissions %o are too permissive - must be no more permissive than 0600", fileInfo.Mode().Perm())
+			}
+			logger.Debugf("rego file permissions check passed: %o", fileInfo.Mode().Perm())
+		} else {
+			logger.Warnf("skipping rego file permission check - file may have permissions more permissive than 0600")
+		}
 	}
 
 	if options.enableMiddleware && options.regoFilePath == "" {
 		return "", fmt.Errorf("rego file path not provided, please provide the policy file path using the --rego-file flag")
 	}
-
-	fileLock := flock.New(options.regoFilePath)
-
-	locked, err := fileLock.TryLock()
-	if err != nil {
-		return "", fmt.Errorf("error acquiring lock on rego file: %v", err)
-	}
-	if !locked {
-		return "", fmt.Errorf("unable to acquire lock on rego file, it may be in use by another process")
-	}
-
-	// Change file permissions to read-only
-	err = os.Chmod(options.regoFilePath, 0400)
-	if err != nil {
-		fileLock.Unlock()
-		return "", fmt.Errorf("error changing rego file permissions: %v", err)
-	}
-	options.regoFileLock = fileLock
-
 	return options.regoFilePath, nil
 }
 
@@ -162,15 +156,13 @@ func createRouterOptions(
 
 // checkRegoFileValidity verifies that the given rego file exists and has the right file extension.
 func checkRegoFileValidity(regoFilePath string) error {
-	fmt.Println("filepath in checkRegoFileValidity = ", regoFilePath)
 	if _, err := os.Stat(regoFilePath); os.IsNotExist(err) {
 		return fmt.Errorf("provided Rego file path does not exist: %s", regoFilePath)
 	}
 
 	// Check if the file has a valid extension (.rego)
 	fileExt := strings.ToLower(filepath.Ext(regoFilePath))
 
-	fmt.Println("fileExt = ", fileExt)
 	if fileExt != ".rego" {
 		return fmt.Errorf("invalid file extension for Rego file. Only .rego files are supported")
 	}

docs/opa-middleware.md

@@ -38,8 +38,19 @@ Once you are ready with your policy document, use the `--enable-middleware` flag
 
 Note: The `--rego-file` flag is required when `--enable-middleware` is set.
 
-Example: 
-`sudo bin/finch-daemon --debug --socket-owner $UID --socket-addr /run/finch-test.sock --pidfile /run/finch-test.pid --enable-middleware --rego-file /<path-to>/finch-daemon/docs/sample-rego-policies/default.rego &`
+The daemon enforces strict permissions (0600 or more restrictive) on the Rego policy file to prevent unauthorized modifications. You can bypass this check using the `--skip-rego-perm-check` flag.
+
+Examples:
+
+Standard secure usage:
+```bash
+sudo bin/finch-daemon --debug --socket-owner $UID --socket-addr /run/finch-test.sock --pidfile /run/finch-test.pid --enable-middleware --rego-file /path/to/policy.rego
+```
+
+With permission check bypassed:
+```bash
+sudo bin/finch-daemon --debug --socket-owner $UID --socket-addr /run/finch-test.sock --pidfile /run/finch-test.pid --enable-middleware --rego-file /path/to/policy.rego --skip-rego-perm-check
+```
 
 
 # Best practices for secure rego policies
@@ -109,7 +120,21 @@ The finch-daemon's inability to start due to policy issues could impact system o
   - Monitor for unexpected denials
 
 
-### Critical Security Considerations : Rego Policy File Protection
+### Critical Security Considerations: Rego Policy File Protection
+
+### Rego File Permissions
+By default, the daemon requires the Rego policy file to have permissions no more permissive than 0600 (readable and writable only by the owner). This restriction helps prevent unauthorized modifications to the policy file.
+
+The `--skip-rego-perm-check` flag can be used to bypass this permission check. However, using this flag comes with significant security risks:
+- More permissive file permissions could allow unauthorized users to modify the policy
+- Changes to the policy file could go unnoticed
+- Security controls could be weakened without proper oversight
+
+It is strongly recommended to:
+- Avoid using `--skip-rego-perm-check` in production environments
+- Always use proper file permissions (0600 or more restrictive)
+- Implement additional monitoring if the flag must be used
+
 The Rego policy file is a critical security control. 
 Any user with sudo privileges can:
 
@@ -123,4 +148,4 @@ Any user with sudo privileges can:
   - Restrict sudo access to specific commands 
 - Monitoring
   - Monitor policy file changes
-  - Monitor daemon service status
+  - Monitor daemon service status

docs/sample-rego-policies/default.rego

@@ -7,7 +7,7 @@ default allow = false
 
 allow if {
     not is_container_create
-    not is_networs_api
+    not is_networks_api
     not is_swarm_api
     not is_plugins
 }
@@ -17,7 +17,7 @@ is_container_create if {
     glob.match("/**/containers/create", ["/"], input.Path)
 }
 
-is_networs_api if {
+is_networks_api if {
     input.Method == "GET"
     glob.match("/**/networks", ["/"], input.Path)
 }
@@ -35,4 +35,4 @@ is_plugins if {
 is_forbidden_container if {
     input.Method == "GET"
     glob.match("/**/container/1f576a797a486438548377124f6cb7770a5cb7c8ff6a11c069cb4128d3f59462/json", ["/"], input.Path)
-}
+}

e2e/tests/opa_middleware.go

@@ -9,6 +9,9 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"os/exec"
+	"path/filepath"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -91,18 +94,6 @@ func OpaMiddlewareTest(opt *option.Option) {
 			Expect(res.StatusCode).Should(Equal(http.StatusForbidden))
 		})
 
-		It("should not allow updates to the rego file", func() {
-			regoFilePath := os.Getenv("REGO_FILE_PATH")
-			Expect(regoFilePath).NotTo(BeEmpty(), "REGO_FILE_PATH environment variable should be set")
-
-			fileInfo, err := os.Stat(regoFilePath)
-			Expect(err).NotTo(HaveOccurred(), "Failed to get Rego file info")
-
-			// Check file permissions
-			mode := fileInfo.Mode()
-			Expect(mode.Perm()).To(Equal(os.FileMode(0400)), "Rego file should be read-only (0400)")
-		})
-
 		It("should fail unimplemented API calls, fail via daemon", func() {
 			fmt.Println("incompatibleUrl = ", unimplementedUnspecifiedUrl)
 			res, _ := uClient.Get(unimplementedUnspecifiedUrl)
@@ -116,5 +107,51 @@ func OpaMiddlewareTest(opt *option.Option) {
 
 			Expect(res.StatusCode).Should(Equal(http.StatusNotFound))
 		})
+
+		// Add this test to OpaMiddlewareTest function
+		It("should handle rego file permissions correctly", func() {
+			// Create a temporary rego file with overly permissive permissions
+			tmpDir, err := os.MkdirTemp("", "rego_test")
+			Expect(err).NotTo(HaveOccurred())
+			defer os.RemoveAll(tmpDir)
+
+			regoPath := filepath.Join(tmpDir, "test.rego")
+			regoContent := []byte(`package finch.authz
+			default allow = false`)
+
+			err = os.WriteFile(regoPath, regoContent, 0644)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Try to start daemon with overly permissive file
+			cmd := exec.Command(GetFinchDaemonExe(), //nolint:gosec // G204: This is a test file with controlled inputs
+				"--socket-addr", "/run/test.sock",
+				"--pidfile", "/run/test.pid",
+				"--rego-file", regoPath,
+				"--enable-middleware")
+			output, err := cmd.CombinedOutput()
+
+			// Should fail due to permissions
+			Expect(err).To(HaveOccurred())
+			Expect(string(output)).To(ContainSubstring("rego file permissions 644 are too permissive - must be no more permissive than 0600"))
+
+			// For the second test with skip-check:
+			cmd = exec.Command(GetFinchDaemonExe(), //nolint:gosec // G204: This is a test file with controlled inputs
+				"--socket-addr", "/run/test.sock",
+				"--pidfile", "/run/test.pid",
+				"--rego-file", regoPath,
+				"--enable-middleware",
+				"--skip-rego-perm-check")
+
+			// Start the process in background
+			err = cmd.Start()
+			Expect(err).NotTo(HaveOccurred())
+
+			// Give it a moment to initialize
+			time.Sleep(1 * time.Second)
+
+			// Kill the process
+			err = cmd.Process.Kill()
+			Expect(err).NotTo(HaveOccurred())
+		})
 	})
 }

e2e/tests/tests.go

@@ -226,3 +226,11 @@ func GetFinchExe() string {
 	}
 	return finchexe
 }
+
+func GetFinchDaemonExe() string {
+	daemonPath := os.Getenv("DAEMON_ROOT")
+	if daemonPath == "" {
+		daemonPath = "./bin/finch-daemon" // fallback
+	}
+	return daemonPath
+}