diff mbox

daemon: Set ownership of kept build directories to the calling user.

Message ID 1479302172-18524-1-git-send-email-h.goebel@crazy-compilers.com
State New
Headers show

Commit Message

Hartmut Goebel Nov. 16, 2016, 1:16 p.m. UTC
This also closes bug #15890.

* nix/libstore/worker-protocol.hh (PROTOCOL_VERSION): Increment it.
* guix/store.scm (protocol-version): Increment it to the same value.
  (set-build-options): Send uid and gid of evoking user.
* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (performOp)[wopSetOptions] Read clientUid
  and clientGid.
* nix/libstore/build.cc (DerivationGoal::deleteTmpDir): Change
  ownership of build directory if it is kept.
---
 guix/store.scm                  |  7 ++++++-
 nix/libstore/build.cc           | 21 +++++++++++++++++++++
 nix/libstore/globals.hh         |  6 ++++++
 nix/libstore/worker-protocol.hh |  2 +-
 nix/nix-daemon/nix-daemon.cc    |  9 +++++++++
 5 files changed, 43 insertions(+), 2 deletions(-)

Comments

Ludovic Courtès Nov. 16, 2016, 9:40 p.m. UTC | #1
Hi Hartmut,

Great that you’re working on this long-standing issue, that’ll make all
of us happier!  :-)

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> This also closes bug #15890.

Please use the same wording as in other commits, to simplify grepping
and access:

  Fixes <http://bugs.gnu.org/15890>.

> * nix/libstore/worker-protocol.hh (PROTOCOL_VERSION): Increment it.
> * guix/store.scm (protocol-version): Increment it to the same value.
>   (set-build-options): Send uid and gid of evoking user.
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (performOp)[wopSetOptions] Read clientUid
>   and clientGid.
> * nix/libstore/build.cc (DerivationGoal::deleteTmpDir): Change
>   ownership of build directory if it is kept.

[...]

> +    (when (>= (nix-server-minor-version server) 16)
> +          ;; Send uid and gid of evoking user. If either of it is zero (root),
> +          ;; send -1 wich means: do not change.
> +          (send (integer (or (getuid) -1)) (integer (or (getgid) -1))))

I think this can and should be avoided (the client could pass in any
UID).

In nix-daemon.cc:921, the UID and GID of the caller are retrieved using
SO_PEERCRED.  I think we should just pass them down in
‘processConnection’ and then probably in the ‘LocalStore’ constructor.

WDYT?

> +            if (settings.clientUid != (uid_t) -1) {
> +              // FIXME: Is this the correct way to to it? Or it there a better
> +              // way?
> +              Strings args;
> +              char ids[32];
> +              if (settings.clientGid != (gid_t) -1)
> +                // both uid and gid are set, change owner and group
> +                snprintf(ids, 31, "%lu:%lu",
> +                         (unsigned long int) settings.clientUid,
> +                         (unsigned long int) settings.clientGid);
> +              else
> +                // only uid is set, change only the owner
> +                snprintf(ids, 31, "%lu",
> +                         (unsigned long int) settings.clientUid);
> +              args.push_back("-R");
> +              args.push_back(ids);
> +              args.push_back(tmpDir.c_str());
> +              string signature = runProgram("chown", true, args);

This should be done in C++, with code comparable to ‘_deletePath’ in
util.cc.

BTW, in Emacs I use: C-c . stroustrup RET.  That roughly corresponds to
the existing style.

So, how does that sound?  :-)

Thanks,
Ludo’.
diff mbox

Patch

diff --git a/guix/store.scm b/guix/store.scm
index 43cfda9..2023875 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -1,5 +1,6 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013, 2014, 2015, 2016 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2016 Hartmut Goebel <h.goebel@crazy-compilers.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -136,7 +137,7 @@ 
             direct-store-path
             log-file))
 
-(define %protocol-version #x10f)
+(define %protocol-version #x110)
 
 (define %worker-magic-1 #x6e697863)               ; "nixc"
 (define %worker-magic-2 #x6478696f)               ; "dxio"
@@ -580,6 +581,10 @@  encoding conversion errors."
                            `(("locale" . ,locale))
                            '()))))
         (send (string-pairs pairs))))
+    (when (>= (nix-server-minor-version server) 16)
+          ;; Send uid and gid of evoking user. If either of it is zero (root),
+          ;; send -1 wich means: do not change.
+          (send (integer (or (getuid) -1)) (integer (or (getgid) -1))))
     (let loop ((done? (process-stderr server)))
       (or done? (process-stderr server)))))
 
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index ae78e65..e819135 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2617,6 +2617,27 @@  void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // if clientUid and clientGid are set change the ownership
+            if (settings.clientUid != (uid_t) -1) {
+              // FIXME: Is this the correct way to to it? Or it there a better
+              // way?
+              Strings args;
+              char ids[32];
+              if (settings.clientGid != (gid_t) -1)
+                // both uid and gid are set, change owner and group
+                snprintf(ids, 31, "%lu:%lu",
+                         (unsigned long int) settings.clientUid,
+                         (unsigned long int) settings.clientGid);
+              else
+                // only uid is set, change only the owner
+                snprintf(ids, 31, "%lu",
+                         (unsigned long int) settings.clientUid);
+              args.push_back("-R");
+              args.push_back(ids);
+              args.push_back(tmpDir.c_str());
+              string signature = runProgram("chown", true, args);
+              // FIXME: Is it necessary to free signature
+            }
         }
         else
             deletePath(tmpDir);
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 8c07e36..dc6a004 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -70,6 +70,12 @@  struct Settings {
        subgoal of the same goal) fails. */
     bool keepGoing;
 
+    /* User and groud id of the client issuing the buld request.  Used to set
+       the owner and group of the keept temporary directories of failed
+       builds. */
+    uid_t clientUid;
+    gid_t clientGid;
+
     /* Whether, if we cannot realise the known closure corresponding
        to a derivation, we should try to normalise the derivation
        instead. */
diff --git a/nix/libstore/worker-protocol.hh b/nix/libstore/worker-protocol.hh
index 7b7be4a..99c1ee2 100644
--- a/nix/libstore/worker-protocol.hh
+++ b/nix/libstore/worker-protocol.hh
@@ -6,7 +6,7 @@  namespace nix {
 #define WORKER_MAGIC_1 0x6e697863
 #define WORKER_MAGIC_2 0x6478696f
 
-#define PROTOCOL_VERSION 0x10f
+#define PROTOCOL_VERSION 0x110
 #define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00)
 #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff)
 
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 35c284f..a1fce25 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -571,6 +571,15 @@  static void performOp(bool trusted, unsigned int clientVersion,
                     settings.set(trusted ? name : "untrusted-" + name, value);
             }
         }
+        if (GET_PROTOCOL_MINOR(clientVersion) >= 16) {
+          // FIXME: Does readInt always fit into uid_t/gid_t?
+          settings.clientUid = (uid_t) readInt(from);
+          settings.clientGid = (gid_t) readInt(from);
+        } else {
+          settings.clientUid = (uid_t) -1;
+          settings.clientGid = (gid_t) -1;
+        }
+
         settings.update();
         startWork();
         stopWork();