Re: [PATCH] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

Hi Hartmut,

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

Hartmut Goebel <address@hidden> 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’.