Sandbox does not ensure working environment

[gtristan]

There needs to be some accountability for ensuring that things work when running a sandbox.

For instance, when running a specific command in a sandbox, it should be the caller's responsibility to ensure that the command is in fact staged, and referred to by it's full path, or, has setup the PATH variable correctly.

However sandboxlib itself also runs some code in the sandbox, a side effect of not taking care of this is described in this comment on ybd issue 224:

devcurmudgeon/ybd#224 (comment)

In the above case, we find that when the python code itself fails to launch something, in this case it is because python received ENOENT (no such file or directory) for the program which the calling code asked to execute (in the above case, caller wanted to launch a shell, but did not provide a shell), then sandboxlib fails to deliver the error message / exception to the caller.

This is because sandboxlib does not take care of providing a suitable python environment for it's own code to run in the sandbox, in ybd issue 224; we have a case where the caller happened to stage python3, which caused sandboxlib to mostly work mostly by coincidence - except when it tries to report an error because of missing 'string-escape' encoding library expected to be built into python 2.7.

Surely in the above case, the caller is at fault for trying to invoke a shell which it did not provide, but sandboxlib must also be responsible for ensuring it's own runtime requirements in it's own sandbox.

This seems related with #17

I had hoped calling os.chroot() directly from Python would mean that we didn't need to care what's inside the sandbox. But this 'string-escape' problem suggests I was wrong. The tricksy part is that if we want to allow setting the current working directory inside the sandbox, then there needs to be a way of running code after we call chroot(). That rules out using /usr/bin/chroot unless we want to mandate that /bin/sh -c 'cd $dir; $run_sandboxed_command is possible inside whatever sandbox we're given.

Perhaps we could statically compile a simple C program that we insert into the sandbox ourselves, which is capable of changing directory then running the actual command we want? Then sandboxlib depends on the host C compiler, of course, but I guess if you want to use Baserock you need a host C compiler regardless. It would mean that sandboxlib itself wouldn't need to mandate anything about the contents of the sandbox, which I think is a good thing... I don't think we should get into trying to second guess what the caller wants, the OS should give a clear enough error if the caller tells us to run a program that doesn't exist... (modulo sandboxlib hiding that error, of course, but that's a bug which is hopefully now fixed)

[gtristan]

Indeed, I did not see issue #17 but it's the same issue I guess.

I wonder if we could work around this issue by forcing python to load everything it's going to need before the chroot, maybe with an explicit 'import' or such.

"(modulo sandboxlib hiding that error, of course, but that's a bug which is hopefully now fixed)"

The above is not fixed without fixing this, unless the sandbox happens to have python2.7 staged - in fact the main and possibly only side effect of this bug is that the child process cannot serialize the exception to report to the parent process without the string-escape module loaded.

Note the sandbox python code which runs in the sandbox is not much at all, and the major issue is that when the python code itself hits an exception, it will use 'pickle' to serialize the exception which we want to send back to the parent process through the pipe. At that point pickle will need string-escape encoding to do some of the work of serializing that exception.

As long as we can assert that the python process already has everything it's going to need in memory before forking (or threading, not sure what it's doing here), then the child process will not need to lookup any python modules in the sandbox.

[devcurmudgeon]

In other conversations the topic of Flatpak came up.... might there be any value in considering using Flatpak for sandboxing somehow, and deprecating sandboxlib, or is there still a clear need for this?

[gtristan]

Currently, when the python code hits an exception and python2.7 is not staged:

Traceback (most recent call last):
  File "/store/BASEROCK/ybd/ybd/sandbox.py", line 170, in run_sandboxed
    env=env, **config)
  File "/store/BASEROCK/sandboxlib/sandboxlib/chroot.py", line 242, in run_sandbox_with_redirection
    exit, out, err = run_sandbox(command, **sandbox_config)
  File "/store/BASEROCK/sandboxlib/sandboxlib/chroot.py", line 239, in run_sandbox
    raise Exception ('Received exception from chroot, child process traceback:\n%s\n' % tb)
Exception: Received exception from chroot, child process traceback:
Traceback (most recent call last):
  File "/store/BASEROCK/sandboxlib/sandboxlib/chroot.py", line 195, in run_command_in_chroot
  File "/store/BASEROCK/sandboxlib/sandboxlib/__init__.py", line 260, in _run_command
  File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
  File "/usr/lib/python2.7/subprocess.py", line 1339, in _execute_child
  File "/usr/lib/python2.7/pickle.py", line 1388, in loads
  File "/usr/lib/python2.7/pickle.py", line 864, in load
  File "/usr/lib/python2.7/pickle.py", line 977, in load_string
LookupError: unknown encoding: string-escape

And I found a HACK !!!

The following is the desired stack trace/exception, which could be made a bit more informative if run_command_in_chroot() could include the attempted command in the reported exception:

Traceback (most recent call last):
  File "/store/BASEROCK/ybd/ybd/sandbox.py", line 170, in run_sandboxed
    env=env, **config)
  File "/store/BASEROCK/sandboxlib/sandboxlib/chroot.py", line 242, in run_sandbox_with_redirection
    exit, out, err = run_sandbox(command, **sandbox_config)
  File "/store/BASEROCK/sandboxlib/sandboxlib/chroot.py", line 239, in run_sandbox
    raise Exception ('Received exception from chroot, child process traceback:\n%s\n' % tb)
Exception: Received exception from chroot, child process traceback:
Traceback (most recent call last):
  File "/store/BASEROCK/sandboxlib/sandboxlib/chroot.py", line 195, in run_command_in_chroot
  File "/store/BASEROCK/sandboxlib/sandboxlib/__init__.py", line 260, in _run_command
  File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
  File "/usr/lib/python2.7/subprocess.py", line 1340, in _execute_child
OSError: [Errno 2] No such file or directory

I was able to get that exception printed without python 2.7 staged with the following hack:

    with mount_all(filesystem_root, extra_mounts):

+        # Awful hack to ensure string-escape is loaded !
+        unused = "Some Text".encode('string-escape')

        process = multiprocessing.Process(
            target=run_command_in_chroot,
            args=(pipe_child, stdout, stderr, extra_mounts, filesystem_root,
                  command, cwd, env))
        process.start()
        process.join()

Before launching the process, we just encode something with string-escape and force python to load the module, could be a viable short term at least...

[gtristan]

@devcurmudgeon: We could look into using bubblewrap, which is the part of flatpak which could take care of sandboxing - we should look into using this specifically for avoiding the need of root privileges, needs some investigation.

But jumping on that right away is tricky because it's also a C compiled host tool to depend on, and not widely distributed yet either; using sandboxlib at least in the short term alleviates the pain by being relatively portable python code.

Another route might be to consider yet-another-sandboxlib-backend which uses bubblewrap if available, I have to investigate to what extent we can use root privileges in a bubblewrap sandbox though, I need to see it create and tar up a root owned file to believe it, but should be possible.

bubblewrap (the core sandboxing helper tool of flatpak) is basically an improved linux-user-chroot. The original motivation for sandboxlib was that linux-user-chroot was (a) Linux only and (b) not widely packaged even on Linux. With Flatpak growing in popularity, (b) is going to stop being a problem eventually. I've no idea if anyone has plans to work on (a), and it wouldn't be trivial.

So basically the question is whether you still care about YBD working on Mac OSX (and *BSD) -- if not, you can switch back to just bubblewrap / linux-user-chroot right now.

@gtristan, the string-escape hack looks like a good immediate solution, good find!

[gtristan]

Created pull request for string-escape hack:

#20

[devcurmudgeon]

@ssssam in practice i think no-one is using YBD on anything except Linux. For MacOS I use Vagrant into a Linux VM and it works fine.

So I think I'd be happy with dropping the MacOS requirement for building... but i'm doing some testing/development on MacOS for earlier stages (eg parsing definiitions, cache-key calculation, concourse pipeline creation etc)

OK. I'd recommend dropping sandboxlib altogether then and using Bubblewrap directly. It'll be packaged in any distro that has packaged Flatpak. I've not actually tried Bubblewrap yet, but it should be able to do everything linux-user-chroot could do and more. I believe that build sandboxing is a specific use case, too, so if we need to add anything I imagine they'll be receptive to pull requests.

That said, I hope Tristan's pull request solves the issue of sandboxlib eating the real exception, in which case there would be no harm in continuing to maintain sandboxlib... the only benefit would be that we could fall back to chroot() on systems that don't have Bubblewrap installed, but I think such systems will become fewer and fewer over the next few years.