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.

Sign up for GitHub

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

Jump to bottom

nixos/nginx: forced change permissions #76174

Merged
merged 1 commit into from Dec 26, 2019
Merged

nixos/nginx: forced change permissions #76174

merged 1 commit into from Dec 26, 2019

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Dec 22, 2019 •
edited

Motivation for this change

Fixed this error - #56255 (comment)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

asymmetric
asymmetric requested changes Dec 22, 2019
View reviewed changes
Copy link
Contributor

@asymmetric asymmetric left a comment

Choose a reason for hiding this comment

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

Could you mention in the commit message that the change is there to make migration from the old permissions to the new one possible?

E.g.:

nixos/nginx: recursively change logs directory owner/group

This change brings pre-existing installations (where the logfiles are owned by root) in line with the new permssions (where logfiles are owned by the nginx user)

Also, I'd change the commit message to something more descriptive

aanderse
aanderse reviewed Dec 22, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment •
edited

Choose a reason for hiding this comment

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

I believe order is important. I think you should put this at the end instead of beginning. Can't check at the moment, though...

Will follow up.

@flokli
Copy link
Contributor

flokli commented Dec 22, 2019

As described in #56255 (comment), please use

z ${cfg.stateDir}/logs/* 6?? ...`

We don't want to make log files executable.

We should however have a discussion about ?? (the permissions for group and other), as well as about how to handle ${cfg.stateDir}/*_temp (and contents in there).

@Izorkin
nixos/nginx: recursively change logs directory owner/group
f33b6b0 
This change brings pre-existing installations (where the logfiles
are owned by root) in line with the new permssions (where logfiles
are owned by the nginx user)
@Izorkin
Copy link
Contributor Author

Izorkin commented Dec 22, 2019

Thanks, fixed.

@danbst danbst merged commit b0bbacb into NixOS:master Dec 26, 2019
@danbst
Copy link
Contributor

danbst commented Dec 26, 2019

I've merged this as nginx service is broken without this. Discussion about *_temp is still welcome

@flokli
Copy link
Contributor

flokli commented Dec 26, 2019 •
edited

@danbst: the tmpfiles rule is still wrong - it should be a lowercase z, and a glob pattern matching files - please see #76174 (comment)

We're now making log files executable.

Can you post a fixup PR which does that?

@danbst
Copy link
Contributor

danbst commented Dec 26, 2019

@flokli yes

@danbst
Copy link
Contributor

danbst commented Dec 26, 2019 •
edited

EDIT: just noticed a glob mention. :doh:

@flokli I'm testing z (small-z) and it doesn't change ownership. But big-Z does change ownership:

With

    systemd.tmpfiles.rules = [
      "z '${config.services.nginx.stateDir}/logs' 0750 nginx nginx - -"
    ];

[root@s1:~]# chown root:root /var/spool/nginx/logs/somefile 

[root@s1:~]# ls -la /var/spool/nginx/logs
total 16
drwxr-x--- 2 nginx nginx 4096 Dec 26 12:00 .
drwxr-x--- 8 nginx nginx 4096 Dec 26 12:00 ..
-rw-r--r-- 1 nginx nginx   88 Dec 26 12:05 access.log
-rw-r--r-- 1 nginx nginx  380 Dec 26 12:00 error.log
-rw-r--r-- 1 root  root     0 Dec 26 12:00 somefile

[root@s1:~]# nixos-rebuild switch
building Nix...
building the system configuration...
updating GRUB 2 menu...
activating the configuration...
setting up /etc...
reloading user units for root...
setting up tmpfiles

[root@s1:~]# ls -la /var/spool/nginx/logs
total 16
drwxr-x--- 2 nginx nginx 4096 Dec 26 12:00 .
drwxr-x--- 8 nginx nginx 4096 Dec 26 12:00 ..
-rw-r--r-- 1 nginx nginx   88 Dec 26 12:05 access.log
-rw-r--r-- 1 nginx nginx  380 Dec 26 12:00 error.log
-rw-r--r-- 1 root  root     0 Dec 26 12:00 somefile

With

    systemd.tmpfiles.rules = [
      "Z '${config.services.nginx.stateDir}/logs' 0750 nginx nginx - -"
    ];

[root@s1:~]# chown root:root /var/spool/nginx/logs/somefile 

[root@s1:~]# ls -la /var/spool/nginx/logs
total 16
drwxr-x--- 2 nginx nginx 4096 Dec 26 12:00 .
drwxr-x--- 8 nginx nginx 4096 Dec 26 12:00 ..
-rwxr-x--- 1 nginx nginx   88 Dec 26 12:05 access.log
-rwxr-x--- 1 nginx nginx  380 Dec 26 12:00 error.log
-rwxr-x--- 1 root  root     0 Dec 26 12:00 somefile

[root@s1:~]# nixos-rebuild switch
building Nix...
building the system configuration...
updating GRUB 2 menu...
activating the configuration...
setting up /etc...
reloading user units for root...
setting up tmpfiles

[root@s1:~]# ls -la /var/spool/nginx/logs
total 16
drwxr-x--- 2 nginx nginx 4096 Dec 26 12:00 .
drwxr-x--- 8 nginx nginx 4096 Dec 26 12:00 ..
-rwxr-x--- 1 nginx nginx   88 Dec 26 12:05 access.log
-rwxr-x--- 1 nginx nginx  380 Dec 26 12:00 error.log
-rwxr-x--- 1 nginx nginx    0 Dec 26 12:00 somefile

@Izorkin
Copy link
Contributor Author

Izorkin commented Dec 26, 2019

@danbst doesn’t work for me either

@danbst danbst mentioned this pull request Dec 26, 2019
Merged
@danbst
Copy link
Contributor

danbst commented Dec 26, 2019

@Izorkin it is because little-z expects glob-pattern, like ${stateDir}/logs/*. In the end I went with big-Z but without changing mode

@Izorkin
Copy link
Contributor Author

Izorkin commented Dec 26, 2019

@danbst correct worked with:
Z '/var/spool/nginx/logs/*.log' 0650 nginx nginx - -

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 16, 2020
@Izorkin @dtzWill
nixos/nginx: recursively change logs directory owner/group (NixOS#76174)
a8e9d66 
This change brings pre-existing installations (where the logfiles
are owned by root) in line with the new permssions (where logfiles
are owned by the nginx user)

(cherry picked from commit b0bbacb)
@Izorkin Izorkin deleted the nginx-fix branch March 7, 2020 12:59
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Mar 31, 2020
@Izorkin @dtzWill
nixos/nginx: recursively change logs directory owner/group (NixOS#76174)
3ead06e 
This change brings pre-existing installations (where the logfiles
are owned by root) in line with the new permssions (where logfiles
are owned by the nginx user)

(cherry picked from commit b0bbacb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse left review comments

@asymmetric asymmetric asymmetric requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Izorkin @flokli @danbst @asymmetric @aanderse