daemon: Add new ModifyYumRepo DBus API #1780

kalev · 2019-03-08T14:43:07Z

This allows clients such as gnome-software to enable and disable
yum repositories.

Closes: #1771

(Note that this is completely untested, ran out of time today and going to be offline the whole weekend. I'll just put it up in case someone wants to have an early look. Will test it and fix things up on Monday.)

jlebon

Looks sane at a high level! I think this is missing a new action in os_authorize_method()?

jlebon · 2019-03-08T15:32:32Z

src/daemon/org.projectatomic.rpmostree1.xml

+    <!-- Set options in yum .repo files -->
+    <method name="ModifyYumRepo">
+      <arg type="s" name="repo_id" direction="in"/>
+      <arg type="a{ss}" name="repo_options" direction="in"/>


Bikeshed: maybe just options?

Hm, I called it repo_options to avoid confusing it with options such as "dry-run" that other DBus API has. Not sure it's a huge problem :) Happy to change it if you think it's best.

settings or parameters could work too? Just writing the polkit messages and "Authentication is required to modify repository settings" is what I have right now and "settings" seems to work well in that sentence.

Sure, settings sounds good to me!

src/daemon/rpmostreed-transaction-types.c

jlebon · 2019-03-11T19:06:57Z

bot, retest this please

cgwalters · 2019-03-13T02:58:32Z

Thanks for working on this!

I'm a bit uncertain though long term about exposing every configuration option via DBus. An obvious 🐘 in the room here is that we don't expose a DBus API to configure the OSTree repositories either.

In the end if one can configure arbitrary yum repos it's equivalent to arbitrary code execution on the host, not going to be easy to wrap it in anything that constrains it with polkit.

For example, while it looks like systemd does have a DBus API to set unit file properties, in the end the vast majority of tooling to configure systemd just ends up writing unit files, symlinks to /dev/null to disable - because the filesystem is a well-specified API in this case. Concretely for example systemctl edit directly writes unit files rather than doing so over DBus.

I'm not saying we shouldn't do a PR like this - but let's at least consider the (IMO far simpler) alternative of just having gnome-software basically run pkexec cp /tmp/user/repos.d /etc/yum.repos.d?

kalev · 2019-03-14T11:49:03Z

I originally only wanted to add an API to enable and disable repos, I don't need to edit any of the other properties. Only did the more generic version because jlebon thought it would make sense :)

I'd be happy to change the API to EnableYumRepo with string repo_id and bool enable parameters if it seems less scary.

Sure, implementing a new dbus privilage separation daemon that can edit the .repo files for gnome-software is certainly an option if you don't want it in rpm-ostreed. It would be easier to have it in rpm-ostreed, but anything really works.

jlebon · 2019-03-14T14:04:06Z

In the end if one can configure arbitrary yum repos it's equivalent to arbitrary code execution on the host, not going to be easy to wrap it in anything that constrains it with polkit.

That's true, though UpdateDeployment() is more or less the same way, no? E.g. "install-local-packages" + "reboot=true" is a much more direct way to gain code execution without much differentiation wrt to what's actually being installed.

Also the proposed API here is essentially the same PackageKit already offers with RepoSetData().

So focusing more on whether rpm-ostree should be in the business of editing config files. I agree we want to avoid that and not proactively add APIs that do this, though I can imagine situations where it'd be useful, esp. in scenarios where rpm-ostree is essentially part of the "host API" (an easy one that comes to mind is containers talking to rpm-ostree over D-Bus). So I guess my opinion is: ideally no, but if necessary yes? :)

but let's at least consider the (IMO far simpler) alternative of just having gnome-software basically run pkexec cp /tmp/user/repos.d /etc/yum.repos.d?

That's... pretty cool. Hadn't looked at pkexec before.

Sure, implementing a new dbus privilage separation daemon that can edit the .repo files for gnome-software is certainly an option if you don't want it in rpm-ostreed.

I think what @cgwalters is suggesting (do correct me if not!) is GNOME Software copies the yum repos to some user owned space, does whatever modifications it wants, then literally spawns pkexec cp ... to get it written back to the privileged spot. So it wouldn't have to be a separate daemon. WDYT?

kalev · 2019-03-14T14:19:29Z

Not sure I like the pkexec idea. There's no way to customize the polkit message there for starters.

cgwalters · 2019-03-14T15:06:18Z

Sorry I'd glossed over the fact that this was originally only about enabling/disabling repos. I had thought we were scoping in adding/removing.

So focusing more on whether rpm-ostree should be in the business of editing config files. ... esp. in scenarios where rpm-ostree is essentially part of the "host API" (an easy one that comes to mind is containers talking to rpm-ostree over D-Bus). So I guess my opinion is: ideally no, but if necessary yes? :)

Talking about the machine-config-operator is a perfect example here because we have the host rootfs mounted. When we go to write systemd units it's directly via the filesystem. If we were to add an rpmRepositories entry to MachineConfig or so, I would almost certainly lean towards just writing the files to /rootfs/etc/yum.repos.d directly in the code rather than talking DBus.

(Partly because DBus in golang is...eh, but a lot just because it's simple and works)

kalev · 2019-03-14T19:18:17Z

Alright, I tested this now and seems to work fine :) Pushed a fixup with jlebon's review comments and added the missing polkit support.

The API questions are still open though: 1) should it be generic ModifyYumRepo or just EnableYumRepo?
2) If it's ModifyYumRepo should the a{ss} param be called repo_options, options, settings, or parameters?

jlebon · 2019-03-14T19:55:20Z

Talking about the machine-config-operator is a perfect example here because we have the host rootfs mounted.

I think if a container can do this, it's clearly better. Though the MCD is basically a super-SPC (well, that's redundant, let's say mega-SPC). Not to say mounting D-Bus isn't equally dangerous if the container is also running as root.

Not sure I like the pkexec idea. There's no way to customize the polkit message there for starters.

Hmm, yeah I do see this in pkexec(1):

ACTION AND AUTHORIZATIONS
       By default, the org.freedesktop.policykit.exec action is used. To use another action,
       use the org.freedesktop.policykit.exec.path annotation on an action with the value set
       to the full path of the program. In addition to specifying the program, the
       authentication message, description, icon and defaults can be specified.

Though I think that'd mean you have to (1) ship a little helper in e.g. /usr/lib/gnome-software/..., then (2) add a policy for executing the helper? @cgwalters Is there an easier way to do this?

This is somewhat related to the discussions we've had in #1768. Unless PackageKit grows some kind of rpm-ostree backend, then it sorta falls on rpm-ostree to provide some of the same basic APIs. The alternative is e.g. GNOME Software and Discover having to learn a whole new way of asking for privileged actions.

jlebon · 2019-03-14T21:28:05Z

should it be generic ModifyYumRepo or just EnableYumRepo?

One hybrid option is keeping as ModifyYumRepo and the method signature the same, but only accepting the enabled key for now. If another convincing use case shows up that requires broader control, then we can loosen it under a different action.

kalev · 2019-03-15T07:49:30Z

Ahh, I think that makes sense. Pushed a commit that changes it to only accept enabled for now.

jlebon

This looks good to me, just some minor nits! Could you also just squash all your commits into one?
Will let @cgwalters take a look at the final approach here.

jlebon · 2019-03-15T13:33:30Z

src/daemon/org.projectatomic.rpmostree1.xml

+    <!-- Set options in yum .repo files -->
+    <method name="ModifyYumRepo">
+      <arg type="s" name="repo_id" direction="in"/>
+      <arg type="a{ss}" name="repo_options" direction="in"/>


Sure, settings sounds good to me!

src/daemon/rpmostreed-transaction-types.c

This allows clients such as gnome-software to enable and disable yum repositories. The API is generic, but for now we only allow changing the 'enabled' key. If needed, it's easy to allow changing other settings in the future. See the discussion in the PR for the reasoning. Closes: coreos#1771

kalev · 2019-03-15T14:21:37Z

Squashed all commits and fixed the two nits! Thanks!

cgwalters · 2019-03-21T12:45:49Z

@rh-atomic-bot r+ ae5fc1a

rh-atomic-bot · 2019-03-21T12:45:54Z

⌛ Testing commit ae5fc1a with merge 891f5b7...

rh-atomic-bot · 2019-03-21T13:33:04Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 891f5b7 to master...

jlebon reviewed Mar 8, 2019

View reviewed changes

jlebon reviewed Mar 15, 2019

View reviewed changes

kalev force-pushed the ModifyYumRepo branch from d283949 to ae5fc1a Compare March 15, 2019 14:20

cgwalters added the reviewed/approved label Mar 21, 2019

rh-atomic-bot closed this in 891f5b7 Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Add new ModifyYumRepo DBus API #1780

daemon: Add new ModifyYumRepo DBus API #1780

kalev commented Mar 8, 2019

jlebon left a comment

jlebon Mar 8, 2019

kalev Mar 14, 2019

kalev Mar 14, 2019

jlebon Mar 15, 2019

jlebon commented Mar 11, 2019

cgwalters commented Mar 13, 2019

kalev commented Mar 14, 2019

jlebon commented Mar 14, 2019

kalev commented Mar 14, 2019

cgwalters commented Mar 14, 2019

kalev commented Mar 14, 2019 •

edited

Loading

jlebon commented Mar 14, 2019

jlebon commented Mar 14, 2019

kalev commented Mar 15, 2019

jlebon left a comment

jlebon Mar 15, 2019

kalev commented Mar 15, 2019

cgwalters commented Mar 21, 2019

rh-atomic-bot commented Mar 21, 2019

rh-atomic-bot commented Mar 21, 2019

daemon: Add new ModifyYumRepo DBus API #1780

daemon: Add new ModifyYumRepo DBus API #1780

Conversation

kalev commented Mar 8, 2019

jlebon left a comment

Choose a reason for hiding this comment

jlebon Mar 8, 2019

Choose a reason for hiding this comment

kalev Mar 14, 2019

Choose a reason for hiding this comment

kalev Mar 14, 2019

Choose a reason for hiding this comment

jlebon Mar 15, 2019

Choose a reason for hiding this comment

jlebon commented Mar 11, 2019

cgwalters commented Mar 13, 2019

kalev commented Mar 14, 2019

jlebon commented Mar 14, 2019

kalev commented Mar 14, 2019

cgwalters commented Mar 14, 2019

kalev commented Mar 14, 2019 • edited Loading

jlebon commented Mar 14, 2019

jlebon commented Mar 14, 2019

kalev commented Mar 15, 2019

jlebon left a comment

Choose a reason for hiding this comment

jlebon Mar 15, 2019

Choose a reason for hiding this comment

kalev commented Mar 15, 2019

cgwalters commented Mar 21, 2019

rh-atomic-bot commented Mar 21, 2019

rh-atomic-bot commented Mar 21, 2019

kalev commented Mar 14, 2019 •

edited

Loading