"chown" must be ran before "${cfg.package}/bin/nginx ... -t", otherwise an error will be observed. More details in #48875 (review)
or move ${cfg.preStart}

My example PreStart script to check permissions

test -d "${cfg.stateDir}" || mkdir -m 750 "${cfg.stateDir}"
test -d "${cfg.stateDir}/logs" || mkdir -m 750 -p "${cfg.stateDir}/logs"
test `stat -c %a:%U:%G "${cfg.stateDir}"` = "750:root:${cfg.group}" || (chmod 750 "${cfg.stateDir}" && chown root:${cfg.group} "${cfg.stateDir}")
test `stat -c %a:%U:%G "${cfg.stateDir}/logs"` = "750:root:${cfg.group}" || (chmod 750 "${cfg.stateDir}/logs" && chown root:${cfg.group} "${cfg.stateDir}/logs")

There is already a chown in the default cfg.preStart -- and yes, it's a bit ugly that it needs to be done before and after, but I couldn't convince the nginx binary to not create the log files as root:root when in test mode. I toyed with running the nginx config test unprivileged, but unfortunately this then failed because the config test fails to bind 80/443.

I use this method:

  services.logrotate = {
    config = ''
      ${cfgNginxSpool}/logs/access.log
      ${cfgNginxSpool}/logs/error.log
      {
        create 0640 ${cfg.user} ${cfg.group}
        postrotate
          [ ! -f /run/nginx.pid ] || kill -USR1 `cat /run/nginx.pid`
        endscript
      }
    '';
  };

Please check thats variant:

      preStart =  mkOption {
        type = types.lines;
        default = ''
          test -d "${cfg.stateDir}" || mkdir -m 750 "${cfg.stateDir}"
          test -d "${cfg.stateDir}/logs" || mkdir -m 750 -p "${cfg.stateDir}/logs"
          test `stat -c %a:%U:%G "${cfg.stateDir}"` = "750:${cfg.user}:${cfg.group}" || (chmod 750 "${cfg.stateDir}" && chown ${cfg.user}:${cfg.group} "${cfg.stateDir}")
          test `stat -c %a:%U:%G "${cfg.stateDir}/logs"` = "750:${cfg.user}:${cfg.group}" || (chmod 750 "${cfg.stateDir}/logs" && chown ${cfg.user}:${cfg.group} "${cfg.stateDir}/logs")
          ${pkgs.su-exec}/bin/su-exec ${cfg.user}:${cfg.group} /run/wrappers/bin/nginx -c ${configFile} -p ${cfg.stateDir} -t
        '';
...
    systemd.services.nginx = {
      preStart = 
      ''
        ${cfg.preStart}
      '';
...
    security.wrappers.nginx = {
      source = "${cfg.package}/bin/nginx";
      capabilities = "cap_net_bind_service+eip";
      owner = cfg.user;
      group = cfg.group;
      permissions = "u+rx,g+x";
    };

Instead of fixing permissions of files created by "nginx -t", we can run "nginx -t" with dropped privileges, using su-exec. In this case nginx will create log files with correct permissions. So all we need to do is create /var/spool/nginx writeable for nginx (maybe even for nginx group).

I like this, hadn't thought of using a wrapper to add cap_net_bind_service! Done.

There is no setuid binary:

-r-x--x--- 1 nginx  nginx      17704 Dec  6 01:18 nginx

That binary is however able to listen on <1024 ports through capabilities. I think this could just be mode 500 though. Let me change that.

It was a mistake to expose preStart as an option in #48875 in the first place. Now we have to do ugly workarounds.

My prestart scripts normal worked, not ugly.
nginx -t running from root incorrectly sets permissions log

@Mic92 please make sure you're looking at the latest version of this PR -- it has close to no changes to the preStart script. This comment thread was started on an older version.

@Izorkin I don't understand why the preStart script permissions dance should change in any way from what is currently in master. The current version in this PR does the right thing.

@delroth i have no deprecation in PR.

@delroth
You can rename to security.wrappers.test-nginx ?

Done.

Huh what's the reason behind this? This has nothing to do with testing

"chown" must be ran before "${cfg.package}/bin/nginx ... -t", otherwise an error will be observed. More details in #48875 (review) and #51551 (comment)

@infinisil it does have to do with testing, because this wrapper for the nginx binary is only used for the config testing done at pre-startup. The reason why it needs a wrapper is that for some reason the nginx config test checks if ports can be listened on, and this fails for privileged ports without the proper cap. In serving situations, that cap is given by the systemd service. In pre-exec the cap is not provided, hence the need for this wrapper (which is only runnable by user nginx, so doesn't provide a universal cap privesc).

It's not very elegant and probably a better way would be to convince upstream to have an option to not do this listen port check. But frankly this is more effort than I'm willing to put into this -- way more effort than is required to rebase this PR in my local fork to get the benefits of it myself.

@delroth there is still such a method https://github.com/nixcloud/nixcloud-webservices/blob/master/modules/web/webserver/lib/nginx_check_config.nix