8000 Validate file permissions during config apply (#1284) · nginx/agent@3df2793 · GitHub
[go: up one dir, main page]
Skip to content

Commit 3df2793

Browse files
spencerugbospencerugbo
authored
Validate file permissions during config apply (#1284)
* prevent execute permissions from being set * Update file_manager_service.go * cleanup * fix permissions in integration tests * add unit tests * parse user's permissions and remove execute bit * refactor permission reset logic * add more unit test cases * refactor validator functions
1 parent 86fe87e commit 3df2793

File tree

3 files changed

+172
-1
lines changed

3 files changed

+172
-1
lines changed

‎internal/file/file_manager_service.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"os"
1616
"path"
1717
"path/filepath"
18+
"strconv"
1819
"sync"
1920

2021
"google.golang.org/grpc"
@@ -36,6 +37,7 @@ const (
3637
maxAttempts = 5
3738
dirPerm = 0o755
3839
filePerm = 0o600
40+
executePerm = 0o111
3941
)
4042

4143
type (
@@ -660,6 +662,49 @@ func (fms *FileManagerService) checkAllowedDirectory(checkFiles []*mpi.File) err
660662
return nil
661663
}
662664

665+
func (fms *FileManagerService) validateAndUpdateFilePermissions(ctx context.Context, fileList []*mpi.File) error {
666+
for _, file := range fileList {
667+
if fms.areExecuteFilePermissionsSet(file) {
668+
resetErr := fms.removeExecuteFilePermissions(ctx, file)
669+
if resetErr != nil {
670+
return fmt.Errorf("failed to reset permissions for %s: %w", file.GetFileMeta().GetName(), resetErr)
671+
}
672+
}
673+
}
674+
675+
return nil
676+
}
677+
678+
func (fms *FileManagerService) areExecuteFilePermissionsSet(file *mpi.File) bool {
679+
filePermission := file.GetFileMeta().GetPermissions()
680+
681+
permissionOctal, err := strconv.ParseUint(filePermission, 8, 32)
682+
if err != nil || len(filePermission) != 4 {
683+
return false
684+
}
685+
686+
return permissionOctal&executePerm > 0
687+
}
688+
689+
func (fms *FileManagerService) removeExecuteFilePermissions(ctx context.Context, file *mpi.File) error {
690+
filePermission := file.GetFileMeta().GetPermissions()
691+
692+
permissionOctal, err := strconv.ParseUint(filePermission, 8, 32)
693+
if err != nil {
694+
return fmt.Errorf("falied to parse file permissions: %w", err)
695+
}
696+
697+
permissionOctal &^= executePerm
698+
699+
newPermission := "0" + strconv.FormatUint(permissionOctal, 8)
700+
file.FileMeta.Permissions = newPermission
701+
702+
slog.DebugContext(ctx, "Permissions have been changed", "file", file.GetFileMeta().GetName(),
703+
"old_permissions", filePermission, "new_permissions", newPermission)
704+
705+
return nil
706+
}
707+
663708
func (fms *FileManagerService) convertToManifestFileMap(
664709
currentFiles map[string]*mpi.File,
665710
referenced bool,

‎internal/file/file_manager_service_test.go‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,132 @@ func TestFileManagerService_checkAllowedDirectory(t *testing.T) {
335335
require.Error(t, err)
336336
}
337337

338+
func TestFileManagerService_validateAndUpdateFilePermissions(t *testing.T) {
339+
ctx := context.Background()
340+
fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
341+
342+
testFiles := []*mpi.File{
343+
{
344+
FileMeta: &mpi.FileMeta{
345+
Name: "exec.conf",
346+
Permissions: "0700",
347+
},
348+
},
349+
{
350+
FileMeta: &mpi.FileMeta{
351+
Name: "normal.conf",
352+
Permissions: "0620",
353+
},
354+
},
355+
}
356+
357+
err := fileManagerService.validateAndUpdateFilePermissions(ctx, testFiles)
358+
require.NoError(t, err)
359+
assert.Equal(t, "0600", testFiles[0].GetFileMeta().GetPermissions())
360+
assert.Equal(t, "0620", testFiles[1].GetFileMeta().GetPermissions())
361+
}
362+
363+
func TestFileManagerService_areExecuteFilePermissionsSet(t *testing.T) {
364+
fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
365+
366+
tests := []struct {
367+
name string
368+
permissions string
369+
expectBool bool
370+
}{
371+
{
372+
name: "Test 1: File with read and write permissions for owner",
373+
permissions: "0600",
374+
expectBool: false,
375+
},
376+
{
377+
name: "Test 2: File with read/write and execute permissions for owner",
378+
permissions: "0700",
379+
expectBool: true,
380+
},
381+
{
382+
name: "Test 3: File with read/write and execute permissions for owner and group",
383+
permissions: "0770",
384+
expectBool: true,
385+
},
386+
{
387+
name: "Test 4: File with read and execute permissions for everyone",
388+
permissions: "0555",
389+
expectBool: true,
390+
},
391+
{
392+
name: "Test 5: File with malformed permissions",
393+
permissions: "abcde",
394+
expectBool: false,
395+
},
396+
{
397+
name: "Test 6: File with invalid permissions",
398+
permissions: "000070",
399+
expectBool: false,
400+
},
401+
}
402+
403+
for _, test := range tests {
404+
t.Run(test.name, func(t *testing.T) {
405+
file := &mpi.File{
406+
FileMeta: &mpi.FileMeta{
407+
Name: "test.conf",
408+
Permissions: test.permissions,
409+
},
410+
}
411+
412+
got := fileManagerService.areExecuteFilePermissionsSet(file)
413+
assert.Equal(t, test.expectBool, got)
414+
})
415+
}
416+
}
417+
418+
func TestFileManagerService_removeExecuteFilePermissions(t *testing.T) {
419+
fileManagerService := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
420+
421+
tests := []struct {
422+
name string
423+
permissions string
424+
errorMsg string
425+
expectPermissions string
426+
expectError bool
427+
}{
428+
{
429+
name: "Test 1: File with execute permissions for owner and others",
430+
permissions: "0703",
431+
expectError: false,
432+
expectPermissions: "0602",
433+
},
434+
{
435+
name: "Test 2: File with malformed permissions",
436+
permissions: "abcde",
437+
expectError: true,
438+
errorMsg: "falied to parse file permissions",
439+
},
440+
}
441+
442+
for _, test := range tests {
443+
t.Run(test.name, func(t *testing.T) {
444+
file := &mpi.File{
445+
FileMeta: &mpi.FileMeta{
446+
Name: "test.conf",
447+
Permissions: test.permissions,
448+
},
449+
}
450+
451+
parseErr := fileManagerService.removeExecuteFilePermissions(t.Context(), file)
452+
453+
if test.expectError {
454+
require.Error(t, parseErr)
455+
assert.Contains(t, parseErr.Error(), test.errorMsg)
456+
} else {
457+
require.NoError(t, parseErr)
458+
assert.Equal(t, test.expectPermissions, file.GetFileMeta().GetPermissions())
459+
}
460+
})
461+
}
462+
}
463+
338464
//nolint:usetesting // need to use MkDirTemp instead of t.tempDir for rollback as t.tempDir does not accept a pattern
339465
func TestFileManagerService_ClearCache(t *testing.T) {
340466
tempDir := t.TempDir()

‎test/helpers/test_containers_utils.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818
"github.com/testcontainers/testcontainers-go/wait"
1919
)
2020

21-
const configFilePermissions = 0o700
21+
const configFilePermissions = 0o600
2222

2323
type Parameters struct {
2424
NginxConfigPath string

0 commit comments

Comments
 (0)
0