[Openstack-devel] Bug#732387: Bug#732387: Adds user ceilometer to group nova and libvirt on every upgrade

[Thomas Goirand]

On 12/18/2013 05:42 PM, Gaudenz Steinlin wrote:
> Thomas Goirand <zigo at debian.org> writes:
> 
>> Hi Gaudenz,
>>
>> First of all, thanks for taking the time to send bug reports. I'm very
>> happy to see that OpenStack gets more and more traction within the
>> Debian community.
>>
>> On 12/17/2013 10:20 PM, Gaudenz Steinlin wrote:
>>> But this is actually not respecting local admin configurations. If an
>>> administrator decides (for whatever reason) that the ceilometer user
>>> should not be part of these groups
>>
>> Well, if an admin decides this, then Ceilometer will not work anymore.
>> That admin might as well just remove ceilometer-common...
> 
> Well, it won't work anymore without configuration changes to other
> packages sure. But it's very well possible to configure libvirt to use
> another group too.

We're not talking about libvirt, but about Nova here. Sure, it must be
possible to do whatever, run everything under the nobody user/group, or
even as root. Though that's not what is intended by the upstream
authors, and that's not what the package will support. The package is
going to support what is needed for its operation.

>>> this decision is reverted on every
>>> package upgrade. This violates Debian Policy section 10.7.3 which states
>>> that local modifications must be preserved.
>>>
>>> The user should only be added to these groups on first install. On
>>> upgrades the group membership should not be changed.
>>
>> The section 10.7.3 that you mention is under the chapter "Configuration
>> files" and has nothing to do with managing Unix user and groups.
> 
> As these changes ultimately end up in /etc/group I would say that this
> policy section applies. It does not matter if you change the file
> directly or with a tool. But I agree that the question if users and
> group are configuration is a bit a grey area.

I don't agree. It's not because the groups are stored in flat files that
it applies to a chapter like 10.7. They well could be stored in a db for
example (which is possible with nss-mysql).

Plus this chapter talks about CONFFILES and configuration files owned by
a package, and how to handle them. If we push the reasoning as much as
you do, then that would mean that a package CANNOT add a username in the
system, because that would mean touching a configuration file that isn't
owned by the package. Clearly, that's not what the policy intends to
say. I don't agree at all that we're in a grey area on what the chapter
is about (eg: configuration files and not unix user/group management).

> As you can't lock the user database anyway this won't prevent
> (mis)configuration either.
> 
> IMO you should at least add a check like the following before adding the
> group:
> id -Gn ceilometer | grep -qE "(^| )nova( |$)" || adduser ceilometer nova
> id -Gn ceilometer | grep -qE "(^| )libvirt( |$)" || adduser ceilometer libvirt
> 
> This is only a quick  example to show the intention. Feel free to
> improve it.

If you wish to provide a patch for the above, I'll apply it. Though as
IMO, this is hardly a cosmetic issue, I wont spend the time to work on
it. Do you think I can just apply what's above as-is? If so, I'll do it
just right after you confirm it: it looks ok-ish to me, even though I
often prefer to use getent rather than id (not sure why though...;)).

>> Also, removing the unconditionality of the unix user/group management
>> would make this particular maintainer script not idempotent anymore,
>> which is a path that is very dangerous to take.
> 
> I don't understand what you mean here. Indempotence means that you can
> call the postinst several times with the SAME arguments and this will
> not have any undesired side effects (like creating something on every
> call that is only needed once). I don't see how this is affected by the
> proposal to only add the user to these groups on fresh installs as the
> arguments to the postinst differ from fresh installs to upgrades. 
> 
> If the postinst script fails, it will be called with the same arguments
> on retries, so a condition checking for a fresh install will still
> execute.

My experience as a Debian developer is that trying to have different
behaviors on install or upgrade leads to all sorts of problem. I would
prefer to avoid this if possible, and I believe that in this case, it's
possible to not do it.

>> Also, I fail to see where else in the policy manual it is written that a
>> package cannot impose a particular user to be inside a specific group.
>> Please point to the correct policy manual section if you still think
>> this is wrong.
> 
> I agree that we are lacking clear policy on user management in
> maintainer scripts. This is a problem as every package does his own
> thing. But nothing we can fix in ceilometer.

I very much agree with this. Like for example, when a package is
removed, there's no clear policy on what we should do with the system
users that have added by the package (I took the decision a long time
ago to just leave them on the system to avoid UID/GID re-usability which
can be a security issue).

>> Whatever your reply will be, I don't think this bug deserves a severity
>> "serious" (to quote the release team: not all policy violation are RC
>> bugs), so I'm downgrading the severity.
> 
> I'm fine with that. I don't think the severity matters much as long as
> bugs get fixed.
> 
> Gaudenz

No worries, I'm all for fixing bugs too, even those with lower severity.

Thomas