Subject: [bug#75810] [PATCH v5 00/14] Rootless guix-daemon

From: Ludovic Courtès <[email-masked]>
Date: Tue, 18 Mar 2025 10:34:49 +0100

Hi!

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

Reepca Russelstein <[email-masked]> skribis:

> Ludovic Courtès <[email-masked]> 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’.