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

scripts: Drop most capabilities #1099

Closed
wants to merge 4 commits into from
Closed

Conversation

cgwalters
Copy link
Member

Note this PR requires bubblewrap 0.2.0.

Change our bwrap invocations drop truly dangerous capabilities like
cap_sys_admin and cap_sys_module just like Docker does today. Because of the
popularity of Docker, we can be pretty sure that most RPM scripts should have
adapted to this (although a problematic area here is that traditional librpm
doesn't actually error out if scripts fail).

There are two reasons to do this:

  • We want "offline" updates by default; updates shouldn't affect the
    running system. If we prepare the new root in the background, a
    %post shouldn't restart a service for example. We already "handle"
    this by making systemctl a symlink to /bin/true, but this approach
    also shuts off %posts that do e.g. insmod.
  • Protection against accidental system damage

Note this PR requires [bubblewrap 0.2.0](https://github.com/projectatomic/bubblewrap/releases/tag/v0.2.0).

Change our bwrap invocations drop truly dangerous capabilities like
`cap_sys_admin` and `cap_sys_module` just like Docker does today. Because of the
popularity of Docker, we can be pretty sure that most RPM scripts should have
adapted to this (although a problematic area here is that traditional librpm
doesn't actually error out if scripts fail).

There are two reasons to do this:

 - We want "offline" updates by default; updates shouldn't affect the
   running system.  If we prepare the new root in the background, a
   %post shouldn't restart a service for example.  We already "handle"
   this by making `systemctl` a symlink to `/bin/true`, but this approach
   also shuts off `%post`s that do e.g. `insmod`.
 - Protection against accidental system damage
@jlebon
Copy link
Member

jlebon commented Nov 13, 2017

Nice! I tested this with the usual suspects and it worked fine. Tests are failing though because they're not running with bubblewrap 0.2.0 yet. Maybe let's just add it to the list of rdgo pkgs we rsync in install.sh?

"--cap-add", "cap_setgid",
"--cap-add", "cap_setuid",
"--cap-add", "cap_setpcap",
"--cap-add", "cap_net_bind_service",
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop this one too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should. I originally was worried about apps which use TCP for local management...IIRC bind at least used to do this? But OTOH we already break this by using an isolated loop-only netns so...yeah, let's remove it.

@cgwalters
Copy link
Member Author

Pushed a fixup ⬆️

@cgwalters
Copy link
Member Author

Let's try to get away with just adding a hard dependency now?

@jlebon
Copy link
Member

jlebon commented Dec 4, 2017

But CentOS is still on the old one, right? We can just hold this until it makes it there I guess.

@cgwalters
Copy link
Member Author

For RHEL we bundle bwrap in the rpm-ostree package, and for CAHC we already get it from git builds right?

@cgwalters
Copy link
Member Author

Although I get your point that we won't get this past c7-primary until we change the build there. Hmm....

@jlebon
Copy link
Member

jlebon commented Dec 4, 2017

We can switch to smoketested, that one should have it... probably.

@jlebon
Copy link
Member

jlebon commented Dec 5, 2017

@rh-atomic-bot r+ f284886

@rh-atomic-bot
Copy link

⌛ Testing commit f284886 with merge 90f9fe8...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 90f9fe8 to master...

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

Successfully merging this pull request may close these issues.

None yet

3 participants