[ovs-dev] [PATCH v2 2/2] netdev-dpdk: Support user-defined socket attribs

Fri Jun 10 18:54:56 UTC 2016

On 10 June 2016 at 10:51, Aaron Conole <aconole at redhat.com> wrote:

> Aaron Conole <aconole at redhat.com> writes:
>
> > Christian Ehrhardt <christian.ehrhardt at canonical.com> writes:
> >
> >> On Tue, May 24, 2016 at 4:10 PM, Aaron Conole <aconole at redhat.com>
> wrote:
> >>
> >>> Daniele Di Proietto <diproiettod at vmware.com> writes:
> >>>
> >>> > Hi Aaron,
> >>> >
> >>> > I'm still a little bit nervous about calling chown on a (partially)
> >>> > user controlled file name.
> >>>
> >>> I agree, that always seems scary.
> >>>
> >>> > Before moving forward I wanted to discuss a couple of other options:
> >>> >
> >>> > * Ansis (in CC) suggested using -runas parameter in qemu.  This way
> >>> > qemu can open the socket as root and drop privileges before starting
> >>> > guest execution.
> >>>
> >>> I'm not sure how to do this with libvirt, or via the OpenStack Neutron
> >>> plugin.  I also don't know if it would be an acceptable workaround for
> >>> users.  Additionally, I recall there being something of a "don't even
> >>> know if this works" around it.  Maybe Christian or Ansis (both in CC)
> >>> can expound on it.
> >>>
> >>
> >> Hi,
> >> IIRC we kind of agree that long term a proper MAC will be much better
> but
> >> most involved people needed something to get it working like "now".
> >> Since they are complementary (other than the fix removing a bit of the
> >> urgency for more MAC) it was kind of the least bad option.
> >>
> >> You have to be aware that I brought up the discussion on dev at dpdk.org
> - see
> >> [1] and [2]:
> >> But this will take time and eventually still be the applications task to
> >> "do something" - no matter if via API or via the chmod's right now.
> >> So Aaron is trying to get something that works now until the long term
> >> things are in place, which I appreciate.
> >>
> >> FYI - I was even more in a hurry as it was clear that OVS-2.5 won't get
> >> this in time I run with [3] for now.
> >> I never intended to suggest that, but with the discussion in place, one
> >> could ask if you (Aaron) want to pick up that instead.
> >> That would keep OVS free for now until DPDK made up the API (see [2])
> for
> >> socket ownership control and this then could be implemented in OVS?
> >>
> >> (I hope) In some months/years we will all be happy to drop this bunch of
> >> interim solutions, never the less we need it for now.
> >>
> >> [1]: http://dpdk.org/dev/patchwork/patch/12222/
> >> [2]: http://dpdk.org/ml/archives/dev/2015-December/030326.html
> >> [3]:
> >>
> https://git.launchpad.net/~ubuntu-server/dpdk/commit/?h=ubuntu-xenial-to-dpdk2.2&id=f3c7aa1b2ddea8e092ad4a89e41a0e19d01ed4e7
> >>
> >> [...]
> >>
> >>
> >>> I think originally we quickly discussed 4 possible solutions (and
> >>> hopefully I captured them correctly):
> >>>
> >>> 1. OVS downgrades to the ovs user, and kvm runs under the ovs
> >>>    group.  I don't actually like this solution because kvm could then
> >>>    pollute the ovs database.
> >>>
> >>> 2. OVS runs as some user and sets the user/group ownership of the
> socket
> >>>    via chown/chmod where permissions come from the database (the
> >>>    original context had ovs running as root - but as I described above
> >>>    it doesn't need to be root provided ovs+DPDK can start without
> root).
> >>>
> >>> 3. OVS runs as some user, kvm starts as root, opens the socket and
> >>>    downgrades.  IIRC, this doesn't actually work, or it may have
> >>>    implications on other projects.  I don't remember exactly what was
> >>>    not as great about this solution, TBH.
> >>>
> >>> 4. OVS and KVM run as whatever users; MAC is used to enforce the
> >>>    layering between them.
> >>>
> >>> I think solution 2 and solution 4 don't actually interfere with each
> >>> other, and can be used to a complementary effect (if implemented
> >>> properly) so that the MAC layer enforces access, but even without MAC,
> >>> the DAC layer can provide appropriate whitelisting behavior.
> >>>
> >>
> >> I also remember several complex changes needed for the #1 and #3 that
> >> always would end up with huge effort and a high risk not being accepted.
> >> Probably that is what you refer to with "implications on other
> projects".
> >>
> >> Also keep in mind the position of dpdk out of the last few discussions
> >> which I'd like to summarize as "dpdk got this path from an app, so this
> app
> >> OWNS that path".
> >
> > I'd like to continue on, but I am not sure what the concerns are right
> > now.  Is it possible to enumerate them point by point so that I can
> > understand them?  I think there are two outstanding concerns right now:
> >
> > 1. the proposed approach is not good enough (vis-a-vis DAC vs. MAC)
> >
> > 2. the proposed approach would be better implemented in the utility
> >    that wants access to the sockets (vis-a-vis the libvirt discussion)
> >
> > Am I understanding the concerns correctly?
>
> Ping?
>

I believe this "ping" was intended to Christian, but I will weigh in my
opinion anyway:

1. MAC was brought up only because it is hard to properly implement --user
flag in Open vSwitch across all ovs-* daemons (this --user flag would be
about "confining" Open vSwitch). However, your patch is about feature in
OVS that would let OVS to participate in confining other processes like
QEMU by granting them access to certain resources created by Open vSwitch.
2. All my past comments on who should "chown()" the socket were based on
*incorrect* assumption that the goal of this patch was to get
Libvirt+OVS/DPDK integration going on. Clearly my assumption about the goal
was wrong and I believe this patch is primarily targeted for super-users
and developers who start QEMU from command line and don't rely on Libvirt
to configure access control for QEMU.

Having said that and understanding the purpose of your patch better I am
fine with the concept of it. Though I would like to point out that it
brings OVS into business of changing system's access control - just like
what Libvirt does today with code that they have here
https://github.com/libvirt/libvirt/tree/master/src/security.

Again, I am fine with the concept of the patch. Though, there are two
things to think about regarding the DB schema design:
1. I think this patch sets a precedent where in future someone else may
propose that Open vSwitch also needs to set SELinux file/socket context. I
think the OVSDB schema that you propose here would not conflict with such
future request, but just wanted to make sure.
2. I think this patch may also set a precedent where in future someone else
may ask for other non-DPDK Unix Domain Sockets to be chown()'ed and
chmod()'ed in a similar way as DPDK socket (e.g. controller socket, ovsdb
socket and unixctl socket). In that case maybe the method to specify user
and permission bits should be made consistent so that the feature could be
generalized eventually? For example, rename "vhost-sock-owner" to something
more generic like "sock-owner" so that Open vSwitch users would have
consistent way to deal with all the unix domain sockets and we could reuse
the same man page.

And there is third thing regarding implementation:

3. OVS also has internal access control for some file system related
operations - see http://openvswitch.org/pipermail/dev/2016-June/072476.html for
more details. Perhaps it would be better for 1/2 patch to have a check
inside ovs_chown() where it would make sure that chown()'ing can happen
only inside ovs_rundir()? Anyway what you have here is correct because you
have only one caller that already does this validation for you (after
commit d8a8f353c2 "netdev-dpdk: Restrict vhost_sock_dir" ). Though, I am
wondering if this would make other folks involved in thread to feel more
comfortable - let's hear what they they have to say...

Thanks,
Ansis