log: Only chmod if permission bits differ

[mholt]

Follow-up to #6314 and https://caddy.community/t/caddy-2-9-0-breaking-change/27576/11

Maybe someone with more sysadmin experience could double-check my logic here. We want to chmod only if the permission bits differ. Did I get this right? (I would just write a test for this, but I'm not sure I have enough skills to write the test for this correctly.)

[jjlin]

The permissions check looks okay to me, but I think there are actually a few more general issues with the log file mode design/implementation:

• Currently, between 1-4 octal digits are accepted, similar to chmod. However, I don't see a legit use case for allowing the user to set the setuid/setgid/sticky bits (the most significant octal digit) on a log file, and Go actually does not define setuid/setgid/sticky as bits 9-11 in os.FileMode, so I think setting those bits wouldn't work as expected anyway. I think the file mode should accept only 1-3 octal digits (or maybe 4, if we want to be more consistent with the chmod format but document that the highest digit is ignored). In any case, the file mode should be masked with os.ModePerm to retain only the permission bits.
• If I'm reading the code correctly, the current implementation would accept invalid file mode values like 77777 silently, when it should probably be returning an error.
• I still think the default file mode should be 644 to maintain backward compatibility with 2.8.4 and previous versions.

I might be able to work on this if no one else is interested, but I probably wouldn't be able to get it done until sometime next week.

[mholt]

Thanks for the review!

Currently, between 1-4 octal digits are accepted, similar to chmod. However, I don't see a legit use case for allowing the user to set the setuid/setgid/sticky bits (the most significant octal digit) on a log file, and Go actually does not define setuid/setgid/sticky as bits 9-11 in os.FileMode, so I think setting those bits wouldn't work as expected anyway. I think the file mode should accept only 1-3 octal digits (or maybe 4, if we want to be more consistent with the chmod format but document that the highest digit is ignored).

Hm, yeah, I don't know a ton about this. I'm OK with accepting only 3 digits since that's all I ever use.

In any case, the file mode should be masked with os.ModePerm to retain only the permission bits.

Already being done. 👍 https://github.com/caddyserver/caddy/pull/6761/files#diff-d0fffc245f4f6c772e4468b6bcd9bc130157f323bb9c40dce2964afca0796da0R183

If I'm reading the code correctly, the current implementation would accept invalid file mode values like 77777 silently, when it should probably be returning an error.

That may be the case, I'm not sure. We have some tests around it; maybe we just need a few more?

I still think the default file mode should be 644 to maintain backward compatibility with 2.8.4 and previous versions.

Unfortunately, 644 is less secure, and I don't think that's good posture for us to default to. (I thought we were defaulting to 0600 all along, I guess a dependency changed something at some point, unbeknownst to me!)

I have a hard time thinking of this having been a "breaking change" because what I think it's really doing is exposing a potential misconfiguration. Not saying that's the case, but it's surprising I think if Caddy is not the owner of its own files (log files or otherwise).

[ririsoft]

• Currently, between 1-4 octal digits are accepted, similar to chmod. However, I don't see a legit use case for allowing the user to set the setuid/setgid/sticky bits (the most significant octal digit) on a log file, and Go actually does not define setuid/setgid/sticky as bits 9-11 in os.FileMode, so I think setting those bits wouldn't work as expected anyway. I think the file mode should accept only 1-3 octal digits (or maybe 4, if we want to be more consistent with the chmod format but document that the highest digit is ignored). In any case, the file mode should be masked with os.ModePerm to retain only the permission bits.

While I agree that setting logs file setuid/setgid does not make sense for log files, I do not understand your point regarding Go's ability to set such bits: https://go.dev/src/os/types.go defines those bits and a quick test on MacOS sets the sticky bit.

• If I'm reading the code correctly, the current implementation would accept invalid file mode values like 77777 silently, when it should probably be returning an error.

There is an undocumented behavior from Go's os.Chmod, it truncates the octal value.
Here is a small test that reproduces the thing, maybe worth adding to the tests @mholt ? I am not sure this is worth documenting such thing though. The behavior just saves users from a mistake.

func TestTruncateOctallMode(t *testing.T) {
	tests := []struct {
		name     string
		fw       FileWriter
		wantMode os.FileMode
	}{
		{
			name: "large octal with sticky bit",
			fw: FileWriter{
				Roll: &off,
				Mode: 0o7777777,
			},
			wantMode: 0o777 | fs.ModeSticky,
		},
		{
			name: "large octal without sticky bit",
			fw: FileWriter{
				Roll: &off,
				Mode: 0o666666,
			},
			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.Errorf("got OpenWriter error: %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.Fatalf("file mode is %v, want %v", st.Mode(), tt.wantMode)
			}
		})
	}
}

• I still think the default file mode should be 644 to maintain backward compatibility with 2.8.4 and previous versions.

I would rather document this as a breaking change and have users define their own mode if they do not want the more secured default.

Maybe someone with more sysadmin experience could double-check my logic here. We want to chmod only if the permission bits differ. Did I get this right? (I would just write a test for this, but I'm not sure I have enough skills to write the test for this correctly.)

I can contribute some tests to validate the whole thing. I will submit a PR on the branch during the week-end @mholt.

[ririsoft]

In addition to my comments above can someone here explain again the issue we are trying to solve ? What is the user story ? I am a bit confused by the comments in https://caddy.community/t/caddy-2-9-0-breaking-change/27576/11, sorry :(

[jjlin]

• Currently, between 1-4 octal digits are accepted, similar to chmod. However, I don't see a legit use case for allowing the user to set the setuid/setgid/sticky bits (the most significant octal digit) on a log file, and Go actually does not define setuid/setgid/sticky as bits 9-11 in os.FileMode, so I think setting those bits wouldn't work as expected anyway. I think the file mode should accept only 1-3 octal digits (or maybe 4, if we want to be more consistent with the chmod format but document that the highest digit is ignored). In any case, the file mode should be masked with os.ModePerm to retain only the permission bits.

While I agree that setting logs file setuid/setgid does not make sense for log files, I do not understand your point regarding Go's ability to set such bits: https://go.dev/src/os/types.go defines those bits and a quick test on MacOS sets the sticky bit.

As I mentioned, Go does not use bits 9-11 for setuid/setgid/sticky in os.FileMode, so if the user specifies 1777 as the permissions, the log file would not have the sticky bit set as it would if you ran chmod 1777 <logfile>. If the goal is to model the Caddy file mode permissions after the numeric format of chmod, then it would be less misleading to only accept up to 3 digits for the file permissions, since the highest digit will be effectively ignored anyway unless you want to add extra code to map those bits to os.ModeSetuid, os.ModeSetgid, and os.ModeSticky, which seems pointless to implement for log files.

Compare (https://go.dev/play/p/UycfqT9tM5T):

fmt.Printf("%v\n", os.FileMode(0o1777))
fmt.Printf("%v\n", os.FileMode(0o777|os.ModeSticky))

which outputs:

-rwxrwxrwx
trwxrwxrwx

[mholt]

Sorry, getting back to this now.

Hmm, I guess I'm a little lost. It seems there are two conflicting assertions? @ririsoft's has a test, @jjlin has evidence from a Go playground... but I do not have enough skills with the nitty-gritty here to know whether I should merge this PR or if there is a bug.

Any advice?

PS. @ririsoft:

In addition to my comments above can someone here explain again the issue we are trying to solve ? What is the user story ? I am a bit confused by the comments in https://caddy.community/t/caddy-2-9-0-breaking-change/27576/11, sorry :(

Basically, there are complaints that the new (?) permissions of 0600 are too restrictive, so a mode should be able to be configured, but if the mode is already what is configured, we should not try setting it, since Caddy may not own its own log file (??) and thus the command will fail. So the point of this PR is to only change the mode if the mode does not match what is configured.

[jjlin]

Sorry, getting back to this now.

Hmm, I guess I'm a little lost. It seems there are two conflicting assertions? @ririsoft's has a test, @jjlin has evidence from a Go playground... but I do not have enough skills with the nitty-gritty here to know whether I should merge this PR or if there is a bug.

Any advice?

I think this PR is acceptable as-is for now. The other issues I mentioned are related to improved behavior/diagnostics when the user does something unusual (and arguably wrong). Those aspects could be improved in a future PR.

PS. @ririsoft:

In addition to my comments above can someone here explain again the issue we are trying to solve ? What is the user story ? I am a bit confused by the comments in https://caddy.community/t/caddy-2-9-0-breaking-change/27576/11, sorry :(

Basically, there are complaints that the new (?) permissions of 0600 are too restrictive, so a mode should be able to be configured, but if the mode is already what is configured, we should not try setting it, since Caddy may not own its own log file (??) and thus the command will fail. So the point of this PR is to only change the mode if the mode does not match what is configured.

When the user has set things up such that the log file is writable to the user under which Caddy is running, but not actually owned by that user, then Caddy can't chmod the file. For example:

• Log file is owned by root but has 666 permissions, so anyone can write to it, including the Caddy user.
• Log file is owned by root and by a group that the Caddy user belongs to, but has 660 permissions, so the Caddy user can still write to it.
• Log file is owned by root, with 600 permissions, but has an ACL set to allow the Caddy user to write to it. (This is the scenario reported in https://caddy.community/t/caddy-2-9-0-breaking-change/27576/11.)

In all of these cases, Caddy can write to the log file, but can't chmod it. Caddy did not try to chmod the log file previously (e.g., in 2.8.4), but now it tries to and fails, which is the reported issue.

[jjlin]

In the release notes, please call out the fact that the new default log file permissions are 600 and that Caddy will try to chmod the log file if its existing permissions don't match, as these are definitely breaking changes.

[mholt]

@jjlin Thanks. Shouldn't the chmod only happen though if permissions are configured (and with this patch, are different from current)? I don't think that's a breaking change, since the user needs to configure it.

[jjlin]

I guess the fact that it tries to chmod at all under some scenarios could be considered breaking, but I suppose the main thing is just that the new default is 600, so if the user wasn't using/expecting those permissions before, then they may need to change something.

For me, Caddy was running as root inside a container with the log file set to 644 (previous default), and I was running some log monitoring stuff outside the container under a non-root user, but after upgrading to 2.9.0, the log files were changed to 600 and no longer readable.