GNU bug report logs - #75810
[PATCH 0/6] Rootless guix-daemon

Previous Next

Full log

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Reepca Russelstein <reepca <at> russelstein.xyz>
Cc: 75810 <at> debbugs.gnu.org
Subject: Re: [bug#75810] [PATCH v5 00/14] Rootless guix-daemon
Date: Tue, 18 Mar 2025 10:34:49 +0100

Hi!

(Oops, forgot to hit “send” on this one…)

Reepca Russelstein <reepca <at> russelstein.xyz> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>>   • In ‘guix-daemon.service.in’, set
>>     ‘GUIX_DATABASE_DIRECTORY=/var/guix’ for forward compatibility
>>     (I’m thinking of eventually changing the default database
>>     location when not running as root).
>
> Did you intend GUIX_STATE_DIRECTORY here, or GUIX_DATABASE_DIRECTORY in
> the patch?  GUIX_STATE_DIRECTORY is what's in the patch.

Sorry, I meant GUIX_STATE_DIRECTORY.

> Minor note: this could be implemented solely in terms of close_range, by
> sorting the exceptions and iterating over the gaps between them.  It's
> fine as it is though.

Yes.  In practice ‘exceptions’ is always empty in this code, so we
should eventually remove the argument.

> (in patch 06/14, in doc/guix.texi)
>
> In the third sentence: s/requires the user/requires the use/

Noted.

>> +When using this option, you only need to create one user account, and
>> +@command{guix-daemon} will run with the authority of that account:

[...]

> It may be somewhat confusing to the reader looking at this page in
> isolation whether they still need to create this account after running
> the installation script, since it was mentioned immediately before this.

Note that this doesn’t change the situation compared to ‘master’: it
describes what guix-daemon expects, without regard for the installation
method.  I’ll see how this can be clarified though.

>> +@quotation Warning
>> +Using this option is not recommended since it allows build processes to
>> +gain access to undeclared dependencies, to interfere with one another,
>> +and more generally to do anything that can be done with the authority of
>> +the daemon---which includes at least the ability to tamper with any file
>> +in the store!
>> +
>> +You may find it necessary, though, when support for Linux unprivileged
>> +user namespaces is missing (@pxref{Build Environment Setup}).  Use at
>> +your own risk!
>> +@end quotation
>
> Note that the "do anything that can be done with the authority of the
> daemon" part is only true in the case where the builder and the daemon
> run under the same user.  For example, on Hurd, we use --disable-chroot
> but still use the separate builder users.

Oh right.

[...]

> Perhaps this would be a good place to recommend setting the permissions
> of LOCALSTATEDIR/guix/daemon-socket to allow only trusted users to
> access it?

I don’t think so; we’re really just describing ‘--disable-chroot’ here.

>> +    /* Pipe to notify readiness to the child process when using unprivileged
>> +       user namespaces.  */
>> +    Pipe readiness;
>> +
>>      /* Check that the derivation outputs all exist and register them
>>         as valid. */
>>      void registerOutputs();
>
> (in nix/libstore/build.cc)
>
> This is inside of DerivationGoal.  We found out while hunting that
> deadlock bug that it doesn't always get disposed of in a timely fashion.
> So we probably shouldn't rely on the AutoCloseFD type of
> readiness.readSide and readiness.writeSide to close them in a timely
> fashion, opting instead to explicitly close the end we use immediately
> after we're done with it.

Yes, agreed.

> On a slightly-related note, while investigating what we currently do for
> the other file-descriptor-bearing objects in DerivationGoal, I noticed
> that we appear to never explicitly close builderOut.readSide in the
> child (we do implicitly close it eventually with closeMostFDs).
> Probably not a problem in practice, since the parent never writes to the
> pipe, and if the parent process gets suddenly wiped out, the possibility
> of the child blocking indefinitely is probably the least of our worries,
> but maybe it would be good to close it in commonChildInit.

Agreed.

>> +    if [ ! -f /proc/sys/kernel/unprivileged_userns_clone ] \
>> +       || [ "$(cat /proc/sys/kernel/unprivileged_userns_clone)" -eq 1 ]; then
>> +	extra_options=""
>> +    else
>> +	extra_options="--disable-chroot"

[...]

> I think this test for /proc/sys/kernel/unprivileged_userns_clone will
> only work on linux, no?  On Hurd it will see that the file doesn't exist
> and assume that means it shouldn't pass --disable-chroot.

Oops yes.

> Maybe also require that something like /proc/self/ns/user exists?  I see
> in (gnu build linux-container) this is what we do for
> 'user-namespace-supported?'.  Perhaps
> 'unprivileged-user-namespace-supported?' should also be updated to only
> return true if user namespaces are supported at all?  Otherwise many of
> the test skips in this patch are going to do the opposite of what they
> should on Hurd.

OK.

> Also, perhaps the "Launch the daemon without chroot support because is
> ..."  comment should be updated (also s/is/it/ there) or removed.

OK.

>> +(unless (unprivileged-user-namespace-supported?)
>> +  (test-skip 1))
>> +(test-assert "build root cannot be made world-readable"

[...]

> It may be good to forego actually creating the setuid guile here, since
> it's not part of what's actually being tested, just a looming threat to
> motivate what's being tested.  In the event that this test fails
> someday, let's be kind to our testers and leave them with only a
> world-readable setuid empty file or file containing a frowny face or
> something.
>
> ... actually, on closer inspection, won't this test never fail?  The
> gexp doesn't actually create #$output, so the build will always fail,
> and the store-protocol-error will always be signaled, no?

Oops!  Done.

> (in patch 12/14, in etc/guix-daemon.service.in)
>
> s/cran create/can create/

Done.

> Also, I'm curious how this interacts with the changes to the installer
> script.  What happens if the installer detects that the rootless daemon
> isn't supported, but the guix that it's installing only has this version
> of guix-daemon.service?  I haven't checked, but if the answer is "the
> service is broken", perhaps we should have two variants of the service
> file, and sys_enable_guix_daemon decides which one to symlink
> /etc/systemd/system/guix-daemon.service to?

The assumption is indeed that unprivileged user namespaces are
supported.

I have reasons to believe that this assumption holds these days, which
is why I didn’t bother with the fallback you suggest.

>> +can_install_unprivileged_daemon()
>> +{ # Return true if we can install guix-daemon running without privileges.
>> +    [ "$INIT_SYS" = systemd ] && \
>> +	grep -q "User=guix-daemon" \
>> +	     ~root/.config/guix/current/lib/systemd/system/guix-daemon.service \
>> +	&& ([ ! -f /proc/sys/kernel/unprivileged_userns_clone ] \
>> +		|| [ "$(cat /proc/sys/kernel/unprivileged_userns_clone)" -eq 1 ])
>> +}
>
> (in patch 13/14, in etc/guix-install.sh)
>
> I vaguely recall hearing that systemd only supports linux, so it might
> not be strictly necessary to check whether user namespaces are supported
> at all here like it was in test-env.  If we get support for the rootless
> daemon working for the other init systems a check like that may be
> necessary, though.

Right, systemd is Linux-only, but you’re right that this should be
changed like ./test-env for other systems.

> It does indeed look like it's going to be necessary to make some changes
> to make the script able to support installing a newer guix to a
> systemd-using system that doesn't support unprivileged user namespaces.

A system running systemd definitely supports unprivileged user
namespaces; it is still possible to disable it manually, but I’m willing
to assume that it’s uncommon.

I’m sending v6 with these changes.

Ludo’.

Previous Next

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