Skip to content

Commit d2ba616

Browse files
committed
chore: add file permission check
Signed-off-by: Arjun Raja Yogidas <[email protected]>
1 parent a425762 commit d2ba616

File tree

9 files changed

+139
-65
lines changed

9 files changed

+139
-65
lines changed

.github/workflows/ci.yaml

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -105,17 +105,19 @@ jobs:
105105
run: |
106106
sudo ls /etc/cni/net.d
107107
sudo rm /etc/cni/net.d/87-podman-bridge.conflist
108-
- name: Start finch-daemon
109-
run: sudo bin/finch-daemon --debug --socket-owner $UID &
110-
- name: Run e2e test
111-
run: sudo make test-e2e
112-
- name: Clean up Daemon socket
113-
run: sudo rm /var/run/finch.sock && sudo rm /run/finch.pid
114108
- name: Verify Rego file presence
115109
run: ls -l ${{ github.workspace }}/docs/sample-rego-policies/default.rego
116110
- name: Set Rego file path
117111
run: echo "REGO_FILE_PATH=${{ github.workspace }}/docs/sample-rego-policies/default.rego" >> $GITHUB_ENV
118112
- name: Start finch-daemon with opa Authz
119-
run: sudo bin/finch-daemon --debug --enable-middleware --rego-file ${{ github.workspace }}/docs/sample-rego-policies/default.rego --socket-owner $UID &
113+
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 &
120114
- name: Run opa e2e tests
121115
run: sudo -E make test-e2e-opa
116+
- name: Clean up Daemon socket
117+
run: sudo rm /run/finch.sock && sudo rm /run/finch.pid
118+
- name: Start finch-daemon
119+
run: sudo bin/finch-daemon --debug --socket-owner $UID &
120+
- name: Run e2e test
121+
run: sudo make test-e2e
122+
- name: Clean up Daemon socket
123+
run: sudo rm /var/run/finch.sock && sudo rm /run/finch.pid

Makefile

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,10 @@ test-e2e: linux
117117
.PHONY: test-e2e-opa
118118
test-e2e-opa: linux
119119
DOCKER_HOST="unix:///run/finch.sock" \
120-
DOCKER_API_VERSION="v1.43" \
120+
DOCKER_API_VERSION="v1.41" \
121121
MIDDLEWARE_E2E=1 \
122-
TEST_E2E=1 \
122+
TEST_E2E=0 \
123+
DAEMON_ROOT="$(BIN)/finch-daemon" \
123124
$(GINKGO) $(GFLAGS) ./e2e/...
124125

125126
.PHONY: licenses
@@ -134,4 +135,4 @@ coverage: linux
134135
.PHONY: release
135136
release: linux
136137
@echo "$@"
137-
@$(FINCH_DAEMON_PROJECT_ROOT)/scripts/create-releases.sh $(RELEASE_TAG)
138+
@$(FINCH_DAEMON_PROJECT_ROOT)/scripts/create-releases.sh $(RELEASE_TAG)

api/router/router.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -135,20 +135,26 @@ 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)
138+
fmt.Printf("[OPA Debug] Input being evaluated: Method=%s, Path=%s\n", input.Method, input.Path)
139+
fmt.Printf("[OPA Debug] Query being executed: %s\n", query)
140+
139141
rs, err := preppedQuery.Eval(r.Context(), rego.EvalInput(input))
140142
if err != nil {
143+
fmt.Printf("[OPA Error] Policy evaluation failed: %v\n", err)
141144
response.SendErrorResponse(w, http.StatusInternalServerError, errInput)
142145
return
143146
}
144147

148+
fmt.Printf("[OPA Debug] Evaluation results: %+v\n", rs)
149+
fmt.Printf("[OPA Debug] Number of results: %d\n", len(rs))
150+
145151
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)
152+
fmt.Printf("[OPA Denied] Request denied: Method=%s, Path=%s\n", r.Method, r.URL.Path)
148153
response.SendErrorResponse(w, http.StatusForbidden,
149154
fmt.Errorf("method %s not allowed for path %s", r.Method, r.URL.Path))
150155
return
151156
}
157+
fmt.Printf("[OPA Allowed] Request allowed: Method=%s, Path=%s\n", r.Method, r.URL.Path)
152158
newReq := r.WithContext(r.Context())
153159
next.ServeHTTP(w, newReq)
154160
})

cmd/finch-daemon/main.go

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,16 @@ const (
4343
)
4444

4545
type DaemonOptions struct {
46-
debug bool
47-
socketAddr string
48-
socketOwner int
49-
debugAddress string
50-
configPath string
51-
pidFile string
52-
regoFilePath string
53-
enableMiddleware bool
54-
regoFileLock *flock.Flock
46+
debug bool
47+
socketAddr string
48+
socketOwner int
49+
debugAddress string
50+
configPath string
51+
pidFile string
52+
regoFilePath string
53+
enableMiddleware bool
54+
skipRegoPermCheck bool
55+
regoFileLock *flock.Flock
5556
}
5657

5758
var options = new(DaemonOptions)
@@ -72,6 +73,8 @@ func main() {
7273
rootCmd.Flags().StringVar(&options.pidFile, "pidfile", defaultPidFile, "pid file location")
7374
rootCmd.Flags().StringVar(&options.regoFilePath, "rego-file", "", "Rego Policy Path")
7475
rootCmd.Flags().BoolVar(&options.enableMiddleware, "enable-middleware", false, "turn on middleware for allowlisting")
76+
rootCmd.Flags().BoolVar(&options.skipRegoPermCheck, "skip-rego-perm-check", false, "skip the rego file permission check (allows permissions more permissive than 0600)")
77+
7578
if err := rootCmd.Execute(); err != nil {
7679
log.Printf("got error: %v", err)
7780
log.Fatal(err)
@@ -228,7 +231,7 @@ func newRouter(options *DaemonOptions, logger *flog.Logrus) (http.Handler, error
228231

229232
var regoFilePath string
230233
if options.enableMiddleware {
231-
regoFilePath, err = sanitizeRegoFile(options)
234+
regoFilePath, err = sanitizeRegoFile(options, logger)
232235
if err != nil {
233236
return nil, err
234237
}

cmd/finch-daemon/router_utils.go

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414
"github.com/containerd/containerd/v2/pkg/namespaces"
1515
"github.com/containerd/nerdctl/v2/pkg/api/types"
1616
"github.com/containerd/nerdctl/v2/pkg/config"
17-
"github.com/gofrs/flock"
1817
toml "github.com/pelletier/go-toml/v2"
1918
"github.com/runfinch/finch-daemon/api/router"
2019
"github.com/runfinch/finch-daemon/internal/backend"
@@ -97,7 +96,7 @@ func createContainerdClient(conf *config.Config) (*backend.ContainerdClientWrapp
9796
// sanitizeRegoFile validates and prepares the Rego policy file for use.
9897
// It checks validates the file, acquires a file lock,
9998
// and sets rego file to be read-only.
100-
func sanitizeRegoFile(options *DaemonOptions) (string, error) {
99+
func sanitizeRegoFile(options *DaemonOptions, logger *flog.Logrus) (string, error) {
101100
if options.regoFilePath != "" {
102101
if !options.enableMiddleware {
103102
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) {
106105
if err := checkRegoFileValidity(options.regoFilePath); err != nil {
107106
return "", err
108107
}
108+
109+
if !options.skipRegoPermCheck {
110+
fileInfo, err := os.Stat(options.regoFilePath)
111+
if err != nil {
112+
return "", fmt.Errorf("error checking rego file permissions: %v", err)
113+
}
114+
115+
if fileInfo.Mode().Perm() > 0600 {
116+
return "", fmt.Errorf("rego file permissions %o are too permissive - must be no more permissive than 0600", fileInfo.Mode().Perm())
117+
}
118+
logger.Debugf("rego file permissions check passed: %o", fileInfo.Mode().Perm())
119+
} else {
120+
logger.Warnf("skipping rego file permission check - file may have permissions more permissive than 0600")
121+
}
109122
}
110123

111124
if options.enableMiddleware && options.regoFilePath == "" {
112125
return "", fmt.Errorf("rego file path not provided, please provide the policy file path using the --rego-file flag")
113126
}
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-
133127
return options.regoFilePath, nil
134128
}
135129

@@ -162,15 +156,13 @@ func createRouterOptions(
162156

163157
// checkRegoFileValidity verifies that the given rego file exists and has the right file extension.
164158
func checkRegoFileValidity(regoFilePath string) error {
165-
fmt.Println("filepath in checkRegoFileValidity = ", regoFilePath)
166159
if _, err := os.Stat(regoFilePath); os.IsNotExist(err) {
167160
return fmt.Errorf("provided Rego file path does not exist: %s", regoFilePath)
168161
}
169162

170163
// Check if the file has a valid extension (.rego)
171164
fileExt := strings.ToLower(filepath.Ext(regoFilePath))
172165

173-
fmt.Println("fileExt = ", fileExt)
174166
if fileExt != ".rego" {
175167
return fmt.Errorf("invalid file extension for Rego file. Only .rego files are supported")
176168
}

docs/opa-middleware.md

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,19 @@ Once you are ready with your policy document, use the `--enable-middleware` flag
3838

3939
Note: The `--rego-file` flag is required when `--enable-middleware` is set.
4040

41-
Example:
42-
`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 &`
41+
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.
42+
43+
Examples:
44+
45+
Standard secure usage:
46+
```bash
47+
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
48+
```
49+
50+
With permission check bypassed:
51+
```bash
52+
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
53+
```
4354

4455

4556
# 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
109120
- Monitor for unexpected denials
110121

111122

112-
### Critical Security Considerations : Rego Policy File Protection
123+
### Critical Security Considerations: Rego Policy File Protection
124+
125+
### Rego File Permissions
126+
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.
127+
128+
The `--skip-rego-perm-check` flag can be used to bypass this permission check. However, using this flag comes with significant security risks:
129+
- More permissive file permissions could allow unauthorized users to modify the policy
130+
- Changes to the policy file could go unnoticed
131+
- Security controls could be weakened without proper oversight
132+
133+
It is strongly recommended to:
134+
- Avoid using `--skip-rego-perm-check` in production environments
135+
- Always use proper file permissions (0600 or more restrictive)
136+
- Implement additional monitoring if the flag must be used
137+
113138
The Rego policy file is a critical security control.
114139
Any user with sudo privileges can:
115140

@@ -123,4 +148,4 @@ Any user with sudo privileges can:
123148
- Restrict sudo access to specific commands
124149
- Monitoring
125150
- Monitor policy file changes
126-
- Monitor daemon service status
151+
- Monitor daemon service status

docs/sample-rego-policies/default.rego

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ default allow = false
77

88
allow if {
99
not is_container_create
10-
not is_networs_api
10+
not is_networks_api
1111
not is_swarm_api
1212
not is_plugins
1313
}
@@ -17,7 +17,7 @@ is_container_create if {
1717
glob.match("/**/containers/create", ["/"], input.Path)
1818
}
1919

20-
is_networs_api if {
20+
is_networks_api if {
2121
input.Method == "GET"
2222
glob.match("/**/networks", ["/"], input.Path)
2323
}
@@ -35,4 +35,4 @@ is_plugins if {
3535
is_forbidden_container if {
3636
input.Method == "GET"
3737
glob.match("/**/container/1f576a797a486438548377124f6cb7770a5cb7c8ff6a11c069cb4128d3f59462/json", ["/"], input.Path)
38-
}
38+
}

e2e/tests/opa_middleware.go

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ import (
99
"fmt"
1010
"net/http"
1111
"os"
12+
"os/exec"
13+
"path/filepath"
14+
"time"
1215

1316
. "github.com/onsi/ginkgo/v2"
1417
. "github.com/onsi/gomega"
@@ -91,18 +94,6 @@ func OpaMiddlewareTest(opt *option.Option) {
9194
Expect(res.StatusCode).Should(Equal(http.StatusForbidden))
9295
})
9396

94-
It("should not allow updates to the rego file", func() {
95-
regoFilePath := os.Getenv("REGO_FILE_PATH")
96-
Expect(regoFilePath).NotTo(BeEmpty(), "REGO_FILE_PATH environment variable should be set")
97-
98-
fileInfo, err := os.Stat(regoFilePath)
99-
Expect(err).NotTo(HaveOccurred(), "Failed to get Rego file info")
100-
101-
// Check file permissions
102-
mode := fileInfo.Mode()
103-
Expect(mode.Perm()).To(Equal(os.FileMode(0400)), "Rego file should be read-only (0400)")
104-
})
105-
10697
It("should fail unimplemented API calls, fail via daemon", func() {
10798
fmt.Println("incompatibleUrl = ", unimplementedUnspecifiedUrl)
10899
res, _ := uClient.Get(unimplementedUnspecifiedUrl)
@@ -116,5 +107,51 @@ func OpaMiddlewareTest(opt *option.Option) {
116107

117108
Expect(res.StatusCode).Should(Equal(http.StatusNotFound))
118109
})
110+
111+
// Add this test to OpaMiddlewareTest function
112+
It("should handle rego file permissions correctly", func() {
113+
// Create a temporary rego file with overly permissive permissions
114+
tmpDir, err := os.MkdirTemp("", "rego_test")
115+
Expect(err).NotTo(HaveOccurred())
116+
defer os.RemoveAll(tmpDir)
117+
118+
regoPath := filepath.Join(tmpDir, "test.rego")
119+
regoContent := []byte(`package finch.authz
120+
default allow = false`)
121+
122+
err = os.WriteFile(regoPath, regoContent, 0644)
123+
Expect(err).NotTo(HaveOccurred())
124+
125+
// Try to start daemon with overly permissive file
126+
cmd := exec.Command(GetFinchDaemonExe(), //nolint:gosec // G204: This is a test file with controlled inputs
127+
"--socket-addr", "/run/test.sock",
128+
"--pidfile", "/run/test.pid",
129+
"--rego-file", regoPath,
130+
"--enable-middleware")
131+
output, err := cmd.CombinedOutput()
132+
133+
// Should fail due to permissions
134+
Expect(err).To(HaveOccurred())
135+
Expect(string(output)).To(ContainSubstring("rego file permissions 644 are too permissive - must be no more permissive than 0600"))
136+
137+
// For the second test with skip-check:
138+
cmd = exec.Command(GetFinchDaemonExe(), //nolint:gosec // G204: This is a test file with controlled inputs
139+
"--socket-addr", "/run/test.sock",
140+
"--pidfile", "/run/test.pid",
141+
"--rego-file", regoPath,
142+
"--enable-middleware",
143+
"--skip-rego-perm-check")
144+
145+
// Start the process in background
146+
err = cmd.Start()
147+
Expect(err).NotTo(HaveOccurred())
148+
149+
// Give it a moment to initialize
150+
time.Sleep(1 * time.Second)
151+
152+
// Kill the process
153+
err = cmd.Process.Kill()
154+
Expect(err).NotTo(HaveOccurred())
155+
})
119156
})
120157
}

e2e/tests/tests.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,3 +226,11 @@ func GetFinchExe() string {
226226
}
227227
return finchexe
228228
}
229+
230+
func GetFinchDaemonExe() string {
231+
daemonPath := os.Getenv("DAEMON_ROOT")
232+
if daemonPath == "" {
233+
daemonPath = "./bin/finch-daemon" // fallback
234+
}
235+
return daemonPath
236+
}

0 commit comments

Comments
 (0)