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

Add support for wrapping binaries (rpm) #1789

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

cgwalters
Copy link
Member

We need to be friendlier to people who are transitioning from
"traditional" yum managed systems. This patchset starts to lay
out the groundwork for supporting "intercepting" binaries that
are in the tree.

To start with for example, we wrap /usr/bin/rpm and cause it
to drop privileges. This way it can't corrupt anything; we're
not just relying on the read-only bind mount. For example nothing
will accidentally get written to /var/lib/rpm.

Now a tricky thing with this one is we do want it to write if
we're in an unlocked state.

There are tons of other examples of binaries we want to intercept,
among them:

  • grubby -> rpm-ostree kargs
  • dracut -> rpm-ostree initramfs
  • yum -> well...we'll talk about that later

@cgwalters
Copy link
Member Author

Also I promise at some point I'll stop collecting WIP PRs and actually finish one or two of them...

@cgwalters
Copy link
Member Author

Testing this one nicely really wants support for doing a compose and then booting just like sysusers: #1679 (comment)

@rh-atomic-bot
Copy link

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

rust/src/cliwrap.rs Outdated Show resolved Hide resolved
@rh-atomic-bot
Copy link

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

@cgwalters
Copy link
Member Author

@jlebon (or anyone else) any thoughts on this?

I think the main thing blocking right now from my PoV is changing the tests to support composing and booting which led me down the rabbit hole of better cosa support for that.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

@jlebon (or anyone else) any thoughts on this?

I like the containerization idea a lot.

For the CLI switch filtering, let's make sure that the canonical host "query" APIs aren't hitting the scary warning & delay? Otherwise, it can lead to confusing and frustrating UX. The big one is the rpm --query API which I added a comment about below. There's also dracut --list-modules. Haven't checked grubby.

One worry though going down this road is maintenance burden. ISTM like the longer term cleaner approach is to instead integrate rpm-ostree awareness in those projects directly? Not opposed to trying it out though.

rust/src/cliwrap/rpm.rs Outdated Show resolved Hide resolved
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Jun 12, 2019
So now that we have OSTree layer capabilities in rpm-ostree
(coreos/rpm-ostree#1830), I think it
makes more sense overall to make the overlay an OSTree layer.

The fact that it *is* an RPM, but differs from all the other RPMs in
various ways will (and already has) come up multiple times when we talk
about signing, changelogs, lockfiles, overrides, etc... (See [1] and [2]
for some already filed issues). Switching to an OSTree layer sidesteps
many of those issues.

Of course, we do lose some things from moving away from RPM:
1. it's harder to determine from where the overlay files come from on a
   host system (see [3] related to this)
2. invisible to `rpm-ostree db diff` (but then again, the diff we
   currently get isn't helpful)
3. not possible to issue an advisory for it to be consumed like the rest
   of the advisories (though with the current approach, making use of
   this without going through Bodhi would've required some non-trivial
   amount of work too)

Overall though, I think those are things we could (and should strive to)
fix in rpm-ostree rather than avoiding layers entirely.

[1] coreos#544
[2] coreos#553 (comment)
[3] coreos/rpm-ostree#1789
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Jun 14, 2019
So now that we have OSTree layer capabilities in rpm-ostree
(coreos/rpm-ostree#1830), I think it
makes more sense overall to make the overlay an OSTree layer.

The fact that it *is* an RPM, but differs from all the other RPMs in
various ways will (and already has) come up multiple times when we talk
about signing, changelogs, lockfiles, overrides, etc... (See [1] and [2]
for some already filed issues). Switching to an OSTree layer sidesteps
many of those issues.

Of course, we do lose some things from moving away from RPM:
1. it's harder to determine from where the overlay files come from on a
   host system (see [3] related to this)
2. invisible to `rpm-ostree db diff` (but then again, the diff we
   currently get isn't helpful)
3. not possible to issue an advisory for it to be consumed like the rest
   of the advisories (though with the current approach, making use of
   this without going through Bodhi would've required some non-trivial
   amount of work too)

Overall though, I think those are things we could (and should strive to)
fix in rpm-ostree rather than avoiding layers entirely.

[1] coreos#544
[2] coreos#553 (comment)
[3] coreos/rpm-ostree#1789
jlebon added a commit to coreos/coreos-assembler that referenced this pull request Jun 14, 2019
So now that we have OSTree layer capabilities in rpm-ostree
(coreos/rpm-ostree#1830), I think it
makes more sense overall to make the overlay an OSTree layer.

The fact that it *is* an RPM, but differs from all the other RPMs in
various ways will (and already has) come up multiple times when we talk
about signing, changelogs, lockfiles, overrides, etc... (See [1] and [2]
for some already filed issues). Switching to an OSTree layer sidesteps
many of those issues.

Of course, we do lose some things from moving away from RPM:
1. it's harder to determine from where the overlay files come from on a
   host system (see [3] related to this)
2. invisible to `rpm-ostree db diff` (but then again, the diff we
   currently get isn't helpful)
3. not possible to issue an advisory for it to be consumed like the rest
   of the advisories (though with the current approach, making use of
   this without going through Bodhi would've required some non-trivial
   amount of work too)

Overall though, I think those are things we could (and should strive to)
fix in rpm-ostree rather than avoiding layers entirely.

[1] #544
[2] #553 (comment)
[3] coreos/rpm-ostree#1789
@rh-atomic-bot
Copy link

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

@cgwalters cgwalters force-pushed the cli-wrap branch 2 times, most recently from 9d0a79c to c13ce60 Compare August 14, 2019 18:26
@rh-atomic-bot
Copy link

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

@rh-atomic-bot
Copy link

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

@cgwalters
Copy link
Member Author

Any further comments on this?

@jlebon
Copy link
Member

jlebon commented Apr 6, 2020

Any thoughts on

One worry though going down this road is maintenance burden. ISTM like the longer term cleaner approach is to instead integrate rpm-ostree awareness in those projects directly?

?

@cgwalters
Copy link
Member Author

ISTM like the longer term cleaner approach is to instead integrate rpm-ostree awareness in those projects directly?

That was rejected for RPM:
rpm-software-management/rpm#320

@cgwalters
Copy link
Member Author

/test sanity

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

The code itself looks reasonable to me, though part of me still feels a bit uneasy about hijacking those well-known binaries. Overall, I still think a more sustainable approach is integrating this logic directly in the various upstream projects. There was a suggestion in that rpm patch there for example to implement this as a plugin instead.

Another parallel path is continuing the work on making immutable more parts of the filesystem so that even if an app isn't "OSTree-aware", it still can't really do much harm.

That said, I'm not totally against this and open to experimenting with it and seeing how it feels. (Though would definitely want to get more input from others before e.g. turning this on in FCOS.)

rust/src/cliwrap.rs Outdated Show resolved Hide resolved
rust/src/cliwrap/cliutil.rs Outdated Show resolved Hide resolved
rust/src/cliwrap/cliutil.rs Outdated Show resolved Hide resolved
rust/src/cliwrap/cliutil.rs Outdated Show resolved Hide resolved
rust/src/cliwrap/dracut.rs Outdated Show resolved Hide resolved
rust/src/cliwrap/grubby.rs Outdated Show resolved Hide resolved
rust/src/cliwrap/rpm.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member Author

(Though would definitely want to get more input from others before e.g. turning this on in FCOS.)

Right, that's an important part of this - it's not on by default yet.

@cgwalters
Copy link
Member Author

There was a suggestion in that rpm patch there for example to implement this as a plugin instead.

The thing particularly for rpm is...we already have a project binding rpm and ostree - this one. Having an RPM plugin that called into either ostree or rpm-ostree makes everything circular.

One idea I did have later is it could make a lot of sense to do this type of thing for the traditional dnf/yum/zypper vs rpm too...for a long time yum would emit a warning like: warning: rpmdb modified outside of yum that was eventually removed. I think the idea is one wanted to be able to execute rpm even if yum was broken, but as long as there's an escape hatch for that (and there is in this case) that addresses the concern. So it probably wouldn't be a plugin in the .so sense but just execv(/usr/bin/rpm-ostree) or execv(/usr/bin/dnf).

@cgwalters cgwalters force-pushed the cli-wrap branch 6 times, most recently from 3aec01d to d45f12b Compare April 9, 2020 20:50
We need to be friendlier to people who are transitioning from
"traditional" yum managed systems.  This patchset starts to lay
out the groundwork for supporting "intercepting" binaries that
are in the tree.

For backwards compatibility, this feature is disabled by default,
to enable it, one can add `cliwrap: true` to the manifest.

To start with for example, we wrap `/usr/bin/rpm` and cause it
to drop privileges.  This way it can't corrupt anything; we're
not just relying on the read-only bind mount.  For example nothing
will accidentally get written to `/var/lib/rpm`.

Now a tricky thing with this one is we *do* want it to write if
we're in an unlocked state.

There are various other examples of binaries we want to intercept,
among them:

 - `grubby` -> `rpm-ostree kargs`
 - `dracut` -> `rpm-ostree initramfs`
 - `yum` -> well...we'll talk about that later
@jlebon
Copy link
Member

jlebon commented Apr 15, 2020

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit e41a8ab into coreos:master Apr 15, 2020
cgwalters added a commit to cgwalters/fedora-coreos-config that referenced this pull request Jan 28, 2021
This functionality has been around for quite a while in rpm-ostree
but I'm not aware of anyone using it.  I'd like to turn it
on here so we can gain the benefits, which right now are
better protection against non-ostree-aware commands run as
root (`rpm` and `dracut` namely).

A bit more in coreos/rpm-ostree#1789
@cgwalters
Copy link
Member Author

Also note to self; we should add support for intercepting at least ostree admin upgrade etc.

cgwalters added a commit to cgwalters/fedora-coreos-config that referenced this pull request Feb 18, 2021
This functionality has been around for quite a while in rpm-ostree
but I'm not aware of anyone using it.  I'd like to turn it
on here so we can gain the benefits, which right now are
better protection against non-ostree-aware commands run as
root (`rpm` and `dracut` namely).

A bit more in coreos/rpm-ostree#1789
cgwalters added a commit to cgwalters/fedora-coreos-config that referenced this pull request Feb 18, 2021
This functionality has been around for quite a while in rpm-ostree
but I'm not aware of anyone using it.  I'd like to turn it
on here so we can gain the benefits, which right now are
better protection against non-ostree-aware commands run as
root (`rpm` and `dracut` namely).

A bit more in coreos/rpm-ostree#1789
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.

6 participants