logging: add DirMode options and propagate FileMode to rotations (#7335)

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

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"
 					}
 				}
 			}

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 <filename> {
 //	    mode          <mode>
+//	    dir_mode      <mode|inherit|from_file>
 //	    roll_disabled
 //	    roll_size     <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
 

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")
+	}
+}

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)
+			}
+		})
+	}
+}