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

Reuse the same rootfs for the inner and outer container #1048

Open
apyrgio opened this issue Jan 9, 2025 · 0 comments · May be fixed by #1049
Open

Reuse the same rootfs for the inner and outer container #1048

apyrgio opened this issue Jan 9, 2025 · 0 comments · May be fixed by #1049
Assignees
Labels
container enhancement New feature or request
Milestone

Comments

@apyrgio
Copy link
Contributor

apyrgio commented Jan 9, 2025

When building the Dangerzone container image, we use multi-stage builds, where we create the inner container first, and then copy it to the outer container, under /home/dangerzone/dangerzone-image/rootfs:

COPY --from=dangerzone-image / /home/dangerzone/dangerzone-image/rootfs

This was the most clean way to use nested containers, and was implemented during #590. Now that we plan to use Debian Stable for the base image of Dangerzone (see #1046), it would be nice to merge these two container images into one, to avoid the double overhead of the Debian container image.

I have experimented with a way we can achieve this, and the following seem to work:

  1. Remove the /home/dangerzone/dangerzone-image/ dir, and write instead the OCI config to /config.json, i.e., at the root of the container image.
  2. Change the OCI config to use the container root (/) as the image bundle, instead of /home/dangerzone/dangerzone-image/rootfs.

These changes are reflected in the following diff:

--- a/dangerzone/gvisor_wrapper/entrypoint.py
+++ b/dangerzone/gvisor_wrapper/entrypoint.py
@@ -56,7 +56,7 @@ oci_config: dict[str, typing.Any] = {
             {"type": "RLIMIT_NOFILE", "hard": 4096, "soft": 4096},
         ],
     },
-    "root": {"path": "rootfs", "readonly": True},
+    "root": {"path": "/", "readonly": True},
     "hostname": "dangerzone",
     "mounts": [
         {
@@ -133,7 +142,7 @@ if os.environ.get("RUNSC_DEBUG"):
     json.dump(oci_config, sys.stderr, indent=2, sort_keys=True)
     # json.dump doesn't print a trailing newline, so print one here:
     log("")
-with open("/home/dangerzone/dangerzone-image/config.json", "w") as oci_config_out:
+with open("/config.json", "w") as oci_config_out:
     json.dump(oci_config, oci_config_out, indent=2, sort_keys=True)
 
 # Run gVisor.
@@ -150,7 +159,7 @@ if os.environ.get("RUNSC_DEBUG"):
     runsc_argv += ["--debug=true", "--alsologtostderr=true"]
 if os.environ.get("RUNSC_FLAGS"):
     runsc_argv += [x for x in shlex.split(os.environ.get("RUNSC_FLAGS", "")) if x]
-runsc_argv += ["run", "--bundle=/home/dangerzone/dangerzone-image", "dangerzone"]
+runsc_argv += ["run", "--bundle=/", "dangerzone"]
 log(
     "Running gVisor with command line: {}", " ".join(shlex.quote(s) for s in runsc_argv)
 )

My question is, do we risk something security-wise by doing this? Note that we instruct GVisor to treat the root filesystem as read-only (not that GVisor would write to these files in any case). Also, we mask sensitive system dirs (/proc, /tmp, /dev, sys, /home/dangerzone). There are some extra files that are mounted to the outer container on /etc/ and /run that we can mask, although they don't seem to give any sensitive info:

$ mount
[...]
tmpfs on /run/.containerenv type tmpfs (rw,nosuid,nodev,relatime,size=3258360k,nr_inodes=814590,mode=700,uid=1000,gid=958,inode64)
tmpfs on /etc/hostname type tmpfs (rw,nosuid,nodev,relatime,size=3258360k,nr_inodes=814590,mode=700,uid=1000,gid=958,inode64)
tmpfs on /etc/hosts type tmpfs (rw,nosuid,nodev,relatime,size=3258360k,nr_inodes=814590,mode=700,uid=1000,gid=958,inode64)
[...]
$ cat /etc/hostname
cec0ac615cbe
$ cat /etc/hosts
127.0.0.1       localhost
::1     localhost
192.168.1.23    host.containers.internal host.docker.internal
127.0.0.1       cec0ac615cbe dangerzone-doc-to-pixels-KwY1lA
$ cat /run/.containerenv

Overall, I'm inclined to go forward with this, but I'd like a second set of 👀 to validate my assumptions.

CCing @EtiennePerot for obvious reasons.

@apyrgio apyrgio added the enhancement New feature or request label Jan 9, 2025
@apyrgio apyrgio self-assigned this Jan 9, 2025
@apyrgio apyrgio added this to the 0.9.0 milestone Jan 9, 2025
@apyrgio apyrgio moved this from Todo to In Progress in Dangerzone ✨ Jan 9, 2025
apyrgio added a commit that referenced this issue Jan 14, 2025
Remove the need to copy the Dangerzone container image (used by the
inner container) within a wrapper gVisor image (used by the outer
container). Instead, use the root of the container filesystem for both
containers. We can do this safely because we don't mount any secrets to
the container, and because gVisor offers a read-only view of the
underlying filesystem

Fixes #1048
@apyrgio apyrgio linked a pull request Jan 14, 2025 that will close this issue
@apyrgio apyrgio moved this from In Progress to PR Review in Dangerzone ✨ Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
container enhancement New feature or request
Projects
Status: PR Review
Development

Successfully merging a pull request may close this issue.

1 participant