| Message ID | 20220417210453.27884-4-ludo@gnu.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add "least authority" program wrapper | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Ludovic Courtès schreef op zo 17-04-2022 om 23:04 [+0200]: > +(define %precious-variables > + ;; Environment variables preserved by the wrapper by default. > + '("HOME" "USER" "LOGNAME" "DISPLAY" "XAUTHORITY" "TERM" "TZ" "PAGER")) This appears to be duplicated from (guix profiles), so there seems to be a risk here of them going out-of-sync; would it make send for (guix profiles) to read (guix least-authority) here? Greetings, Maxime.
Ludovic Courtès schreef op zo 17-04-2022 om 23:04 [+0200]: > +(define* (least-authority-wrapper program > + #:key (name "pola-wrapper") > + (guest-uid 1000) > + (guest-gid 1000) > + (mappings '()) > + (namespaces %namespaces) > + (directory "/") > + (preserved-environment-variables > + %precious-variables)) Could there be an option to define environment variables? E.g. set GUIX_LOCPATH for Guile packages that need locale data to read non-ASCII file names. As is, it seems like an environment-setting wrapper has to be inserted inside the pola wrapper to do this. Greetings, Maxime.
Maxime Devos <maximedevos@telenet.be> skribis: > Ludovic Courtès schreef op zo 17-04-2022 om 23:04 [+0200]: >> +(define %precious-variables >> + ;; Environment variables preserved by the wrapper by default. >> + '("HOME" "USER" "LOGNAME" "DISPLAY" "XAUTHORITY" "TERM" "TZ" "PAGER")) > > This appears to be duplicated from (guix profiles), so there seems to > be a risk here of them going out-of-sync; would it make send for > (guix profiles) to read (guix least-authority) here? It is duplicated, but OTOH the use case is different. So I think it’s OK to have a different definition here. (Perhaps eventually we’ll do something fancier, like preserving XAUTHORITY and DISPLAY if and only if we’re running an X11 program.) Ludo’.
Maxime Devos <maximedevos@telenet.be> skribis: > Ludovic Courtès schreef op zo 17-04-2022 om 23:04 [+0200]: >> +(define* (least-authority-wrapper program >> + #:key (name "pola-wrapper") >> + (guest-uid 1000) >> + (guest-gid 1000) >> + (mappings '()) >> + (namespaces %namespaces) >> + (directory "/") >> + (preserved-environment-variables >> + %precious-variables)) > > Could there be an option to define environment variables? E.g. set > GUIX_LOCPATH for Guile packages that need locale data to read non-ASCII > file names. As is, it seems like an environment-setting wrapper has to > be inserted inside the pola wrapper to do this. Yes, good point. I’m tempted to wait until the first use case comes up though. :-) Thanks for taking a look!
Hello, Ludovic Courtès <ludo@gnu.org> writes: > + (reify-exit-status > + (call-with-container mounts > + (lambda () > + (chdir #$directory) > + (environ variables) > + (apply execl #$program #$program (cdr (command-line)))) I'm a bit concerned about running arbitrary commands as PID 1 of process namespaces. A process running as PID 1 (even in a child namespace) is a special case and is treated differently by the Linux kernel than any other process, so it needs to be a program that has been designed to work in that situation. There are two differences from regular processes: 1. PID 1 inherits orphan processes and needs to wait() on them when they quit, in order to avoid accumulating zombie processes in the system. 2. Unlike regular processes, PID 1 doesn't have default signal handlers. Both of these aspects are described in more detail here: https://github.com/krallin/tini/issues/8#issuecomment-146135930 So to avoid an accumulation of zombie processes and other signal-related problems, I suggest adding a “(init-program ,tini)” parameter to ‘least-authority-wrapper’ and executing ‘program’ as a subprocess of ‘tini’ or whatever was passed as the #:init-program (perhaps #f could mean running ‘program’ directly as PID 1). I mention this because I'm currently dealing with a problem that has exactly this root cause: I'm working on updating the public-inbox package to the latest version, and the testsuite is failing because it tests that lei's daemon process is correctly terminated. But that doesn't work because “guix build” doesn't use a proper init program as PID 1 and thus the daemon process goes to zombie state and the testsuite thinks that it didn't go away. I'm hoping to send a patch to fix that issue.
Hi! Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > I'm a bit concerned about running arbitrary commands as PID 1 of process > namespaces. A process running as PID 1 (even in a child namespace) is a > special case and is treated differently by the Linux kernel than any > other process, so it needs to be a program that has been designed to > work in that situation. There are two differences from regular > processes: > > 1. PID 1 inherits orphan processes and needs to wait() on them when they > quit, in order to avoid accumulating zombie processes in the system. > > 2. Unlike regular processes, PID 1 doesn't have default signal handlers. Good points. > Both of these aspects are described in more detail here: > > https://github.com/krallin/tini/issues/8#issuecomment-146135930 > > So to avoid an accumulation of zombie processes and other signal-related > problems, I suggest adding a “(init-program ,tini)” parameter to > ‘least-authority-wrapper’ and executing ‘program’ as a subprocess of > ‘tini’ or whatever was passed as the #:init-program (perhaps #f could > mean running ‘program’ directly as PID 1). Hmm yes. It’s not great that the choice is between ‘unshare’—efficient but the process lives in the parent PID namespace—and ‘clone’—but then you have to fork twice. But yeah, you’re right. I’ll try what you suggest and send a v2. > I mention this because I'm currently dealing with a problem that has > exactly this root cause: I'm working on updating the public-inbox > package to the latest version, and the testsuite is failing because it > tests that lei's daemon process is correctly terminated. But that > doesn't work because “guix build” doesn't use a proper init program as > PID 1 and thus the daemon process goes to zombie state and the testsuite > thinks that it didn't go away. I'm hoping to send a patch to fix that > issue. Now that you mention it, this was discussed before: https://issues.guix.gnu.org/30948 I think we should do something about it in gnu-build-system.scm. Thanks for your feedback! Ludo’.
Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > I'm a bit concerned about running arbitrary commands as PID 1 of process > namespaces. A process running as PID 1 (even in a child namespace) is a > special case and is treated differently by the Linux kernel than any > other process, so it needs to be a program that has been designed to > work in that situation. There are two differences from regular > processes: > > 1. PID 1 inherits orphan processes and needs to wait() on them when they > quit, in order to avoid accumulating zombie processes in the system. > > 2. Unlike regular processes, PID 1 doesn't have default signal handlers. Actually right now ‘make-forkexec-constructor/container’ runs processes as PID 1. AFAIK this hasn’t been a problem in practice, probably for two reasons: (1) we’re wrapping daemons that don’t fork (unlike Jenkins…), and (2) ‘call-with-container’ installs a SIGINT handler and probably daemons also install SIGTERM and related handlers of their own. Anyway, it’s a class of problem that would be best avoided in the first place! Ludo’.
Hi Ludo! Ludovic Courtès <ludo@gnu.org> writes: > Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: >> So to avoid an accumulation of zombie processes and other signal-related >> problems, I suggest adding a “(init-program ,tini)” parameter to >> ‘least-authority-wrapper’ and executing ‘program’ as a subprocess of >> ‘tini’ or whatever was passed as the #:init-program (perhaps #f could >> mean running ‘program’ directly as PID 1). > > Hmm yes. It’s not great that the choice is between ‘unshare’—efficient > but the process lives in the parent PID namespace—and ‘clone’—but then > you have to fork twice. Yeah, the signals part of the Unix design isn't great. > But yeah, you’re right. I’ll try what you suggest and send a v2. Thank you for making these changes! I had a look at v2 and it looks great. >> I mention this because I'm currently dealing with a problem that has >> exactly this root cause: I'm working on updating the public-inbox >> package to the latest version, and the testsuite is failing because it >> tests that lei's daemon process is correctly terminated. But that >> doesn't work because “guix build” doesn't use a proper init program as >> PID 1 and thus the daemon process goes to zombie state and the testsuite >> thinks that it didn't go away. I'm hoping to send a patch to fix that >> issue. > > Now that you mention it, this was discussed before: > > https://issues.guix.gnu.org/30948 > > I think we should do something about it in gnu-build-system.scm. Nice! Thank you for the link. The discussion there was very informative. I'll try to implement your idea of adding a new build phase to install the appropriate signal handlers. Probably even steal your child reaping code from the v2 patches. > Thanks for your feedback! Thank you for taking it into account!
Ludovic Courtès <ludo@gnu.org> skribis: > gexp: Add 'references-file'. > file-systems: Avoid load-time warnings when attempting to load (guix > store). > linux-container: 'call-with-container' relays SIGTERM and SIGINT. > linux-container: Ensure signal-handling asyncs get a chance to run. > linux-container: Add #:child-is-pid1? parameter to > 'call-with-container'. > Add (guix least-authority). > services: dicod: Rewrite using 'least-authority-wrapper'. > services: dicod: Use 'make-inetd-constructor'. > services: bitlbee: Use 'make-inetd-constructor'. > services: ipfs: Adjust for Shepherd 0.9. > services: ipfs: Use 'least-authority-wrapper'. > services: wesnothd: Grant write access to /var/run/wesnothd. > services: wesnothd: Use 'least-authority-wrapper'. > services: quassel: Use 'least-authority-wrapper'. > services: opendht: Use 'least-authority-wrapper'. Pushed as fee06d5aaa71a965ea0bc06c1ff15c138a8bb2c8, thanks again for reviewing! Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > Ludovic Courtès <ludo@gnu.org> skribis: > >> gexp: Add 'references-file'. >> file-systems: Avoid load-time warnings when attempting to load (guix >> store). >> linux-container: 'call-with-container' relays SIGTERM and SIGINT. >> linux-container: Ensure signal-handling asyncs get a chance to run. >> linux-container: Add #:child-is-pid1? parameter to >> 'call-with-container'. >> Add (guix least-authority). >> services: dicod: Rewrite using 'least-authority-wrapper'. >> services: dicod: Use 'make-inetd-constructor'. >> services: bitlbee: Use 'make-inetd-constructor'. >> services: ipfs: Adjust for Shepherd 0.9. >> services: ipfs: Use 'least-authority-wrapper'. >> services: wesnothd: Grant write access to /var/run/wesnothd. >> services: wesnothd: Use 'least-authority-wrapper'. >> services: quassel: Use 'least-authority-wrapper'. >> services: opendht: Use 'least-authority-wrapper'. > > Pushed as fee06d5aaa71a965ea0bc06c1ff15c138a8bb2c8, thanks again for > reviewing! That's great! Thank you for addressing the PID 1 issue!
diff --git a/Makefile.am b/Makefile.am index fecce7c6f7..d0d58da4e3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -130,6 +130,7 @@ MODULES = \ guix/cache.scm \ guix/cve.scm \ guix/workers.scm \ + guix/least-authority.scm \ guix/ipfs.scm \ guix/build-system.scm \ guix/build-system/android-ndk.scm \ diff --git a/gnu/build/shepherd.scm b/gnu/build/shepherd.scm index d52e53eb78..f4caefce3c 100644 --- a/gnu/build/shepherd.scm +++ b/gnu/build/shepherd.scm @@ -31,7 +31,8 @@ (define-module (gnu build shepherd) exec-command %precious-signals) #:autoload (shepherd system) (unblock-signals) - #:export (make-forkexec-constructor/container + #:export (default-mounts + make-forkexec-constructor/container fork+exec-command/container)) ;;; Commentary: diff --git a/guix/least-authority.scm b/guix/least-authority.scm new file mode 100644 index 0000000000..806c47508f --- /dev/null +++ b/guix/least-authority.scm @@ -0,0 +1,131 @@ +;;; GNU Guix --- Functional package management for GNU +;;; Copyright © 2022 Ludovic Courtès <ludo@gnu.org> +;;; +;;; This file is part of GNU Guix. +;;; +;;; GNU Guix is free software; you can redistribute it and/or modify it +;;; under the terms of the GNU General Public License as published by +;;; the Free Software Foundation; either version 3 of the License, or (at +;;; your option) any later version. +;;; +;;; GNU Guix is distributed in the hope that it will be useful, but +;;; WITHOUT ANY WARRANTY; without even the implied warranty of +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;;; GNU General Public License for more details. +;;; +;;; You should have received a copy of the GNU General Public License +;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. + +(define-module (guix least-authority) + #:use-module (guix gexp) + #:use-module (guix modules) + #:use-module ((guix store) #:select (%store-prefix)) + #:autoload (gnu build linux-container) (%namespaces) + #:autoload (gnu system file-systems) (file-system-mapping + file-system-mapping-source + spec->file-system + file-system->spec + file-system-mapping->bind-mount) + #:export (least-authority-wrapper)) + +;;; Commentary: +;;; +;;; This module provides tools to execute programs with the least authority +;;; necessary, using Linux namespaces. +;;; +;;; Code: + +(define %precious-variables + ;; Environment variables preserved by the wrapper by default. + '("HOME" "USER" "LOGNAME" "DISPLAY" "XAUTHORITY" "TERM" "TZ" "PAGER")) + +(define* (least-authority-wrapper program + #:key (name "pola-wrapper") + (guest-uid 1000) + (guest-gid 1000) + (mappings '()) + (namespaces %namespaces) + (directory "/") + (preserved-environment-variables + %precious-variables)) + "Return a wrapper of PROGRAM that executes it with the least authority. + +PROGRAM is executed in separate namespaces according to NAMESPACES, a list of +symbols; it turns with GUEST-UID and GUEST-GID. MAPPINGS is a list of +<file-system-mapping> records indicating directories mirrored inside the +execution environment of PROGRAM. DIRECTORY is the working directory of the +wrapped process. Each environment listed in PRESERVED-ENVIRONMENT-VARIABLES +is preserved; other environment variables are erased." + (define code + (with-imported-modules (source-module-closure + '((gnu system file-systems) + (gnu build shepherd) + (gnu build linux-container))) + #~(begin + (use-modules (gnu system file-systems) + (gnu build linux-container) + ((gnu build shepherd) #:select (default-mounts)) + (srfi srfi-1)) + + (define variables + (filter-map (lambda (variable) + (let ((value (getenv variable))) + (and value + (string-append variable "=" value)))) + '#$preserved-environment-variables)) + + (define (read-file file) + (call-with-input-file file read)) + + (define references + (delete-duplicates + (append-map read-file + '#$(map references-file + (cons program + (map file-system-mapping-source + mappings)))))) + + (define (store? file-system) + (string=? (file-system-mount-point file-system) + #$(%store-prefix))) + + (define mounts + (append (map (lambda (item) + (file-system-mapping->bind-mount + (file-system-mapping (source item) + (target item)))) + references) + (remove store? + (default-mounts + #:namespaces '#$namespaces)) + (map spec->file-system + '#$(map (compose file-system->spec + file-system-mapping->bind-mount) + mappings)))) + + (define (reify-exit-status status) + (cond ((status:exit-val status) => exit) + ((or (status:term-sig status) + (status:stop-sig status)) + => (lambda (signal) + (format (current-error-port) + "~a terminated with signal ~a~%" + #$program signal) + (exit 126))))) + + ;; Note: 'call-with-container' creates a sub-process that this one + ;; waits for. This might seem suboptimal but unshare(2) isn't + ;; really applicable: the process would still run in the same PID + ;; namespace. + + (reify-exit-status + (call-with-container mounts + (lambda () + (chdir #$directory) + (environ variables) + (apply execl #$program #$program (cdr (command-line)))) + #:guest-uid #$guest-uid + #:guest-gid #$guest-gid + #:namespaces '#$namespaces))))) + + (program-file name code))