fix: update tests and add logging

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

Author: coderbirju

.github/workflows/ci.yaml

@@ -112,10 +112,10 @@ jobs:
       - 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 }}/sample.rego
+        run: ls -l ${{ github.workspace }}/docs/sample-rego-policies/default.rego
       - name: Set Rego file path
-        run: echo "REGO_FILE_PATH=${{ github.workspace }}/sample.rego" >> $GITHUB_ENV
+        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 }}/sample.rego --socket-owner $UID &
+        run: sudo bin/finch-daemon --debug --enable-middleware --rego-file ${{ github.workspace }}/docs/sample-rego-policies/default.rego --socket-owner $UID &
       - name: Run opa e2e tests
         run: sudo -E make test-e2e-opa

api/router/router.go

@@ -135,13 +135,16 @@ 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)
 			rs, err := preppedQuery.Eval(r.Context(), rego.EvalInput(input))
 			if err != nil {
 				response.SendErrorResponse(w, http.StatusInternalServerError, errInput)
 				return
 			}
 
 			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)
 				response.SendErrorResponse(w, http.StatusForbidden,
 					fmt.Errorf("method %s not allowed for path %s", r.Method, r.URL.Path))
 				return

cmd/finch-daemon/main.go

@@ -149,6 +149,10 @@ func run(options *DaemonOptions) error {
 	logger := flog.NewLogrus()
 	r, err := newRouter(options, logger)
 	if err != nil {
+		// call regoFile cleanup function here to unlock previously locked file
+		if options.regoFilePath != "" {
+			cleanupRegoFile(options, logger)
+		}
 		return fmt.Errorf("failed to create a router: %w", err)
 	}
 
@@ -198,20 +202,7 @@ func run(options *DaemonOptions) error {
 		}
 	}()
 
-	defer func() {
-		if options.regoFileLock != nil {
-			// unlock the rego file upon daemon exit
-			if err := options.regoFileLock.Unlock(); err != nil {
-				logrus.Errorf("failed to unlock Rego file: %v", err)
-			}
-			logger.Infof("rego file unlocked")
-
-			// make rego file editable upon daemon exit
-			if err := os.Chmod(options.regoFilePath, 0600); err != nil {
-				logrus.Errorf("failed to change file permissions of rego file: %v", err)
-			}
-		}
-	}()
+	defer cleanupRegoFile(options, logger)
 
 	sdNotify(daemon.SdNotifyReady, logger)
 	serverWg.Wait()
@@ -297,58 +288,3 @@ func defineDockerConfig(uid int) error {
 		return true
 	})
 }
-
-// checkRegoFileValidity verifies that the given rego file exists and has the right file extension.
-func checkRegoFileValidity(filePath string) error {
-	if _, err := os.Stat(filePath); os.IsNotExist(err) {
-		return fmt.Errorf("provided Rego file path does not exist: %s", filePath)
-	}
-
-	// Check if the file has a valid extension (.rego)
-	fileExt := strings.ToLower(filepath.Ext(options.regoFilePath))
-
-	if fileExt != ".rego" {
-		return fmt.Errorf("invalid file extension for Rego file. Only .rego files are supported")
-	}
-
-	return nil
-}
-
-// 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) {
-	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?
-		}
-
-		if err := checkRegoFileValidity(options.regoFilePath); err != nil {
-			return "", err
-		}
-	}
-
-	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
-}

cmd/finch-daemon/router_utils.go

@@ -7,11 +7,14 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 
 	containerd "github.com/containerd/containerd/v2/client"
 	"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"
@@ -26,6 +29,7 @@ import (
 	"github.com/runfinch/finch-daemon/pkg/archive"
 	"github.com/runfinch/finch-daemon/pkg/ecc"
 	"github.com/runfinch/finch-daemon/pkg/flog"
+	"github.com/sirupsen/logrus"
 	"github.com/spf13/afero"
 )
 
@@ -90,6 +94,45 @@ func createContainerdClient(conf *config.Config) (*backend.ContainerdClientWrapp
 	return backend.NewContainerdClientWrapper(client), nil
 }
 
+// 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) {
+	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?
+		}
+
+		if err := checkRegoFileValidity(options.regoFilePath); err != nil {
+			return "", err
+		}
+	}
+
+	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
+}
+
 // createRouterOptions creates router options by initializing all required services.
 func createRouterOptions(
 	conf *config.Config,
@@ -116,3 +159,40 @@ func createRouterOptions(
 		RegoFilePath:        regoFilePath,
 	}
 }
+
+// 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")
+	}
+
+	return nil
+}
+
+func cleanupRegoFile(options *DaemonOptions, logger *flog.Logrus) {
+	if options.regoFileLock == nil {
+		return // Already cleaned up or nothing to clean
+	}
+
+	// unlock the rego file
+	if err := options.regoFileLock.Unlock(); err != nil {
+		logrus.Errorf("failed to unlock Rego file: %v", err)
+	}
+	logger.Infof("rego file unlocked")
+
+	// make rego file editable
+	if err := os.Chmod(options.regoFilePath, 0600); err != nil {
+		logrus.Errorf("failed to change file permissions of rego file: %v", err)
+	}
+
+	options.regoFileLock = nil
+}

cmd/finch-daemon/router_utils_test.go

@@ -4,10 +4,14 @@
 package main
 
 import (
+	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/containerd/nerdctl/v2/pkg/config"
+	"github.com/gofrs/flock"
+	"github.com/runfinch/finch-daemon/pkg/flog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -71,3 +75,129 @@ namespace = "test_namespace"
 	assert.Equal(t, "test_address", cfg.Address)
 	assert.Equal(t, "test_namespace", cfg.Namespace)
 }
+
+func TestCleanupRegoFile(t *testing.T) {
+	tests := []struct {
+		name      string
+		setupFunc func() (*DaemonOptions, *flog.Logrus, func())
+	}{
+		{
+			name: "successful cleanup",
+			setupFunc: func() (*DaemonOptions, *flog.Logrus, func()) {
+				tmpFile, err := os.CreateTemp("", "test.rego")
+				require.NoError(t, err)
+
+				fileLock := flock.New(tmpFile.Name())
+				_, err = fileLock.TryLock()
+				require.NoError(t, err)
+
+				err = os.Chmod(tmpFile.Name(), 0400)
+				require.NoError(t, err)
+
+				logger := flog.NewLogrus()
+
+				cleanup := func() {
+					os.Remove(tmpFile.Name())
+				}
+
+				return &DaemonOptions{
+					regoFilePath: tmpFile.Name(),
+					regoFileLock: fileLock,
+				}, logger, cleanup
+			},
+		},
+		{
+			name: "nil lock handle",
+			setupFunc: func() (*DaemonOptions, *flog.Logrus, func()) {
+				return &DaemonOptions{
+					regoFileLock: nil,
+				}, flog.NewLogrus(), func() {}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			options, logger, cleanup := tt.setupFunc()
+			defer cleanup()
+
+			cleanupRegoFile(options, logger)
+
+			if options.regoFilePath != "" {
+				// Verify file permissions are restored
+				info, err := os.Stat(options.regoFilePath)
+				require.NoError(t, err)
+				assert.Equal(t, os.FileMode(0600), info.Mode().Perm())
+			}
+
+			// Verify lock is released
+			assert.Nil(t, options.regoFileLock)
+		})
+	}
+}
+
+func TestCheckRegoFileValidity(t *testing.T) {
+	tests := []struct {
+		name          string
+		setupFunc     func() (string, func())
+		expectedError string
+	}{
+		{
+			name: "valid rego file",
+			setupFunc: func() (string, func()) {
+				// Create a temporary directory
+				tmpDir, err := os.MkdirTemp("", "rego_test")
+				require.NoError(t, err)
+
+				// Create a file with .rego extension and proper content
+				regoPath := filepath.Join(tmpDir, "test.rego")
+				regoContent := `package finch.authz
+
+import future.keywords.if
+import rego.v1
+
+default allow = false
+`
+				fmt.Println("regopath = ", regoPath)
+				err = os.WriteFile(regoPath, []byte(regoContent), 0600)
+				require.NoError(t, err)
+
+				return regoPath, func() {
+					os.RemoveAll(tmpDir)
+				}
+			},
+			expectedError: "",
+		},
+		{
+			name: "non-existent file",
+			setupFunc: func() (string, func()) {
+				return filepath.Join(os.TempDir(), "nonexistent.rego"), func() {}
+			},
+			expectedError: "provided Rego file path does not exist",
+		},
+		{
+			name: "wrong extension",
+			setupFunc: func() (string, func()) {
+				tmpFile, err := os.CreateTemp("", "test.txt")
+				require.NoError(t, err)
+				return tmpFile.Name(), func() { os.Remove(tmpFile.Name()) }
+			},
+			expectedError: "invalid file extension",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			filePath, cleanup := tt.setupFunc()
+			defer cleanup()
+
+			err := checkRegoFileValidity(filePath)
+
+			if tt.expectedError != "" {
+				assert.ErrorContains(t, err, tt.expectedError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}