See bug 161041, http://marc.theaimsgroup.com/?l=gentoo-user-de&m=115148403700916&w=2 ebuild(5) claims: RESTRICT = [strip,mirror,fetch,userpriv] ... sandbox disable sandbox (do not use without very good reason). However a quick glance at portage.py shows RESTRICT=sandbox is never checked. Which way are we going to let this fall - change the docs, or change portage.py?
Created attachment 106133 [details, diff] respect-restrict-sandbox.patch Well, this is what I think.
OK, that works, but bug 161041 will still exist because actionmap has actionmap = { "depend": {"cmd":ebuild_sh, "args":{"droppriv":1, "free":0, "sesandbox": 0}}, "setup": {"cmd":ebuild_sh, "args":{"droppriv":0, "free":1, "sesandbox": 0}}, "unpack": {"cmd":ebuild_sh, "args":{"droppriv":1, "free":0, "sesandbox": sesandbox}}, "compile":{"cmd":ebuild_sh, "args":{"droppriv":1, "free":nosandbox, "sesandbox": sesandbox}}, "test": {"cmd":ebuild_sh, "args":{"droppriv":1, "free":nosandbox, "sesandbox": sesandbox}}, "install":{"cmd":ebuild_sh, "args":{"droppriv":0, "free":0, "sesandbox": sesandbox}}, "rpm": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox": 0}}, "package":{"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox": 0}}, } For RESTRICT="sandbox" to do the Right Thing, it'll need to be able to disable sandbox in unpack and install phases. Should we set "free":nosandbox for those phases, or introduce a "restrictsandbox" variable? OK, nosandbox is currently about userpriv (and usersandbox). Does userpriv turning off sandbox make sense in unpack and install?
Answering my own question: yes for unpack, no for install; root can't drop to portage:portage for src_install as it needs to be able to create various-owned and setuid/setgid binaries. So it looks like we will need a restrictsandbox variable.
Created attachment 106137 [details, diff] respect-restrict-sandbox.patch Updated patch. Also killed some unreached code. I'm not entirely happy with actionmap, though; there's some logic in there that could be more declarative.
Created attachment 106141 [details, diff] alternate-respect-restrict-sandbox.patch More logic-based version.
you should also post a patch to repoman that prevents people from setting SANDBOX_ON (declare it a readonly var to check in repoman)
I think we need to provide a way for the user to filter packages with sandbox restriction. If they have sandbox enabled, then RESTRICT=sandbox shouldn't disable it without the user's permission.
i dont quite follow everyone has FEATURES=sandbox as it's in all of our profiles with that logic, users will be unable to emerge these two packages that are completely broken inside of the sandbox (gcl and emacs)
(In reply to comment #8) > with that logic, users will be unable to emerge these two packages that are > completely broken inside of the sandbox (gcl and emacs) They'll be able to emerge them if they manually disable sandbox or set some flag that indicates to emerge/portage that packages with RESTRICT=sandbox are okay. For example, we could introduce a new variable called ACCEPT_RESTRICT. If the user has ACCEPT_RESTRICT="sandbox", then packages with RESTRICT=sandbox will be silently installed even if the user has FEATURES=sandbox enabled. ACCEPT_RESTRICT would also be useful for other restrictions such as ebuilds that require interactivity (bug #151113) or ebuilds that restrict userpriv (bug #159876).
(In reply to comment #6) > you should also post a patch to repoman that prevents people from setting > SANDBOX_ON (declare it a readonly var to check in repoman) > The primary purpose of repoman (IMHO) is to detect common mistakes. I don't really see a check like this being very useful (although probably fairly trivial to implement). How many poeple do this now?
(In reply to comment #9) > For example, we could introduce a new variable called ACCEPT_RESTRICT. If the > user has ACCEPT_RESTRICT="sandbox", then packages with RESTRICT=sandbox will be > silently installed even if the user has FEATURES=sandbox enabled. Wouldn't this need to be settable per-package, though? Can we cope with an /etc/portage/package.accept_restrict?
(In reply to comment #8) > everyone has FEATURES=sandbox as it's in all of our profiles > > with that logic, users will be unable to emerge these two packages that are > completely broken inside of the sandbox (gcl and emacs) What's the point of making a rather dangerous RESTRICT valid? For these two packages only? Nothing else broken in this way should ever be committed to the tree, and noone should encourage developers to fiddle with sandbox in as easy way as is RESTRICT=sandbox. SANDBOX_ON is an undocumented hack and should remain as such, we don't need RESTRICT=sandbox at all.
Created attachment 106320 [details] ebuild.5-no-restrict-sandbox.patch Well, I thought it had been documented for a reason. No problem to remove, though.
the primary purpose of repoman is to make sure bad QA doesnt make it into the tree whether it's common is irrelevant (how can you justify labeling USE as a readonly var in that vein of thinking ? how common was it for ebuilds to set that ?)
Removed RESTRICT=sandbox from ebuild(5) until this bug is resolved.
Anyone object if I close this?
PMS specifies what the package manager must do when RESTRICT=sandbox is set: "The sandbox tool must not be used when building the package." As I see it, we have the following possibilities here: 1. Make Portage compliant with what PMS specifies. 2. Remove RESTRICT=sandbox from PMS, and document the SANDBOX_ON variable.
(In reply to Ulrich Müller from comment #17) imo we should drop RESTRICT=sandbox. i haven't seen a compelling case yet where it should be supported for an entire package. every case in the tree is a matter of limited surgery to deal with a specific problem in the package they're building. there's no reason at all we should ever consider letting someone add an ebuild using RESTRICT=sandbox to the main tree. yes, we could have repoman reject it every time out of hand, but i don't think the reasonable real world needs of packages even line up with RESTRICT. here's a survey of the whole tree: * app-editors/emacs: (limited to src_compile) see bug 131505; most likely due to the insane build system they have where they dump a memory image of itself during build time and then reload that at runtime * app-editors/emacs-vcs: same as above * dev-lisp/cmucl: (limited to src_compile) no idea why, but it's only in the old versions, so it'll get cleaned out * virtualx.eclass: (limited to `virtualmake`) looks like a hack to spawn an Xvfb server for building purposes * sys-libs/libunwind: the testsuite involves probing its own memory/backtraces at least for ecmacs/libunwind, they don't want to turn off the sandbox because they need to violate paths. they do it because the current sandbox implementation is a LD_PRELOAD library that is in the same memory space. a transparent sandbox (like ptrace, or seccomp) would still be fine. i can't speak to the exact needs of virtualx though. if they truly wanted to violate permission restrictions, they could have used the already existing `addwrite /` escape hatch. SANDBOX_ON is an ugly hack that is a pita to use (it's more a sandbox implementation detail leaking). whether we standardize that exact env var or come up with another command to go alongside `addwrite` and friends, i have no opinion. the backwards compat aspect would be trivial to deal with by adding a wrapper in eutils.eclass.
I suggest to drop RESTRICT=sandbox from PMS. Retroactively, because it is neither used in the tree nor supported by Portage. And for the next EAPI: 1. Add a sandboxoff command. 2. Limit the scope of all sandbox commands to the current phase. 3. Add a pair of sandboxsave and sandboxrestore commands that would save/restore the current state. This is to avoid such constructs as we have in cvs.eclass: http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/eclass/cvs.eclass?revision=1.84&view=markup#l276 Alternatively, these could be sandboxpush/sandboxpop, but I'm not sure if nesting would be needed.
(In reply to Ulrich Müller from comment #19) > I suggest to drop RESTRICT=sandbox from PMS. Retroactively, because it is > neither used in the tree nor supported by Portage. Sounds good. Resolving as wontfix. > And for the next EAPI: > 1. Add a sandboxoff command. > 2. Limit the scope of all sandbox commands to the current phase. > 3. Add a pair of sandboxsave and sandboxrestore commands that would > save/restore the current state. This is to avoid such constructs as we have > in cvs.eclass: > http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/eclass/cvs. > eclass?revision=1.84&view=markup#l276 > Alternatively, these could be sandboxpush/sandboxpop, but I'm not sure if > nesting would be needed. We might also consider adding a variable defined by PMS which refers to a persistent writable directory for vcs eclasses to use, with no need for sandbox interaction.
RESTRICT=sandbox dropped from PMS: http://gitweb.gentoo.org/proj/pms.git/commit/?id=4055e2091ab22d34b45790462513bbaf3d70a1bf
