Repository: caddyserver/caddy (language: go)

Issue:
Caddy log file permissions
Caddy uses https://github.com/natefinch/lumberjack and some time ago they switched the log file permissions from 0600 to 0644: https://github.com/natefinch/lumberjack/pull/83/files

Now, some users (me included) have issues due to these too-restrictive file permissions: https://caddy.community/t/change-file-mask-for-caddy-log-files/22519

There is an issue in the upstream library, but it currently doesn't look like it will be fixed (although to be honest, the proposed PRs don't look very good to me) https://github.com/natefinch/lumberjack/issues/164

However, what lumberjack does is, it will take the permissions of the file if it exists. Now, in Caddy's filewriter.go we already have

```go
// OpenWriter opens a new file writer.
func (fw FileWriter) OpenWriter() (io.WriteCloser, error) {
	// roll log files by default
	if fw.Roll == nil || *fw.Roll {
		if fw.RollSizeMB == 0 {
			fw.RollSizeMB = 100
		}
		if fw.RollCompress == nil {
			compress := true
			fw.RollCompress = &compress
		}
		if fw.RollKeep == 0 {
			fw.RollKeep = 10
		}
		if fw.RollKeepDays == 0 {
			fw.RollKeepDays = 90
		}

		return &lumberjack.Logger{
			Filename:   fw.Filename,
			MaxSize:    fw.RollSizeMB,
			MaxAge:     fw.RollKeepDays,
			MaxBackups: fw.RollKeep,
			LocalTime:  fw.RollLocalTime,
			Compress:   *fw.RollCompress,
		}, nil
	}

	// otherwise just open a regular file
	return os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666)
}
```

So I'm proposing to change this so we always create the file (even if no rolling is configured), so that lumberjack will pick up these permissions. This would also help to prevent the issue that log files are created with mode 0666, but when users configure log rotation, the log files suddenly get created with mode 0600.

What do you think?


Discussion / hints:
I think someone should fork Lumberjack and rewrite it in such a way that it works better for Caddy :sweat_smile: see the discussion in https://github.com/natefinch/lumberjack/discussions/160 We would do it ourselves if we had time, but it's too big an ask to maintain for us in addition to the rest of Caddy at the moment.
Hello,

My first post here.
I have just switched from Apache to Caddy and would like to congratulate every one for the amazing job.

I am reaching to this issue with the following user story:

I use [netdata](https://www.netdata.cloud) to monitor my infrastructure. I have activated caddy's metrics for prometheus monitoring. I would like per host metrics. I understand this is being worked on through #6279 and related issues. But I also would like to run other monitoring tools on the access logs: fail2ban and netdata web_log analysis plugin. Those tools need read permissions to the access logs.

I understand that there is no bandwidth to maintain [natefinch/lumberjack](https://github.com/natefinch/lumberjack).
@pascalgn suggested a work around which is to create the file with the right permissions so that lumberjack reuse it.

What do you think about this workaround ? Would you be open to implement it ? It looks like a one-liner maybe ?
@ririsoft Welcome to the project!

I don't think we know the filename Lumberjack will use, unfortunately:

https://github.com/natefinch/lumberjack/blob/305d3e2979e73ec269fa12dab3d82e2f829c1c84/lumberjack.go#L331-L338

Maybe I am misreading (it's very early here)
I do not get your concern @mholt , sorry.
The filename is known : see https://github.com/caddyserver/caddy/blob/master/modules/logging/filewriter.go#L102.

As [@pascalgn mentioned above](https://github.com/caddyserver/caddy/issues/6295#issue-2277258251) Lumberjack reuses the same permissions if the file already exists: https://github.com/natefinch/lumberjack/blob/305d3e2979e73ec269fa12dab3d82e2f829c1c84/lumberjack.go#L304

In such case we could have Caddy create the log file first with the right permissions before calling Lumberjack.
Something like:

```go
// OpenWriter opens a new file writer.
func (fw FileWriter) OpenWriter() (io.WriteCloser, error) {
	// roll log files by default
	if fw.Roll == nil || *fw.Roll {
		if fw.RollSizeMB == 0 {
			fw.RollSizeMB = 100
		}
		if fw.RollCompress == nil {
			compress := true
			fw.RollCompress = &compress
		}
		if fw.RollKeep == 0 {
			fw.RollKeep = 10
		}
		if fw.RollKeepDays == 0 {
			fw.RollKeepDays = 90
		}

               // create the log file with read permissions at group level
               f_tmp := os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0640)
               f_tmp.Close()

		return &lumberjack.Logger{
			Filename:   fw.Filename,
			MaxSize:    fw.RollSizeMB,
			MaxAge:     fw.RollKeepDays,
			MaxBackups: fw.RollKeep,
			LocalTime:  fw.RollLocalTime,
			Compress:   *fw.RollCompress,
		}, nil
	}

	// otherwise just open a regular file
	return os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666)
}
```

Please excuse my Go, I am a rust developer and it has been a long time since I have practiced the language ;-) 

That would be a first step and easy workaround, allowing administrators to add tools to the "caddy" group if they want to grant log read access. And of course this need to be tested, because we are just assuming this should work reading Lumberjack code.

This would allow most users to drop logrotate with "copytruncate" option which has a race condition that may lose some few logs.

A second step would be to make the permissions (and even group) configurable, but that is definitely more work.

What do you think ?

@ririsoft Could you test to make sure that works (including through rotation)? If so we could accept a patch along those lines I think.
Sure, I will try to test that when time permit
Ok so I just tested it (it was so easy to onboard with caddy development I could not resist) and it works as expected. I will submit a PR soon.
Awesome! Thanks

Fix patch:
diff --git a/modules/logging/filewriter.go b/modules/logging/filewriter.go
index 3b1001b7c2b..393228fda44 100644
--- a/modules/logging/filewriter.go
+++ b/modules/logging/filewriter.go
@@ -15,6 +15,7 @@
 package logging
 
 import (
+	"encoding/json"
 	"fmt"
 	"io"
 	"math"
@@ -33,6 +34,43 @@ func init() {
 	caddy.RegisterModule(FileWriter{})
 }
 
+// fileMode is a string made of 1 to 4 octal digits representing
+// a numeric mode as specified with the `chmod` unix command.
+// `"0777"` and `"777"` are thus equivalent values.
+type fileMode os.FileMode
+
+// UnmarshalJSON satisfies json.Unmarshaler.
+func (m *fileMode) UnmarshalJSON(b []byte) error {
+	if len(b) == 0 {
+		return io.EOF
+	}
+
+	var s string
+	if err := json.Unmarshal(b, &s); err != nil {
+		return err
+	}
+
+	mode, err := parseFileMode(s)
+	if err != nil {
+		return err
+	}
+
+	*m = fileMode(mode)
+	return err
+}
+
+// parseFileMode parses a file mode string,
+// adding support for `chmod` unix command like
+// 1 to 4 digital octal values.
+func parseFileMode(s string) (os.FileMode, error) {
+	modeStr := fmt.Sprintf("%04s", s)
+	mode, err := strconv.ParseUint(modeStr, 8, 32)
+	if err != nil {
+		return 0, err
+	}
+	return os.FileMode(mode), nil
+}
+
 // FileWriter can write logs to files. By default, log files
 // are rotated ("rolled") when they get large, and old log
 // files get deleted, to ensure that the process does not
@@ -41,6 +79,10 @@ type FileWriter struct {
 	// Filename is the name of the file to write.
 	Filename string `json:"filename,omitempty"`
 
+	// The file permissions mode.
+	// 0600 by default.
+	Mode fileMode `json:"mode,omitempty"`
+
 	// Roll toggles log rolling or rotation, which is
 	// enabled by default.
 	Roll *bool `json:"roll,omitempty"`
@@ -100,6 +142,10 @@ func (fw FileWriter) WriterKey() string {
 
 // OpenWriter opens a new file writer.
 func (fw FileWriter) OpenWriter() (io.WriteCloser, error) {
+	if fw.Mode == 0 {
+		fw.Mode = 0o600
+	}
+
 	// roll log files by default
 	if fw.Roll == nil || *fw.Roll {
 		if fw.RollSizeMB == 0 {
@@ -116,6 +162,9 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) {
 			fw.RollKeepDays = 90
 		}
 
+		f_tmp, _ := os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, os.FileMode(fw.Mode))
+		f_tmp.Close()
+
 		return &lumberjack.Logger{
 			Filename:   fw.Filename,
 			MaxSize:    fw.RollSizeMB,
@@ -127,12 +176,13 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) {
 	}
 
 	// otherwise just open a regular file
-	return os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666)
+	return os.OpenFile(fw.Filename, os.O_WRONLY|os.O_APPEND|os.O_CREATE, os.FileMode(fw.Mode))
 }
 
 // UnmarshalCaddyfile sets up the module from Caddyfile tokens. Syntax:
 //
 //	file <filename> {
+//	    mode          <mode>
 //	    roll_disabled
 //	    roll_size     <size>
 //	    roll_uncompressed
@@ -150,7 +200,7 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) {
 // The roll_keep_for duration has day resolution.
 // Fractional values are rounded up to the next whole number of days.
 //
-// If any of the roll_size, roll_keep, or roll_keep_for subdirectives are
+// If any of the mode, roll_size, roll_keep, or roll_keep_for subdirectives are
 // omitted or set to a zero value, then Caddy's default value for that
 // subdirective is used.
 func (fw *FileWriter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
@@ -165,6 +215,17 @@ func (fw *FileWriter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 
 	for d.NextBlock(0) {
 		switch d.Val() {
+		case "mode":
+			var modeStr string
+			if !d.AllArgs(&modeStr) {
+				return d.ArgErr()
+			}
+			mode, err := parseFileMode(modeStr)
+			if err != nil {
+				return d.Errf("parsing mode: %v", err)
+			}
+			fw.Mode = fileMode(mode)
+
 		case "roll_disabled":
 			var f bool
 			fw.Roll = &f


Test patch:
diff --git a/modules/logging/filewriter_test.go b/modules/logging/filewriter_test.go
new file mode 100644
index 00000000000..2787eeff197
--- /dev/null
+++ b/modules/logging/filewriter_test.go
@@ -0,0 +1,308 @@
+// Copyright 2015 Matthew Holt and The Caddy Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build !windows
+
+package logging
+
+import (
+	"encoding/json"
+	"os"
+	"path"
+	"syscall"
+	"testing"
+
+	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
+)
+
+func TestFileCreationMode(t *testing.T) {
+	on := true
+	off := false
+
+	tests := []struct {
+		name     string
+		fw       FileWriter
+		wantMode os.FileMode
+	}{
+		{
+			name: "default mode no roll",
+			fw: FileWriter{
+				Roll: &off,
+			},
+			wantMode: 0o600,
+		},
+		{
+			name: "default mode roll",
+			fw: FileWriter{
+				Roll: &on,
+			},
+			wantMode: 0o600,
+		},
+		{
+			name: "custom mode no roll",
+			fw: FileWriter{
+				Roll: &off,
+				Mode: 0o666,
+			},
+			wantMode: 0o666,
+		},
+		{
+			name: "custom mode roll",
+			fw: FileWriter{
+				Roll: &on,
+				Mode: 0o666,
+			},
+			wantMode: 0o666,
+		},
+	}
+
+	m := syscall.Umask(0o000)
+	defer syscall.Umask(m)
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir, err := os.MkdirTemp("", "caddytest")
+			if err != nil {
+				t.Fatalf("failed to create tempdir: %v", err)
+			}
+			defer os.RemoveAll(dir)
+			fpath := path.Join(dir, "test.log")
+			tt.fw.Filename = fpath
+
+			logger, err := tt.fw.OpenWriter()
+			if err != nil {
+				t.Fatalf("failed to create file: %v", err)
+			}
+			defer logger.Close()
+
+			st, err := os.Stat(fpath)
+			if err != nil {
+				t.Fatalf("failed to check file permissions: %v", err)
+			}
+
+			if st.Mode() != tt.wantMode {
+				t.Errorf("file mode is %v, want %v", st.Mode(), tt.wantMode)
+			}
+		})
+	}
+}
+
+func TestFileRotationPreserveMode(t *testing.T) {
+	m := syscall.Umask(0o000)
+	defer syscall.Umask(m)
+
+	dir, err := os.MkdirTemp("", "caddytest")
+	if err != nil {
+		t.Fatalf("failed to create tempdir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+
+	fpath := path.Join(dir, "test.log")
+
+	roll := true
+	mode := fileMode(0o640)
+	fw := FileWriter{
+		Filename:   fpath,
+		Mode:       mode,
+		Roll:       &roll,
+		RollSizeMB: 1,
+	}
+
+	logger, err := fw.OpenWriter()
+	if err != nil {
+		t.Fatalf("failed to create file: %v", err)
+	}
+	defer logger.Close()
+
+	b := make([]byte, 1024*1024-1000)
+	logger.Write(b)
+	logger.Write(b[0:2000])
+
+	files, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatalf("failed to read temporary log dir: %v", err)
+	}
+
+	// We might get 2 or 3 files depending
+	// on the race between compressed log file generation,
+	// removal of the non compressed file and reading the directory.
+	// Ordering of the files are [ test-*.log test-*.log.gz test.log ]
+	if len(files) < 2 || len(files) > 3 {
+		t.Log("got files: ", files)
+		t.Fatalf("got %v files want 2", len(files))
+	}
+
+	wantPattern := "test-*-*-*-*-*.*.log"
+	test_date_log := files[0]
+	if m, _ := path.Match(wantPattern, test_date_log.Name()); m != true {
+		t.Fatalf("got %v filename want %v", test_date_log.Name(), wantPattern)
+	}
+
+	st, err := os.Stat(path.Join(dir, test_date_log.Name()))
+	if err != nil {
+		t.Fatalf("failed to check file permissions: %v", err)
+	}
+
+	if st.Mode() != os.FileMode(mode) {
+		t.Errorf("file mode is %v, want %v", st.Mode(), mode)
+	}
+
+	test_dot_log := files[len(files)-1]
+	if test_dot_log.Name() != "test.log" {
+		t.Fatalf("got %v filename want test.log", test_dot_log.Name())
+	}
+
+	st, err = os.Stat(path.Join(dir, test_dot_log.Name()))
+	if err != nil {
+		t.Fatalf("failed to check file permissions: %v", err)
+	}
+
+	if st.Mode() != os.FileMode(mode) {
+		t.Errorf("file mode is %v, want %v", st.Mode(), mode)
+	}
+}
+
+func TestFileModeConfig(t *testing.T) {
+	tests := []struct {
+		name    string
+		d       *caddyfile.Dispenser
+		fw      FileWriter
+		wantErr bool
+	}{
+		{
+			name: "set mode",
+			d: caddyfile.NewTestDispenser(`
+file test.log {
+	mode 0666
+}
+`),
+			fw: FileWriter{
+				Mode: 0o666,
+			},
+			wantErr: false,
+		},
+		{
+			name: "set mode 3 digits",
+			d: caddyfile.NewTestDispenser(`
+file test.log {
+	mode 666
+}
+`),
+			fw: FileWriter{
+				Mode: 0o666,
+			},
+			wantErr: false,
+		},
+		{
+			name: "set mode 2 digits",
+			d: caddyfile.NewTestDispenser(`
+file test.log {
+	mode 66
+}
+`),
+			fw: FileWriter{
+				Mode: 0o066,
+			},
+			wantErr: false,
+		},
+		{
+			name: "set mode 1 digits",
+			d: caddyfile.NewTestDispenser(`
+file test.log {
+	mode 6
+}
+`),
+			fw: FileWriter{
+				Mode: 0o006,
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid mode",
+			d: caddyfile.NewTestDispenser(`
+file test.log {
+	mode foobar
+}
+`),
+			fw:      FileWriter{},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fw := &FileWriter{}
+			if err := fw.UnmarshalCaddyfile(tt.d); (err != nil) != tt.wantErr {
+				t.Fatalf("UnmarshalCaddyfile() error = %v, want %v", err, tt.wantErr)
+			}
+			if fw.Mode != tt.fw.Mode {
+				t.Errorf("got mode %v, want %v", fw.Mode, tt.fw.Mode)
+			}
+		})
+	}
+}
+
+func TestFileModeJSON(t *testing.T) {
+	tests := []struct {
+		name    string
+		config  string
+		fw      FileWriter
+		wantErr bool
+	}{
+		{
+			name: "set mode",
+			config: `
+{
+	"mode": "0666"
+}
+`,
+			fw: FileWriter{
+				Mode: 0o666,
+			},
+			wantErr: false,
+		},
+		{
+			name: "set mode invalid value",
+			config: `
+{
+	"mode": "0x666"
+}
+`,
+			fw:      FileWriter{},
+			wantErr: true,
+		},
+		{
+			name: "set mode invalid string",
+			config: `
+{
+	"mode": 777
+}
+`,
+			fw:      FileWriter{},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fw := &FileWriter{}
+			if err := json.Unmarshal([]byte(tt.config), fw); (err != nil) != tt.wantErr {
+				t.Fatalf("UnmarshalJSON() error = %v, want %v", err, tt.wantErr)
+			}
+			if fw.Mode != tt.fw.Mode {
+				t.Errorf("got mode %v, want %v", fw.Mode, tt.fw.Mode)
+			}
+		})
+	}
+}
diff --git a/modules/logging/filewriter_test_windows.go b/modules/logging/filewriter_test_windows.go
new file mode 100644
index 00000000000..d32a8d2c081
--- /dev/null
+++ b/modules/logging/filewriter_test_windows.go
@@ -0,0 +1,55 @@
+// Copyright 2015 Matthew Holt and The Caddy Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build windows
+
+package logging
+
+import (
+	"os"
+	"path"
+	"testing"
+)
+
+// Windows relies on ACLs instead of unix permissions model.
+// Go allows to open files with a particular mode put it is limited to read or write.
+// See https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/syscall/syscall_windows.go;l=708.
+// This is pretty restrictive and has few interest for log files and thus we just test that log files are
+// opened with R/W permissions by default on Windows too.
+func TestFileCreationMode(t *testing.T) {
+	dir, err := os.MkdirTemp("", "caddytest")
+	if err != nil {
+		t.Fatalf("failed to create tempdir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+
+	fw := &FileWriter{
+		Filename: path.Join(dir, "test.log"),
+	}
+
+	logger, err := fw.OpenWriter()
+	if err != nil {
+		t.Fatalf("failed to create file: %v", err)
+	}
+	defer logger.Close()
+
+	st, err := os.Stat(fw.Filename)
+	if err != nil {
+		t.Fatalf("failed to check file permissions: %v", err)
+	}
+
+	if st.Mode().Perm()&0o600 != 0o600 {
+		t.Fatalf("file mode is %v, want rw for user", st.Mode().Perm())
+	}
+}