[bug#47323] [PATCH] services: export sysctl-configuration record field accessors

[bug#47323] [PATCH] services: export sysctl-configuration record field accessors

[muradm]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: gnu-services-sysctl --]
[-- Type: text/x-patch, Size: 931 bytes --]

From 0928d70c1cd5a98efd7671c05b38757400941790 Mon Sep 17 00:00:00 2001
From: muradm <mail@muradm.net>
Date: Mon, 22 Mar 2021 19:09:48 +0300
Subject: [PATCH] services: export sysctl-configuration record field accessors

* gnu/services/sysctl.scm (sysctl-configuration-sysctl): new public function
* gnu/services/sysctl.scm (sysctl-configuration-settings): new public function

Signed-off-by: muradm <mail@muradm.net>
---
 gnu/services/sysctl.scm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gnu/services/sysctl.scm b/gnu/services/sysctl.scm
index aaea7cc30d..80ed2ff46f 100644
--- a/gnu/services/sysctl.scm
+++ b/gnu/services/sysctl.scm
@@ -25,6 +25,8 @@
   #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
   #:export (sysctl-configuration
+            sysctl-configuration-sysctl
+            sysctl-configuration-settings
             sysctl-service-type
             %default-sysctl-settings))
 
-- 
2.31.0

[bug#47323] [PATCH] services: export sysctl-configuration record field accessors

[muradm]

As per discussion with Leo on IRC #guix in relation to #47013 and
#47323.

There is a need to have important sysctl settings
fs.protected_hardlinks and fs.protected_symlinks for all
installations of Guix in the world unless explicitly stated
otherwise. Currently in Linux kernel they are unset by default. It
is also stated that other distributions do the same.

In perfect world I would go for Solution 1 below, as it is most
effectful, and clean.

Solution 1: From this statement, it seems that the first resort
whould be Linux kernel it self. If it would be possible to
configure them with Kconfig, that would be best place. As of my
brief look at linux/fs, they are not configurable, but may be I
miss somthing. Any way preferred solution would be just compile
kernel with protected hardlinks and symlinks set to 1. Since other
distributions do the same, it could be reasonable to expose these
two settings via Kconfig, and solve it there.
- pros: great for the world
- cons: have to do enhancement in mainline Linux

Solution 2: If it is not possible to have these two settings in
kernel as per Solution 1, Guix may maintain a patch to kernel that
would do this.
- pros: no need to enhance mainline Linux
- cons: will impact users who do use Guix and compile Linux kernel
    them selves

Solution 3: Handle in Guix configuration. Everything below related
to solution 3 and current issue #47323.

Currently it is set as folowing:

;; gnu/services/sysctl.scm
(define-module ....
    #:export (....
                    %default-sysctl-settings)

(define %default-sysctl-settings
    ;; Default kernel parameters enabled with sysctl.
    '(("fs.protected_hardlinks" . "1")
      ("fs.protected_symlinks" . "1")))

(define-record-type* <sysctl-configuration>
    sysctl-configuration make-sysctl-configuration
    sysctl-configuration?
    (sysctl   sysctl-configuration-sysctl    ; path of the 'sysctl'
    command
              (default (file-append procps "/sbin/sysctl")))
    (settings sysctl-configuration-settings  ; alist of string pairs
              (default %default-sysctl-settings)))

;; ends- gnu/services/sysctl.scm

And sysctl-service-type it self is added to the
%base-services. Since sysctl-configuration-settings function to
access settings field of sysctl-configuration instance is not
exported, I have to do the following in my configuration:

(define nomad-gx1-os
    (operating-system
      (inherit my-base-nomad-os) ;; important line-#1
      ...
      (services
        (modify-services my-base-nomad-services
          (sysctl-service-type config =>
            (inherit config)
            (settings
              (append
                %default-sysctl-settings ;; from
                gnu/services/sysctl.scm
                '(("fs.inotify.max_user_watches" . "524288")
                  ("fs.inotify.max_user_instances" . "16384")
                  ("fs.inotify.max_queued_events" . "65536")))))))))

This is fine, until I extend sysctl-service-type in
my-base-nomad-os. Then I have to export
my-base-nomad-sysctl-settings and join them with
%default-sysctl-settings and extra settings for
nomad-gx1-os. While it is bearable for one or two levels of
inheritance, it becomes hard to keep track for more levels and/or
many hosts.

If sysctl-configuration-settings would be exported,
then my configuration would become simplier:

(services
    (modify-services my-base-nomad-services
      (sysctl-service-type config =>
        (inherit config)
        (settings
          (append
             (sysctl-configuration-settings config) ;; now I can't
             do this
             '(("fs.inotify.max_user_watches" . "524288")
               ("fs.inotify.max_user_instances" . "16384")
               ("fs.inotify.max_queued_events" . "65536")))))))))

In this case, if Guix documentation will include
sysctl-configuration-settings, then most likely people won't
forget use %default-sysctl-settings, and it is still possible to
override them if one desires not to use protected symlinks and
hardlinks.

-- 
muradm

bug#47323: [PATCH] services: export sysctl-configuration record field accessors

[Ludovic Courtès]

Hi,

muradm <mail@muradm.net> skribis:

>>From 0928d70c1cd5a98efd7671c05b38757400941790 Mon Sep 17 00:00:00 2001
> From: muradm <mail@muradm.net>
> Date: Mon, 22 Mar 2021 19:09:48 +0300
> Subject: [PATCH] services: export sysctl-configuration record field accessors
>
> * gnu/services/sysctl.scm (sysctl-configuration-sysctl): new public function
> * gnu/services/sysctl.scm (sysctl-configuration-settings): new public function
>
> Signed-off-by: muradm <mail@muradm.net>

I tweaked the commit log and applied.

> As per discussion with Leo on IRC #guix in relation to #47013 and
> #47323.
>
> There is a need to have important sysctl settings
> fs.protected_hardlinks and fs.protected_symlinks for all
> installations of Guix in the world unless explicitly stated
> otherwise. Currently in Linux kernel they are unset by default. It
> is also stated that other distributions do the same.
>
> In perfect world I would go for Solution 1 below, as it is most
> effectful, and clean.
>
> Solution 1: From this statement, it seems that the first resort
> whould be Linux kernel it self. If it would be possible to
> configure them with Kconfig, that would be best place. As of my
> brief look at linux/fs, they are not configurable, but may be I
> miss somthing. Any way preferred solution would be just compile
> kernel with protected hardlinks and symlinks set to 1. Since other
> distributions do the same, it could be reasonable to expose these
> two settings via Kconfig, and solve it there.
> - pros: great for the world
> - cons: have to do enhancement in mainline Linux
>
> Solution 2: If it is not possible to have these two settings in
> kernel as per Solution 1, Guix may maintain a patch to kernel that
> would do this.
> - pros: no need to enhance mainline Linux
> - cons: will impact users who do use Guix and compile Linux kernel
>    them selves
>
> Solution 3: Handle in Guix configuration. Everything below related
> to solution 3 and current issue #47323.
>
> Currently it is set as folowing:
>
> ;; gnu/services/sysctl.scm
> (define-module ....
>    #:export (....
>                    %default-sysctl-settings)
>
> (define %default-sysctl-settings
>    ;; Default kernel parameters enabled with sysctl.
>    '(("fs.protected_hardlinks" . "1")
>      ("fs.protected_symlinks" . "1")))
>
> (define-record-type* <sysctl-configuration>
>    sysctl-configuration make-sysctl-configuration
>    sysctl-configuration?
>    (sysctl   sysctl-configuration-sysctl    ; path of the 'sysctl'
>    command
>              (default (file-append procps "/sbin/sysctl")))
>    (settings sysctl-configuration-settings  ; alist of string pairs
>              (default %default-sysctl-settings)))
>
> ;; ends- gnu/services/sysctl.scm
>
> And sysctl-service-type it self is added to the
> %base-services. Since sysctl-configuration-settings function to
> access settings field of sysctl-configuration instance is not
> exported, I have to do the following in my configuration:
>
> (define nomad-gx1-os
>    (operating-system
>      (inherit my-base-nomad-os) ;; important line-#1
>      ...
>      (services
>        (modify-services my-base-nomad-services
>          (sysctl-service-type config =>
>            (inherit config)
>            (settings
>              (append
>                %default-sysctl-settings ;; from
>                gnu/services/sysctl.scm
>                '(("fs.inotify.max_user_watches" . "524288")
>                  ("fs.inotify.max_user_instances" . "16384")
>                  ("fs.inotify.max_queued_events" . "65536")))))))))
>
> This is fine, until I extend sysctl-service-type in
> my-base-nomad-os. Then I have to export
> my-base-nomad-sysctl-settings and join them with
> %default-sysctl-settings and extra settings for
> nomad-gx1-os. While it is bearable for one or two levels of
> inheritance, it becomes hard to keep track for more levels and/or
> many hosts.
>
> If sysctl-configuration-settings would be exported,
> then my configuration would become simplier:
>
> (services
>    (modify-services my-base-nomad-services
>      (sysctl-service-type config =>
>        (inherit config)
>        (settings
>          (append
>             (sysctl-configuration-settings config) ;; now I can't
>             do this
>             '(("fs.inotify.max_user_watches" . "524288")
>               ("fs.inotify.max_user_instances" . "16384")
>               ("fs.inotify.max_queued_events" . "65536")))))))))
>
> In this case, if Guix documentation will include
> sysctl-configuration-settings, then most likely people won't
> forget use %default-sysctl-settings, and it is still possible to
> override them if one desires not to use protected symlinks and
> hardlinks.

Indeed, this is a discussion Leo Famulari and I had while preparing the
patch for this security issue.  Like you write, there are different
tradeoffs, and this solution is one possibility that looked reasonable.

Thanks!

Ludo’.