fcgiwrap: additional options for logging and unix domain sockets

  • Open
  • quality assurance status badge
Details
3 participants
  • Arun Isaac
  • Florian Dold
  • Ludovic Courtès
Owner
unassigned
Submitted by
Florian Dold
Severity
normal

Debbugs page

F
F
Florian Dold wrote on 3 Jan 2019 12:02
(address . guix-patches@gnu.org)
624ba072-d5fe-b159-46af-61e79caf22f1@gmail.com
Hi Guix,

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.

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.

- Florian
Attachment: file
L
L
Ludovic Courtès wrote on 9 Jan 2019 08:17
(name . Florian Dold)(address . florian.dold@gmail.com)(address . 33966@debbugs.gnu.org)
87a7k94xhe.fsf@gnu.org
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’.
L
L
Ludovic Courtès wrote on 9 Jan 2019 08:17
control message for bug #33966
(address . control@debbugs.gnu.org)
878szt4xh8.fsf@gnu.org
tags 33966 security
A
A
Arun Isaac wrote on 25 May 2019 00:57
Re: [bug#33966] fcgiwrap: additional options for logging and unix domain sockets
(name . Florian Dold)(address . florian.dold@gmail.com)
cu71s0nnezi.fsf@systemreboot.net
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.

I think it's ok to break existing system definitions when security is at
stake.
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEf3MDQ/Lwnzx3v3nTLiXui2GAK7MFAlzo9WEACgkQLiXui2GA
K7O++Qf9GvX/SfKF/qOxoYI/9veC5+aapyuiVFAT9OAixqrKUIbqzna9Hmaydcyn
D4PviGI/nxlLP+v5SuZNwdwwOy9PmPa81giXwKocaAULCzwASilBUnuLZiUM2brn
99YG3gpF86MtDALP6t12+s3MVJhlNmnkw5f7ZvoQ9saYPq9U0FXAtbw6VUGfiIe3
JwmW1Lrax18cSk72ul0t4SmFz0yJci/zA7RcrWjqFaqjMSaUy2WAt9upBh4UShMS
WmlZI6yUl7h72h4rdytJ1NtvNbiMfCmbfidU0KsAtm37wiFxQXQgvPFCWyM6DjFa
sokSlHKxpyAovi6zhEM8irkTlpAnPA==
=CjjU
-----END PGP SIGNATURE-----

?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 33966@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 33966
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch