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

Sandbox: Always mount every directories under / (gets rid of OPAM_USER_PATH_RO) #4795

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

kit-ty-kate
Copy link
Member

Alternative fix for #4783 and #4751.

This also helps make Linux and macOS sandbox behave the same: by default sandbox-exec mounts the whole file system (including external disks) readonly and the OPAM_USER_PATH_RO related code did not do anything.

I don't think there is any need to restrict access to non-standard directories since it is all read-only.
This also helps users on NixOS for instance as well as people using several disks/mountpoints to host opam as highlighted by the two issues above.

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

This will probably be better and easier to maintain in the long run 👍

Do we want to leave access to /sys ? (Real question, I am not sure of the implications)

src/state/shellscripts/bwrap.sh Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

Do we want to leave access to /sys ? (Real question, I am not sure of the implications)

No idea either but that's a fair point and I think since it was also disabled before, it's more prudent to keep it that way. Thanks!

All the concerns and comments have been addressed I think. Everything should be green.

@rjbou rjbou added this to the 2.2.0~alpha milestone Sep 2, 2021
@rjbou rjbou merged commit 65ea1c3 into ocaml:master Sep 13, 2021
@dra27 dra27 mentioned this pull request Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants