Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logging: Allow setting log file permissions #6314

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ririsoft
Copy link

Adding a "mode" option to overwrite the default logfile permissions.
Default remains "0600" which is the one currently used by lumberjack.

Fixes #6295

I am assuming that Mode = 0 triggers usage of default mode "0600". That implies that users cannot use a "0000" mode for their log files. This sounds reasonable to me, but your opinion here would be appreciated.

I did not test the PR in production. I only tested in local and all went fine. Beware that your umask may interfere with the mode from the config: this is why umask is unset in the tests.

This is my first PR here. I would appreciate great care in the code review, I am fully open to your comments of course.

Cheers.

@CLAassistant
Copy link

CLAassistant commented May 12, 2024

CLA assistant check
All committers have signed the CLA.

@@ -41,6 +41,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 os.FileMode `json:"mode,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The os.FileMode type is an alias to fs.FileMode, which is defined as uint32. The mode 777, for example, is ought interpreted as octal by those parsing the string (pun-intended) of characters in the context of file mode. However, the initial (un)marshaling of the value in Go treats it as decimal. Going back to the example of 777, the user is expected to pass the integer 511 (the equivalent of o777) to get the effect of o777, which is confusing.

I'm torn between simply using a string versus defining a new type, e.g. type FileMode string with custom UnmarshalJSON method (like caddy.Duration) that can accept integers (assuming the digits are octal) as well as strings to be interpreted in the form rwxrwxrwx, but the latter may be an overkill.

I know you're not a Go dev. Sorry for making this more complicated/work for you 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's an interesting idea. I'd be willing to contribute that custom unmarshaler but need some time before I can get around to it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about reproducing the behavior of the unix chmod command ?

A numeric mode is from one to four octal digits (0-7), derived by adding up the bits with values 4, 2, and 1. Omitted digits are assumed to be leading zeros.

The following would be equivalent for instance:

file access.log {
  mode 777
}

file access.log {
  mode 0777
}

That way we keep users in the same confort zone they are with chmod and filesystem permissions.
What do you think ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, that's only the Caddyfile config. You need to also consider JSON config. The Caddyfile adapter produces a JSON config, but it should also be erogonomic for JSON config users to configure this. The type of the field itself dictates how its configured via JSON.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're not a Go dev. Sorry for making this more complicated/work for you 🙂

I used to code in Go a few years ago, I am having fun rediscovering it, no worry.

I'm torn between simply using a string versus defining a new type, e.g. type FileMode string with custom UnmarshalJSON method (like caddy.Duration) that can accept integers (assuming the digits are octal)

Considering that leading zeros are invalid json number it seems that if we want to allow users to use octal number with a leading zero we have no choice to use a json string I guess ? Tell me what you think and I will submit a new commit with an implementation proposal, and squash all the work once you are all ok with the PR.

as well as strings to be interpreted in the form rwxrwxrwx, but the latter may be an overkill.

Would you still accept my pull request if I let this one for somebody else's implementation ? Definitely overkill to me :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ririsoft That's what @mohammed90 was referring to: if we make our own type, we can decode a chmod-style string as an octal number.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mholt, I will submit a new commit with such proposal in a few days then.

@francislavoie
Copy link
Member

Alternatively (or in addition) to this, we could allow specifying the permissions like #4741 as in /path/to/file.txt|0666

@francislavoie francislavoie changed the title allow to set the log file permissions logging: Allow setting log file permissions May 13, 2024
@francislavoie
Copy link
Member

Tests are failing on Windows - I think what you'll need to do is only do this test on non-Windows (same way, with //go:build !windows).

@ririsoft
Copy link
Author

Ok so now errors are more clear to me. Let me try a few things to have some minimal testing on windows too.

@mholt
Copy link
Member

mholt commented May 15, 2024

Nice work cranking through this! :) I'll circle back after the 2.8 release.

@ririsoft
Copy link
Author

I have a race condition in the tests with the generation of compressed log file. I will fix that so that you can review the PR with CI OK.

Adding a "mode" option to overwrite the default logfile permissions.
Default remains "0600" which is the one currently used by lumberjack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caddy log file permissions
5 participants