From 294dfff4435c31b7c8d20d26067365b0a0016610 Mon Sep 17 00:00:00 2001 From: Dean Ruina <81315494+DeRuina@users.noreply.github.com> Date: Mon, 23 Feb 2026 09:27:27 +0200 Subject: [PATCH] logging: add DirMode options and propagate FileMode to rotations (#7335) Co-authored-by: Francis Lavoie --- .../log_roll_days.caddyfiletest | 54 ++++- modules/logging/filewriter.go | 161 +++++++++++-- modules/logging/filewriter_test.go | 222 ++++++++++++++++++ modules/logging/filewriter_test_windows.go | 38 +++ 4 files changed, 450 insertions(+), 25 deletions(-) diff --git a/caddytest/integration/caddyfile_adapt/log_roll_days.caddyfiletest b/caddytest/integration/caddyfile_adapt/log_roll_days.caddyfiletest index 3ead4ac18..3e2aa0d81 100644 --- a/caddytest/integration/caddyfile_adapt/log_roll_days.caddyfiletest +++ b/caddytest/integration/caddyfile_adapt/log_roll_days.caddyfiletest @@ -1,7 +1,9 @@ :80 -log { +log one { output file /var/log/access.log { + mode 0644 + dir_mode 0755 roll_size 1gb roll_uncompressed roll_local_time @@ -9,18 +11,33 @@ log { roll_keep_for 90d } } +log two { + output file /var/log/access-2.log { + mode 0777 + dir_mode from_file + roll_size 1gib + roll_interval 12h + roll_at 00:00 06:00 12:00,18:00 + roll_minutes 10 40 45,46 + roll_keep 10 + roll_keep_for 90d + } +} ---------- { "logging": { "logs": { "default": { "exclude": [ - "http.log.access.log0" + "http.log.access.one", + "http.log.access.two" ] }, - "log0": { + "one": { "writer": { + "dir_mode": "0755", "filename": "/var/log/access.log", + "mode": "0644", "output": "file", "roll_gzip": false, "roll_keep": 5, @@ -29,7 +46,34 @@ log { "roll_size_mb": 954 }, "include": [ - "http.log.access.log0" + "http.log.access.one" + ] + }, + "two": { + "writer": { + "dir_mode": "from_file", + "filename": "/var/log/access-2.log", + "mode": "0777", + "output": "file", + "roll_at": [ + "00:00", + "06:00", + "12:00", + "18:00" + ], + "roll_interval": 43200000000000, + "roll_keep": 10, + "roll_keep_days": 90, + "roll_minutes": [ + 10, + 40, + 45, + 46 + ], + "roll_size_mb": 1024 + }, + "include": [ + "http.log.access.two" ] } } @@ -42,7 +86,7 @@ log { ":80" ], "logs": { - "default_logger_name": "log0" + "default_logger_name": "two" } } } diff --git a/modules/logging/filewriter.go b/modules/logging/filewriter.go index c3df562cb..e9ca4013a 100644 --- a/modules/logging/filewriter.go +++ b/modules/logging/filewriter.go @@ -90,6 +90,15 @@ type FileWriter struct { // 0600 by default. Mode fileMode `json:"mode,omitempty"` + // DirMode controls permissions for any directories created to reach Filename. + // Default: 0700 (current behavior). + // + // Special values: + // - "inherit" → copy the nearest existing parent directory's perms (with r→x normalization) + // - "from_file" → derive from the file Mode (with r→x), e.g. 0644 → 0755, 0600 → 0700 + // Numeric octal strings (e.g. "0755") are also accepted. Subject to process umask. + DirMode string `json:"dir_mode,omitempty"` + // Roll toggles log rolling or rotation, which is // enabled by default. Roll *bool `json:"roll,omitempty"` @@ -177,11 +186,33 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { // roll log files as a sensible default to avoid disk space exhaustion roll := fw.Roll == nil || *fw.Roll - // create the file if it does not exist; create with the configured mode, or default - // to restrictive if not set. (timberjack will reuse the file mode across log rotation) - if err := os.MkdirAll(filepath.Dir(fw.Filename), 0o700); err != nil { - return nil, err + // Ensure directory exists before opening the file. + dirPath := filepath.Dir(fw.Filename) + switch strings.ToLower(strings.TrimSpace(fw.DirMode)) { + case "", "0": + // Preserve current behavior: locked-down directories by default. + if err := os.MkdirAll(dirPath, 0o700); err != nil { + return nil, err + } + case "inherit": + if err := mkdirAllInherit(dirPath); err != nil { + return nil, err + } + case "from_file": + if err := mkdirAllFromFile(dirPath, os.FileMode(fw.Mode)); err != nil { + return nil, err + } + default: + dm, err := parseFileMode(fw.DirMode) + if err != nil { + return nil, fmt.Errorf("dir_mode: %w", err) + } + if err := os.MkdirAll(dirPath, dm); err != nil { + return nil, err + } } + + // create/open the file file, err := os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, modeIfCreating) if err != nil { return nil, err @@ -234,13 +265,70 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { RotateAtMinutes: fw.RollAtMinutes, RotateAt: fw.RollAt, BackupTimeFormat: fw.BackupTimeFormat, + FileMode: os.FileMode(fw.Mode), }, nil } +// normalizeDirPerm ensures that read bits also have execute bits set. +func normalizeDirPerm(p os.FileMode) os.FileMode { + if p&0o400 != 0 { + p |= 0o100 + } + if p&0o040 != 0 { + p |= 0o010 + } + if p&0o004 != 0 { + p |= 0o001 + } + return p +} + +// mkdirAllInherit creates missing dirs using the nearest existing parent's +// permissions, normalized with r→x. +func mkdirAllInherit(dir string) error { + if fi, err := os.Stat(dir); err == nil && fi.IsDir() { + return nil + } + cur := dir + var parent string + for { + next := filepath.Dir(cur) + if next == cur { + parent = next + break + } + if fi, err := os.Stat(next); err == nil { + if !fi.IsDir() { + return fmt.Errorf("path component %s exists and is not a directory", next) + } + parent = next + break + } + cur = next + } + perm := os.FileMode(0o700) + if fi, err := os.Stat(parent); err == nil && fi.IsDir() { + perm = fi.Mode().Perm() + } + perm = normalizeDirPerm(perm) + return os.MkdirAll(dir, perm) +} + +// mkdirAllFromFile creates missing dirs using the file's mode (with r→x) so +// 0644 → 0755, 0600 → 0700, etc. +func mkdirAllFromFile(dir string, fileMode os.FileMode) error { + if fi, err := os.Stat(dir); err == nil && fi.IsDir() { + return nil + } + perm := normalizeDirPerm(fileMode.Perm()) | 0o200 // ensure owner write on dir so files can be created + return os.MkdirAll(dir, perm) +} + // UnmarshalCaddyfile sets up the module from Caddyfile tokens. Syntax: // // file { // mode +// dir_mode // roll_disabled // roll_size // roll_uncompressed @@ -284,6 +372,22 @@ func (fw *FileWriter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { } fw.Mode = fileMode(mode) + case "dir_mode": + var val string + if !d.AllArgs(&val) { + return d.ArgErr() + } + val = strings.TrimSpace(val) + switch strings.ToLower(val) { + case "inherit", "from_file": + fw.DirMode = val + default: + if _, err := parseFileMode(val); err != nil { + return d.Errf("parsing dir_mode: %v", err) + } + fw.DirMode = val + } + case "roll_disabled": var f bool fw.Roll = &f @@ -352,31 +456,48 @@ func (fw *FileWriter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { fw.RollInterval = duration case "roll_minutes": - var minutesArrayStr string - if !d.AllArgs(&minutesArrayStr) { + // Accept either a single comma-separated argument or + // multiple space-separated arguments. Collect all + // remaining args on the line and split on commas. + args := d.RemainingArgs() + if len(args) == 0 { return d.ArgErr() } - minutesStr := strings.Split(minutesArrayStr, ",") - minutes := make([]int, len(minutesStr)) - for i := range minutesStr { - ms := strings.Trim(minutesStr[i], " ") - m, err := strconv.Atoi(ms) - if err != nil { - return d.Errf("parsing roll_minutes number: %v", err) + var minutes []int + for _, arg := range args { + parts := strings.SplitSeq(arg, ",") + for p := range parts { + ms := strings.TrimSpace(p) + if ms == "" { + return d.Errf("parsing roll_minutes: empty value") + } + m, err := strconv.Atoi(ms) + if err != nil { + return d.Errf("parsing roll_minutes number: %v", err) + } + minutes = append(minutes, m) } - minutes[i] = m } fw.RollAtMinutes = minutes case "roll_at": - var timeArrayStr string - if !d.AllArgs(&timeArrayStr) { + // Accept either a single comma-separated argument or + // multiple space-separated arguments. Collect all + // remaining args on the line and split on commas. + args := d.RemainingArgs() + if len(args) == 0 { return d.ArgErr() } - timeStr := strings.Split(timeArrayStr, ",") - times := make([]string, len(timeStr)) - for i := range timeStr { - times[i] = strings.Trim(timeStr[i], " ") + var times []string + for _, arg := range args { + parts := strings.SplitSeq(arg, ",") + for p := range parts { + ts := strings.TrimSpace(p) + if ts == "" { + return d.Errf("parsing roll_at: empty value") + } + times = append(times, ts) + } } fw.RollAt = times diff --git a/modules/logging/filewriter_test.go b/modules/logging/filewriter_test.go index 2a246156c..915784b53 100644 --- a/modules/logging/filewriter_test.go +++ b/modules/logging/filewriter_test.go @@ -385,3 +385,225 @@ func TestFileModeModification(t *testing.T) { t.Errorf("file mode is %v, want %v", st.Mode(), want) } } + +func TestDirMode_Inherit(t *testing.T) { + m := syscall.Umask(0) + defer syscall.Umask(m) + + parent := t.TempDir() + if err := os.Chmod(parent, 0o755); err != nil { + t.Fatal(err) + } + + targetDir := filepath.Join(parent, "a", "b") + fw := &FileWriter{ + Filename: filepath.Join(targetDir, "test.log"), + DirMode: "inherit", + Mode: 0o640, + Roll: func() *bool { f := false; return &f }(), + } + w, err := fw.OpenWriter() + if err != nil { + t.Fatal(err) + } + _ = w.Close() + + st, err := os.Stat(targetDir) + if err != nil { + t.Fatal(err) + } + if got := st.Mode().Perm(); got != 0o755 { + t.Fatalf("dir perm = %o, want 0755", got) + } +} + +func TestDirMode_FromFile(t *testing.T) { + m := syscall.Umask(0) + defer syscall.Umask(m) + + base := t.TempDir() + + dir1 := filepath.Join(base, "logs1") + fw1 := &FileWriter{ + Filename: filepath.Join(dir1, "app.log"), + DirMode: "from_file", + Mode: 0o644, // => dir 0755 + Roll: func() *bool { f := false; return &f }(), + } + w1, err := fw1.OpenWriter() + if err != nil { + t.Fatal(err) + } + _ = w1.Close() + + st1, err := os.Stat(dir1) + if err != nil { + t.Fatal(err) + } + if got := st1.Mode().Perm(); got != 0o755 { + t.Fatalf("dir perm = %o, want 0755", got) + } + + dir2 := filepath.Join(base, "logs2") + fw2 := &FileWriter{ + Filename: filepath.Join(dir2, "app.log"), + DirMode: "from_file", + Mode: 0o600, // => dir 0700 + Roll: func() *bool { f := false; return &f }(), + } + w2, err := fw2.OpenWriter() + if err != nil { + t.Fatal(err) + } + _ = w2.Close() + + st2, err := os.Stat(dir2) + if err != nil { + t.Fatal(err) + } + if got := st2.Mode().Perm(); got != 0o700 { + t.Fatalf("dir perm = %o, want 0700", got) + } +} + +func TestDirMode_ExplicitOctal(t *testing.T) { + m := syscall.Umask(0) + defer syscall.Umask(m) + + base := t.TempDir() + dest := filepath.Join(base, "logs3") + fw := &FileWriter{ + Filename: filepath.Join(dest, "app.log"), + DirMode: "0750", + Mode: 0o640, + Roll: func() *bool { f := false; return &f }(), + } + w, err := fw.OpenWriter() + if err != nil { + t.Fatal(err) + } + _ = w.Close() + + st, err := os.Stat(dest) + if err != nil { + t.Fatal(err) + } + if got := st.Mode().Perm(); got != 0o750 { + t.Fatalf("dir perm = %o, want 0750", got) + } +} + +func TestDirMode_Default0700(t *testing.T) { + m := syscall.Umask(0) + defer syscall.Umask(m) + + base := t.TempDir() + dest := filepath.Join(base, "logs4") + fw := &FileWriter{ + Filename: filepath.Join(dest, "app.log"), + Mode: 0o640, + Roll: func() *bool { f := false; return &f }(), + } + w, err := fw.OpenWriter() + if err != nil { + t.Fatal(err) + } + _ = w.Close() + + st, err := os.Stat(dest) + if err != nil { + t.Fatal(err) + } + if got := st.Mode().Perm(); got != 0o700 { + t.Fatalf("dir perm = %o, want 0700", got) + } +} + +func TestDirMode_UmaskInteraction(t *testing.T) { + _ = syscall.Umask(0o022) // typical umask; restore after + defer syscall.Umask(0) + + base := t.TempDir() + dest := filepath.Join(base, "logs5") + fw := &FileWriter{ + Filename: filepath.Join(dest, "app.log"), + DirMode: "0755", + Mode: 0o644, + Roll: func() *bool { f := false; return &f }(), + } + w, err := fw.OpenWriter() + if err != nil { + t.Fatal(err) + } + _ = w.Close() + + st, err := os.Stat(dest) + if err != nil { + t.Fatal(err) + } + // 0755 &^ 0022 still 0755 for dirs; this just sanity-checks we didn't get stricter unexpectedly + if got := st.Mode().Perm(); got != 0o755 { + t.Fatalf("dir perm = %o, want 0755 (considering umask)", got) + } +} + +func TestCaddyfile_DirMode_Inherit(t *testing.T) { + d := caddyfile.NewTestDispenser(` +file /var/log/app.log { + dir_mode inherit + mode 0640 +}`) + var fw FileWriter + if err := fw.UnmarshalCaddyfile(d); err != nil { + t.Fatal(err) + } + if fw.DirMode != "inherit" { + t.Fatalf("got %q", fw.DirMode) + } + if fw.Mode != 0o640 { + t.Fatalf("mode = %o", fw.Mode) + } +} + +func TestCaddyfile_DirMode_FromFile(t *testing.T) { + d := caddyfile.NewTestDispenser(` +file /var/log/app.log { + dir_mode from_file + mode 0600 +}`) + var fw FileWriter + if err := fw.UnmarshalCaddyfile(d); err != nil { + t.Fatal(err) + } + if fw.DirMode != "from_file" { + t.Fatalf("got %q", fw.DirMode) + } + if fw.Mode != 0o600 { + t.Fatalf("mode = %o", fw.Mode) + } +} + +func TestCaddyfile_DirMode_Octal(t *testing.T) { + d := caddyfile.NewTestDispenser(` +file /var/log/app.log { + dir_mode 0755 +}`) + var fw FileWriter + if err := fw.UnmarshalCaddyfile(d); err != nil { + t.Fatal(err) + } + if fw.DirMode != "0755" { + t.Fatalf("got %q", fw.DirMode) + } +} + +func TestCaddyfile_DirMode_Invalid(t *testing.T) { + d := caddyfile.NewTestDispenser(` +file /var/log/app.log { + dir_mode nope +}`) + var fw FileWriter + if err := fw.UnmarshalCaddyfile(d); err == nil { + t.Fatal("expected error for invalid dir_mode") + } +} diff --git a/modules/logging/filewriter_test_windows.go b/modules/logging/filewriter_test_windows.go index a032d1c28..254d5c30e 100644 --- a/modules/logging/filewriter_test_windows.go +++ b/modules/logging/filewriter_test_windows.go @@ -53,3 +53,41 @@ func TestFileCreationMode(t *testing.T) { t.Fatalf("file mode is %v, want rw for user", st.Mode().Perm()) } } + +func TestDirMode_Windows_CreateSucceeds(t *testing.T) { + dir, err := os.MkdirTemp("", "caddytest") + if err != nil { + t.Fatalf("failed to create tempdir: %v", err) + } + defer os.RemoveAll(dir) + + tests := []struct { + name string + dirMode string + }{ + {"inherit", "inherit"}, + {"from_file", "from_file"}, + {"octal", "0755"}, + {"default", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + subdir := path.Join(dir, "logs-"+tt.name) + fw := &FileWriter{ + Filename: path.Join(subdir, "test.log"), + DirMode: tt.dirMode, + Mode: 0o600, + } + w, err := fw.OpenWriter() + if err != nil { + t.Fatalf("failed to open writer: %v", err) + } + defer w.Close() + + if _, err := os.Stat(fw.Filename); err != nil { + t.Fatalf("expected file to exist: %v", err) + } + }) + } +}