Hi Florian, Florian Dold skribis: > this patch adds additional options to the fcgiwrap service. In > particular it allows > > 1. writing the output of the fcgi process to a file (with the 'log-file' > option) > > 2. arranging for a directory to be created so that the fcgiwrap process > can create its listening socket without running into permission problems > (with the 'ensure-socket-dir?' option) > > 3. adjusting the permissions on the listening unix domain socket, > typically so that users in the fcgiwrap group have read and write access > to that socket (with the 'adjusted-socket-permissions' option) > > Additionally, a potentially left-over fcgiwrap socket is cleaned up > before starting the service, which would otherwise lead to the process > refusing to run. > > The documentation is also changed to address a potential security issue, > now recommending against running fcgiwrap as root. Thanks for working on it! > The configuration defaults are not ideal (a tcp socket with unrestricted > access from any local user), but impossible to change without breaking > existing system definitions. Yeah. Perhaps we could print a warning or something to encourage users to switch? Overall LGTM. Some minor comments below: > From 3ac9c6fa536faff23291b21d4e649b85386fedfc Mon Sep 17 00:00:00 2001 > From: Florian Dold > Date: Thu, 3 Jan 2019 14:22:49 +0100 > Subject: [PATCH] services: fcgiwrap: Implement additional options > > The fcgiwrap service now supports logging and can be run > on a unix domain socket as unprivileged user. > > * doc/guix.texi (Web Services): Document new options and replace > dangerous advice about running fcgiwrap as root. > * gnu/services/web.scm: Add the options 'log-file', > 'adjusted-socket-permissions' and 'ensure-socket-dir?'. It’d be great if you could list the modified variables for web.scm; otherwise I can do it for you. > (define-record-type* fcgiwrap-configuration > make-fcgiwrap-configuration > fcgiwrap-configuration? > - (package fcgiwrap-configuration-package ; > - (default fcgiwrap)) > - (socket fcgiwrap-configuration-socket > - (default "tcp:127.0.0.1:9000")) > - (user fcgiwrap-configuration-user > - (default "fcgiwrap")) > - (group fcgiwrap-configuration-group > - (default "fcgiwrap"))) > + (package fcgiwrap-configuration-package ; > + (default fcgiwrap)) > + (socket fcgiwrap-configuration-socket > + (default "tcp:127.0.0.1:9000")) > + (user fcgiwrap-configuration-user > + (default "fcgiwrap")) > + (group fcgiwrap-configuration-group > + (default "fcgiwrap")) > + (log-file fcgiwrap-log-file > + (default #f)) > + ;; boolean or octal mode integer > + (adjusted-socket-permissions fcgiwrap-adjusted-socket-permissions? > + (default #f)) Maybe just ‘socket-permissions’ and also leave out interpretation of #t as #o666? Also the accessor should then be ‘fcgiwrap-socket-permissions’. > + (ensure-socket-dir? fcgiwrap-ensure-socket-dir? > + (default #f))) s/dir/directory/ please. :-) Also please remove tabs from the file. Could you make sure “make check-system TESTS=cgit” still passes after the change? The rest LGTM. Could you send an updated patch? Thank you! Ludo’.