[bug#72398] services: Add readymedia-service-type.

  Hey Arun,

Thanks for reviewing the patch and for the useful feedback, I really
appreciate it! Please find my comments/answers below. Patch v2 is
attached.

On 2024-08-13, 00:19 +0100, Arun Isaac <arunisaac@systemreboot.net> wrote:
> Could you also suggest some quick way for me to test this service
> without actually having to reconfigure my system?

Good point. There might be more clever ways to go about it, but here's
my testing process.

- Create a folder, e.g. '/tmp/foo', and populate it with at least one
  music file, e.g. '/tmp/foo/foo.mp3'.

- Save this system definition in a file, e.g. '/tmp/config.scm'. Note
  the insecure user credentials.

(use-modules (gnu))
(use-package-modules video)
(use-service-modules desktop upnp)

(operating-system
  (host-name "host")
  (bootloader (bootloader-configuration
               (bootloader grub-bootloader)
               (targets '("/dev/vda"))))
  (file-systems (cons (file-system
                        (device "/dev/vda1")
                        (mount-point "/")
                        (type "ext4"))
                      %base-file-systems))
  (users (cons*
          (user-account (name "user")
                        (group "users")
                        (supplementary-groups '("wheel"))
                        (password (crypt "password" "foo")))
          %base-user-accounts))
  (sudoers-file (plain-file
                 "sudoers"
                 (string-append
                  (plain-file-content %sudoers-specification)
                  "%wheel ALL = NOPASSWD: ALL")))
  (packages (cons* vlc %base-packages))
  (services (cons*
             (service gnome-desktop-service-type)
             (service readymedia-service-type
                      (readymedia-configuration
                       (media-dirs
                        (list
                         (readymedia-media-dir (path "/media/music")
                                               (type "A"))))))
             %desktop-services)))

- From within the Guix repository checkout, once the ReadyMedia service
  patch has been applied, build and launch the VM with:

$(./pre-inst-env guix system vm \
    --share=/tmp/foo=/media/music \
    /tmp/config.scm) -m 2048 -smp 2

- Log in as 'user'. Open a terminal and verify that the ReadyMedia
  service is running with 'sudo herd status'.

- Open VLC and follow these instructions
  https://www.vlchelp.com/access-media-upnp-dlna/ to verify that the
  ReadyMedia service is running and that the 'foo.mp3' file can be
  played.

- Open a browser and verify that the ReadyMedia web page is also
  reachable at 'http://127.0.0.1:8200'.

This should be it, testing-wise.

>> +(define %readymedia-cache-dir "/var/cache/readymedia")
>> +(define %readymedia-log-dir "/var/log/readymedia")
>
> Can we have these two in the <readymedia-configuration> record?

Fixed in v2.

> Nitpick: Just to be consistent with other services, I would indent
> this (and the other fields) like so with the default on the next line:
>
>>   (readymedia readymedia-configuration-readymedia
>>               (default readymedia))

Fixed.

>> +(define (readymedia-configuration->config-file config)

> Could you use mixed-text-file here instead of plain-file? Or, you
> could also try computed-file if that's more succinct.

'mixed-text-file' improves things a bit, see v2. WDYT?

>> +(define (readymedia-shepherd-service config)

> Re-format by putting the first file-system-mapping on the same line as
> the cons*. It's customary to format lisp function calls that way. It
> makes it easier to see what the arguments are.

Fixed.

> Likwise with map. Put the lambda on the same line as the map.

Fixed.

> Looking forward to a v2 patch!

v2 attached. :)

Thanks Arun, let me know what you think. Should you spot anything else
just let me know.

Cheers, F.
  

Hi Fabio,

On 2024-08-19 01:27, Fabio Natali via Guix-patches via wrote:
> +(operating-system
> +  ;; @dots{}
> +  (services
> +   (list
> +    (service readymedia-service-type
> +             (readymedia-configuration
> +              (media-dirs
> +               (list (readymedia-media-dir (path "/media/audio")
> +                                           (type "A"))
> +                     (readymedia-media-dir (path "/media/video")
> +                                           (type "V"))
> +                     (readymedia-media-dir (path "/media/misc"))))))

[…]

> +@item @code{media-dirs} (type: list)
> +The list of media folders to serve content from.  Each item is a
> +@code{readymedia-media-dir}.
> +
> +@item @code{cache-dir} (default: @code{"/var/cache/readymedia"}) (type: string)
> +A folder for ReadyMedia's cache files. If not existing already, the
> +folder will be created as part of the service activation and the
> +ReadyMedia user will be assigned ownership.
> +
> +@item @code{log-dir} (default: @code{"/var/log/readymedia"}) (type: string)
> +A folder for ReadyMedia's log files. If not existing already, the
> +folder will be created as part of the service activation and the
> +ReadyMedia user will be assigned ownership.

Expand these to media-directories, cache-directory, etc.

> +@item @code{port} (default: @code{#f}) (type: maybe-integer)
> +A custom port that the service will be listening on.
> +
> +@item @code{extra-config} (default: @code{'()}) (type: list-of-strings)
> +A list of further options, to be passed as key-value strings as
> +accepted by ReadyMedia.

Do you have an example on this?
Given the description perhaps an alist would work better here.

> +
> +@end table
> +
> +@end deftp
> +
> +@c %end of fragment
> +
> +@c %start of fragment
> +
> +@deftp {Data Type} readymedia-media-dir
> +A @code{media-dirs} entry includes a @code{path} and, optionally, a
> +media type string.

Likewise, expand to readymedia-media-directory.

> +
> +@table @asis
> +@item @code{path} (type: string)
> +The media folder location.
> +
> +@item @code{type} (default: @code{""}) (type: string)
> +Valid media types are @code{"A"} for audio, @code{"P"} for pictures,
> +@code{"V"} for video, and a combination of those individual letters
> +for mixed types.  An empty string means no type specified.

I'd use a list of symbols (or enum) here.

> +(define %readymedia-user-account "readymedia")
> +(define %readymedia-user-group "readymedia")

I think it would be better to expose this in the
readymedia-configuration record-type and have it be oriented
around user-account and user-group record-types, i.e.

--8<---------------cut here---------------start------------->8---
(define %readymedia-user-group
  (user-group
    (name "readymedia")
    (system? #t)))

(define %readymedia-user-account
  (user-account
    (name "readymedia")
    (group "readymedia")
    (system? #t)
    (comment "ReadyMedia/MiniDLNA daemon user")
    (home-directory "/var/empty")
    (shell (file-append shadow "/sbin/nologin"))))

(define-record-type* <readymedia-configuration> …
  …
  (user readymedia-configuration-user
        (default %readymedia-user-account))
  (group readymedia-configuration-group
        (default %readymedia-user-group))))

(define (readymedia-account-service config)
  (match-record config <readymedia-configuration> (group user)
    (list group user)))

;; … and adjust service-type extension accordingly
--8<---------------cut here---------------end--------------->8---

This way you can allow for users to fine-tune the account permissions, 
groups & co. used by readymedia.

> +(define (readymedia-activation config)
> +  "Set up directories for ReadyMedia/MiniDLNA."
> +  (let ((cache-dir (readymedia-configuration-cache-dir config))
> +        (log-dir (readymedia-configuration-log-dir config)))
> +    #~(begin
> +        (use-modules (guix build utils))
> +        (define %user (getpw #$%readymedia-user-account))
> +        (mkdir-p #$cache-dir)
> +        (chown #$cache-dir (passwd:uid %user) (passwd:gid %user))
> +        (mkdir-p #$log-dir)
> +        (chown #$log-dir (passwd:uid %user) (passwd:gid %user)))))

I'd avoid using activation-service-type since it doesn't account for
shepherd dependencies (which implies file-system mounts), consequence
being that this service will be broken if any of these directories
happen to be located outside of the root filesystem.
(My advice is to avoid using activation-service-type unless you're
sure of how the chain of action in guix+shepherd goes)

Instead, do these within the start action of shepherd-service,
see the "prologue"/(before make-forkexec-constructor is called) of
mympd-service-type in gnu/services/audio.scm for an idea [1].

[1]: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/services/audio.scm?id=00245fdcd4909d7e6b20fe88f5d089717115adc1#n919
  

Hi Bruno,

Thanks for providing feedback on this and thanks for the help provided
on IRC. I've gone through your comments and did my best to address
them. See my replies inline below.

On 2024-08-20, 03:14 +0100, Bruno Victal <mirai@makinata.eu> wrote:
>> +@item @code{media-dirs} (type: list)
>> +The list of media folders to serve content from.  Each item is a
>> +@code{readymedia-media-dir}.
>> +
>> +@item @code{cache-dir} (default: @code{"/var/cache/readymedia"}) (type: string)
>> +A folder for ReadyMedia's cache files. If not existing already, the
>> +folder will be created as part of the service activation and the
>> +ReadyMedia user will be assigned ownership.
>> +
>> +@item @code{log-dir} (default: @code{"/var/log/readymedia"}) (type: string)
>> +A folder for ReadyMedia's log files. If not existing already, the
>> +folder will be created as part of the service activation and the
>> +ReadyMedia user will be assigned ownership.
>
> Expand these to media-directories, cache-directory, etc.

Good point, now fixed.

>> +@item @code{extra-config} (default: @code{'()}) (type: list-of-strings)
>> +A list of further options, to be passed as key-value strings as
>> +accepted by ReadyMedia.
>
> Do you have an example on this?
> Given the description perhaps an alist would work better here.

True, great point. That's now an alist. Example added too.

>> +@deftp {Data Type} readymedia-media-dir
>> +A @code{media-dirs} entry includes a @code{path} and, optionally, a
>> +media type string.
>
> Likewise, expand to readymedia-media-directory.

Fixed.

>> +@item @code{type} (default: @code{""}) (type: string)
>> +Valid media types are @code{"A"} for audio, @code{"P"} for pictures,
>> +@code{"V"} for video, and a combination of those individual letters
>> +for mixed types.  An empty string means no type specified.
>
> I'd use a list of symbols (or enum) here.

Fixed, switched to symbols.

>> +(define %readymedia-user-account "readymedia")
>> +(define %readymedia-user-group "readymedia")
>
> I think it would be better to expose this in the
> readymedia-configuration record-type and have it be oriented around
> user-account and user-group record-types, i.e.
[...]
> This way you can allow for users to fine-tune the account permissions,
> groups & co. used by readymedia.

Fixed, although I'm not sure I'm 100% on board with this.

I'm not completely sure but I have the feeling that a configurable
ReadyMedia user might theoretically weaken the POLA, e.g. if the user
chose their own user for this service.

Following up on a related conversation we started on IRC, I suppose we
should either go all in with flexibility (i.e. allow the user to switch
off the least-authority-wrapper and set the service user) or adopt a
slightly more rigid approach (mandated POLA and fixed user).

I think I might have a slight preference for the latter, prioritising
compartmentalisation over flexibility - but I'm keen to know what you,
Arun, and all other Guixers may think about this.

I'm glad to send a new version in case, where I switch back to a
mandated, non-configurable 'readymedia' user.

>> +(define (readymedia-activation config)
>> +  "Set up directories for ReadyMedia/MiniDLNA."
[...]
> I'd avoid using activation-service-type since it doesn't account for
> shepherd dependencies (which implies file-system mounts), consequence
> being that this service will be broken if any of these directories
> happen to be located outside of the root filesystem.
> (My advice is to avoid using activation-service-type unless you're
> sure of how the chain of action in guix+shepherd goes)

Ha, ok, I'd have never thought of this! With a bit of a
don't-know-what-i'm-doing feeling, I might have fixed this too. :)

Thanks to you and Arun for all the helpful feedback!

I hope v3 is in a better shape now (to follow shortly).

Thanks, cheers, Fabio.
  

> I'd avoid using activation-service-type since it doesn't account for
> shepherd dependencies (which implies file-system mounts), consequence
> being that this service will be broken if any of these directories
> happen to be located outside of the root filesystem.  (My advice is to
> avoid using activation-service-type unless you're sure of how the
> chain of action in guix+shepherd goes)

This is a good point. I hadn't thought of this.

> Instead, do these within the start action of shepherd-service,
> see the "prologue"/(before make-forkexec-constructor is called) of
> mympd-service-type in gnu/services/audio.scm for an idea [1].

And, a clever solution too. Today I learnt!
  

>>> +(define %readymedia-user-account "readymedia")
>>> +(define %readymedia-user-group "readymedia")
>>
>> I think it would be better to expose this in the
>> readymedia-configuration record-type and have it be oriented around
>> user-account and user-group record-types, i.e.
>
> Fixed, although I'm not sure I'm 100% on board with this.
>
> I'm not completely sure but I have the feeling that a configurable
> ReadyMedia user might theoretically weaken the POLA, e.g. if the user
> chose their own user for this service.
>
> Following up on a related conversation we started on IRC, I suppose we
> should either go all in with flexibility (i.e. allow the user to switch
> off the least-authority-wrapper and set the service user) or adopt a
> slightly more rigid approach (mandated POLA and fixed user).
>
> I think I might have a slight preference for the latter, prioritising
> compartmentalisation over flexibility - but I'm keen to know what you,
> Arun, and all other Guixers may think about this.

I am with Fabio on this. Many (almost all, maybe?) services use a fixed
user account that cannot be configured. And, that's ok.

I don't think we should make the least authority wrapper optional
either. Making it optional would be too much complexity for little
benefit. The goal of Guix services isn't to provide total
configurability, but rather to be slightly opinionated so as to nudge
users in the right direction.

Let me know if I'm missing something important.

Cheers!
  

Hi Arun,

On 2024-08-23 00:28, Arun Isaac wrote:
> 
>>>> +(define %readymedia-user-account "readymedia")
>>>> +(define %readymedia-user-group "readymedia")
>>>
>>> I think it would be better to expose this in the
>>> readymedia-configuration record-type and have it be oriented around
>>> user-account and user-group record-types, i.e.
>>
>> Fixed, although I'm not sure I'm 100% on board with this.
>>
>> I'm not completely sure but I have the feeling that a configurable
>> ReadyMedia user might theoretically weaken the POLA, e.g. if the user
>> chose their own user for this service.
>>
>> Following up on a related conversation we started on IRC, I suppose we
>> should either go all in with flexibility (i.e. allow the user to switch
>> off the least-authority-wrapper and set the service user) or adopt a
>> slightly more rigid approach (mandated POLA and fixed user).
>>
>> I think I might have a slight preference for the latter, prioritising
>> compartmentalisation over flexibility - but I'm keen to know what you,
>> Arun, and all other Guixers may think about this.
> 
> I am with Fabio on this. Many (almost all, maybe?) services use a fixed
> user account that cannot be configured. And, that's ok.

Without delving into the quantifying, there's at least a few of them
that offer this feature. (in my experience, I've had to rely on this for a
few services already so it's not merely a theoretical concern)

Should you ever need to "tweak" a fixed user-account service
you're going to end up with something like [1] (beginning from line 21,
rationale given at line 39). Not exactly desirable and although the
example above pertains to nginx + cgit if I'm not mistaken, a similar
situation arises in the following (fictional) setup:

/media/NFS/my-media/…             (owner: foo, group: bigmedia, #o750)
/media/jumbodisk/my-media/…       (owner: bar, group: bigmedia, #o750)
/media/something-else/library/…   (owner: baz, group: bigmedia, #o750)

and wholesame chown'ing them to "readymedia" wouldn't make sense/be
a good idea (say, each of the directories is under control by a
downloader/synchronizing daemon with it's own user-account).

> I don't think we should make the least authority wrapper optional
> either. Making it optional would be too much complexity for little
> benefit. (…)

I don't think so, it amounts to:
• a boolean field named least-authority-wrapped? in the configuration record-type
• an if statement, e.g. (if least-authority-wrapped? (least-authority-wrapper …) readymedia)

As for the reason of this, consider a setup where the media directories
contain symlinks to directories outside of it. It can be infeasible to
duplicate the files or "just move them then", in those cases an escape
hatch makes sense to be. It's not as secure as the least-authority wrapped
 one but that's a compromise opted in by the user.

> (…) The goal of Guix services isn't to provide total
> configurability, but rather to be slightly opinionated so as to nudge
> users in the right direction.

I'm not against this idea, just pointing out that it's overly rigid right
now and that users with a non "uniform" setup will simply resort to
harder to understand manipulations like [1] or wholesale duplicate
gnu/services/upnp.scm and tweak it themselves.

Let me know if there's anything I missed,


[1]: <https://git.dthompson.us/guix-config/tree/dthompson/machines/takemi.scm?id=b14a123560dbfc4b7b9ceedf12cc5730558e2418#n39>
  

Hi Bruno,

>> I am with Fabio on this. Many (almost all, maybe?) services use a fixed
>> user account that cannot be configured. And, that's ok.
>
> Without delving into the quantifying, there's at least a few of them
> that offer this feature. (in my experience, I've had to rely on this for a
> few services already so it's not merely a theoretical concern)
>
> Should you ever need to "tweak" a fixed user-account service
> you're going to end up with something like [1] (beginning from line 21,
> rationale given at line 39). Not exactly desirable and although the
> example above pertains to nginx + cgit if I'm not mistaken, a similar
> situation arises in the following (fictional) setup:
>
> /media/NFS/my-media/…             (owner: foo, group: bigmedia, #o750)
> /media/jumbodisk/my-media/…       (owner: bar, group: bigmedia, #o750)
> /media/something-else/library/…   (owner: baz, group: bigmedia, #o750)
>
> and wholesame chown'ing them to "readymedia" wouldn't make sense/be
> a good idea (say, each of the directories is under control by a
> downloader/synchronizing daemon with it's own user-account).

You're right about this problem. It's been discussed here as well:
https://issues.guix.gnu.org/67288 But, like I mention there, I am
worried that adding configurable user and group fields to every service
isn't very composable. Ideally, we'd want to have a separate
"add-user-to-group" service that can modify configured users to have
more groups. Such a solution may be more composable. WDYT?

>> I don't think we should make the least authority wrapper optional
>> either. Making it optional would be too much complexity for little
>> benefit. (…)
>
> I don't think so, it amounts to:
> • a boolean field named least-authority-wrapped? in the configuration record-type
> • an if statement, e.g. (if least-authority-wrapped? (least-authority-wrapper …) readymedia)
>
> As for the reason of this, consider a setup where the media directories
> contain symlinks to directories outside of it. It can be infeasible to
> duplicate the files or "just move them then", in those cases an escape
> hatch makes sense to be. It's not as secure as the least-authority wrapped
>  one but that's a compromise opted in by the user.

Another solution could be to add a "mappings" field that specifies
additional directories to map into the container. I do this in some
services in
guix-forge. https://guix-forge.systemreboot.net/manual/dev/en/#item27237
It's probably not the most elegant solution, but it works without
completely disabling the container. Would this be acceptable to you?

Cheers, and happy hacking!
Arun
  

Hi Arun, Bruno,

On 2024-08-28, 23:51 +0100, Arun Isaac <arunisaac@systemreboot.net> wrote:
> You're right about this problem. It's been discussed here as well:
> https://issues.guix.gnu.org/67288 But, like I mention there, I am
> worried that adding configurable user and group fields to every
> service isn't very composable. Ideally, we'd want to have a separate
> "add-user-to-group" service that can modify configured users to have
> more groups. Such a solution may be more composable. WDYT?

As far as I understand, a separate `add-user-to-group' service seems
like a good general way of addressing this - although outside the scope
of this patch. As a stopgap solution, I'd be glad to add a
`supplementary-groups' field a la #67288 - do you think that might work
in this context? Or we could keep the service as it is (v5) until a
`add-user-to-group' service is in place?

> Another solution could be to add a "mappings" field that specifies
> additional directories to map into the container. I do this in some
> services in
> guix-forge. https://guix-forge.systemreboot.net/manual/dev/en/#item27237

Hm, I'm sure I'm missing something here, but isn't this what the patch
does already with the "media-directories" field?

--8<---------------cut here---------------start------------->8---
(readymedia (least-authority-wrapper
             (file-append
              (readymedia-configuration-readymedia config)
              "/sbin/minidlnad")
             #:name "minidlna"
             #:mappings
             (cons*
                    ...
                    (map
                     (lambda (e)
                       (file-system-mapping
                        (source (readymedia-media-directory-path e))
                        (target source)
                        (writable? #f)))
                     media-directories))
             #:namespaces (delq 'net %namespaces))))
             ...
--8<---------------cut here---------------end--------------->8---

Thanks, cheers, F.