Skip to content

Commit a9cab54

Browse files
coderbirjucoderbirju
committed
fix: update tests and add logging
Signed-off-by: Arjun Raja Yogidas <[email protected]>
1 parent 7c56d42 commit a9cab54

File tree

11 files changed

+378
-100
lines changed

11 files changed

+378
-100
lines changed

‎.github/workflows/ci.yaml‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,10 +112,10 @@ jobs:
112112
- name: Clean up Daemon socket
113113
run: sudo rm /var/run/finch.sock && sudo rm /run/finch.pid
114114
- name: Verify Rego file presence
115-
run: ls -l ${{ github.workspace }}/sample.rego
115+
run: ls -l ${{ github.workspace }}/docs/sample-rego-policies/default.rego
116116
- name: Set Rego file path
117-
run: echo "REGO_FILE_PATH=${{ github.workspace }}/sample.rego" >> $GITHUB_ENV
117+
run: echo "REGO_FILE_PATH=${{ github.workspace }}/docs/sample-rego-policies/default.rego" >> $GITHUB_ENV
118118
- name: Start finch-daemon with opa Authz
119-
run: sudo bin/finch-daemon --debug --enable-middleware --rego-file ${{ github.workspace }}/sample.rego --socket-owner $UID &
119+
run: sudo bin/finch-daemon --debug --enable-middleware --rego-file ${{ github.workspace }}/docs/sample-rego-policies/default.rego --socket-owner $UID &
120120
- name: Run opa e2e tests
121121
run: sudo -E make test-e2e-opa

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
GINKGO = go run github.com/onsi/ginkgo/v2/ginkgo
55
# Common ginkgo options: -v for verbose mode, --focus="test name" for running single tests
6-
GFLAGS ?= --race --randomize-all --randomize-suites --vv
6+
GFLAGS ?= --race --randomize-all --randomize-suites
77
BIN = $(PWD)/bin
88
FINCH_DAEMON_PROJECT_ROOT ?= $(shell pwd)
99

‎api/router/router.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,13 +135,16 @@ func CreateRegoMiddleware(regoFilePath string) (func(next http.Handler) http.Han
135135
Path: r.URL.Path,
136136
}
137137

138+
fmt.Printf("Evaluating policy rules for API request with Method = %s and Path = %s \n", input.Method, input.Path)
138139
rs, err := preppedQuery.Eval(r.Context(), rego.EvalInput(input))
139140
if err != nil {
140141
response.SendErrorResponse(w, http.StatusInternalServerError, errInput)
141142
return
142143
}
143144

144145
if !rs.Allowed() {
146+
// need to log evaluation result in order to mitigate Repudiation threat
147+
fmt.Printf("Evaluation result: failed, method %s not allowed for path %s \n", r.Method, r.URL.Path)
145148
response.SendErrorResponse(w, http.StatusForbidden,
146149
fmt.Errorf("method %s not allowed for path %s", r.Method, r.URL.Path))
147150
return

‎cmd/finch-daemon/main.go‎

Lines changed: 5 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,10 @@ func run(options *DaemonOptions) error {
149149
logger := flog.NewLogrus()
150150
r, err := newRouter(options, logger)
151151
if err != nil {
152+
// call regoFile cleanup function here to unlock previously locked file
153+
if options.regoFilePath != "" {
154+
cleanupRegoFile(options, logger)
155+
}
152156
return fmt.Errorf("failed to create a router: %w", err)
153157
}
154158

@@ -198,20 +202,7 @@ func run(options *DaemonOptions) error {
198202
}
199203
}()
200204

201-
defer func() {
202-
if options.regoFileLock != nil {
203-
// unlock the rego file upon daemon exit
204-
if err := options.regoFileLock.Unlock(); err != nil {
205-
logrus.Errorf("failed to unlock Rego file: %v", err)
206-
}
207-
logger.Infof("rego file unlocked")
208-
209-
// make rego file editable upon daemon exit
210-
if err := os.Chmod(options.regoFilePath, 0600); err != nil {
211-
logrus.Errorf("failed to change file permissions of rego file: %v", err)
212-
}
213-
}
214-
}()
205+
defer cleanupRegoFile(options, logger)
215206

216207
sdNotify(daemon.SdNotifyReady, logger)
217208
serverWg.Wait()
@@ -297,58 +288,3 @@ func defineDockerConfig(uid int) error {
297288
return true
298289
})
299290
}
300-
301-
// checkRegoFileValidity verifies that the given rego file exists and has the right file extension.
302-
func checkRegoFileValidity(filePath string) error {
303-
if _, err := os.Stat(filePath); os.IsNotExist(err) {
304-
return fmt.Errorf("provided Rego file path does not exist: %s", filePath)
305-
}
306-
307-
// Check if the file has a valid extension (.rego)
308-
fileExt := strings.ToLower(filepath.Ext(options.regoFilePath))
309-
310-
if fileExt != ".rego" {
311-
return fmt.Errorf("invalid file extension for Rego file. Only .rego files are supported")
312-
}
313-
314-
return nil
315-
}
316-
317-
// sanitizeRegoFile validates and prepares the Rego policy file for use.
318-
// It checks validates the file, acquires a file lock,
319-
// and sets rego file to be read-only.
320-
func sanitizeRegoFile(options *DaemonOptions) (string, error) {
321-
if options.regoFilePath != "" {
322-
if !options.enableMiddleware {
323-
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?
324-
}
325-
326-
if err := checkRegoFileValidity(options.regoFilePath); err != nil {
327-
return "", err
328-
}
329-
}
330-
331-
if options.enableMiddleware && options.regoFilePath == "" {
332-
return "", fmt.Errorf("rego file path not provided, please provide the policy file path using the --rego-file flag")
333-
}
334-
335-
fileLock := flock.New(options.regoFilePath)
336-
337-
locked, err := fileLock.TryLock()
338-
if err != nil {
339-
return "", fmt.Errorf("error acquiring lock on rego file: %v", err)
340-
}
341-
if !locked {
342-
return "", fmt.Errorf("unable to acquire lock on rego file, it may be in use by another process")
343-
}
344-
345-
// Change file permissions to read-only
346-
err = os.Chmod(options.regoFilePath, 0400)
347-
if err != nil {
348-
fileLock.Unlock()
349-
return "", fmt.Errorf("error changing rego file permissions: %v", err)
350-
}
351-
options.regoFileLock = fileLock
352-
353-
return options.regoFilePath, nil
354-
}

‎cmd/finch-daemon/router_utils.go‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,14 @@ import (
77
"errors"
88
"fmt"
99
"os"
10+
"path/filepath"
11+
"strings"
1012

1113
containerd "github.com/containerd/containerd/v2/client"
1214
"github.com/containerd/containerd/v2/pkg/namespaces"
1315
"github.com/containerd/nerdctl/v2/pkg/api/types"
1416
"github.com/containerd/nerdctl/v2/pkg/config"
17+
"github.com/gofrs/flock"
1518
toml "github.com/pelletier/go-toml/v2"
1619
"github.com/runfinch/finch-daemon/api/router"
1720
"github.com/runfinch/finch-daemon/internal/backend"
@@ -26,6 +29,7 @@ import (
2629
"github.com/runfinch/finch-daemon/pkg/archive"
2730
"github.com/runfinch/finch-daemon/pkg/ecc"
2831
"github.com/runfinch/finch-daemon/pkg/flog"
32+
"github.com/sirupsen/logrus"
2933
"github.com/spf13/afero"
3034
)
3135

@@ -90,6 +94,45 @@ func createContainerdClient(conf *config.Config) (*backend.ContainerdClientWrapp
9094
return backend.NewContainerdClientWrapper(client), nil
9195
}
9296

97+
// sanitizeRegoFile validates and prepares the Rego policy file for use.
98+
// It checks validates the file, acquires a file lock,
99+
// and sets rego file to be read-only.
100+
func sanitizeRegoFile(options *DaemonOptions) (string, error) {
101+
if options.regoFilePath != "" {
102+
if !options.enableMiddleware {
103+
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?
104+
}
105+
106+
if err := checkRegoFileValidity(options.regoFilePath); err != nil {
107+
return "", err
108+
}
109+
}
110+
111+
if options.enableMiddleware && options.regoFilePath == "" {
112+
return "", fmt.Errorf("rego file path not provided, please provide the policy file path using the --rego-file flag")
113+
}
114+
115+
fileLock := flock.New(options.regoFilePath)
116+
117+
locked, err := fileLock.TryLock()
118+
if err != nil {
119+
return "", fmt.Errorf("error acquiring lock on rego file: %v", err)
120+
}
121+
if !locked {
122+
return "", fmt.Errorf("unable to acquire lock on rego file, it may be in use by another process")
123+
}
124+
125+
// Change file permissions to read-only
126+
err = os.Chmod(options.regoFilePath, 0400)
127+
if err != nil {
128+
fileLock.Unlock()
129+
return "", fmt.Errorf("error changing rego file permissions: %v", err)
130+
}
131+
options.regoFileLock = fileLock
132+
133+
return options.regoFilePath, nil
134+
}
135+
93136
// createRouterOptions creates router options by initializing all required services.
94137
func createRouterOptions(
95138
conf *config.Config,
@@ -116,3 +159,40 @@ func createRouterOptions(
116159
RegoFilePath: regoFilePath,
117160
}
118161
}
162+
163+
// checkRegoFileValidity verifies that the given rego file exists and has the right file extension.
164+
func checkRegoFileValidity(regoFilePath string) error {
165+
fmt.Println("filepath in checkRegoFileValidity = ", regoFilePath)
166+
if _, err := os.Stat(regoFilePath); os.IsNotExist(err) {
167+
return fmt.Errorf("provided Rego file path does not exist: %s", regoFilePath)
168+
}
169+
170+
// Check if the file has a valid extension (.rego)
171+
fileExt := strings.ToLower(filepath.Ext(regoFilePath))
172+
173+
fmt.Println("fileExt = ", fileExt)
174+
if fileExt != ".rego" {
175+
return fmt.Errorf("invalid file extension for Rego file. Only .rego files are supported")
176+
}
177+
178+
return nil
179+
}
180+
181+
func cleanupRegoFile(options *DaemonOptions, logger *flog.Logrus) {
182+
if options.regoFileLock == nil {
183+
return // Already cleaned up or nothing to clean
184+
}
185+
186+
// unlock the rego file
187+
if err := options.regoFileLock.Unlock(); err != nil {
188+
logrus.Errorf("failed to unlock Rego file: %v", err)
189+
}
190+
logger.Infof("rego file unlocked")
191+
192+
// make rego file editable
193+
if err := os.Chmod(options.regoFilePath, 0600); err != nil {
194+
logrus.Errorf("failed to change file permissions of rego file: %v", err)
195+
}
196+
197+
options.regoFileLock = nil
198+
}

‎cmd/finch-daemon/router_utils_test.go‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,14 @@
44
package main
55

66
import (
7+
"fmt"
78
"os"
9+
"path/filepath"
810
"testing"
911

1012
"github.com/containerd/nerdctl/v2/pkg/config"
13+
"github.com/gofrs/flock"
14+
"github.com/runfinch/finch-daemon/pkg/flog"
1115
"github.com/stretchr/testify/assert"
1216
"github.com/stretchr/testify/require"
1317
)
@@ -71,3 +75,129 @@ namespace = "test_namespace"
7175
assert.Equal(t, "test_address", cfg.Address)
7276
assert.Equal(t, "test_namespace", cfg.Namespace)
7377
}
78+
79+
func TestCleanupRegoFile(t *testing.T) {
80+
tests := []struct {
81+
name string
82+
setupFunc func() (*DaemonOptions, *flog.Logrus, func())
83+
}{
84+
{
85+
name: "successful cleanup",
86+
setupFunc: func() (*DaemonOptions, *flog.Logrus, func()) {
87+
tmpFile, err := os.CreateTemp("", "test.rego")
88+
require.NoError(t, err)
89+
90+
fileLock := flock.New(tmpFile.Name())
91+
_, err = fileLock.TryLock()
92+
require.NoError(t, err)
93+
94+
err = os.Chmod(tmpFile.Name(), 0400)
95+
require.NoError(t, err)
96+
97+
logger := flog.NewLogrus()
98+
99+
cleanup := func() {
100+
os.Remove(tmpFile.Name())
101+
}
102+
103+
return &DaemonOptions{
104+
regoFilePath: tmpFile.Name(),
105+
regoFileLock: fileLock,
106+
}, logger, cleanup
107+
},
108+
},
109+
{
110+
name: "nil lock handle",
111+
setupFunc: func() (*DaemonOptions, *flog.Logrus, func()) {
112+
return &DaemonOptions{
113+
regoFileLock: nil,
114+
}, flog.NewLogrus(), func() {}
115+
},
116+
},
117+
}
118+
119+
for _, tt := range tests {
120+
t.Run(tt.name, func(t *testing.T) {
121+
options, logger, cleanup := tt.setupFunc()
122+
defer cleanup()
123+
124+
cleanupRegoFile(options, logger)
125+
126+
if options.regoFilePath != "" {
127+
// Verify file permissions are restored
128+
info, err := os.Stat(options.regoFilePath)
129+
require.NoError(t, err)
130+
assert.Equal(t, os.FileMode(0600), info.Mode().Perm())
131+
}
132+
133+
// Verify lock is released
134+
assert.Nil(t, options.regoFileLock)
135+
})
136+
}
137+
}
138+
139+
func TestCheckRegoFileValidity(t *testing.T) {
140+
tests := []struct {
141+
name string
142+
setupFunc func() (string, func())
143+
expectedError string
144+
}{
145+
{
146+
name: "valid rego file",
147+
setupFunc: func() (string, func()) {
148+
// Create a temporary directory
149+
tmpDir, err := os.MkdirTemp("", "rego_test")
150+
require.NoError(t, err)
151+
152+
// Create a file with .rego extension and proper content
153+
regoPath := filepath.Join(tmpDir, "test.rego")
154+
regoContent := `package finch.authz
155+
156+
import future.keywords.if
157+
import rego.v1
158+
159+
default allow = false
160+
`
161+
fmt.Println("regopath = ", regoPath)
162+
err = os.WriteFile(regoPath, []byte(regoContent), 0600)
163+
require.NoError(t, err)
164+
165+
return regoPath, func() {
166+
os.RemoveAll(tmpDir)
167+
}
168+
},
169+
expectedError: "",
170+
},
171+
{
172+
name: "non-existent file",
173+
setupFunc: func() (string, func()) {
174+
return filepath.Join(os.TempDir(), "nonexistent.rego"), func() {}
175+
},
176+
expectedError: "provided Rego file path does not exist",
177+
},
178+
{
179+
name: "wrong extension",
180+
setupFunc: func() (string, func()) {
181+
tmpFile, err := os.CreateTemp("", "test.txt")
182+
require.NoError(t, err)
183+
return tmpFile.Name(), func() { os.Remove(tmpFile.Name()) }
184+
},
185+
expectedError: "invalid file extension",
186+
},
187+
}
188+
189+
for _, tt := range tests {
190+
t.Run(tt.name, func(t *testing.T) {
191+
filePath, cleanup := tt.setupFunc()
192+
defer cleanup()
193+
194+
err := checkRegoFileValidity(filePath)
195+
196+
if tt.expectedError != "" {
197+
assert.ErrorContains(t, err, tt.expectedError)
198+
} else {
199+
assert.NoError(t, err)
200+
}
201+
})
202+
}
203+
}

0 commit comments

Comments
 (0)