Hi Florian,
Florian Dold <florian.dold@gmail.com> skribis:
Toggle quote (21 lines)
> 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!
Toggle quote (4 lines)
> 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:
Toggle quote (13 lines)
> From 3ac9c6fa536faff23291b21d4e649b85386fedfc Mon Sep 17 00:00:00 2001
> From: Florian Dold <flo@dold.me>
> 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.
Toggle quote (25 lines)
> (define-record-type* <fcgiwrap-configuration> fcgiwrap-configuration
> make-fcgiwrap-configuration
> fcgiwrap-configuration?
> - (package fcgiwrap-configuration-package ;<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 ;<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’.
Toggle quote (3 lines)
> + (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’.
