GNU bug report logs - #62298
[PATCH 0/8] Extensible define-configuration & mpd/mympd service fixes

Previous Next

Full log

Message #41 received at 62298 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: ludo <at> gnu.org, 62298 <at> debbugs.gnu.org, maxim.cournoyer <at> gmail.com
Subject: Re: [PATCH 7/8] services: mpd: Use user-account (resp. user-group)
 for user (resp. group) fields.
Date: Tue, 21 Mar 2023 02:10:05 +0000

Hi Liliana,

On 2023-03-20 19:33, Liliana Marie Prikler wrote:
> Am Montag, dem 20.03.2023 um 17:07 +0000 schrieb Bruno Victal:
>> Deprecate using strings for these fields and prefer user-account
>> (resp. user-group) instead to avoid duplication within account-
>> service-type.  If a string value is encountered, it is ignored and a
>> predefined variable is used instead. This is essentially a rollback
>> to how it used to be before
>> '5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1'.
> I already wrote this in private in IRC, but falling back to a constant
> when a string value is given is very silly.  IIUC the reason to do so
> is because you would need to sanitize the user account and group at the
> same time so that the former has access to the latter.
> 
> 
> I think it's possible to use one of the following approaches to get a
> better result:
> 1. In (mpd-accounts), check if the user group equals the group name and
> raise a warning (or error) if not.
> 2. Use a special unique symbol, e.g. (make-symbol "%mpd-group") to
> denote the value to be lazily inserted by the serializer.

After giving some thought to this, IMO I think it's simply uninteresting for these
fields to accept string values.
Prior to the 5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1 refactor, the names were hardcoded
and the refactor allowed them to be changed.


But with the observed interactions in [1], it became clear that:

1. This service isn't meant to be used with a non-interactive user, a home shepherd service
should be used instead. (granted the bug isn't due to this, it merely surfaced up here.
Setting group to “nobody” would have also caused the same kind of problem.)

2. Accepting strings was deemed undesirable since it then results in a “race” between
user-accounts from operating-system and account-service-type extensions (or even among the extensions),
with the winner getting to “build” the account as it sees fit.


In the end, the daemon requires a user-account + user-group to work (unless you're in the
mood for running it as root?), so something still has to be made.
The choices that I think make sense for user/group fields are:

1. Leave it with a default value. The service creates what it needs.

2. Give it a user-account/group. This is considered an _advanced_ use-case and it's highly likely (though not mandatory)
to be used within a let-form. You use this when you need fine control over the account properties.


Accepting strings is simply uninteresting (or bad) since:

* A string doesn't uniquely identify an account and results in buggy behavior [1].

* Since the string values are only used to set the 'name' of the user-account/group records, which
is specific to the service as they're created within the mpd-account procedure, it's simply setting
a vanity value. It's as interesting as allowing the filename in (mixed-text-file "mpd.conf" ...)
to be set by the user.

* It's clearly unsanitizable since it would require accessing other fields.
Monkeying within (mpd-accounts) with special symbols just obfuscates the code with
no clear benefits to be had, in addition to defeating the point of having a sanitizer in the first place.


I fail to see the utility in ever accepting strings here for what amounts to a vanity change in 'ps aux'
output. Maybe my imagination is failing here but I really don't think it's worth the trouble to support strings here.
This vanity change is still achievable with some extra-typing if someone really wishes it.

As such, I think the best course of action is to simply use user-account/group record objects from now on,
with string usages deprecated and properly communicated to the user that they're not supported and are ignored. (a non API breaking change)


[1]: <https://issues.guix.gnu.org/61570>



Cheers,
Bruno

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.