bug#18994: Daemon does not preserve supplementary groups of build users

bug#18994: Daemon does not preserve supplementary groups of build users

[Ludovic Courtès]

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’.

bug#18994: [PATCH] Preserve supplementary groups of build users

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

ludo@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’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2004 bytes --]

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() ||

bug#18994: [PATCH] Preserve supplementary groups of build users

[Eelco Dolstra]

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/

bug#18994: [PATCH] Preserve supplementary groups of build users

[Ludovic Courtès]

Hi Eelco,

Eelco Dolstra <eelco.dolstra@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’.