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.

Sign up for GitHub

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

Fix "follows" paths in flake input lockfiles #4641

Merged
merged 2 commits into from
Aug 23, 2021
Merged

Fix "follows" paths in flake input lockfiles #4641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3c424f
Fix follows paths in subordinate lockfiles
RealityAnomaly Jul 12, 2021
57b9ba0
Add tests for flake follow paths
RealityAnomaly Jul 12, 2021
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
67 changes: 51 additions & 16 deletions src/libexpr/flake/flake.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,21 +329,22 @@ LockedFlake lockFlake(
const FlakeInputs & flakeInputs,
std::shared_ptr<Node> node,
const InputPath & inputPathPrefix,
std::shared_ptr<const Node> oldNode)>
std::shared_ptr<const Node> oldNode,
const LockParent parent, const Path parentPath)>
computeLocks;

computeLocks = [&](
const FlakeInputs & flakeInputs,
std::shared_ptr<Node> node,
const InputPath & inputPathPrefix,
std::shared_ptr<const Node> oldNode)
std::shared_ptr<const Node> oldNode,
const LockParent parent, const Path parentPath)
{
debug("computing lock file node '%s'", printInputPath(inputPathPrefix));

/* Get the overrides (i.e. attributes of the form
'inputs.nixops.inputs.nixpkgs.url = ...'). */
// FIXME: check this
for (auto & [id, input] : flake.inputs) {
for (auto & [id, input] : flakeInputs) {
for (auto & [idOverride, inputOverride] : input.overrides) {
auto inputPath(inputPathPrefix);
inputPath.push_back(id);
Expand Down Expand Up @@ -379,15 +380,23 @@ LockedFlake lockFlake(
path we haven't processed yet. */
if (input.follows) {
InputPath target;
if (hasOverride || input.absolute)
/* 'follows' from an override is relative to the
root of the graph. */

if (parent.absolute && !hasOverride) {
target = *input.follows;
else {
/* Otherwise, it's relative to the current flake. */
target = inputPathPrefix;
} else {
if (hasOverride)
{
target = inputPathPrefix;
target.pop_back();
}
else
{
target = parent.path;
}

for (auto & i : *input.follows) target.push_back(i);
}

debug("input '%s' follows '%s'", inputPathS, printInputPath(target));
node->inputs.insert_or_assign(id, target);
continue;
Expand Down Expand Up @@ -433,7 +442,7 @@ LockedFlake lockFlake(
if (hasChildUpdate) {
auto inputFlake = getFlake(
state, oldLock->lockedRef, false, flakeCache);
computeLocks(inputFlake.inputs, childNode, inputPath, oldLock);
computeLocks(inputFlake.inputs, childNode, inputPath, oldLock, parent, parentPath);
} else {
/* No need to fetch this flake, we can be
lazy. However there may be new overrides on the
Expand All @@ -450,12 +459,11 @@ LockedFlake lockFlake(
} else if (auto follows = std::get_if<1>(&i.second)) {
fakeInputs.emplace(i.first, FlakeInput {
.follows = *follows,
.absolute = true
});
}
}

computeLocks(fakeInputs, childNode, inputPath, oldLock);
computeLocks(fakeInputs, childNode, inputPath, oldLock, parent, parentPath);
}

} else {
Expand All @@ -467,7 +475,18 @@ LockedFlake lockFlake(
throw Error("cannot update flake input '%s' in pure mode", inputPathS);

if (input.isFlake) {
auto inputFlake = getFlake(state, *input.ref, useRegistries, flakeCache);
Path localPath = parentPath;
FlakeRef localRef = *input.ref;

// If this input is a path, recurse it down.
// This allows us to resolve path inputs relative to the current flake.
if (localRef.input.getType() == "path")
{
localRef.input.parent = parentPath;
localPath = canonPath(parentPath + "/" + *input.ref->input.getSourcePath());
}

auto inputFlake = getFlake(state, localRef, useRegistries, flakeCache);

/* Note: in case of an --override-input, we use
the *original* ref (input2.ref) for the
Expand All @@ -488,6 +507,13 @@ LockedFlake lockFlake(
parents.push_back(*input.ref);
Finally cleanup([&]() { parents.pop_back(); });

// Follows paths from existing inputs in the top-level lockfile are absolute,
// whereas paths in subordinate lockfiles are relative to those lockfiles.
LockParent newParent {
.path = inputPath,
.absolute = oldLock ? true : false
};

/* Recursively process the inputs of this
flake. Also, unless we already have this flake
in the top-level lock file, use this flake's
Expand All @@ -497,7 +523,8 @@ LockedFlake lockFlake(
oldLock
? std::dynamic_pointer_cast<const Node>(oldLock)
: LockFile::read(
inputFlake.sourceInfo->actualPath + "/" + inputFlake.lockedRef.subdir + "/flake.lock").root);
inputFlake.sourceInfo->actualPath + "/" + inputFlake.lockedRef.subdir + "/flake.lock").root,
newParent, localPath);
}

else {
Expand All @@ -515,9 +542,17 @@ LockedFlake lockFlake(
}
};

LockParent parent {
.path = {},
.absolute = true
};

// Bring in the current ref for relative path resolution if we have it
auto parentPath = canonPath(flake.sourceInfo->actualPath + "/" + flake.lockedRef.subdir);

computeLocks(
flake.inputs, newLockFile.root, {},
lockFlags.recreateLockFile ? nullptr : oldLockFile.root);
lockFlags.recreateLockFile ? nullptr : oldLockFile.root, parent, parentPath);

for (auto & i : lockFlags.inputOverrides)
if (!overridesUsed.count(i.first))
Expand Down
10 changes: 9 additions & 1 deletion src/libexpr/flake/flake.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ struct FlakeInput
std::optional<FlakeRef> ref;
bool isFlake = true; // true = process flake to get outputs, false = (fetched) static source path
std::optional<InputPath> follows;
bool absolute = false; // whether 'follows' is relative to the flake root
FlakeInputs overrides;
};

Expand Down Expand Up @@ -125,6 +124,15 @@ struct LockFlags
std::set<InputPath> inputUpdates;
};

struct LockParent {
/* The path to this parent */
InputPath path;

/* Whether we are currently inside a top-level lockfile (inputs absolute)
or subordinate lockfile (inputs relative) */
bool absolute;
};

LockedFlake lockFlake(
EvalState & state,
const FlakeRef & flakeRef,
Expand Down
3 changes: 3 additions & 0 deletions src/libfetchers/fetchers.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ struct Input
bool immutable = false;
bool direct = true;

/* path of the parent of this input, used for relative path resolution */
std::optional<Path> parent;

public:
static Input fromURL(const std::string & url);

Expand Down
22 changes: 19 additions & 3 deletions src/libfetchers/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,34 @@ struct PathInputScheme : InputScheme

std::pair<Tree, Input> fetch(ref<Store> store, const Input & input) override
{
std::string absPath;
auto path = getStrAttr(input.attrs, "path");

// FIXME: check whether access to 'path' is allowed.
if (path[0] != '/' && input.parent)
{
auto parent = canonPath(*input.parent);

// the path isn't relative, prefix it
absPath = canonPath(parent + "/" + path);

auto storePath = store->maybeParseStorePath(path);
// for security, ensure that if the parent is a store path, it's inside it
if (!parent.rfind(store->storeDir, 0) && absPath.rfind(store->storeDir, 0))
throw BadStorePath("relative path '%s' points outside of its parent's store path %s, this is a security violation", path, parent);
}
Comment on lines +95 to +98
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it looks like this causes subflakes that reference the parent flake to error out.

For example, https://github.com/divnix/digga/blob/56f94dbdaaac5d3c63eed1def0fc4760ec447a38/examples/groupByConfig/flake.nix#L8 implements a test flake for the "parent" flake library.

I guess we now would have to decide if this particular case is a valid use case.

I guess this error was implemented to fairly advertise the import ../../, which indeed is illegal. (Nice that you fixed this!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a complete error output on our CI for additional context:

$ check-downstream --show-trace
warning: Git tree '/home/blaggacao/ghq/github.com/divnix/digga/examples/downstream' is dirty
error: relative path '../../' points outside of its parent's store path /nix/store/24vs6gdb680393b0aidh4fyp3ijyshd7-source, this is a security violation

       … while fetching the input 'path:../../'

       … while updating the flake input 'digga'

       … while updating the lock file of flake 'git+file:///home/blaggacao/ghq/github.com/divnix/digga/examples/downstream'
exit 1: command [/nix/store/j3mddzlpyapqhx1443dp5hq707w92dkj-nix-2.4pre20210601_5985b8b/bin/nix flake lock --update-input digga "$@"] failed

Copy link
Contributor

@blaggacao blaggacao Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error seems more contrived than I initially thought.

It seems that input.parent condition is triggered only when I replace the digga reference of the downstream submodule https://github.com/divnix/digga/tree/56f94dbdaaac5d3c63eed1def0fc4760ec447a38/examples with path:../../ but the error doesn't occur, with https://github.com/divnix/digga/blob/56f94dbdaaac5d3c63eed1def0fc4760ec447a38/examples/groupByConfig/flake.nix#L8

I have to find out more about input.parent, to give a better error report.

This change-set divnix/digga@2a95a9c works around the problem by avoiding the use of path:../../ in favor of a proper URL reference in that particular case.

Please note. that the disadvantage of this workarond is that I now need to push the current branch first to github.com/divnix/digga (I need push access) in order to run local tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I analyzed a bit further and the impression I'm getting is that happens only when a git submodule boundary is crossed. //cc @nrdxp

I guess, this is another pair of shoes and not part of this PR.

else
{
absPath = path;
}

// FIXME: check whether access to 'path' is allowed.
auto storePath = store->maybeParseStorePath(absPath);

if (storePath)
store->addTempRoot(*storePath);

if (!storePath || storePath->name() != "source" || !store->isValidPath(*storePath))
// FIXME: try to substitute storePath.
storePath = store->addToStore("source", path);
storePath = store->addToStore("source", absPath);

return {
Tree(store->toRealPath(*storePath), std::move(*storePath)),
Expand Down
95 changes: 93 additions & 2 deletions tests/flakes.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,15 @@ nonFlakeDir=$TEST_ROOT/nonFlake
flakeA=$TEST_ROOT/flakeA
flakeB=$TEST_ROOT/flakeB
flakeGitBare=$TEST_ROOT/flakeGitBare
flakeFollowsA=$TEST_ROOT/follows/flakeA
flakeFollowsB=$TEST_ROOT/follows/flakeA/flakeB
flakeFollowsC=$TEST_ROOT/follows/flakeA/flakeB/flakeC
flakeFollowsD=$TEST_ROOT/follows/flakeA/flakeD
flakeFollowsE=$TEST_ROOT/follows/flakeA/flakeE

for repo in $flake1Dir $flake2Dir $flake3Dir $flake7Dir $templatesDir $nonFlakeDir $flakeA $flakeB; do
for repo in $flake1Dir $flake2Dir $flake3Dir $flake7Dir $templatesDir $nonFlakeDir $flakeA $flakeB $flakeFollowsA; do
rm -rf $repo $repo.tmp
mkdir $repo
mkdir -p $repo
git -C $repo init
git -C $repo config user.email "foobar@example.com"
git -C $repo config user.name "Foobar"
Expand Down Expand Up @@ -681,3 +686,89 @@ git -C $flakeB commit -a -m 'Foo'

# Test list-inputs with circular dependencies
nix flake metadata $flakeA

# Test flake follow paths
mkdir -p $flakeFollowsB
mkdir -p $flakeFollowsC
mkdir -p $flakeFollowsD
mkdir -p $flakeFollowsE

cat > $flakeFollowsA/flake.nix <<EOF
{
description = "Flake A";
inputs = {
B = {
url = "path:./flakeB";
inputs.foobar.follows = "D";
};

D.url = "path:./flakeD";
foobar.url = "path:./flakeE";
};
outputs = { ... }: {};
}
EOF

cat > $flakeFollowsB/flake.nix <<EOF
{
description = "Flake B";
inputs = {
foobar.url = "path:./../flakeE";
C = {
url = "path:./flakeC";
inputs.foobar.follows = "foobar";
};
};
outputs = { ... }: {};
}
EOF

cat > $flakeFollowsC/flake.nix <<EOF
{
description = "Flake C";
inputs = {
foobar.url = "path:./../../flakeE";
};
outputs = { ... }: {};
}
EOF

cat > $flakeFollowsD/flake.nix <<EOF
{
description = "Flake D";
inputs = {};
outputs = { ... }: {};
}
EOF

cat > $flakeFollowsE/flake.nix <<EOF
{
description = "Flake D";
inputs = {};
outputs = { ... }: {};
}
EOF

git -C $flakeFollowsA add flake.nix flakeB/flake.nix \
flakeB/flakeC/flake.nix flakeD/flake.nix flakeE/flake.nix

nix flake lock $flakeFollowsA

[[ $(jq -c .nodes.B.inputs.C $flakeFollowsA/flake.lock) = '"C"' ]]
[[ $(jq -c .nodes.B.inputs.foobar $flakeFollowsA/flake.lock) = '["D"]' ]]
[[ $(jq -c .nodes.C.inputs.foobar $flakeFollowsA/flake.lock) = '["B","foobar"]' ]]

# Ensure a relative path is not allowed to go outside the store path
cat > $flakeFollowsA/flake.nix <<EOF
{
description = "Flake A";
inputs = {
B.url = "path:./../../flakeB";
};
outputs = { ... }: {};
}
EOF

git -C $flakeFollowsA add flake.nix

nix flake lock $flakeFollowsA 2>&1 | grep 'this is a security violation'