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

Move restricted/pure-eval access control out of the evaluator and into the accessor #9497

Merged
merged 10 commits into from
Dec 8, 2023

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Nov 30, 2023

Motivation

Extracted from lazy-trees.

Context

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 30, 2023
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Nov 30, 2023
@Ericson2314
Copy link
Member

Didn't read all the code while it is still draft, but the concept is quite nice!

@edolstra edolstra marked this pull request as ready for review November 30, 2023 21:47
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Symlinks strike again.
Otherwise looks like the right direction.

src/libexpr/primops.cc Show resolved Hide resolved
src/libfetchers/filtering-input-accessor.hh Show resolved Hide resolved
src/libfetchers/filtering-input-accessor.hh Outdated Show resolved Hide resolved
tests/functional/restricted.sh Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

We have a regression with symlinks. Needs a test case.
Currently:

ls -l tunnel.d/
total 4
lrwxrwxrwx 1 user user 2 Nov 30 23:46 tunnel -> ..

$ ./result/bin/nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./tunnel.d; } ]; in builtins.readFile <foo/tunnel/aliens-truth>' -I tunnel.d
"They're among us.\n"

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added some tests and PosixSourceAccessor now aggressively checks against symlinks.

edolstra and others added 6 commits December 5, 2023 16:33
All path components must not be symlinks now (so the user needs to
call `resolveSymlinks()` when needed).
Since we're doing a lot of them in assertNoSymlinks().
@roberth roberth self-requested a review December 8, 2023 20:41
@roberth roberth merged commit d4f6b1d into NixOS:master Dec 8, 2023
7 of 8 checks passed
@infinisil
Copy link
Member

This caused a regression: #9901

@infinisil infinisil mentioned this pull request Feb 19, 2024
9 tasks
srid added a commit to srid/nixos-config that referenced this pull request Mar 26, 2024
gador added a commit to gador/bento that referenced this pull request Oct 26, 2024
on newer nix versions (> 2.18) the "path:" settings
will lead to evaluation errors when the flake uses
symbolic links.

a typical error message would be:

`error: access to absolute path '/lib' is forbidden in pure evaluation
mode (use '--impure' to override)`

when `/lib` actually is `./lib`.

When "path:" is replaced by just using the flake's path
no evaluation error is shown. As per the man page of `nix flake`
the "path" attribute reffers to the local path of the flake.

This can just be removed (AFAIK) by referencing to the path as a
positional argument.

Possible related issues:
NixOS/nix#11030
original PR introducing the error message NixOS/nix#9497

Signed-off-by: Florian Brandes <[email protected]>
huang12zheng pushed a commit to huang12zheng/nixos-config that referenced this pull request Dec 5, 2024
@edolstra edolstra deleted the move-access-control branch December 9, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants