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

WIP: Implement Linux User Namespaces support #248

Closed
wants to merge 1 commit into from
Closed

Conversation

apyrgio
Copy link
Contributor

@apyrgio apyrgio commented Nov 10, 2022

Implement support for Linux User Namespaces in Dangerzone, as described in #221.

Issue #221 has moved to the 0.5.0 milestone, but since it's a complex subject matter, I wanted to push the work I have done so far on this subject, before switching to other things that have higher priority. The current state is that the proposed implementation steps seem to work, and the tests pass. The code needs a bit more polishing, but other than that, it should be simple to review and merge.

Fixes #221

Copy link
Contributor

@deeplow deeplow left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It took me a while to understand how this works, but I think I got it and it makes sense.

This is already better than we had. However, if we run multiple containers at the same time, aren't we allocating the same namespace to all of them? [this can go on a future PR and it isn't as concerning as host-container separation. This blog post had a great summary.

Comment on lines 244 to 252
os.makedirs(safe_dir, exist_ok=True)

if get_runtime_name() == "podman":
subprocess.run([
"podman", "unshare", "chown", "-R", "1001:1001", tmpdir.name
], check=True)

# Convert document to pixels
command = ["/usr/bin/python3", "/usr/local/bin/dangerzone.py", "document-to-pixels"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about grouping together the "unshares" with a context manager? The core code would look a bit more organized:

    [...]
    with namespaced_tmpdir(tmpdir):
        # Convert document to pixels
        command = ["/usr/bin/python3", "/usr/local/bin/dangerzone.py", "document-to-pixels"]
        ...

The context manager I'm using looks like:

from contextlib import contextmanager

@contextmanager
def namespaced_tmpdir(tmpdir: tempfile.TemporaryDirectory):
    """Some more detailed explanation"""
    try:
        if get_runtime_name() == "podman":
            unshare_cmd = ["podman", "unshare", "chown", "-R", "1001:1001", tmpdir.name]
            log.debug("> " + " ".join(unshare_cmd))
            subprocess.run(unshare_cmd, check=True)
        yield
    finally:
        if get_runtime_name() == "podman":
            unshare_cmd = ["podman", "unshare", "chown", "-R", "0:0", tmpdir.name]
            log.debug("> " + " ".join(unshare_cmd))
            subprocess.run(unshare_cmd, check=True)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have implemented an example of this here 3de0633.

@apyrgio
Copy link
Contributor Author

apyrgio commented Jun 12, 2024

This PR has been superceded by the gVisor integration PR (#590), which puts the Linux user that runs the rasterization logic of Dangerzone into a different user namespace. This approach was Linux-only, and hence more limited in scope, so we can safely discard it.

@apyrgio apyrgio closed this Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defense in Depth
2 participants