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: support recursive bwrap #172

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@cgwalters
Member

cgwalters commented Jan 31, 2017

WIP: ⚠️ Do not merge! ⚠️

This is a variation on #101 - which only attempts the unprivileged (i.e. userns) case. It should also fix #164

However, it doesn't work right now which I think is related to systemd/systemd#4252

Haven't debugged yet, so this is a WIP.

@cgwalters cgwalters changed the title from Keepcaps to WIP: Keepcaps Jan 31, 2017

@rh-atomic-bot

This comment has been minimized.

rh-atomic-bot commented Feb 27, 2017

☔️ The latest upstream changes (presumably b6370de) made this pull request unmergeable. Please resolve the merge conflicts.

@giuseppe

This comment has been minimized.

Member

giuseppe commented Jun 19, 2017

@cgwalters I think the EPERM failure can be related to the fact that the sandbox application runs in a chroot environment, from unshare(2):

       EPERM (since Linux 3.9)
              CLONE_NEWUSER was specified in flags and the caller is in a chroot environment (i.e., the caller's root directory does not match the root directory of the mount  namespace
              in which it resides).
@giuseppe

This comment has been minimized.

Member

giuseppe commented Jun 19, 2017

I confirm the issue is with the chroot environment. If I replace the chroot with a hacky pivot_root, I have:

./bwrap --cap-add ALL --uid 0 --gid 0 --unshare-user --bind / /  --bind /proc /proc ./bwrap --unshare-user --bind / / echo hi
hi
@cgwalters

This comment has been minimized.

Member

cgwalters commented Jun 22, 2017

Rebased 🏄

And now using pivot_root(). There was a subtlety around supporting bwrap --ro-bind / /, where I had to hunt for a place to put the oldroot.

* We're aiming to make /newroot the real root, and get rid of /oldroot. To do
* that we need a temporary place to store it before we can unmount it.
*/
{ char *pivot_tmp = xstrdup ("bwrap-pivot-old-XXXXXX");

This comment has been minimized.

@giuseppe

giuseppe Jun 22, 2017

Member

should this have cleanup_free?

This comment has been minimized.

@cgwalters

cgwalters Jun 23, 2017

Member

Fixup ⬇️

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Jun 26, 2017

The /newroot paths leak a bit into users, and we have this hardcoded in a few places in flatpak, so if this is merged, care must be taken about that.

@cgwalters cgwalters changed the title from WIP: Keepcaps to --keepcaps Jun 28, 2017

@cgwalters

This comment has been minimized.

Member

cgwalters commented Jun 28, 2017

In the document portal? Hmm. Need to investigate why that code is required then.

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Jun 28, 2017

Also in xdg-desktop-portal.
Its needed because the /newroot part of the path leaks out when reading the symlinks from /proc/sys/fd/ fro the host side.

@rh-atomic-bot

This comment has been minimized.

rh-atomic-bot commented Jun 29, 2017

☔️ The latest upstream changes (presumably 215aa3e) made this pull request unmergeable. Please resolve the merge conflicts.

@cgwalters

This comment has been minimized.

Member

cgwalters commented Aug 15, 2017

Rebased 🏄

@cgwalters

This comment has been minimized.

Member

cgwalters commented Aug 15, 2017

And now we can drop my --keep-caps commit and just do --cap-add ALL.

@cgwalters cgwalters added the WIP label Aug 15, 2017

@cgwalters cgwalters changed the title from --keepcaps to WIP: support recursive bwrap Aug 15, 2017

@cgwalters

This comment has been minimized.

Member

cgwalters commented Aug 15, 2017

But marking WIP since we need to fix flatpak's use of /newroot first.

cgwalters added a commit to cgwalters/flatpak that referenced this pull request Oct 1, 2017

document-portal: Handle bubblewrap changing to drop /newroot
There's an oustanding bubblewrap PR where we'd like to change how
we set up the rootfs; a side effect of this will be that /newroot
disappears from the `/proc` links:
[bubblewrap pull 172](projectatomic/bubblewrap#172).

I took a stab here at adapting the code to work in both the old and new cases.
Just compile tested at the moment. There's a lot of subtleties in this code; in
particular how we end up mutating-in-place the path buffer and how that
interacts with inspecting it.

rh-atomic-bot added a commit to flatpak/flatpak that referenced this pull request Oct 5, 2017

document-portal: Handle bubblewrap changing to drop /newroot
There's an oustanding bubblewrap PR where we'd like to change how
we set up the rootfs; a side effect of this will be that /newroot
disappears from the `/proc` links:
[bubblewrap pull 172](projectatomic/bubblewrap#172).

I took a stab here at adapting the code to work in both the old and new cases.
Just compile tested at the moment. There's a lot of subtleties in this code; in
particular how we end up mutating-in-place the path buffer and how that
interacts with inspecting it.

Closes: #1063
Approved by: alexlarsson
@rh-atomic-bot

This comment has been minimized.

rh-atomic-bot commented Oct 6, 2017

☔️ The latest upstream changes (presumably 3983c1c) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters added some commits Jun 22, 2017

Use pivot_root() instead of chroot() for final root
This is preparatory work for supporting recursive bwrap. Without this, using
`mount()` on the second `/` won't work, since it won't be a mount point.
tests: Add a test case for recursive bwrap
Now that we have `--cap-add ALL`, and the previous commit fixed
things to use `pivot_root`, we can cleanly support recursive `bwrap`
invocations.  This is useful for a variety of cases; today for example
we often run `rpm-ostree compose` inside docker, but there's no reason
the outer container couldn't be bwrap based.
@cgwalters

This comment has been minimized.

Member

cgwalters commented Oct 6, 2017

OK, rebased 🏄

I now submitted flatpak/xdg-desktop-portal#124 as well.

So I think we have two options here. First, we could add a new option like --pivot that used the new mode. Then e.g. rpm-ostree could learn to use it.

Alternatively, we could wait say a month for updates to flatpak to propagate, and then merge this.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 12, 2018

WIP: mock --old-chroot compat
We're hitting issues with old mock + old nspawn, let's try
doing the container gymnastics to support running bwrap inside
`mock --old-chroot`.

See also https://gist.github.com/jlebon/fb6e7c6dcc3ce17d3e2a86f5938ec033
and projectatomic/bubblewrap#172

This works for me but I want to add more testing.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 18, 2018

WIP: mock --old-chroot compat
We're hitting issues with old mock + old nspawn, let's try
doing the container gymnastics to support running bwrap inside
`mock --old-chroot`.

See also https://gist.github.com/jlebon/fb6e7c6dcc3ce17d3e2a86f5938ec033
and projectatomic/bubblewrap#172

This works for me but I want to add more testing.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 18, 2018

WIP: mock --old-chroot compat
We're hitting issues with old mock + old nspawn, let's try
doing the container gymnastics to support running bwrap inside
`mock --old-chroot`.

See also https://gist.github.com/jlebon/fb6e7c6dcc3ce17d3e2a86f5938ec033
and projectatomic/bubblewrap#172

This works for me but I want to add more testing.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 19, 2018

WIP: mock --old-chroot compat
We're hitting issues with old mock + old nspawn, let's try
doing the container gymnastics to support running bwrap inside
`mock --old-chroot`.

See also https://gist.github.com/jlebon/fb6e7c6dcc3ce17d3e2a86f5938ec033
and projectatomic/bubblewrap#172

This works for me but I want to add more testing.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 23, 2018

mock --old-chroot compat
We're hitting issues with old mock + old nspawn, let's try
doing the container gymnastics to support running bwrap inside
`mock --old-chroot`.

See also https://gist.github.com/jlebon/fb6e7c6dcc3ce17d3e2a86f5938ec033
and projectatomic/bubblewrap#172

We also handle `config_opts['internal_dev_setup'] = False` by mounting
our own `devtmpfs` if necessary.
@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 21, 2018

@cgwalters is there anything more blocking this PR?

if (mkdtemp (pivot_tmp) == NULL)
{
/* If the user did a bind mount of /, try /tmp */
if (errno == EPERM || errno == EACCES)

This comment has been minimized.

@giuseppe

giuseppe Feb 21, 2018

Member

could you please also add EROFS here? I've seen this issue locally when '/' was mounted read only

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 21, 2018

one issue I am seeing for a recursive bwrap is:

# /usr/bin/bwrap --bind / / --unshare-pid --unshare-user --cap-add ALL --uid 0 --gid 0 --proc /proc  /usr/bin/bwrap --bind / / --unshare-pid --unshare-user --proc /proc echo hi
bwrap: Can't mount proc on /newroot/proc: Operation not permitted

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 22, 2018

the issue I posted before seems to be related to the "sys", "sysrq-trigger", "irq", "bus" cover mount points we have under /proc, if I drop this part then I am able to run bwrap recursively.

We could probably skip these mount points when we keep CAP_SYS_ADMIN in the container, as anyway the user could just use umount to take rid of them. What do you think?

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 22, 2018

@rhatdan with this PR (+ the comments I've added) I was able to run bwrap inside bwrap and get the entire "buildah run" to work

@rhatdan

This comment has been minimized.

Member

rhatdan commented Feb 22, 2018

So you are using bwrap-oci as the container runtime?

@cgwalters

This comment has been minimized.

Member

cgwalters commented Feb 22, 2018

cover mount points we have under /proc

It's because user namespaces explicitly disallows covering mounts; the idea is a sysadmin could have intentionally "overmounted" on the host, and with user namespaces allowing unmounting it in a new mount ns, users could gain access to the files underneath.

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 22, 2018

yes, bwrap-oci, we can get this sorted out first, and then we can investigate the usage of runc.

One thing is that bwrap-oci+bubblewrap takes care of creating the user namespace when needed, why this logic is not in runc, i.e. it expects the mappings to be specified in the config.json file (in crun I automatically fallback to what bwrap-oci does when this information is missing). We will need to add it to atomic for properly using runc

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 22, 2018

@cgwalters ah true, thanks for the explanation. It is still possible though to umount these mounts if the user has CAP_SYS_ADMIN, unless MS_UNBINDABLE is used (I guess this part, I have not tried).

Given that, could we relax these mounts only to the case the use doesn't keep CAP_SYS_ADMIN in the container?

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 22, 2018

@alexlarsson @cgwalters if you are fine with it, I can keep working on this PR:

  • add a fixup for the EROFS case
  • add a patch for skipping the /proc/[sys,sysrq-trigger,irq,bus] mounts when the user has CAP_SYS_ADMIN.

With these changes, we will have something for running Buildah as non-root user.

@giuseppe

This comment has been minimized.

Member

giuseppe commented Feb 23, 2018

I've opened a PR here with this patch and the additional fix for /proc:

#256

With the version in #256 and the latest upstream version of bwrap-oci I was able to run Buildah with an user container

@cgwalters

This comment has been minimized.

Member

cgwalters commented Apr 30, 2018

Obsoleted in favor of #256

@cgwalters cgwalters closed this Apr 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment