From b730feffa260b02357fe8d813a8f649821b3b606 Mon Sep 17 00:00:00 2001 From: Liza Tsibur Date: Tue, 9 Dec 2025 13:21:22 -0700 Subject: [PATCH] Revert "Disable permission flag checks on imported logs (#82)" This reverts commit fd1bbe326310641720970815af0d0a41313390cd. --- cmd/import.go | 15 ++++++-------- cmd/import_test.go | 50 +++++++++++++++++++--------------------------- 2 files changed, 26 insertions(+), 39 deletions(-) diff --git a/cmd/import.go b/cmd/import.go index 33cbf8e..4299ddc 100644 --- a/cmd/import.go +++ b/cmd/import.go @@ -161,19 +161,15 @@ func RunImportCmd(startTime time.Time, cfg *config.Config, afs afero.Fs, logDir // get list of hourly log maps of all days of log files in directory logMap, walkErrors, err := WalkFiles(afs, logDir, db.Rolling) + if err != nil { + return importResults, err + } - // log any errors that occurred during the walk, before returning - // this is especially useful when all files in the directory are invalid - // instead of only logging 'no valid files found' + // log any errors that occurred during the walk for _, walkErr := range walkErrors { logger.Debug().Str("path", walkErr.Path).Err(walkErr.Error).Msg("file was left out of import due to error or incompatibility") } - // return if the walk failed completely - if err != nil { - return importResults, err - } - var elapsedTime int64 // loop through each day @@ -429,7 +425,8 @@ func WalkFiles(afs afero.Fs, root string, rolling bool) ([]HourlyZeekLogs, []Wal } // check if the file is readable - if _, err := afs.Open(path); err != nil { + _, err := afs.Open(path) + if err != nil || !(info.Mode().Perm()&0444 == 0444) { walkErrors = append(walkErrors, WalkError{Path: path, Error: ErrInsufficientReadPermissions}) return nil //nolint:nilerr // log the issue and continue walking } diff --git a/cmd/import_test.go b/cmd/import_test.go index 09ff067..0ef1c65 100644 --- a/cmd/import_test.go +++ b/cmd/import_test.go @@ -622,6 +622,7 @@ func createExpectedResults(logs []cmd.HourlyZeekLogs) []cmd.HourlyZeekLogs { } func TestWalkFiles(t *testing.T) { + afs := afero.NewMemMapFs() tests := []struct { name string @@ -1158,33 +1159,25 @@ func TestWalkFiles(t *testing.T) { }, expectedError: cmd.ErrNoValidFilesFound, }, - - // Previously, read permissions were checked with !(info.Mode().Perm()&0444 == 0444), but - // this requires all read permissions (user, group, others)/0644 to be set which is not ideal. - // A better check would be to see if any read permission is set, i.e., (info.Mode().Perm()&0444 != 0). - // However, since some ACL systems/SELinux might interfere with this, it's better to let the Open() call - // return an error if permission is denied. - // Unfortunately, afero.MemMapFs does not support file permissions when using Open, so this test is skipped. - // https://github.com/spf13/afero/issues/150 - // { - // name: "No Read Permissions on Files", - // directory: "/logs", - // directoryPermissions: iofs.FileMode(0o775), - // filePermissions: iofs.FileMode(0o000), - // files: []string{ - // "conn.log", "dns.log", "http.log", "ssl.log", "open_conn.log", "open_http.log", "open_ssl.log", - // }, - // expectedWalkErrors: []cmd.WalkError{ - // {Path: "/logs/conn.log", Error: cmd.ErrInsufficientReadPermissions}, - // {Path: "/logs/dns.log", Error: cmd.ErrInsufficientReadPermissions}, - // {Path: "/logs/http.log", Error: cmd.ErrInsufficientReadPermissions}, - // {Path: "/logs/ssl.log", Error: cmd.ErrInsufficientReadPermissions}, - // {Path: "/logs/open_conn.log", Error: cmd.ErrInsufficientReadPermissions}, - // {Path: "/logs/open_http.log", Error: cmd.ErrInsufficientReadPermissions}, - // {Path: "/logs/open_ssl.log", Error: cmd.ErrInsufficientReadPermissions}, - // }, - // expectedError: cmd.ErrNoValidFilesFound, - // }, + { + name: "No Read Permissions on Files", + directory: "/logs", + directoryPermissions: iofs.FileMode(0o775), + filePermissions: iofs.FileMode(0o000), + files: []string{ + "conn.log", "dns.log", "http.log", "ssl.log", "open_conn.log", "open_http.log", "open_ssl.log", + }, + expectedWalkErrors: []cmd.WalkError{ + {Path: "/logs/conn.log", Error: cmd.ErrInsufficientReadPermissions}, + {Path: "/logs/dns.log", Error: cmd.ErrInsufficientReadPermissions}, + {Path: "/logs/http.log", Error: cmd.ErrInsufficientReadPermissions}, + {Path: "/logs/ssl.log", Error: cmd.ErrInsufficientReadPermissions}, + {Path: "/logs/open_conn.log", Error: cmd.ErrInsufficientReadPermissions}, + {Path: "/logs/open_http.log", Error: cmd.ErrInsufficientReadPermissions}, + {Path: "/logs/open_ssl.log", Error: cmd.ErrInsufficientReadPermissions}, + }, + expectedError: cmd.ErrNoValidFilesFound, + }, { name: "No Files, Only SubDirectories", directory: "/logs", @@ -1224,8 +1217,6 @@ func TestWalkFiles(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - // create a new in-memory filesystem for each test - afs := afero.NewMemMapFs() // Create the directory if test.directory != "" { @@ -1307,7 +1298,6 @@ func TestWalkFiles(t *testing.T) { if test.expectedError == nil { require.NoError(t, err, "running WalkFiles should not produce an error") } else { - require.Error(t, err, "running WalkFiles should produce an error") require.ErrorIs(t, err, test.expectedError, "error should match expected value") }