unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [Patch v2] daemon: Set ownership of kept build directories to the calling user.
@ 2016-11-17 11:30 Hartmut Goebel
  2016-11-21 14:13 ` Ludovic Courtès
  0 siblings, 1 reply; 20+ messages in thread
From: Hartmut Goebel @ 2016-11-17 11:30 UTC (permalink / raw)
  To: guix-devel

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

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (pdaemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept.
---
 nix/libstore/build.cc        | 24 ++++++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 13 +++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index ae78e65..b49fb95 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2609,6 +2609,23 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    printMsg(lvlVomit, format("%1%") % path);
+
+    if (chown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2617,6 +2634,13 @@ void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership to "root" for security reasons. So zero is used as
+            // marker for unset.
+            if (settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         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/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 35c284f..e900a7d 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -950,6 +950,19 @@ static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to zero means: do not change, esp. do not
+                   change to "root". */
+                settings.clientUid = 0;
+                settings.clientGid = 0;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-17 11:30 [Patch v2] daemon: Set ownership of kept build directories to the calling user Hartmut Goebel
@ 2016-11-21 14:13 ` Ludovic Courtès
  2016-11-21 14:18   ` Hartmut Goebel
                     ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Ludovic Courtès @ 2016-11-21 14:13 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

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

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (pdaemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept.

[...]

> +static void _chown(const Path & path, uid_t uid, gid_t gid)
> +{
> +    checkInterrupt();
> +
> +    printMsg(lvlVomit, format("%1%") % path);
> +
> +    if (chown(path.c_str(), uid, gid) == -1) {

I think this should use ‘lchown’.

> --- 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;

I don’t like the idea of passing those via the big ‘Settings’
singleton.

Could we instead pass them via the ‘LocalStore’ constructor, with their
default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?

Thank you!

Ludo’.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-21 14:13 ` Ludovic Courtès
@ 2016-11-21 14:18   ` Hartmut Goebel
  2016-11-27 21:04     ` Ludovic Courtès
  2016-11-21 17:36   ` Hartmut Goebel
  2016-11-21 18:29   ` Hartmut Goebel
  2 siblings, 1 reply; 20+ messages in thread
From: Hartmut Goebel @ 2016-11-21 14:18 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> I don’t like the idea of passing those via the big ‘Settings’
> singleton.

Well, the Settings are already used to pass options like --keep-going to
the build process. So the "Singleton" is mutable per-process anyway.
This is why I've put these here, too.

But if you prefer passing them around, I'll try to implement this.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-21 14:13 ` Ludovic Courtès
  2016-11-21 14:18   ` Hartmut Goebel
@ 2016-11-21 17:36   ` Hartmut Goebel
  2016-11-21 18:29   ` Hartmut Goebel
  2 siblings, 0 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-11-21 17:36 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> Could we instead pass them via the ‘LocalStore’ constructor, with their
> default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?

Regarding the default values: I'm using zero as "not set" marker here.
The files will already have owner and group from ‘getuid’ and ‘getgid’,
so there is no need to change them. Or did I miss something?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-21 14:13 ` Ludovic Courtès
  2016-11-21 14:18   ` Hartmut Goebel
  2016-11-21 17:36   ` Hartmut Goebel
@ 2016-11-21 18:29   ` Hartmut Goebel
  2 siblings, 0 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-11-21 18:29 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> Could we instead pass them via the ‘LocalStore’ constructor,

I started implementing this, but adding clientUid/Gid as a attribute
(property) of 'LocalStore' feels wrong. The LocalStore is about the
store: files being there, querying and such. Even "build" is a bit of a
misfit IMO.

Okay, probably you mean, passing to the ‘LocalStore’ constructor and
then forward them to the goals. Which means changing
`makeDerevationGoal', which is used quite some time. From there you
would need to pass it onto the 'DerivationGoal' constructor. These are
quite some changes for the rare case of building, and only if the build
failed and only if the user passed --keep-failed.

I have the impression, that 'Settings' have been introduced exactly to
save passing around such information. "keep-failed" is passed to
'deleteTmpDir()' exactly this way (via Settings). Which means if passing
clientUid/Gid via constructors,  we would have two different ways of
passing information used in the same small function - which would be
inconsistent.

Thus said, I'm in strong favour of using Settings.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-21 14:18   ` Hartmut Goebel
@ 2016-11-27 21:04     ` Ludovic Courtès
  2016-11-28 21:31       ` Hartmut Goebel
  0 siblings, 1 reply; 20+ messages in thread
From: Ludovic Courtès @ 2016-11-27 21:04 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

Hi!

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

> Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
>> I don’t like the idea of passing those via the big ‘Settings’
>> singleton.
>
> Well, the Settings are already used to pass options like --keep-going to
> the build process. So the "Singleton" is mutable per-process anyway.
> This is why I've put these here, too.

Good point.  This and your other message have convinced me!

> Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
>> Could we instead pass them via the ‘LocalStore’ constructor, with their
>> default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?
>
> Regarding the default values: I'm using zero as "not set" marker here.
> The files will already have owner and group from ‘getuid’ and ‘getgid’,
> so there is no need to change them. Or did I miss something?

The problem is that 0 also means “root”, so I’d prefer a different value
to denote that “unset” status, -1 if you want, and explicit checks (and
a comment explaining that ‘clientUid’ and ‘clientGid’ can be either a
valid UID/GID or -1, in which case blablabla).

How does that sound?

Thanks, and sorry for the delay!

Ludo’.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-27 21:04     ` Ludovic Courtès
@ 2016-11-28 21:31       ` Hartmut Goebel
  2016-12-01  0:01         ` Danny Milosavljevic
  0 siblings, 1 reply; 20+ messages in thread
From: Hartmut Goebel @ 2016-11-28 21:31 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 27.11.2016 um 22:04 schrieb Ludovic Courtès:
> The problem is that 0 also means “root”, so I’d prefer a different value
> to denote that “unset” status, -1 if you want, and explicit checks (and
> a comment explaining that ‘clientUid’ and ‘clientGid’ can be either a
> valid UID/GID or -1, in which case blablabla).
>
> How does that sound?

This basically sounds reasonable.

I just need some C programming hint: uid_t is an unsigned int, so
comparing with -1 raises a warning (which IMO is the same as en error).
And casting uid_t to unisgned int might return the same uid as "nobody".
How can I solve this?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-11-28 21:31       ` Hartmut Goebel
@ 2016-12-01  0:01         ` Danny Milosavljevic
  2016-12-05 20:46           ` [PATCH v3] " Hartmut Goebel
  2016-12-05 20:51           ` [Patch v2] " Hartmut Goebel
  0 siblings, 2 replies; 20+ messages in thread
From: Danny Milosavljevic @ 2016-12-01  0:01 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

Hi Hartmut,

On Mon, 28 Nov 2016 22:31:44 +0100
Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:
> I just need some C programming hint: uid_t is an unsigned int, so
> comparing with -1 raises a warning (which IMO is the same as en error).

The system call handler in the Linux kernel does this, among other things:

#define low2highuid(uid) ((uid) == (old_uid_t)-1 ? (uid_t)-1 : (uid_t)(uid))

So I'd say to compare with (uid_t)-1 would be fine

    if (settings.clientUid != (uid_t) -1) {
        ...
    }

... since they do something like it in Linux anyway - which is the one implementing the chown operation in the first place :)

> And casting uid_t to unisgned int might return the same uid as "nobody".

What do you mean? uid_t is __uid_t - which in turn is implementation-defined - but yes, it is unsigned. On Linux it changed size multiple time until now where it is 32 bit unsigned.

On GuixSD, user "nobody" has uid 65534. That's one less than the maximum value for 16 bit (!) uids (the maximum is hopefully invalid as uid since chown needs it as a flag).

(-1) in two-complement 16 bit integral encoding (which is technically not guaranteed to be used in C) would be 65535.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3] daemon: Set ownership of kept build directories to the calling user.
  2016-12-01  0:01         ` Danny Milosavljevic
@ 2016-12-05 20:46           ` Hartmut Goebel
  2016-12-06 15:08             ` Ludovic Courtès
  2016-12-06 20:41             ` [PATCH v3] daemon: Set ownership of kept build directories to the calling user Danny Milosavljevic
  2016-12-05 20:51           ` [Patch v2] " Hartmut Goebel
  1 sibling, 2 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-05 20:46 UTC (permalink / raw)
  To: guix-devel

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

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept and the new owner is not root.
---
 nix/libstore/build.cc        | 23 +++++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 12 ++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 889ee3d..44ff11c 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2631,6 +2631,23 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    printMsg(lvlVomit, format("%1%") % path);
+
+    if (lchown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2639,6 +2656,12 @@ void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership or the group to "root" for security reasons.
+            if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         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/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 682f9a2..6f88e5b 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -960,6 +960,18 @@ static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to -1 means: do not change */
+                settings.clientUid = -1;
+                settings.clientGid = -1;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.
  2016-12-01  0:01         ` Danny Milosavljevic
  2016-12-05 20:46           ` [PATCH v3] " Hartmut Goebel
@ 2016-12-05 20:51           ` Hartmut Goebel
  1 sibling, 0 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-05 20:51 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: guix-devel

Am 01.12.2016 um 01:01 schrieb Danny Milosavljevic:
> So I'd say to compare with (uid_t)-1 would be fine
>
>     if (settings.clientUid != (uid_t) -1) {
>         ...
>     }

Thanks for this tip, Danny. This made it work and I just posted the
updated patch.

> > And casting uid_t to unisgned int might return the same uid as "nobody".
> [...]
>
> (-1) in two-complement 16 bit integral encoding (which is technically not guaranteed to be used in C) would be 65535.

Ahh, well, yes. I'm not quite good on bit operations, esp. on
representations of negative numbers. Thanks to doing this right.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.
  2016-12-05 20:46           ` [PATCH v3] " Hartmut Goebel
@ 2016-12-06 15:08             ` Ludovic Courtès
  2016-12-08 12:12               ` Hartmut Goebel
  2016-12-06 20:41             ` [PATCH v3] daemon: Set ownership of kept build directories to the calling user Danny Milosavljevic
  1 sibling, 1 reply; 20+ messages in thread
From: Ludovic Courtès @ 2016-12-06 15:08 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

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

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept and the new owner is not root.

[...]

> +static void _chown(const Path & path, uid_t uid, gid_t gid)
> +{
> +    checkInterrupt();
> +
> +    printMsg(lvlVomit, format("%1%") % path);

Please remove this line (we wouldn’t be able to track where the message
comes from).

> +    /* User and groud id of the client issuing the buld request.  Used to set
                                                       ^^
Typo.  Also:

  … issuing the build request, or -1 if the UID and GID are not known.

> +       the owner and group of the keept temporary directories of failed
                                      ^^
Typo.

If you have checked that it works as intended, please push with these
changes and email the commit ID to 15890-done@debbugs.gnu.org.

Thank you!

Ludo’.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.
  2016-12-05 20:46           ` [PATCH v3] " Hartmut Goebel
  2016-12-06 15:08             ` Ludovic Courtès
@ 2016-12-06 20:41             ` Danny Milosavljevic
  2016-12-08 12:16               ` Hartmut Goebel
  1 sibling, 1 reply; 20+ messages in thread
From: Danny Milosavljevic @ 2016-12-06 20:41 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

Hi Hartmut,

> +#if defined(SO_PEERCRED)
...
> +#else
> +                /* Setting these to -1 means: do not change */
> +                settings.clientUid = -1;
> +                settings.clientGid = -1;
> +#endif

I think you also have to cast them there, so

               settings.clientUid = (uid_t) -1;
               settings.clientGid = (gid_t) -1;

The reason is because (-1) is a signed integer and clientUid isn't - and neither is clientGid.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.
  2016-12-06 15:08             ` Ludovic Courtès
@ 2016-12-08 12:12               ` Hartmut Goebel
  2016-12-08 12:14                 ` [PATCH v4] " Hartmut Goebel
  2016-12-09 14:22                 ` [PATCH v3] " Ludovic Courtès
  0 siblings, 2 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-08 12:12 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 06.12.2016 um 16:08 schrieb Ludovic Courtès:
> If you have checked that it works as intended, please push with these
> changes and email the commit ID to 15890-done@debbugs.gnu.org.

I can only verify that it worked with revision 6634180 "gnu: guile-ssh:
Update to 0.10.2.", but not with current master.

Starting with 21531ad "offload: Use Guile-SSH instead of GNU lsh" I get
errors, which I could not work around:

guix: offload: command not found
Try `guix --help' for more information.
guix build: error: build failed: unexpected EOF reading a line

So, although I'm confident the code is correct, I can not verify with
current master. Thus I'm posting a new version of the patch asking you
to test and push.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v4] daemon: Set ownership of kept build directories to the calling user.
  2016-12-08 12:12               ` Hartmut Goebel
@ 2016-12-08 12:14                 ` Hartmut Goebel
  2016-12-09 14:23                   ` Ludovic Courtès
  2016-12-09 14:22                 ` [PATCH v3] " Ludovic Courtès
  1 sibling, 1 reply; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-08 12:14 UTC (permalink / raw)
  To: guix-devel

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

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept and the new owner is not root.
---
 nix/libstore/build.cc        | 21 +++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 12 ++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 889ee3d..e823001 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2631,6 +2631,21 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    if (lchown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2639,6 +2654,12 @@ void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership or the group to "root" for security reasons.
+            if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         else
             deletePath(tmpDir);
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 8c07e36..7beb1a5 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 build request.  Used to set
+       the owner and group of the kept 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/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 682f9a2..47b67d5 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -960,6 +960,18 @@ static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to -1 means: do not change */
+                settings.clientUid = (uid_t) -1;
+                settings.clientGid = (gid_t) -1;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.
  2016-12-06 20:41             ` [PATCH v3] daemon: Set ownership of kept build directories to the calling user Danny Milosavljevic
@ 2016-12-08 12:16               ` Hartmut Goebel
  0 siblings, 0 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-08 12:16 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: guix-devel

Am 06.12.2016 um 21:41 schrieb Danny Milosavljevic:
> I think you also have to cast them there, so
>
>                settings.clientUid = (uid_t) -1;
>                settings.clientGid = (gid_t) -1;

Thanks, I changes this accordingly.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.
  2016-12-08 12:12               ` Hartmut Goebel
  2016-12-08 12:14                 ` [PATCH v4] " Hartmut Goebel
@ 2016-12-09 14:22                 ` Ludovic Courtès
  2016-12-09 15:50                   ` Guile-SSH found at configure-time but not at run-time Hartmut Goebel
  1 sibling, 1 reply; 20+ messages in thread
From: Ludovic Courtès @ 2016-12-09 14:22 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

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

> Am 06.12.2016 um 16:08 schrieb Ludovic Courtès:
>> If you have checked that it works as intended, please push with these
>> changes and email the commit ID to 15890-done@debbugs.gnu.org.
>
> I can only verify that it worked with revision 6634180 "gnu: guile-ssh:
> Update to 0.10.2.", but not with current master.
>
> Starting with 21531ad "offload: Use Guile-SSH instead of GNU lsh" I get
> errors, which I could not work around:
>
> guix: offload: command not found
> Try `guix --help' for more information.
> guix build: error: build failed: unexpected EOF reading a line

You need to install Guile-SSH to fix this (Guile-SSH was found at
configure-time but not at run-time, leading to this error.)

Ludo’.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.
  2016-12-08 12:14                 ` [PATCH v4] " Hartmut Goebel
@ 2016-12-09 14:23                   ` Ludovic Courtès
  2016-12-09 14:47                     ` Hartmut Goebel
  0 siblings, 1 reply; 20+ messages in thread
From: Ludovic Courtès @ 2016-12-09 14:23 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

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

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept and the new owner is not root.

OK, please push.

Thank you!

Ludo’.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.
  2016-12-09 14:23                   ` Ludovic Courtès
@ 2016-12-09 14:47                     ` Hartmut Goebel
  0 siblings, 0 replies; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-09 14:47 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 09.12.2016 um 15:23 schrieb Ludovic Courtès:
> OK, please push.
Done. Thanks for testing.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Guile-SSH found at configure-time but not at run-time
  2016-12-09 14:22                 ` [PATCH v3] " Ludovic Courtès
@ 2016-12-09 15:50                   ` Hartmut Goebel
  2016-12-09 20:35                     ` Ludovic Courtès
  0 siblings, 1 reply; 20+ messages in thread
From: Hartmut Goebel @ 2016-12-09 15:50 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

Am 09.12.2016 um 15:22 schrieb Ludovic Courtès:
> You need to install Guile-SSH to fix this (Guile-SSH was found at
> configure-time but not at run-time, leading to this error.)

I wonder how this can happen. I'm running within "guix environment guix".

Maybe I need to run within "./pre-inst guix environment guix"?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Guile-SSH found at configure-time but not at run-time
  2016-12-09 15:50                   ` Guile-SSH found at configure-time but not at run-time Hartmut Goebel
@ 2016-12-09 20:35                     ` Ludovic Courtès
  0 siblings, 0 replies; 20+ messages in thread
From: Ludovic Courtès @ 2016-12-09 20:35 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

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

> Am 09.12.2016 um 15:22 schrieb Ludovic Courtès:
>> You need to install Guile-SSH to fix this (Guile-SSH was found at
>> configure-time but not at run-time, leading to this error.)
>
> I wonder how this can happen. I'm running within "guix environment guix".

In ‘guix environment guix’, Guile-SSH is definitely going to be found;
you can confirm this by looking at ‘config.log’.

However, if you type “make install” from there, then it may be the case
that Guile-SSH is no longer visible in the installed environment.

HTH!

Ludo’.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2016-12-09 20:36 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-17 11:30 [Patch v2] daemon: Set ownership of kept build directories to the calling user Hartmut Goebel
2016-11-21 14:13 ` Ludovic Courtès
2016-11-21 14:18   ` Hartmut Goebel
2016-11-27 21:04     ` Ludovic Courtès
2016-11-28 21:31       ` Hartmut Goebel
2016-12-01  0:01         ` Danny Milosavljevic
2016-12-05 20:46           ` [PATCH v3] " Hartmut Goebel
2016-12-06 15:08             ` Ludovic Courtès
2016-12-08 12:12               ` Hartmut Goebel
2016-12-08 12:14                 ` [PATCH v4] " Hartmut Goebel
2016-12-09 14:23                   ` Ludovic Courtès
2016-12-09 14:47                     ` Hartmut Goebel
2016-12-09 14:22                 ` [PATCH v3] " Ludovic Courtès
2016-12-09 15:50                   ` Guile-SSH found at configure-time but not at run-time Hartmut Goebel
2016-12-09 20:35                     ` Ludovic Courtès
2016-12-06 20:41             ` [PATCH v3] daemon: Set ownership of kept build directories to the calling user Danny Milosavljevic
2016-12-08 12:16               ` Hartmut Goebel
2016-12-05 20:51           ` [Patch v2] " Hartmut Goebel
2016-11-21 17:36   ` Hartmut Goebel
2016-11-21 18:29   ` Hartmut Goebel

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