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

Allow basic sandboxing of containers #1413

Closed
wants to merge 5 commits into from
Closed

Conversation

xdom
Copy link

@xdom xdom commented May 25, 2024

Lets the users decide to unshare host home and root directories, and to create unprivileged containers without dropping security measures. Default behaviour of commands stays as-is. Partially related to issue #28.

xdom added 5 commits May 25, 2024 11:03
distrobox-create now supports following options:
`--unshare-home`: Unshares host home directory
`--unshare-root`: Unshares host root directory
distrobox-create now supports a following option:
`--unprivileged`: Runs container without --privileged switch,
and without disabling labels and apparmor confinement.
dbus mounted into a container provides an access
to the root partition.
@89luca89
Copy link
Owner

Hi @xdom I'll take a deeper look at this ASAP this is a big change and I'd like to do further testing

Copy link

@janvhs janvhs left a comment

Choose a reason for hiding this comment

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

First of all: great job! :D I am reviewing this right now, because I was literally bolting this on top of distrobox via bwrap/bubblewrap for my development containers. Nice to see this PR instead. I am 100% using it ^^

I had some minor nitpicks about grammar, which I marked in the review, and have a few open questions. Don't worry about the amount of comments, I marked basically the same thing in all places for your convenience ^^

Could you explain what the benefit of having an unshared root is? I struggle to see them right now.

After reading this blog entry, the unprivileged mode seems pretty desirable as well. Do you know if it implies any major drawbacks?

If @89luca89 wants a thorough discussion about the unprivileged mode and unshared root feature, could we merge the unshared home feature separately? It's pretty practicable and not as big of a change :D

Comment on lines 357 to +358
unshare_ipc=1
unshare_home=1
Copy link

Choose a reason for hiding this comment

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

These appear to be alphabetically sorted. I'd exchange these to follow the pattern :D

Suggested change
unshare_ipc=1
unshare_home=1
unshare_home=1
unshare_ipc=1

--unprivileged: do not drop security measures when creating a container
--unshare-devsys: do not share host devices and sysfs dirs from host
--unshare-groups: do not forward user's additional groups into the container
--unshare-home: do not mount host home directory into the container
Copy link

Choose a reason for hiding this comment

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

Nice description! I think grammatically, there is a small mistake.
I'm going to mark the other spots for convenience, so you could just click "accept suggestion".

Suggested change
--unshare-home: do not mount host home directory into the container
--unshare-home: do not mount host's home directory into the container

--unshare-ipc: do not share ipc namespace with host
--unshare-netns: do not share the net namespace with host
--unshare-process: do not share process namespace with host
--unshare-process: do not share process namespace with host
--unshare-root: do not mount host root directory into the container
Copy link

Choose a reason for hiding this comment

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

Same as the --unshare-home flag ^^

Suggested change
--unshare-root: do not mount host root directory into the container
--unshare-root: do not mount host's root directory into the container

Comment on lines +434 to +436
elif ! [ "${unshare_home:-0}" -eq 0 ]; then
# Workdir is unshared, we just enter $HOME of the container.
workdir="${container_home}"
Copy link

Choose a reason for hiding this comment

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

Nice fallback!

{
mount -t devpts devpts -o noexec,nosuid,newinstance,ptmxmode=0666,mode=0620,gid=tty /dev/pts/
mount --bind /dev/pts/ptmx /dev/ptmx
} || printf "Warning: Cannot mount devpts\n"
Copy link

Choose a reason for hiding this comment

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

Nice to have a warning there. Is there a particular reason, why you did not prepend it with "distrobox: ..." or why you don't print to stderr? Not sure how distrobox normally handles warnings.

'--unshare-devsys[do not share host devices and sysfs dirs from host]' \
'--unshare-groups[do not forward user''s additional groups into the container]' \
Copy link

Choose a reason for hiding this comment

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

nice catch!

'--unshare-devsys[do not share host devices and sysfs dirs from host]' \
'--unshare-groups[do not forward user''s additional groups into the container]' \
'--unshare-home[do not mount host home directory into the container]' \
Copy link

Choose a reason for hiding this comment

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

see other comment

Suggested change
'--unshare-home[do not mount host home directory into the container]' \
'--unshare-home[do not mount host''s home directory into the container]' \

'--unshare-ipc[do not share ipc namespace with host]' \
'--unshare-netns[do not share the net namespace with host]' \
'--unshare-process[do not share process namespace with host]' \
'--unshare-root[do not mount host root directory into the container]' \
Copy link

Choose a reason for hiding this comment

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

See other comment

Suggested change
'--unshare-root[do not mount host root directory into the container]' \
'--unshare-root[do not mount host''s root directory into the container]' \

--unshare-devsys: do not share host devices and sysfs dirs from host
--unshare-groups: do not forward user\[aq]s additional groups into the container
--unshare-home: do not mount host home directory into the container
Copy link

Choose a reason for hiding this comment

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

Same as above

Suggested change
--unshare-home: do not mount host home directory into the container
--unshare-home: do not mount host's home directory into the container

--unshare-ipc: do not share ipc namespace with host
--unshare-netns: do not share the net namespace with host
--unshare-process: do not share process namespace with host
--unshare-root: do not mount host root directory into the container
Copy link

Choose a reason for hiding this comment

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

Same as above

Suggested change
--unshare-root: do not mount host root directory into the container
--unshare-root: do not mount host's root directory into the container

@janvhs
Copy link

janvhs commented Jul 14, 2024

Have been running your PR today and the unshared root and home features work great! :D
Maybe consider documenting that without the unshared root activated, the user's $HOME is still accessible via /run/host/home/ directory. You could also unmount the user's home in this subdirectory.

Running in unprivileged mode, is not working for me. However, I run openSUSE Aeon which enables SELinux and is in pre-release. Therefore, this is likely an issue with my system and not with your code. These are the steps to reproduce it, just in case you want to take a closer look at it.

$ distrobox create --unprivileged --home "$(mktemp -d)" unpriv
$ distrobox enter unpriv

It fails with a permission denied, which I suspect could be because of SELinux policies.

+ mount --rbind -o ro /run/host/usr/share/zoneinfo/Europe/Berlin /etc/localtime
mount: /etc/localtime: permission denied.
       dmesg(1) may have more information after failed mount system call.
+ printf 'Warning: failed to bind mount %s to %s\n' /run/host/usr/share/zoneinfo/Europe/Berlin /etc/localtime
Warning: failed to bind mount /run/host/usr/share/zoneinfo/Europe/Berlin to /etc/localtime
+ return 1
+ '[' 1 -ne 0 ']'
+ printf 'Error: An error occurred\n'
Error: An error occurred
DEBU[0000] Called logs.PersistentPostRunE(podman --log-level debug logs unpriv) 
DEBU[0000] Shutting down engines                        

The full log is available here: unpriv_log.txt

@89luca89
Copy link
Owner

@xdom @janvhs

I'd like to reflect on the implication of this (and #28 )

What are the use cases for this?

The main use case of using distrobox over podman (or docker) is the integration it has with the system.
And also the "ease of use", as you could do with podman what distrobox does, but distrobox abstracts lots of flags (so you can just distrobox enter and it works)

This invalidates all the usecases of distrobox

  • app/bin exporting
  • host integration
  • ease of use (we need to add all the various flags to use this)

So my question is:

Isn't using podman directly a better alternative?

I feel like all of this PR (and #28 in general) are going in the opposite direction of what the projects aims to do.

Sandboxing is not a target for distrobox, it may add some unsharing bits for useful purposes (say, initful containers) but it's not aiming to replace podman/docker/flatpak

@89luca89 89luca89 mentioned this pull request Jul 15, 2024
@janvhs
Copy link

janvhs commented Jul 16, 2024

Thanks for the response @89luca89. For me the use case is primarily having a separate home for my development distrobox, because the tools I use: (Elixir's mix, Rust's cargo, OCaml's opam etc.) litter my home directory. Therefore, I keep my main home clean and remove the complete distrobox when I decide it's time for some clean-up.

That said, I agree, an unshared root and unprivileged mode might go to far for distrobox's scope and you'd be better of using Podman or bubblewrap directly. This way it's not implied that distrobox is a sandboxing tool.

@89luca89
Copy link
Owner

Thanks for the response @89luca89. For me the use case is primarily having a separate home for my development distrobox, because the tools I use: (Elixir's mix, Rust's cargo, OCaml's opam etc.) litter my home directory. Therefore, I keep my main home clean and remove the complete distrobox when I decide it's time for some clean-up.

That said, I agree, an unshared root and unprivileged mode might go to far for distrobox's scope and you'd be better of using Podman or bubblewrap directly. This way it's not implied that distrobox is a sandboxing tool.

That's why --home is present
it does indeed allow to separate the dotfiles, but keeping the integration (original home is accessible anyway)

then, if a program uses /home/$USER instead of $HOME, it's a problem of that program.

But all the unprivileged thing, unsharing host, and completely unsharing home, feels backwards with what distrobox aims to do, and is useful for.

If one doesn't want an integration with the host system, why are they using distrobox in the first place?

@WayneSherman
Copy link

Sandboxing is not a target for distrobox...

There was a time when this wasn't much of a concern, but because of widespread cybersecurity threats sandboxing needs to be a target for ALL desktop environments. Moving forward, running apps and environments in a sandbox is hard requirement for me. For example, I don't want most apps (e.g. LibreOffice, Gimp, Firefox, vlc) to have general access to all my mounted network shares, or all my dotfiles (especially ~/.ssh folder where my private keys are stored). Same thing for building software, the build environment should not have access to my whole user profile or all my network mounts. If distrobox doesn't have the ability to run apps and environments in a safe way (i.e. sandboxed), then unfortunately I may not be able to use it.

@WayneSherman
Copy link

If one doesn't want an integration with the host system, why are they using distrobox in the first place?

Even on the host system I want to run apps in a sandbox. Whether it is a native host app or a distrobox app I want to define the amount of integration and what is visible to the app and hidden from the app.

@89luca89
Copy link
Owner

Sandboxing is not a target for distrobox...

There was a time when this wasn't much of a concern, but because of widespread cybersecurity threats sandboxing needs to be a target for ALL desktop environments. Moving forward, running apps and environments in a sandbox is hard requirement for me. For example, I don't want most apps (e.g. LibreOffice, Gimp, Firefox, vlc) to have general access to all my mounted network shares, or all my dotfiles (especially ~/.ssh folder where my private keys are stored).

and that's where flatpak exist

Same thing for building software, the build environment should not have access to my whole user profile or all my network mounts. If distrobox doesn't have the ability to run apps and environments in a safe way (i.e. sandboxed), then unfortunately I may not be able to use it.

and that's where regular docker/podman exist

Even on the host system I want to run apps in a sandbox. Whether it is a native host app or a distrobox app I want to define the amount of integration and what is visible to the app and hidden from the app.

Distrobox target is to have multiple mutable environments, for compatibility or development purposes.
If security is a concern (for example, you don't trust the build you're performing) you should use proper docker/podman. It doesn't make sense to have a distrobox that works at 50% (or less) because of unsharing all the stuff.

The point of distrobox is integration, compatibility and ease of use, if you take away this, what is the advantage of running bare podman/docker?

@janvhs
Copy link

janvhs commented Jul 29, 2024

I guess we can close the PR as not planned then ^^

@89luca89
Copy link
Owner

Closing as not planned

@89luca89 89luca89 closed this Oct 10, 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.

4 participants