* [PATCH] daemon: Set ownership of kept build directories to the calling user.
@ 2016-11-16 13:16 Hartmut Goebel
2016-11-16 21:40 ` Ludovic Courtès
0 siblings, 1 reply; 2+ messages in thread
From: Hartmut Goebel @ 2016-11-16 13:16 UTC (permalink / raw)
To: guix-devel
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(-)
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();
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] daemon: Set ownership of kept build directories to the calling user.
2016-11-16 13:16 [PATCH] daemon: Set ownership of kept build directories to the calling user Hartmut Goebel
@ 2016-11-16 21:40 ` Ludovic Courtès
0 siblings, 0 replies; 2+ messages in thread
From: Ludovic Courtès @ 2016-11-16 21:40 UTC (permalink / raw)
To: Hartmut Goebel; +Cc: guix-devel
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’.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-11-16 21:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-16 13:16 [PATCH] daemon: Set ownership of kept build directories to the calling user Hartmut Goebel
2016-11-16 21:40 ` Ludovic Courtès
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).