Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add granular access control for nix store #9287

Draft
wants to merge 61 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
20e574e
Add a library for ACL manipulation
balsoft Nov 1, 2023
337a127
Add a granular store interface
balsoft Nov 1, 2023
5024921
Implement granular access store
balsoft Nov 1, 2023
552e4e5
Add CLI commands to manipulate ACLs
balsoft Nov 1, 2023
61a3cea
Add __permissions to builtins.derivation and builtins.path
balsoft Nov 1, 2023
0be3e5d
Add an integration test for ACL functionality
balsoft Nov 1, 2023
bd56e3e
Add tests/acls.sh
ylecornec Nov 14, 2023
db3a522
acls grant/revoke: Error if group or user does not exists
ylecornec Nov 15, 2023
aba3181
Acls test: permission of dependency.
ylecornec Nov 16, 2023
8dbea38
revokeBuildUserAccess: only revoke permissions added by grantBuildUse…
ylecornec Nov 21, 2023
14e474c
Acls: add test that revokes the permission of a runtime dependency.
ylecornec Nov 22, 2023
afd828b
Acls: Refactor integration tests
ylecornec Nov 23, 2023
5d97559
Acls: disable non integration tests for now
ylecornec Nov 23, 2023
65fe86f
Add json() to AccessStatus
balsoft Nov 30, 2023
db20d22
Add protectByDefault setting
balsoft Nov 30, 2023
1102fdd
Add runtime closure invariant
balsoft Nov 30, 2023
6293167
Run acls.sh test properly
balsoft Nov 30, 2023
9d6c011
Acls: Add tests where a public output depend on a private one
ylecornec Dec 5, 2023
1ed4965
Acls: explicitely access future or current permissions
ylecornec Dec 5, 2023
f9e2c4b
Acls: remove some permission adding code which may not be needed anymore
ylecornec Dec 5, 2023
0c625b0
Acls: Also add future permission to paths of StoreObjectDerivationOutput
ylecornec Dec 5, 2023
7ea4b05
Acls: Add ShouldSync path status
ylecornec Dec 5, 2023
a3d3b71
Acls: tests non trusted user with private file
ylecornec Dec 6, 2023
5f8eef5
Acls: canAccess function, remove default value for use_future parameter
ylecornec Dec 6, 2023
2c00ec5
Merge remote-tracking branch 'origin/master' into acls
balsoft Dec 5, 2023
228d8af
Merge branch 'ylecornec/dep_perms' into acls
balsoft Dec 7, 2023
fccba28
ACL tests
balsoft Dec 8, 2023
7653b07
Add the ability to cache user's groups
balsoft Dec 8, 2023
3994ce1
Prevent segfault
balsoft Dec 13, 2023
3a4914d
Fix darwin build
balsoft Dec 13, 2023
cd72876
Merge remote-tracking branch 'origin/master' into acls
balsoft Dec 13, 2023
eff385d
Fix perl/default.nix
ylecornec Dec 14, 2023
4b66941
Acls: builtins.path set accessStatus
ylecornec Dec 14, 2023
985fe93
Acls: remove PathStatus::ShouldSync
ylecornec Dec 14, 2023
0b92adf
Acls: AccessStatus setter/getter
ylecornec Dec 14, 2023
834219a
Acls tests: Assertions on failing tests output
ylecornec Dec 14, 2023
f9d3f55
Temporarily deactivate ensureAccess
ylecornec Dec 14, 2023
96cb115
Acls: remove canAccess `use_future` argument
ylecornec Dec 14, 2023
2820eb4
Acls: permission check when importing a folder with builtins.path
ylecornec Dec 15, 2023
c1912d8
Acls: Test importing a private folder
ylecornec Dec 15, 2023
9c75782
Don't account for trusted users in ensureAccess
balsoft Dec 18, 2023
8ee4043
Make the 'should be synced' message debug-only
balsoft Dec 18, 2023
af84767
Fix perl bindings build
balsoft Dec 18, 2023
f967eb6
Reactivate runtime closure check
ylecornec Dec 18, 2023
d167252
Acls test: fix for runtime closure checks
ylecornec Dec 18, 2023
53c8eb5
Merge remote-tracking branch 'tweag/acls' into ylecornec/remove_curre…
ylecornec Dec 18, 2023
8841d0d
Acls: reactivate ensureAccess and move the call to setAccessStatus
ylecornec Dec 21, 2023
9f63760
Acls: Fix tests after activating `ensureAccess`
ylecornec Dec 19, 2023
045f1e8
Acls: add tests using flakes
ylecornec Dec 21, 2023
e90e479
Acls: merge {add/remove}AllowedEntities current and future
ylecornec Dec 21, 2023
df135f2
Acls documentation: fix markdown files paths.
ylecornec Dec 21, 2023
d14704c
ACLs: calculate mask correctly
balsoft Jan 12, 2024
51419e5
Merge branch 'ylecornec/remove_current_future' into selective-acl
balsoft Jan 16, 2024
5ef3f14
Ensure access in daemon.cc regardless of current status
balsoft Feb 2, 2024
7a49064
Assign the build directory to the effective user, if present
balsoft Feb 7, 2024
c5f8a40
Fix getUserName behavior
balsoft Feb 7, 2024
5333b25
Add referrer checks for access status
balsoft Feb 10, 2024
9ca2e82
Protect paths if setAccessStatus fail while registering
balsoft Feb 15, 2024
a8ff15f
Automatically deny access for referree derivations
balsoft Mar 14, 2024
946f4f7
Pass through access status from daemon
balsoft Mar 14, 2024
5d5bbbc
chmod if chown fails
balsoft Mar 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Implement granular access store
  • Loading branch information
balsoft committed Nov 1, 2023
commit 5024921c8b6432589669442eecdef50a1043c6bf
52 changes: 41 additions & 11 deletions src/libstore/build/derivation-goal.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#include "derivation-goal.hh"
#include "config.hh"
#include "granular-access-store.hh"
#include "hook-instance.hh"
#include "worker.hh"
#include "builtins.hh"
Expand Down Expand Up @@ -679,13 +681,20 @@ void DerivationGoal::tryToBuild()
return;
}

/* If any of the outputs already exist but are not valid, delete
them. */
/* If any of the outputs already exist but are not valid, delete them. If
any of the outputs are inacessible, set the build mode to `Check` so that
if outputs match, access is granted */
for (auto & [_, status] : initialOutputs) {
if (!status.known || status.known->isValid()) continue;
auto storePath = status.known->path;
debug("removing invalid path '%s'", worker.store.printStorePath(status.known->path));
deletePath(worker.store.Store::toRealPath(storePath));
if (status.known) {
if (status.known->status == PathStatus::Corrupt || status.known->status == PathStatus::Absent) {
auto storePath = status.known->path;
debug("removing invalid path '%s'", worker.store.printStorePath(status.known->path));
deletePath(worker.store.Store::toRealPath(storePath));
} else if (status.known->status == PathStatus::Inaccessible) {
logger->cout("don't have access to path %s; checking outputs", worker.store.printStorePath(status.known->path));
buildMode = bmCheck;
}
}
}

/* Don't do a remote build if the derivation has the attribute
Expand Down Expand Up @@ -1216,7 +1225,11 @@ Path DerivationGoal::openLogFile()
Path logFileName = fmt("%s/%s%s", dir, baseName.substr(2),
settings.compressLog ? ".bz2" : "");

fdLogFile = open(logFileName.c_str(), O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 0666);
bool logFileExisted = std::filesystem::exists(logFileName);

auto mode = experimentalFeatureSettings.isEnabled(Xp::ACLs) ? 0660 : 0666;

fdLogFile = open(logFileName.c_str(), O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, mode);
if (!fdLogFile) throw SysError("creating log file '%1%'", logFileName);

logFileSink = std::make_shared<FdSink>(fdLogFile.get());
Expand All @@ -1226,6 +1239,13 @@ Path DerivationGoal::openLogFile()
else
logSink = logFileSink;

if (experimentalFeatureSettings.isEnabled(Xp::ACLs) && !logFileExisted)
if (auto localStore = dynamic_cast<LocalStore*>(&worker.store)) {
auto storeObject = StoreObjectDerivationLog {drvPath};
auto status = localStore->futurePermissions.contains(storeObject) ? localStore->futurePermissions.at(storeObject) : LocalGranularAccessStore::AccessStatus {};
localStore->setCurrentAccessStatus(logFileName, status);
}

return logFileName;
}

Expand Down Expand Up @@ -1372,21 +1392,31 @@ std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
wantedOutputsLeft.erase(i.first);
if (i.second) {
auto outputPath = *i.second;
bool canAccess = true;
if (experimentalFeatureSettings.isEnabled(Xp::ACLs))
if (auto aclStore = dynamic_cast<LocalGranularAccessStore *>(&worker.store))
canAccess = aclStore->canAccess(outputPath);
info.known = {
.path = outputPath,
.status = !worker.store.isValidPath(outputPath)
? PathStatus::Absent
: !checkHash || worker.pathContentsGood(outputPath)
? PathStatus::Valid
: PathStatus::Corrupt,
: checkHash && !worker.pathContentsGood(outputPath)
? PathStatus::Corrupt
: !canAccess
? PathStatus::Inaccessible
: PathStatus::Valid,
};
}
auto drvOutput = DrvOutput{info.outputHash, i.first};
if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
if (auto real = worker.store.queryRealisation(drvOutput)) {
bool canAccess = true;
if (experimentalFeatureSettings.isEnabled(Xp::ACLs))
if (auto aclStore = dynamic_cast<LocalGranularAccessStore *>(&worker.store))
canAccess = aclStore->canAccess(real->outPath);
info.known = {
.path = real->outPath,
.status = PathStatus::Valid,
.status = canAccess ? PathStatus::Valid : PathStatus::Inaccessible,
};
} else if (info.known && info.known->isValid()) {
// We know the output because it's a static output of the
Expand Down
6 changes: 4 additions & 2 deletions src/libstore/build/derivation-goal.hh
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,20 @@ typedef enum {rpAccept, rpDecline, rpPostpone} HookReply;

/**
* Unless we are repairing, we don't both to test validity and just assume it,
* so the choices are `Absent` or `Valid`.
* so the choices are `Absent`, `Inaccessible` or `Valid`.
*/
enum struct PathStatus {
Corrupt,
Absent,
Valid,
Inaccessible,
};

struct InitialOutputStatus {
StorePath path;
PathStatus status;
/**
* Valid in the store, and additionally non-corrupt if we are repairing
* Valid in the store, accessible, and additionally non-corrupt if we are repairing
*/
bool isValid() const {
return status == PathStatus::Valid;
Expand All @@ -40,6 +41,7 @@ struct InitialOutputStatus {
*/
bool isPresent() const {
return status == PathStatus::Corrupt
|| status == PathStatus::Inaccessible
|| status == PathStatus::Valid;
}
};
Expand Down
137 changes: 111 additions & 26 deletions src/libstore/build/local-derivation-goal.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#include "local-derivation-goal.hh"
#include "acl.hh"
#include "config.hh"
#include "gc-store.hh"
#include "granular-access-store.hh"
#include "hook-instance.hh"
#include "local-fs-store.hh"
#include "worker.hh"
#include "builtins.hh"
#include "builtins/buildenv.hh"
Expand All @@ -17,11 +21,13 @@
#include "personality.hh"
#include "namespaces.hh"

#include <filesystem>
#include <regex>
#include <queue>

#include <sys/un.h>
#include <fcntl.h>
#include <sys/xattr.h>
#include <termios.h>
#include <unistd.h>
#include <sys/mman.h>
Expand Down Expand Up @@ -231,6 +237,22 @@ void LocalDerivationGoal::tryLocalBuild()
}
}

if (experimentalFeatureSettings.isEnabled(Xp::ACLs))
if (auto localStore = dynamic_cast<LocalStore*>(&worker.store)) {
for (auto path : inputPaths) {
if (localStore->getAccessStatus(path).isProtected) {
if (!localStore->canAccess(path))
throw AccessDenied(
"%s (uid %d) does not have access to path %s",
getUserName(localStore->effectiveUser->uid),
localStore->effectiveUser->uid,
localStore->printStorePath(path));
localStore->grantBuildUserAccess(path, ACL::User(getuid()));
localStore->grantBuildUserAccess(path, ACL::User(sandboxUid()));
}
}
}

actLock.reset();

try {
Expand Down Expand Up @@ -265,17 +287,19 @@ static void chmod_(const Path & path, mode_t mode)
directory's parent link ".."). */
static void movePath(const Path & src, const Path & dst)
{
debug("Moving %s to %s", src, dst);
auto st = lstat(src);

bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR));
mode_t mode = st.st_mode;
if (experimentalFeatureSettings.isEnabled(Xp::ACLs))
mode &= ~S_IRWXO;

if (changePerm)
chmod_(src, st.st_mode | S_IWUSR);
chmod_(src, mode | (changePerm ? S_IWUSR : 0));

renameFile(src, dst);

if (changePerm)
chmod_(dst, st.st_mode);
chmod_(dst, mode);
}


Expand All @@ -298,6 +322,15 @@ void LocalDerivationGoal::closeReadPipes()

void LocalDerivationGoal::cleanupHookFinally()
{
if (experimentalFeatureSettings.isEnabled(Xp::ACLs)) {
if (auto localStore = dynamic_cast<LocalStore*>(&worker.store)) {
for (auto path : inputPaths) {
localStore->revokeBuildUserAccess(path, ACL::User(getuid()));
localStore->revokeBuildUserAccess(path, ACL::User(sandboxUid()));
}
}
}

/* Release the build user at the end of this function. We don't do
it right away because we don't want another build grabbing this
uid and then messing around with our output. */
Expand Down Expand Up @@ -358,8 +391,10 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
if (!status.known) continue;
if (buildMode != bmCheck && status.known->isValid()) continue;
auto p = worker.store.toRealPath(status.known->path);
if (pathExists(chrootRootDir + p))
if (pathExists(chrootRootDir + p)) {
deletePath(p);
renameFile((chrootRootDir + p), p);
}
}

return diskFull;
Expand Down Expand Up @@ -702,13 +737,11 @@ void LocalDerivationGoal::startBuilder()
if (buildUser && chown(chrootStoreDir.c_str(), 0, buildUser->getGID()) == -1)
throw SysError("cannot change ownership of '%1%'", chrootStoreDir);

// auto & localStore = getLocalStore();
for (auto & i : inputPaths) {
auto p = worker.store.printStorePath(i);
Path r = worker.store.toRealPath(p);
if (S_ISDIR(lstat(r).st_mode))
dirsInChroot.insert_or_assign(p, r);
else
linkOrCopy(r, chrootRootDir + p);
dirsInChroot.insert_or_assign(p, r);
}

/* If we're repairing, checking or rebuilding part of a
Expand Down Expand Up @@ -797,6 +830,7 @@ void LocalDerivationGoal::startBuilder()
/* Run the builder. */
printMsg(lvlChatty, "executing builder '%1%'", drv->builder);
printMsg(lvlChatty, "using builder args '%1%'", concatStringsSep(" ", drv->args));

for (auto & i : drv->env)
printMsg(lvlVomit, "setting builder env variable '%1%'='%2%'", i.first, i.second);

Expand Down Expand Up @@ -1470,9 +1504,15 @@ void LocalDerivationGoal::startDaemon()
auto workerThread = std::thread([store, remote{std::move(remote)}]() {
FdSource from(remote.get());
FdSink to(remote.get());
AuthenticatedUser user;
if (store->next->effectiveUser) {
user = {NotTrusted, store->next->effectiveUser->uid};
} else {
user = {NotTrusted, 0};
}
try {
daemon::processConnection(store, from, to,
NotTrusted, daemon::Recursive);
user, daemon::Recursive);
debug("terminated daemon connection");
} catch (SysError &) {
ignoreException();
Expand Down Expand Up @@ -1587,7 +1627,6 @@ void LocalDerivationGoal::chownToBuilder(const Path & path)
throw SysError("cannot change ownership of '%1%'", path);
}


void setupSeccomp()
{
#if __linux__
Expand Down Expand Up @@ -1804,22 +1843,52 @@ void LocalDerivationGoal::runChild()
filesystem that we want in the chroot
environment. */
auto doBind = [&](const Path & source, const Path & target, bool optional = false) {
debug("bind mounting '%1%' to '%2%'", source, target);
struct stat st;
if (stat(source.c_str(), &st) == -1) {
if (optional && errno == ENOENT)
auto doMount = [&](const Path & source, const Path & target) {
debug("bind mounting '%1%' to '%2%'", source, target);
struct stat st;
if (stat(source.c_str(), &st) == -1) {
if (optional && errno == ENOENT)
return;
else
throw SysError("getting attributes of path '%1%'", source);
}

if (S_ISDIR(st.st_mode))
createDirs(target);
else {
createDirs(dirOf(target));
writeFile(target, "");
}

if (mount(source.c_str(), target.c_str(), "", MS_BIND | MS_REC, 0) == -1)
throw SysError("bind mount from '%1%' to '%2%' failed", source, target);
};


if (experimentalFeatureSettings.isEnabled(Xp::ACLs) && worker.store.isInStore(source)) {
auto [storePath, subPath] = worker.store.toStorePath(source);

// TODO(ACL) Add tests to check that ACL information is never leaked
// FIXME probably should use a FUSE fs or something?
ssize_t eaSize = llistxattr(source.c_str(), nullptr, 0);
if (subPath == "" && eaSize > 0) {
// The source store path contains extended attributes
// mounting it as-is would preserve them, which is undesireable.
if (std::filesystem::is_directory(source)) {
createDirs(target); // In case the directory is empty
for (auto dirent : std::filesystem::directory_iterator(std::filesystem::directory_entry(source)))
doMount(dirent.path().c_str(), (target + "/" + baseNameOf(dirent.path().c_str())).c_str());
}
else {
std::filesystem::copy(source, target);
}
using namespace std::filesystem;
auto p = status(target).permissions();
permissions(target, (p | ((p & perms::owner_read) != perms::none ? perms::others_read : perms::none) | ((p & perms::owner_exec) != perms::none ? perms::others_exec : perms::none)), perm_options::add);
return;
else
throw SysError("getting attributes of path '%1%'", source);
}
if (S_ISDIR(st.st_mode))
createDirs(target);
else {
createDirs(dirOf(target));
writeFile(target, "");
}
}
if (mount(source.c_str(), target.c_str(), "", MS_BIND | MS_REC, 0) == -1)
throw SysError("bind mount from '%1%' to '%2%' failed", source, target);
doMount(source, target);
};

for (auto & i : dirsInChroot) {
Expand Down Expand Up @@ -2017,6 +2086,7 @@ void LocalDerivationGoal::runChild()
/* Add all our input paths to the chroot */
for (auto & i : inputPaths) {
auto p = worker.store.printStorePath(i);

dirsInChroot[p] = p;
}

Expand Down Expand Up @@ -2370,6 +2440,12 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
auto actualPath = toRealPathChroot(worker.store.printStorePath(*scratchPath));

auto finish = [&](StorePath finalStorePath) {
auto & localStore = getLocalStore();

StoreObjectDerivationOutput thisOutput(drvPath, outputName);
if (localStore.futurePermissions.contains(thisOutput)) {
localStore.setFutureAccessStatus(finalStorePath, localStore.futurePermissions[thisOutput]);
}
/* Store the final path */
finalOutputs.insert_or_assign(outputName, finalStorePath);
/* The rewrite rule will be used in downstream outputs that refer to
Expand Down Expand Up @@ -2653,12 +2729,15 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
worker.store.printStorePath(drvPath), worker.store.toRealPath(finalDestPath));
}

/* Since we verified the build, it's now ultimately trusted. */
/* Since we verified the build, it's now ultimately trusted, and we
can grant access to whoever requested the build */
if (!oldInfo.ultimate) {
oldInfo.ultimate = true;
localStore.signPathInfo(oldInfo);
localStore.registerValidPaths({{oldInfo.path, oldInfo}});
}
if (localStore.effectiveUser && !localStore.canAccess(oldInfo.path))
localStore.addAllowedEntities(oldInfo.path, {*localStore.effectiveUser});

continue;
}
Expand Down Expand Up @@ -2690,6 +2769,12 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
}

if (buildMode == bmCheck) {
auto & localStore = getLocalStore();
StoreObjectDerivationLog log { drvPath };
/* Since all outputs are known to be matching, give access to the log */
if (localStore.effectiveUser && !localStore.canAccess(log))
localStore.addAllowedEntities(log, {*localStore.effectiveUser});

/* In case of fixed-output derivations, if there are
mismatches on `--check` an error must be thrown as this is
also a source for non-determinism. */
Expand Down
Loading