GNU bug report logs - #18994
Daemon does not preserve supplementary groups of build users

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 18994 in the body.
You can then email your comments to 18994 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: bug-guix <at> gnu.org
Subject: Daemon does not preserve supplementary groups of build users
Date: Sat, 08 Nov 2014 15:01:43 +0100

Currently, the build environment made by the daemon does not preserve
supplementary groups of the build users.

Thus, even though the standalone Guix system sets /dev/kvm 660, owned by
root:kvm, and adds the build users to the kvm group, build users are
unable to access it.

This can be see with:

  (gexp->derivation "foo"
    #~(begin (mkdir #$output)(pk (stat:gid (stat "/dev/kvm")) (getgroups))))

The workaround for now is to make /dev/kvm 666.

Ludo’.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: 18994 <at> debbugs.gnu.org, Eelco Dolstra <eelco.dolstra <at> logicblox.com>
Cc: nix-dev <at> lists.science.uu.nl
Subject: [PATCH] Preserve supplementary groups of build users
Date: Wed, 01 Jul 2015 11:12:51 +0200

[Message part 1 (text/plain, inline)]

ludo <at> gnu.org (Ludovic Courtès) skribis:

> Currently, the build environment made by the daemon does not preserve
> supplementary groups of the build users.
>
> Thus, even though the standalone Guix system sets /dev/kvm 660, owned by
> root:kvm, and adds the build users to the kvm group, build users are
> unable to access it.

The following patch is an attempt to address this bug (see
<http://bugs.gnu.org/18994>) by preserving the supplementary groups of
build users in the build environment.

In practice, I would expect that supplementary groups would contain only
one or two groups: the build users group, and possibly the “kvm” group.

WDYT?

Thanks,
Ludo’.

[Message part 2 (text/x-patch, inline)]

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 85a818b..4644810 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -447,6 +447,7 @@ private:
     string user;
     uid_t uid;
     gid_t gid;
+    std::vector<gid_t> supplementaryGIDs;
 
 public:
     UserLock();
@@ -460,6 +461,7 @@ public:
     string getUser() { return user; }
     uid_t getUID() { return uid; }
     uid_t getGID() { return gid; }
+    std::vector<gid_t> getSupplementaryGIDs() { return supplementaryGIDs; }
 
     bool enabled() { return uid != 0; }
 
@@ -539,6 +541,18 @@ void UserLock::acquire()
                 throw Error(format("the Nix user should not be a member of `%1%'")
                     % settings.buildUsersGroup);
 
+	    /* Get the list of supplementary groups of this build user.  This
+	       is usually either empty or contains a group such as "kvm".  */
+	    supplementaryGIDs.resize(10);
+	    int ngroups = supplementaryGIDs.size();
+	    int err = getgrouplist(pw->pw_name, pw->pw_gid,
+				   &supplementaryGIDs.at(0), &ngroups);
+	    if (err == -1)
+		throw Error(format("failed to get list of supplementary groups for `%1'")
+			    % pw->pw_name);
+
+	    supplementaryGIDs.resize(ngroups);
+
             return;
         }
     }
@@ -2179,8 +2193,11 @@ void DerivationGoal::runChild()
         if (buildUser.enabled()) {
             printMsg(lvlChatty, format("switching to user `%1%'") % buildUser.getUser());
 
-            if (setgroups(0, 0) == -1)
-                throw SysError("cannot clear the set of supplementary groups");
+	    /* Preserve supplementary groups of the build user, to allow
+	       admins to specify groups such as "kvm".  */
+            if (setgroups(buildUser.getSupplementaryGIDs().size(),
+			  &buildUser.getSupplementaryGIDs().at(0)) == -1)
+                throw SysError("cannot set supplementary groups of build user");
 
             if (setgid(buildUser.getGID()) == -1 ||
                 getgid() != buildUser.getGID() ||

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Eelco Dolstra <eelco.dolstra <at> logicblox.com>
Cc: 18994 <at> debbugs.gnu.org, nix-dev <at> lists.science.uu.nl
Subject: Re: [PATCH] Preserve supplementary groups of build users
Date: Wed, 01 Jul 2015 16:54:00 +0200

Hi Eelco,

Eelco Dolstra <eelco.dolstra <at> logicblox.com> skribis:

> On 01/07/15 11:12, Ludovic Courtès wrote:
>
>>> Currently, the build environment made by the daemon does not preserve
>>> supplementary groups of the build users.
>>>
>>> Thus, even though the standalone Guix system sets /dev/kvm 660, owned by
>>> root:kvm, and adds the build users to the kvm group, build users are
>>> unable to access it.
>> 
>> The following patch is an attempt to address this bug (see
>> <http://bugs.gnu.org/18994>) by preserving the supplementary groups of
>> build users in the build environment.
>> 
>> In practice, I would expect that supplementary groups would contain only
>> one or two groups: the build users group, and possibly the “kvm” group.
>
> Applied, thanks!

Excellent, thank you!

Ludo’.

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

From: Eelco Dolstra <eelco.dolstra <at> logicblox.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 18994 <at> debbugs.gnu.org
Cc: nix-dev <at> lists.science.uu.nl
Subject: Re: [PATCH] Preserve supplementary groups of build users
Date: Wed, 01 Jul 2015 14:59:29 +0200

Hi Ludo,

On 01/07/15 11:12, Ludovic Courtès wrote:

>> Currently, the build environment made by the daemon does not preserve
>> supplementary groups of the build users.
>>
>> Thus, even though the standalone Guix system sets /dev/kvm 660, owned by
>> root:kvm, and adds the build users to the kvm group, build users are
>> unable to access it.
> 
> The following patch is an attempt to address this bug (see
> <http://bugs.gnu.org/18994>) by preserving the supplementary groups of
> build users in the build environment.
> 
> In practice, I would expect that supplementary groups would contain only
> one or two groups: the build users group, and possibly the “kvm” group.

Applied, thanks!

-- 
Eelco Dolstra | LogicBlox, Inc. | http://nixos.org/~eelco/

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Charles Strahan <notifications <at> github.com>
Cc: 18994 <at> debbugs.gnu.org, NixOS/nix
 <reply+0011d43327cde66fa384a8345ffe1205d196179a23d7e9d092cf0000000111abfb7c92a163ce00b68004 <at> reply.github.com>,
 NixOS/nix <nix <at> noreply.github.com>
Subject: Re: [nix] Preserve supplementary groups of build users (9aed117)
Date: Wed, 01 Jul 2015 23:10:52 +0200

Charles Strahan <notifications <at> github.com> skribis:

> Is there any reason we immediately throw here, instead of checking `ngroups`, resizing the buffer accordingly, and then rerunning `getgrouplist`? I would think that would be a bit more robust.

Sorry, could you quote the code you’re referring to?

If you talking about the ‘getgrouplist’ call, maybe we could do that,
but we would still need to put a hard limit on the maximum number of
groups anyway.

WDYT?

Thanks,
Ludo’.

PS: Please keep 18994 <at> debbugs.gnu.org Cc’d.

Previous Next

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