caddy icon indicating copy to clipboard operation
caddy copied to clipboard

logging: Allow setting log file permissions

Open ririsoft opened this issue 1 year ago • 6 comments

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.

ririsoft avatar May 12 '24 14:05 ririsoft

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 12 '24 14:05 CLAassistant

Alternatively (or in addition) to this, we could allow specifying the permissions like https://github.com/caddyserver/caddy/pull/4741 as in /path/to/file.txt|0666

francislavoie avatar May 13 '24 01:05 francislavoie

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).

francislavoie avatar May 14 '24 01:05 francislavoie

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

ririsoft avatar May 15 '24 13:05 ririsoft

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

mholt avatar May 15 '24 20:05 mholt

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.

ririsoft avatar May 16 '24 08:05 ririsoft

Gotcha. Thanks for the careful consideration and the tests!

Looks good. Let's give it a whirl. Thank you for the contribution!!

Thank you @mholt , @francislavoie , @mohammed90 for your help with this PR. Allow me to congratulate you all, and @mholt in particular, for the amazing work you are doing with Caddy as well as the way you lead the project for contributions in the open. It was really easy to on-board and hack. Well done !

ririsoft avatar Jun 08 '24 11:06 ririsoft

Dear Team,

I am using caddy version v2.8.4 and I am trying to have read access granted to caddy log file to all users. Based on this pull request, I have got Caddyfile with section like ...

log { output file /var/log/caddy/access.log { mode 644 } } This is not working, could someone please guide me in correct direction. I am newbie with caddy / golang.

bhavin-qryptal avatar Aug 20 '24 21:08 bhavin-qryptal

In what way is it not working? Show evidence.

francislavoie avatar Aug 20 '24 21:08 francislavoie

Also probably open a new issue with enough information to reproduce the bug :+1:

mholt avatar Aug 20 '24 21:08 mholt

In what way is it not working? Show evidence.

Thank you for prompt reply.

image

access.log file permission remains 600, I am expecting it to be 644. Please let me know if you need any other details.

bhavin-qryptal avatar Aug 20 '24 22:08 bhavin-qryptal

Also probably open a new issue with enough information to reproduce the bug 👍

At this stage, I am not sure, whether this is a bug OR I am doing something wrong. Is the change discussed in this thread included in v2.8.4 ?

bhavin-qryptal avatar Aug 20 '24 22:08 bhavin-qryptal

Ah, it's not been released, no. It was merged June 6th, last release was June 2nd. You can build from master if you want it now.

francislavoie avatar Aug 20 '24 22:08 francislavoie

log { output file /var/log/caddy/access.log { mode 644 } }

Thanks a lot. I will give it a go.

image Hope this looks reasonable to you.

bhavin-qryptal avatar Aug 20 '24 22:08 bhavin-qryptal

After updating to Caddy 2.9.0, I immediately ran into the issue of my log files being inaccessible. For backward compatibility, I think it would've been better to use the previous default of 644 even if Lumberjack changed the default on their side, but if you don't want to do that, it would be nice to explicitly call this out as a breaking change (sorry in advance if it's actually mentioned somewhere and I missed it).

jjlin avatar Jan 02 '25 20:01 jjlin