diff --git a/doc/manual/rl-next/keep-failed-chown.md b/doc/manual/rl-next/keep-failed-chown.md new file mode 100644 index 000000000..b540e6f69 --- /dev/null +++ b/doc/manual/rl-next/keep-failed-chown.md @@ -0,0 +1,13 @@ +--- +synopsis: "`--keep-failed` chowns the build directory to the user that request the build" +issues: [] +cls: [] +category: Improvements +credits: [horrors] +--- + +Running a build with `--keep-failed` now chowns the temporary directory from the +builder user and group to the user that request the build if the build came from +a local user connected to the daemon. This makes inspecting failed derivations a +lot easier. On Linux the build directory made visible to the user will not be in +the same path as it was in the sandbox and continuing builds will usually break. diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index b687fc3bd..da99d5402 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -31,11 +31,13 @@ #include "path-tree.hh" #include +#include #include #include #include #include +#include #include #include #include @@ -114,7 +116,11 @@ LocalDerivationGoal::~LocalDerivationGoal() noexcept(false) /* Careful: we should never ever throw an exception from a destructor. */ try { killChild(); } catch (...) { ignoreExceptionInDestructor(); } - try { deleteTmpDir(false, true); } catch (...) { ignoreExceptionInDestructor(); } + try { + finalizeTmpDir(false, true); + } catch (...) { + ignoreExceptionInDestructor(); + } } @@ -366,7 +372,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() } #endif - deleteTmpDir(false); + finalizeTmpDir(false); /* Move paths out of the chroot for easier debugging of build failures. */ @@ -385,7 +391,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck() { - deleteTmpDir(true); + finalizeTmpDir(true); } @@ -2384,14 +2390,71 @@ try { co_return result::current_exception(); } +// make `entry` in `parentFd` visible to the given user and group, preserving +// inode modes as much as possible. if the builder sets the mode of any inode +// to not be readable by the owner we keep this; not doing so could interfere +// with error analysis. if the builder used multiple uids or gids we will not +// keep them around and instead collapse them all onto the uid/gid given here +// to not leave around inodes owned by unassigned uids/gids in the system. we +// also clear setuid/setgid/sticky bits just to be safe even though a builder +// should not be able to set them to begin, otherwise we may leave setuid/gid +// executables in the tree even with user/group set to -1/-1. there have been +// enough bugs of this kind in the past to warrant some extra attention here. +static void makeVisible(int parentFd, const char * entry, uid_t user, gid_t group) +{ + struct stat st; + if (fstatat(parentFd, entry, &st, AT_SYMLINK_NOFOLLOW)) { + throw SysError("fstat(%s)", guessOrInventPathFromFD(parentFd)); + } + if (S_ISDIR(st.st_mode)) { + int dirfd = openat(parentFd, entry, O_RDONLY | O_DIRECTORY | O_NOFOLLOW); + if (dirfd < 0) { + throw SysError("openat(%s/%s)", guessOrInventPathFromFD(parentFd), entry); + } + AutoCloseDir dir(fdopendir(dirfd)); + if (!dir) { + close(dirfd); + throw SysError("fdopendir(%s/%s)", guessOrInventPathFromFD(parentFd), entry); + } -void LocalDerivationGoal::deleteTmpDir(bool force, bool duringDestruction) + struct dirent * dirent; + while (errno = 0, dirent = readdir(dir.get())) { + if (strcmp(dirent->d_name, ".") == 0 || strcmp(dirent->d_name, "..") == 0) { + continue; + } + makeVisible(dirfd, dirent->d_name, user, group); + } + } + + // ignore permissions errors for symlinks. linux can't chmod them. + // clear special permission bits while we're here, just to be safe + if (fchmodat(parentFd, entry, st.st_mode & 0777, AT_SYMLINK_NOFOLLOW) && !S_ISLNK(st.st_mode)) { + throw SysError("fchmod(%s)", guessOrInventPathFromFD(parentFd)); + } + if (user != uid_t(-1) && group != gid_t(-1) + && fchownat(parentFd, entry, user, group, AT_SYMLINK_NOFOLLOW)) + { + throw SysError("fchown(%s)", guessOrInventPathFromFD(parentFd)); + } +} + +void LocalDerivationGoal::finalizeTmpDir(bool force, bool duringDestruction) { if (tmpDirRoot != "") { /* Don't keep temporary directories for builtins because they might have privileged stuff (like a copy of netrc). */ if (settings.keepFailed && !force && !drv->isBuiltin()) { printError("note: keeping build directory '%s'", tmpDirRoot); + try { + // always make visible, but don't always chown. if we run as + // root we may not want to chown things to root:root so much + auto creds = worker.store.associatedCredentials(); + makeVisible( + tmpDirFd.get(), ".", creds ? creds->user : -1, creds ? creds->group : -1 + ); + } catch (SysError & e) { + printError("error making '%s' accessible: %s", tmpDir, e.what()); + } chmod(tmpDirRoot.c_str(), 0755); } else if (duringDestruction) diff --git a/lix/libstore/build/local-derivation-goal.hh b/lix/libstore/build/local-derivation-goal.hh index ff3afa150..243aea99b 100644 --- a/lix/libstore/build/local-derivation-goal.hh +++ b/lix/libstore/build/local-derivation-goal.hh @@ -250,9 +250,12 @@ struct LocalDerivationGoal : public DerivationGoal void cleanupPostOutputsRegisteredModeNonCheck() override; /** - * Delete the temporary directory, if we have one. + * Delete the temporary directory or make it visible to the user requesting + * this build, if a temporary directory was created at all. Temporary files + * of derivations using builtin builders are deleted even for `keep-failed` + * builds as otherwise we may expose secrets (e.g. from the system .netrc). */ - void deleteTmpDir(bool force, bool duringDestruction = false); + void finalizeTmpDir(bool force, bool duringDestruction = false); /** * Forcibly kill the child process, if any. diff --git a/lix/libstore/local-store.hh b/lix/libstore/local-store.hh index fa00c4885..54ae46253 100644 --- a/lix/libstore/local-store.hh +++ b/lix/libstore/local-store.hh @@ -135,6 +135,8 @@ private: Sync _gcState; + std::optional association; + public: const Path dbDir; @@ -148,6 +150,16 @@ public: LocalStoreConfig & config() override { return config_; } const LocalStoreConfig & config() const override { return config_; } + std::optional associatedCredentials() const override + { + return association; + } + + void associateWithCredentials(uid_t user, gid_t group) + { + association = {user, group}; + } + private: const PublicKeys & getPublicKeys(); diff --git a/lix/libstore/store-api.hh b/lix/libstore/store-api.hh index a1510efb2..28477200a 100644 --- a/lix/libstore/store-api.hh +++ b/lix/libstore/store-api.hh @@ -234,6 +234,22 @@ protected: Store(const StoreConfig & config); public: + struct AssociatedCredentials + { + uid_t user; + gid_t group; + }; + + /** + * Credentials of the context using this store if this store is proxied + * to somewhere else and the peer context is known. Only the daemon can + * set this to values that make any sense, using unix peer credentials. + */ + virtual std::optional associatedCredentials() const + { + return {}; + } + /** * Perform any necessary effectful operation to make the store up and * running diff --git a/lix/nix/daemon.cc b/lix/nix/daemon.cc index f179e81d4..587085f65 100644 --- a/lix/nix/daemon.cc +++ b/lix/nix/daemon.cc @@ -451,12 +451,15 @@ static void daemonInstance(AsyncIoRoot & aio, std::optional forceTr // Restore normal handling of SIGCHLD. setSigChldAction(false); + auto store = aio.blockOn(openUncachedStore(AllowDaemon::Disallow)); + if (auto local = dynamic_cast(&*store); local && peer.uidKnown && peer.gidKnown) { + local->associateWithCredentials(peer.uid, peer.gid); + } + // Handle the connection. FdSource from(SUBDAEMON_CONNECTION_FD); FdSink to(SUBDAEMON_CONNECTION_FD); - processConnection( - aio, aio.blockOn(openUncachedStore(AllowDaemon::Disallow)), from, to, trusted - ); + processConnection(aio, store, from, to, trusted); } /** diff --git a/tests/nixos/chown-to-user.nix b/tests/nixos/chown-to-user.nix new file mode 100644 index 000000000..ae7ae3cd5 --- /dev/null +++ b/tests/nixos/chown-to-user.nix @@ -0,0 +1,74 @@ +{ lib, config, pkgs, ... }: + +let + failedNormal = config: pkgs.writeText "failed.nix" '' + let utils = builtins.storePath ${config.system.build.extraUtils}; in + derivation { + name = "failed"; + system = builtins.currentSystem; + PATH = "''${utils}/bin"; + builder = "''${utils}/bin/sh"; + args = [ "-c" "mkdir dir; echo test > dir/file" ]; + } + ''; + + failedBuiltin = pkgs.writeText "failed.nix" '' + derivation { + name = "failed"; + system = builtins.currentSystem; + builder = "builtin:fetchurl"; + url = "http://localhost/foo"; + outputHashMode = "flat"; + } + ''; +in +{ + name = "chown-to-user"; + + nodes = { + machine = { lib, pkgs, ... }: { + virtualisation.writableStore = true; + + users.users.test = { + isNormalUser = true; + group = "test"; + }; + users.groups.test = {}; + nix.nrBuildUsers = 1; + }; + }; + + testScript = { nodes, ... }: '' + import re + + machine.wait_for_unit("multi-user.target") + + # builds using the daemon chown tempdirs + out = machine.fail("runuser -u test -- nix-build ${failedNormal nodes.machine} --keep-failed 2>&1") + dir = re.search("keeping build directory '(.+?)'", out) + assert dir + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}").strip() == "root:nixbld:755" + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b").strip() == "test:test:700" + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir").strip() == "test:test:755" + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir/file").strip() == "test:test:644" + + # builds not using the daemon do not chown tempdirs + out = machine.fail("NIX_REMOTE=local nix-build ${failedNormal nodes.machine} --keep-failed 2>&1") + dir = re.search("keeping build directory '(.+?)'", out) + assert dir + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}").strip() == "root:nixbld:755" + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b").strip() == "nixbld1:nixbld:700" + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir").strip() == "nixbld1:nixbld:755" + assert machine.succeed(f"stat -c %U:%G:%a {dir[1]}/b/dir/file").strip() == "nixbld1:nixbld:644" + + # builds using builtin builders using the daemon do not keep tempdirs + out = machine.fail("runuser -u test -- nix-build ${failedBuiltin} --keep-failed 2>&1") + dir = re.search("keeping build directory '(.+?)'", out) + assert not dir + + # builds using builtin builders not using the daemon do not keep tempdirs + out = machine.fail("NIX_REMOTE=local nix-build ${failedBuiltin} --keep-failed 2>&1") + dir = re.search("keeping build directory '(.+?)'", out) + assert not dir + ''; +} diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index e08bc0d3a..30783e081 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -188,4 +188,6 @@ in io_uring = runNixOSTestFor "x86_64-linux" ./io_uring; fetchurl = runNixOSTestFor "x86_64-linux" ./fetchurl.nix; + + chown-to-user = runNixOSTestFor "x86_64-linux" ./chown-to-user.nix; }