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

Block access to files/paths outside of the sandbox #7936

Closed
Globegitter opened this issue Apr 3, 2019 · 3 comments
Closed

Block access to files/paths outside of the sandbox #7936

Globegitter opened this issue Apr 3, 2019 · 3 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request

Comments

@Globegitter
Copy link

Globegitter commented Apr 3, 2019

Description of the problem / feature request:

I have been running into some issue with a non-hermetic rule lately (see bazelbuild/rules_nodejs#612 for background) and one of the things that made it difficult to debug is that the node module resolving looks for modules outside of the sandbox and suceeds. The inputs are missing inside the sandbox. I found there is the flag --sandbox_block_path exactly for this use-case but it only accepts hardcoded paths. And what I would like to block ideally is everything outside the sandbox or if not then at least the external directory inside the output base (i.e. /home/$USER/.cache/bazel/bazel$USER//external/).

As a side node this feature does need sandboxfs so we do not have any symlinks outside the sandbox which would then make this fail.

Feature requests: what underlying problem are you trying to solve with this feature?

Prevent non-hermetic rules from working by accessing files outside the sandbox and indirectly also make the debugging easier as one will get an error instead of potentially surprising behaviour

I can put another case together demonstrating the issue, but basically the rollup_bundle rule used in bazelbuild/rules_nodejs#612, specifically https://github.com/bazelbuild/rules_nodejs/pull/612/files#diff-257614e46dda55801a16d01cdaf60a57R76 showcases the described behaviour.

What operating system are you running Bazel on?

Elementary OS 5.0 (Ubuntu 18.04)

What's the output of bazel info release?

release 0.24.0

Have you found anything relevant by searching the web?

https://stackoverflow.com/questions/43849651/how-to-lock-down-the-bazel-filesystem-sandbox
bazelbuild/sandboxfs#79
#7313
#4963
#6994

Any other information, logs, or outputs that you want to share?

It seems the flags discussed in #7313 or #6994 (comment) get implemented that would be another way of solving the underlying issue here.

@irengrig irengrig added team-Local-Exec Issues and PRs for the Execution (Local) team untriaged labels Apr 8, 2019
@jmmv
Copy link
Contributor

jmmv commented May 24, 2019

The sandboxfs requirement is the easy prerequisite for this. If we denied access to everything, we could then whitelist the targets of the symlinks. The only problem is if tools follow those symlinks and then try to do e.g. a readdir, and there are tools that do that... but conceivably we could fix this same thing with cow files or file copies.

Just saying. I agree that a stricter sandbox would be nice, but so far we have considered that the sandbox should not change the behavior of the rules. And from what you say, you want the sandbox to be stricter so that the rules behave differently when invoking nodejs. I think those nodejs invocations have to be fixed to not reach outside of the sandbox so that they can work with and without sandbox.

@jmmv jmmv added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request and removed untriaged labels May 24, 2019
@Globegitter
Copy link
Author

Yeah I suppose it is arguable if my feature request is about making rules behave differently within the sandbox. But what it is I really want is to have some way to universally error out as soon as possible when tools are misconfigured in a way that the access paths outside the sandbox, follow symlinks, or just just behave this way without any way to configure them. The reason for that is that I have a lot of developers (mostly in the js world but also just ran into an issue with pytest recnetly), who want to use their favourite (js) tool under bazel and it works for them outside but then does seemingly not work under bazel or they get it to work locally but then it fails in CI. And the typical response is that it is bazel's fault.

And yes sandboxfs/cow files will already help with one category of these issues (the symlink issue), but still not with tools accessing file paths outside of the sandbox. The latter issue took me a long time to figure out, but it turns out what a lot of node tools do is that they look for a third party dependency in a given node_modules dir (which is desired behaviour) but then if they do not find the third party dependency there, they traverse up to the root of the disk looking for additional node_modules directories to see if they can find the requested dependency there. And only error out once they have looked at all the traversed paths. So for example when developers did not specify all dependencies correctly running the tool might work on their local machine because they have some "stray" node_modules that has the dependencies the tool is looking for but then it would not work in CI anymore. This has lead some developers to react quite frustrated and as I mentioned blame bazel. I am not saying that it is bazel's fault and I did start to write plugins / patches etc for the tools to behave correctly as well as error out for the aforementioned cases. But I do think bazel can do better and I also think this will make things more obvious for developers and make them blame bazel less if they see an understandable error message. Also one would not have to provide this functionality for each new tool.

Hope this makes my point of view and feature request more understandable.

@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 16, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@sgowroji sgowroji closed this as not planned Won't fix, can't repro, duplicate, stale Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request
Projects
None yet
Development

No branches or pull requests

4 participants