[libvirt] [Patch v2 0/3] Add QEMU network helper support
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> QEMU has a new feature which allows QEMU to execute under an unprivileged user ID and still be able to add a tap device to a Linux network bridge. Below is the link to the QEMU patches for the bridge helper feature: http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03562.html The existing libvirt tap network device support for adding a tap device to a bridge (-netdev tap) works only when connected to a libvirtd instance running as the privileged system account 'root'. When connected to a libvirtd instance running as an unprivileged user (ie. using the session URI) creation of the tap device fails as follows: error: Failed to start domain F14_64 error: Unable to create tap device vnet%d: Operation not permitted With this support, creating a tap device in the above scenario will be possible. Additionally, hot attaching a tap device to a bridge while running when connected to a libvirtd instance running as an unprivileged user will be possible. Richa Marwaha (3): Add -netdev bridge capabilities Add -netdev bridge support apparmor: QEMU bridge helper policy updates examples/apparmor/libvirt-qemu | 21 +++++++++++++- src/qemu/qemu_capabilities.c | 13 ++++++-- src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 61 ++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 31 ++++++++++++++------ 6 files changed, 97 insertions(+), 32 deletions(-)
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> This patch adds the capability in libvirt to check if -netdev bridge option is supported or not. Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- v2 -This is a new patch that helps libvirt to check if -netdev bridge option is supported in the qemu version that is being used to run the guest. In v1 we didnot have the capability to check if qemu version we are using supported -netdev bridge or not. src/qemu/qemu_capabilities.c | 13 +++++++++---- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1e12a39..b2d7be2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -166,6 +166,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "hda-micro", /* 95 */ "dump-guest-memory", "nec-usb-xhci", + "bridge", ); @@ -1005,7 +1006,7 @@ qemuCapsComputeCmdFlags(const char *help, bool check_yajl ATTRIBUTE_UNUSED) { const char *p; - const char *fsdev; + const char *fsdev, *netdev; if (strstr(help, "-no-kqemu")) qemuCapsSet(flags, QEMU_CAPS_KQEMU); @@ -1118,13 +1119,17 @@ qemuCapsComputeCmdFlags(const char *help, if (strstr(help, "-smbios type")) qemuCapsSet(flags, QEMU_CAPS_SMBIOS_TYPE); - if (strstr(help, "-netdev")) { + if ((netdev = strstr(help, "-netdev"))) { /* Disable -netdev on 0.12 since although it exists, * the corresponding netdev_add/remove monitor commands * do not, and we need them to be able to do hotplug. * But see below about RHEL build. */ - if (version >= 13000) - qemuCapsSet(flags, QEMU_CAPS_NETDEV); + if (version >= 13000) { + if (strstr (netdev, "bridge")) + qemuCapsSet(flags, QEMU_CAPS_NETDEV_BRIDGE); + else + qemuCapsSet(flags, QEMU_CAPS_NETDEV); + } } if (strstr(help, "-sdl")) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 83c135b..458a73e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -133,6 +133,7 @@ enum qemuCapsFlags { QEMU_CAPS_HDA_MICRO = 95, /* -device hda-micro */ QEMU_CAPS_DUMP_GUEST_MEMORY = 96, /* dump-guest-memory command */ QEMU_CAPS_NEC_USB_XHCI = 97, /* -device nec-usb-xhci */ + QEMU_CAPS_NETDEV_BRIDGE = 98, /* bridge helper support */ QEMU_CAPS_LAST, /* this must always be the last item */ }; -- 1.7.1
On 29.06.2012 20:08, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch adds the capability in libvirt to check if -netdev bridge option is supported or not.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- v2 -This is a new patch that helps libvirt to check if -netdev bridge option is supported in the qemu version that is being used to run the guest. In v1 we didnot have the capability to check if qemu version we are using supported -netdev bridge or not.
src/qemu/qemu_capabilities.c | 13 +++++++++---- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1e12a39..b2d7be2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -166,6 +166,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "hda-micro", /* 95 */ "dump-guest-memory", "nec-usb-xhci", + "bridge",
);
@@ -1005,7 +1006,7 @@ qemuCapsComputeCmdFlags(const char *help, bool check_yajl ATTRIBUTE_UNUSED) { const char *p; - const char *fsdev; + const char *fsdev, *netdev;
if (strstr(help, "-no-kqemu")) qemuCapsSet(flags, QEMU_CAPS_KQEMU); @@ -1118,13 +1119,17 @@ qemuCapsComputeCmdFlags(const char *help, if (strstr(help, "-smbios type")) qemuCapsSet(flags, QEMU_CAPS_SMBIOS_TYPE);
- if (strstr(help, "-netdev")) { + if ((netdev = strstr(help, "-netdev"))) { /* Disable -netdev on 0.12 since although it exists, * the corresponding netdev_add/remove monitor commands * do not, and we need them to be able to do hotplug. * But see below about RHEL build. */ - if (version >= 13000) - qemuCapsSet(flags, QEMU_CAPS_NETDEV); + if (version >= 13000) { + if (strstr (netdev, "bridge")) + qemuCapsSet(flags, QEMU_CAPS_NETDEV_BRIDGE); + else + qemuCapsSet(flags, QEMU_CAPS_NETDEV); + } }
I think we want both capabilities set as QEMU_CAPS_NETDEV is used later in the code. Moreover, QEMU_CAPS_NETDEV_BRIDGE is just refined version of QEMU_CAPS_NETDEV. That is, if QEMU_CAPS_NETDEV_BRIDGE is set, QEMU_CAPS_NETDEV must be supported as well.
if (strstr(help, "-sdl")) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 83c135b..458a73e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -133,6 +133,7 @@ enum qemuCapsFlags { QEMU_CAPS_HDA_MICRO = 95, /* -device hda-micro */ QEMU_CAPS_DUMP_GUEST_MEMORY = 96, /* dump-guest-memory command */ QEMU_CAPS_NEC_USB_XHCI = 97, /* -device nec-usb-xhci */ + QEMU_CAPS_NETDEV_BRIDGE = 98, /* bridge helper support */
QEMU_CAPS_LAST, /* this must always be the last item */ };
You forgot to add yourself to AUTHORS: diff --git a/AUTHORS b/AUTHORS index 0e179a9..f59550e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -247,6 +247,7 @@ Patches have also been contributed by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Thang Pham <thang.pham@us.ibm.com> Eiichi Tsukata <eiichi.tsukata.xh@hitachi.com> + Richa Marwaha <rmarwah@linux.vnet.ibm.com> [....send patches to get your name here....] And don't forget to update tests/qemuhelptest.c as well. Michal
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> This patch adds the support to run the QEMU network helper under unprivileged user. It also adds the support for attach-interface option in virsh to run under unprivileged user. Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- v2 - This patch attach-interface option is tested on commit cd15303fd123146b0ba53e387d08ef22b707223 src/qemu/qemu_command.c | 61 +++++++++++++++++++++++++++++++++------------- src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 31 ++++++++++++++++------- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6549f57..4eb8cd5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2851,6 +2851,8 @@ error: char * qemuBuildHostNetStr(virDomainNetDefPtr net, + struct qemud_driver *driver, + virBitmapPtr qemuCaps, char type_sep, int vlan, const char *tapfd, @@ -2859,6 +2861,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, bool is_tap = false; virBuffer buf = VIR_BUFFER_INITIALIZER; enum virDomainNetType netType = virDomainNetGetActualType(net); + const char *brname = NULL; if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2868,8 +2871,21 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } switch (netType) { - case VIR_DOMAIN_NET_TYPE_NETWORK: + /* + * If type='bridge', and we're running as privileged user + * or -netdev bridge is not supported then it will fall + * through, -net tap,fd + */ case VIR_DOMAIN_NET_TYPE_BRIDGE: + if (!driver->privileged && + qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE)) { + brname = virDomainNetGetActualBridgeName(net); + virBufferAsprintf(&buf, "bridge%cbr=%s", type_sep, brname); + type_sep = ','; + is_tap = true; + break; + } + case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd); type_sep = ','; @@ -4997,7 +5013,7 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; char *nic, *host; - char tapfd_name[50]; + char tapfd_name[50] = ""; char vhostfd_name[50] = ""; int vlan; int bootindex = bootNet; @@ -5034,17 +5050,26 @@ qemuBuildCommandLine(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, - qemuCaps); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; + /* + * If type='bridge' then we attempt to allocate the tap fd here only if + * running under a privilged user or -netdev bridge option is not + * supported. + */ + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + driver->privileged || + (!qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) { + int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, + qemuCaps); + if (tapfd < 0) + goto error; + + last_good_net = i; + virCommandTransferFD(cmd, tapfd); + + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", + tapfd) >= sizeof(tapfd_name)) + goto no_memory; + } } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { int tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); @@ -5087,8 +5112,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-netdev"); - if (!(host = qemuBuildHostNetStr(net, ',', vlan, - tapfd_name, vhostfd_name))) + if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps, + ',', vlan, tapfd_name, + vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); @@ -5110,8 +5136,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (!(qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { virCommandAddArg(cmd, "-net"); - if (!(host = qemuBuildHostNetStr(net, ',', vlan, - tapfd_name, vhostfd_name))) + if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps, + ',', vlan, tapfd_name, + vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1eafeb3..ebf7ad0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -62,6 +62,8 @@ qemuBuildChrDeviceStr (virDomainChrDefPtr serial, /* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, + struct qemud_driver *driver, + virBitmapPtr qemuCaps, char type_sep, int vlan, const char *tapfd, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2fa75b..deca4cb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -699,12 +699,21 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, - priv->qemuCaps)) < 0) - goto cleanup; - iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) - goto cleanup; + /* + * If type=bridge then we attempt to allocate the tap fd here only if + * running under a privilged user or -netdev bridge option is not + * supported. + */ + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + driver->privileged || + (!qemuCapsGet (priv->qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) { + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, + priv->qemuCaps)) < 0) + goto cleanup; + iface_connected = true; + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + goto cleanup; + } } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, priv->qemuCaps, @@ -752,12 +761,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(netstr = qemuBuildHostNetStr(net, ',', - -1, tapfd_name, vhostfd_name))) + if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps, + ',', -1, tapfd_name, + vhostfd_name))) goto cleanup; } else { - if (!(netstr = qemuBuildHostNetStr(net, ' ', - vlan, tapfd_name, vhostfd_name))) + if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps, + ' ', vlan, tapfd_name, + vhostfd_name))) goto cleanup; } -- 1.7.1
On 29.06.2012 20:08, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch adds the support to run the QEMU network helper under unprivileged user. It also adds the support for attach-interface option in virsh to run under unprivileged user.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- v2 - This patch attach-interface option is tested on commit cd15303fd123146b0ba53e387d08ef22b707223
src/qemu/qemu_command.c | 61 +++++++++++++++++++++++++++++++++------------- src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 31 ++++++++++++++++------- 3 files changed, 67 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6549f57..4eb8cd5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2851,6 +2851,8 @@ error:
char * qemuBuildHostNetStr(virDomainNetDefPtr net, + struct qemud_driver *driver, + virBitmapPtr qemuCaps, char type_sep, int vlan, const char *tapfd, @@ -2859,6 +2861,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, bool is_tap = false; virBuffer buf = VIR_BUFFER_INITIALIZER; enum virDomainNetType netType = virDomainNetGetActualType(net); + const char *brname = NULL;
if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2868,8 +2871,21 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, }
switch (netType) { - case VIR_DOMAIN_NET_TYPE_NETWORK: + /* + * If type='bridge', and we're running as privileged user + * or -netdev bridge is not supported then it will fall + * through, -net tap,fd + */ case VIR_DOMAIN_NET_TYPE_BRIDGE: + if (!driver->privileged && + qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE)) { + brname = virDomainNetGetActualBridgeName(net); + virBufferAsprintf(&buf, "bridge%cbr=%s", type_sep, brname); + type_sep = ','; + is_tap = true; + break; + } + case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd); type_sep = ','; @@ -4997,7 +5013,7 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; char *nic, *host; - char tapfd_name[50]; + char tapfd_name[50] = ""; char vhostfd_name[50] = ""; int vlan; int bootindex = bootNet; @@ -5034,17 +5050,26 @@ qemuBuildCommandLine(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, - qemuCaps); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; + /* + * If type='bridge' then we attempt to allocate the tap fd here only if + * running under a privilged user or -netdev bridge option is not + * supported. + */ + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + driver->privileged || + (!qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) { + int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, + qemuCaps); + if (tapfd < 0) + goto error; + + last_good_net = i; + virCommandTransferFD(cmd, tapfd); + + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", + tapfd) >= sizeof(tapfd_name)) + goto no_memory; + } } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { int tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); @@ -5087,8 +5112,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-netdev"); - if (!(host = qemuBuildHostNetStr(net, ',', vlan, - tapfd_name, vhostfd_name))) + if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps, + ',', vlan, tapfd_name, + vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); @@ -5110,8 +5136,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (!(qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { virCommandAddArg(cmd, "-net"); - if (!(host = qemuBuildHostNetStr(net, ',', vlan, - tapfd_name, vhostfd_name))) + if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps, + ',', vlan, tapfd_name, + vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1eafeb3..ebf7ad0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -62,6 +62,8 @@ qemuBuildChrDeviceStr (virDomainChrDefPtr serial,
/* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, + struct qemud_driver *driver, + virBitmapPtr qemuCaps, char type_sep, int vlan, const char *tapfd, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2fa75b..deca4cb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -699,12 +699,21 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, - priv->qemuCaps)) < 0) - goto cleanup; - iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) - goto cleanup; + /* + * If type=bridge then we attempt to allocate the tap fd here only if + * running under a privilged user or -netdev bridge option is not + * supported. + */ + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + driver->privileged || + (!qemuCapsGet (priv->qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) { + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, + priv->qemuCaps)) < 0) + goto cleanup; + iface_connected = true; + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + goto cleanup; + } } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, priv->qemuCaps, @@ -752,12 +761,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(netstr = qemuBuildHostNetStr(net, ',', - -1, tapfd_name, vhostfd_name))) + if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps, + ',', -1, tapfd_name, + vhostfd_name))) goto cleanup; } else { - if (!(netstr = qemuBuildHostNetStr(net, ' ', - vlan, tapfd_name, vhostfd_name))) + if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps, + ' ', vlan, tapfd_name, + vhostfd_name))) goto cleanup; }
ACK I am not going to review next apparmor patch which has already been reviewed. Michal
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge. Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-) diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012 #include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + } -- 1.7.1
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine. However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec). -- Jamie Strandboge | http://www.canonical.com
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from the domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Is there a way we can specify the configured path to the binary in the current policy we have? Regards Richa Marwaha
-- Jamie Strandboge | http://www.canonical.com
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from the domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker. -- Jamie Strandboge | http://www.canonical.com
Quoting Jamie Strandboge <jamie@canonical.com>:
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from the domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not. Thanks Regards Richa Marwaha
-- Jamie Strandboge | http://www.canonical.com
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from the domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile. -- Jamie Strandboge | http://www.canonical.com
Quoting Jamie Strandboge <jamie@canonical.com>:
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from the domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile.
Hi Jamie I started testing out the policy generation with the approach that if it checks inteface=bridge then only we generate the qemu-bridge-helper policies. I found 2 issues while trying to do that 1) There are a lot of ways to start bridge helper and the way libvirt is starting it is using -netdev bridge br=br0 and the executable is started by the qemu and not libvirt. So the way I can think of changing the path at compile time is to search for the executable in /usr/. Not being a big fan of this approach for searching the executable. 2) The second issue that I face is the generation of the child profile as virt-aa-helper creates a profile with -uuid and the virt-aa-helper appends the libvirt-uuid.files. Is there a way in virt-aa-helper that we could do the generation of child profile in libvirt-uuid profile? Regards Richa
-- Jamie Strandboge | http://www.canonical.com
On 07/26/2012 10:30 AM, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote: > From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> > > This patch provides AppArmor policy updates for the QEMU bridge helper. > The QEMU bridge helper is a SUID executable exec'd by QEMU that drops > capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge. > > Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> > Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> > --- > examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- > 1 files changed, 20 insertions(+), 1 deletions(-) > > diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu > index 10cdd36..766a334 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -1,4 +1,4 @@ > -# Last Modified: Mon Apr 5 15:11:27 2010 > +# Last Modified: Fri Mar 9 14:43:22 2012 > > #include <abstractions/base> > #include <abstractions/consoles> > @@ -108,3 +108,22 @@ > /bin/dash rmix, > /bin/dd rmix, > /bin/cat rmix, > + > + /usr/libexec/qemu-bridge-helper Cx, > + # child profile for bridge helper process > + profile /usr/libexec/qemu-bridge-helper { > + #include <abstractions/base> > + > + capability setuid, > + capability setgid, > + capability setpcap, > + capability net_admin, > + > + network inet stream, > + > + /dev/net/tun rw, > + /etc/qemu/** r, > + owner @{PROC}/*/status r, > + > + /usr/libexec/qemu-bridge-helper rmix, > + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote: the
domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile.
Hi Jamie I started testing out the policy generation with the approach that if it checks inteface=bridge then only we generate the qemu-bridge-helper policies. I found 2 issues while trying to do that 1) There are a lot of ways to start bridge helper and the way libvirt is starting it is using -netdev bridge br=br0 and the executable is started by the qemu and not libvirt. So the way I can think of changing the path at compile time is to search for the executable in /usr/. Not being a big fan of this approach for searching the executable.
Can you add a new tag to the domain XML that allows specification of the helper executable path? For example, adding <helper>: <interface type='bridge'> <source bridge='br0'/> <helper='/usr/libexec/qemu-bridge-helper' /> </interface> That would translate to something like: qemu-kvm -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=br0,id=xyz -device ... -- Regards, Corey
Quoting Corey Bryant <coreyb@linux.vnet.ibm.com>:
On 07/26/2012 10:30 AM, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
Quoting Jamie Strandboge <jamie@canonical.com>:
> On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote: >> From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> >> >> This patch provides AppArmor policy updates for the QEMU bridge helper. >> The QEMU bridge helper is a SUID executable exec'd by QEMU that drops >> capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge. >> >> Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> >> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> >> --- >> examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- >> 1 files changed, 20 insertions(+), 1 deletions(-) >> >> diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu >> index 10cdd36..766a334 100644 >> --- a/examples/apparmor/libvirt-qemu >> +++ b/examples/apparmor/libvirt-qemu >> @@ -1,4 +1,4 @@ >> -# Last Modified: Mon Apr 5 15:11:27 2010 >> +# Last Modified: Fri Mar 9 14:43:22 2012 >> >> #include <abstractions/base> >> #include <abstractions/consoles> >> @@ -108,3 +108,22 @@ >> /bin/dash rmix, >> /bin/dd rmix, >> /bin/cat rmix, >> + >> + /usr/libexec/qemu-bridge-helper Cx, >> + # child profile for bridge helper process >> + profile /usr/libexec/qemu-bridge-helper { >> + #include <abstractions/base> >> + >> + capability setuid, >> + capability setgid, >> + capability setpcap, >> + capability net_admin, >> + >> + network inet stream, >> + >> + /dev/net/tun rw, >> + /etc/qemu/** r, >> + owner @{PROC}/*/status r, >> + >> + /usr/libexec/qemu-bridge-helper rmix, >> + } > > Looking at the child profile itself, this seems fine. > > However, the Cx transition makes it so that all libvirt-managed qemu > processes are allowed to execute this setuid helper and I wonder if that > is too much? Ie, a guest running under libvirt's NAT wouldn't need > access to this helper at all. I wonder if it would be better to adjust > virt-aa-helper to add this transition and child profile instead (the > child profile could theoretically still be in apparmor/libvirt-qemu, but > this is a bit messy)? Can we determine from the domain XML the > circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, > virt-aa-helper could add the access only then. As a side-benefit, > handling this in virt-aa-helper allows us at compile-time to adjust the > path to qemu-bridge-helper to use the configured path to the binary (ie, > some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote: the
domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile.
Hi Jamie I started testing out the policy generation with the approach that if it checks inteface=bridge then only we generate the qemu-bridge-helper policies. I found 2 issues while trying to do that 1) There are a lot of ways to start bridge helper and the way libvirt is starting it is using -netdev bridge br=br0 and the executable is started by the qemu and not libvirt. So the way I can think of changing the path at compile time is to search for the executable in /usr/. Not being a big fan of this approach for searching the executable.
Can you add a new tag to the domain XML that allows specification of the helper executable path? For example, adding <helper>:
<interface type='bridge'> <source bridge='br0'/> <helper='/usr/libexec/qemu-bridge-helper' /> </interface>
That would translate to something like:
qemu-kvm -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=br0,id=xyz -device ...
Corey I can add the tag but the domain xml gets the information from the installation of the guest (correct me if I am wrong in this thought). I did think of adding a new member in the bridge definition currently it has 3 members brname, ipaddr and virtPortProfile. But I would still have to do the search for the qemu-bridge-helper. Regards Richa
-- Regards, Corey
On 07/26/2012 11:54 PM, Corey Bryant wrote:
On 07/26/2012 10:30 AM, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
> On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote: >> From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> >> >> This patch provides AppArmor policy updates for the QEMU bridge helper. >> The QEMU bridge helper is a SUID executable exec'd by QEMU that drops >> capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge. >> >> Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> >> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> >> --- >> examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- >> 1 files changed, 20 insertions(+), 1 deletions(-) >> >> diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu >> index 10cdd36..766a334 100644 >> --- a/examples/apparmor/libvirt-qemu >> +++ b/examples/apparmor/libvirt-qemu >> @@ -1,4 +1,4 @@ >> -# Last Modified: Mon Apr 5 15:11:27 2010 >> +# Last Modified: Fri Mar 9 14:43:22 2012 >> >> #include <abstractions/base> >> #include <abstractions/consoles> >> @@ -108,3 +108,22 @@ >> /bin/dash rmix, >> /bin/dd rmix, >> /bin/cat rmix, >> + >> + /usr/libexec/qemu-bridge-helper Cx, >> + # child profile for bridge helper process >> + profile /usr/libexec/qemu-bridge-helper { >> + #include <abstractions/base> >> + >> + capability setuid, >> + capability setgid, >> + capability setpcap, >> + capability net_admin, >> + >> + network inet stream, >> + >> + /dev/net/tun rw, >> + /etc/qemu/** r, >> + owner @{PROC}/*/status r, >> + >> + /usr/libexec/qemu-bridge-helper rmix, >> + } > > Looking at the child profile itself, this seems fine. > > However, the Cx transition makes it so that all libvirt-managed qemu > processes are allowed to execute this setuid helper and I wonder if that > is too much? Ie, a guest running under libvirt's NAT wouldn't need > access to this helper at all. I wonder if it would be better to adjust > virt-aa-helper to add this transition and child profile instead (the > child profile could theoretically still be in apparmor/libvirt-qemu, but > this is a bit messy)? Can we determine from the domain XML the > circumstances when /usr/libexec/qemu-bridge-helper will be used?
Well, with the code in patch 1/3 and 2/3, if the actualinterface is type='bridge', and libvirt is running unprivileged (!driver->privileged), and the qemu capabilities has QEMU_CAPS_NETDEV_BRIDGE, then the qemu-helper will definitely be used.
> virt-aa-helper could add the access only then. As a side-benefit, > handling this in virt-aa-helper allows us at compile-time to adjust the > path to qemu-bridge-helper to use the configured path to the binary (ie, > some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from
domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example
anyway, and distributions are expected to modify this, so I don't
If so, the policy think
this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile.
Hi Jamie I started testing out the policy generation with the approach that if it checks inteface=bridge then only we generate the qemu-bridge-helper policies. I found 2 issues while trying to do that 1) There are a lot of ways to start bridge helper and the way libvirt is starting it is using -netdev bridge br=br0 and the executable is started by the qemu and not libvirt. So the way I can think of changing the path at compile time is to search for the executable in /usr/. Not being a big fan of this approach for searching the executable.
Can you add a new tag to the domain XML that allows specification of the helper executable path? For example, adding <helper>:
<interface type='bridge'> <source bridge='br0'/> <helper='/usr/libexec/qemu-bridge-helper' /> </interface>
That's a bit opaque about its meaning, and I'm not sure it has a place at this location in the XML - who (I mean "what person") is going to determine the location of the bridge helper, and decide that it's even needed? Especially since the idea here is to allow unprivileged users to get full networking functionality, this config will be provided by an unprivileged user - is there any risk in allowing an unprivileged user to specify this binary's path? (other than the possibility they'll misunderstand it and simply get it wrong, that is) On the other hand, I think it will be useful to have *something* in the XML that indicates this interface wants to use the qemu-helper method of attaching to the bridge, for a reason that will come at the end of the message: To step back a bit and change the topic slightly (but it leads to the "reason" I mention above :-): My reservation about this patch series is that I want to be sure that it is "future proof" in the eventuality that other methods of achieving the same goal (proper networking support for guests started by non-privileged users) become available. In my opinion, having qemu execute a suid binary isn't the proper way to solve the problem (however, it's currently the only way available), and I'm hoping that eventually libvirt can be enhanced to achieve the desired results in a different manner. In particular, the problems I see with the qemu helper binary are: 1) A lot of time and trouble has been spent minimizing the capabilities of qemu in order to reduce the amount of damage that could be done by a compromised qemu process. The suid binary provides a way to "break out". True it is a small and limited binary, but "low risk" does not mean "no risk". 2) the text-file-based ACLs it uses will be cumbersome to manage (and definitely outside the scope of libvirt). This really should be done by policykit instead of home-grown ACL files. 3) the qemu helper binary can only handle creating a tap device and connecting it to a bridge. It can't handle macvtap interfaces, libvirt's virtual networks (unless the user just happens to know the name of the bridge used for that interface), openvswitch bridges, nwfilter rules, bandwidth limiting, 802.1Qb[gh]... You get the picture - there is a *lot* of functionality that's off the table if the qemu helper binary is used (unless it gains a lot of features and becomes hopelessly intertwined with libvirt, and I don't think anyone wants that). The way that I think the problem should be solved is this: 1) All of the network-related functionality in the system instance of libvirt that is used by the qemu, lxc, etc. drivers internal to libvirt (including the nwfilter subsystem, and everything in bridge_driver.c) should be in a separate daemon from libvirtd, and made available via RPC with a public API that uses policykit for authorization/authentication, with separate selinux policies from libvirtd; maybe call it "libvirt-networkd". 2) When the system instance of libvirtd is creating a domain, it should call to libvirt-networkd via this API to do things like create a tap device, connect it to a bridge, add nwfilter rules, etc. 3) likewise, when a session (unprivileged) instance of libvirt is creating a domain, it also should call the same APIs, which (after proper authentication/authorization via policykit) will connect it to the privileged libvirt-networkd (or whatever its called) to perform the operation. The result will be that a guest created by an unprivileged user will (dependent on policykit granting access) have access to *all* of the network functionality that is available to a guest started by the system libvirt instance, including libvirt's virtual networks, macvtap interfaces, nwfilter rules, bandwidth limiting, etc. And managing who gets access to what will be done in the same way as any other resource gated by policykit. Additionally, libvirtd will no longer need many of the capabilities/policies it currently needs to permit setting up the networking (just as libvirt-networkd won't need all the capabilities/policies required by libvirtd to handle other functions of managing a guest's lifecycle). So, how does this relate to my desire to see something in the XML indicating that this interface should use the qemu helper? Well, I'm thinking ahead to the day when the work I've outlined above is completed - when it is included in a libvirt release, there will be people who have guests with network configs that work thanks to the presence of the qemu-helper support and an entry in the qemu-helper's ACL file. If they upgrade their libvirt and their config stops working (due to their uid not being authorized by policykit), they won't be happy. On the other hand, once this more comprehensive solution is available, it should be preferred by default. In order for that to be the case, we need to have some sort of tag in the XML *now* that indicates "use the qemu-helper", and otherwise pretend that it doesn't exist. That way when those people upgrade, even though the new method of setting up networking (which may not work without config changes) will be available, libvirt will understand it should continue to use the "old" method for this particular guest.
Laine Stump wrote:
The way that I think the problem should be solved is this:
1) All of the network-related functionality in the system instance of libvirt that is used by the qemu, lxc, etc. drivers internal to libvirt (including the nwfilter subsystem, and everything in bridge_driver.c) should be in a separate daemon from libvirtd, and made available via RPC with a public API that uses policykit for authorization/authentication, with separate selinux policies from libvirtd; maybe call it "libvirt-networkd".
2) When the system instance of libvirtd is creating a domain, it should call to libvirt-networkd via this API to do things like create a tap device, connect it to a bridge, add nwfilter rules, etc.
3) likewise, when a session (unprivileged) instance of libvirt is creating a domain, it also should call the same APIs, which (after proper authentication/authorization via policykit) will connect it to the privileged libvirt-networkd (or whatever its called) to perform the operation.
I wonder if the quantum project [1], which IIUC provides a lot of the functionality you describe, could be used as "libvirtd-networkd". Regards, Jim [1] https://github.com/openstack/quantum
On Mon, Jul 30, 2012 at 06:33:54PM -0600, Jim Fehlig wrote:
Laine Stump wrote:
The way that I think the problem should be solved is this:
1) All of the network-related functionality in the system instance of libvirt that is used by the qemu, lxc, etc. drivers internal to libvirt (including the nwfilter subsystem, and everything in bridge_driver.c) should be in a separate daemon from libvirtd, and made available via RPC with a public API that uses policykit for authorization/authentication, with separate selinux policies from libvirtd; maybe call it "libvirt-networkd".
2) When the system instance of libvirtd is creating a domain, it should call to libvirt-networkd via this API to do things like create a tap device, connect it to a bridge, add nwfilter rules, etc.
3) likewise, when a session (unprivileged) instance of libvirt is creating a domain, it also should call the same APIs, which (after proper authentication/authorization via policykit) will connect it to the privileged libvirt-networkd (or whatever its called) to perform the operation.
I wonder if the quantum project [1], which IIUC provides a lot of the functionality you describe, could be used as "libvirtd-networkd".
Not as long its its a big mass of python. On the contrary, Quantum could use libvirt's APIs. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/27/2012 04:00 PM, Laine Stump wrote:
On 07/26/2012 11:54 PM, Corey Bryant wrote:
On 07/26/2012 10:30 AM, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote: > Quoting Jamie Strandboge <jamie@canonical.com>: > >> On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote: >>> From: Richa Marwaha <rmarwah@linux.vnet.ibm.com> >>> >>> This patch provides AppArmor policy updates for the QEMU bridge helper. >>> The QEMU bridge helper is a SUID executable exec'd by QEMU that drops >>> capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge. >>> >>> Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> >>> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> >>> --- >>> examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- >>> 1 files changed, 20 insertions(+), 1 deletions(-) >>> >>> diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu >>> index 10cdd36..766a334 100644 >>> --- a/examples/apparmor/libvirt-qemu >>> +++ b/examples/apparmor/libvirt-qemu >>> @@ -1,4 +1,4 @@ >>> -# Last Modified: Mon Apr 5 15:11:27 2010 >>> +# Last Modified: Fri Mar 9 14:43:22 2012 >>> >>> #include <abstractions/base> >>> #include <abstractions/consoles> >>> @@ -108,3 +108,22 @@ >>> /bin/dash rmix, >>> /bin/dd rmix, >>> /bin/cat rmix, >>> + >>> + /usr/libexec/qemu-bridge-helper Cx, >>> + # child profile for bridge helper process >>> + profile /usr/libexec/qemu-bridge-helper { >>> + #include <abstractions/base> >>> + >>> + capability setuid, >>> + capability setgid, >>> + capability setpcap, >>> + capability net_admin, >>> + >>> + network inet stream, >>> + >>> + /dev/net/tun rw, >>> + /etc/qemu/** r, >>> + owner @{PROC}/*/status r, >>> + >>> + /usr/libexec/qemu-bridge-helper rmix, >>> + } >> >> Looking at the child profile itself, this seems fine. >> >> However, the Cx transition makes it so that all libvirt-managed qemu >> processes are allowed to execute this setuid helper and I wonder if that >> is too much? Ie, a guest running under libvirt's NAT wouldn't need >> access to this helper at all. I wonder if it would be better to adjust >> virt-aa-helper to add this transition and child profile instead (the >> child profile could theoretically still be in apparmor/libvirt-qemu, but >> this is a bit messy)? Can we determine from the domain XML the >> circumstances when /usr/libexec/qemu-bridge-helper will be used?
Well, with the code in patch 1/3 and 2/3, if the actualinterface is type='bridge', and libvirt is running unprivileged (!driver->privileged), and the qemu capabilities has QEMU_CAPS_NETDEV_BRIDGE, then the qemu-helper will definitely be used.
>> virt-aa-helper could add the access only then. As a side-benefit, >> handling this in virt-aa-helper allows us at compile-time to adjust the >> path to qemu-bridge-helper to use the configured path to the binary (ie, >> some distros may not use /usr/libexec). > > Thanks a lot reviewing the apparmor patch. We cannot detemine from
> domain XML whether /usr/libexec/qemu-bridge-helper will be used or not > because we cannot determine whether we are running as privileged user > or not. Hmmm, that's too bad.
> Is there a way we can specify the configured path to the > binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example
anyway, and distributions are expected to modify this, so I don't
If so, the policy think
this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile.
Hi Jamie I started testing out the policy generation with the approach that if it checks inteface=bridge then only we generate the qemu-bridge-helper policies. I found 2 issues while trying to do that 1) There are a lot of ways to start bridge helper and the way libvirt is starting it is using -netdev bridge br=br0 and the executable is started by the qemu and not libvirt. So the way I can think of changing the path at compile time is to search for the executable in /usr/. Not being a big fan of this approach for searching the executable.
Can you add a new tag to the domain XML that allows specification of the helper executable path? For example, adding <helper>:
<interface type='bridge'> <source bridge='br0'/> <helper='/usr/libexec/qemu-bridge-helper' /> </interface>
That's a bit opaque about its meaning, and I'm not sure it has a place at this location in the XML - who (I mean "what person") is going to determine the location of the bridge helper, and decide that it's even needed? Especially since the idea here is to allow unprivileged users to get full networking functionality, this config will be provided by an unprivileged user - is there any risk in allowing an unprivileged user to specify this binary's path? (other than the possibility they'll misunderstand it and simply get it wrong, that is)
On the other hand, I think it will be useful to have *something* in the XML that indicates this interface wants to use the qemu-helper method of attaching to the bridge, for a reason that will come at the end of the message:
To step back a bit and change the topic slightly (but it leads to the "reason" I mention above :-):
My reservation about this patch series is that I want to be sure that it is "future proof" in the eventuality that other methods of achieving the same goal (proper networking support for guests started by non-privileged users) become available.
In my opinion, having qemu execute a suid binary isn't the proper way to solve the problem (however, it's currently the only way available), and I'm hoping that eventually libvirt can be enhanced to achieve the desired results in a different manner.
In particular, the problems I see with the qemu helper binary are:
1) A lot of time and trouble has been spent minimizing the capabilities of qemu in order to reduce the amount of damage that could be done by a compromised qemu process. The suid binary provides a way to "break out". True it is a small and limited binary, but "low risk" does not mean "no risk".
2) the text-file-based ACLs it uses will be cumbersome to manage (and definitely outside the scope of libvirt). This really should be done by policykit instead of home-grown ACL files.
3) the qemu helper binary can only handle creating a tap device and connecting it to a bridge. It can't handle macvtap interfaces, libvirt's virtual networks (unless the user just happens to know the name of the bridge used for that interface), openvswitch bridges, nwfilter rules, bandwidth limiting, 802.1Qb[gh]... You get the picture - there is a *lot* of functionality that's off the table if the qemu helper binary is used (unless it gains a lot of features and becomes hopelessly intertwined with libvirt, and I don't think anyone wants that).
The way that I think the problem should be solved is this:
1) All of the network-related functionality in the system instance of libvirt that is used by the qemu, lxc, etc. drivers internal to libvirt (including the nwfilter subsystem, and everything in bridge_driver.c) should be in a separate daemon from libvirtd, and made available via RPC with a public API that uses policykit for authorization/authentication, with separate selinux policies from libvirtd; maybe call it "libvirt-networkd".
2) When the system instance of libvirtd is creating a domain, it should call to libvirt-networkd via this API to do things like create a tap device, connect it to a bridge, add nwfilter rules, etc.
3) likewise, when a session (unprivileged) instance of libvirt is creating a domain, it also should call the same APIs, which (after proper authentication/authorization via policykit) will connect it to the privileged libvirt-networkd (or whatever its called) to perform the operation.
The result will be that a guest created by an unprivileged user will (dependent on policykit granting access) have access to *all* of the network functionality that is available to a guest started by the system libvirt instance, including libvirt's virtual networks, macvtap interfaces, nwfilter rules, bandwidth limiting, etc. And managing who gets access to what will be done in the same way as any other resource gated by policykit. Additionally, libvirtd will no longer need many of the capabilities/policies it currently needs to permit setting up the networking (just as libvirt-networkd won't need all the capabilities/policies required by libvirtd to handle other functions of managing a guest's lifecycle).
So, how does this relate to my desire to see something in the XML indicating that this interface should use the qemu helper? Well, I'm thinking ahead to the day when the work I've outlined above is completed - when it is included in a libvirt release, there will be people who have guests with network configs that work thanks to the presence of the qemu-helper support and an entry in the qemu-helper's ACL file. If they upgrade their libvirt and their config stops working (due to their uid not being authorized by policykit), they won't be happy. On the other hand, once this more comprehensive solution is available, it should be preferred by default. In order for that to be the case, we need to have some sort of tag in the XML *now* that indicates "use the qemu-helper", and otherwise pretend that it doesn't exist. That way when those people upgrade, even though the new method of setting up networking (which may not work without config changes) will be available, libvirt will understand it should continue to use the "old" method for this particular guest.
That sounds like a good future direction that you've outlined. At this point I wonder if we might be able to get away with no XML modifications since we know that we only want to attempt to run the helper when libvirt is running unprivileged. -- Regards, Corey
On Tue, Jul 31, 2012 at 11:26:05AM -0400, Corey Bryant wrote:
At this point I wonder if we might be able to get away with no XML modifications since we know that we only want to attempt to run the helper when libvirt is running unprivileged.
I certainly don't expect there to be any changes to the XML for the purposes of supporting this QEMU bridge helper proxy. It should be automatically used when type=bridge or type=network on the <interface>, as a private implementation detail of libvirt not exposed to applications in the XML. When libvirt changes to separate its nework setup code out of libvirtd, then we will be able to transparently stop using the proxy helper. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/31/2012 12:06 PM, Daniel P. Berrange wrote:
On Tue, Jul 31, 2012 at 11:26:05AM -0400, Corey Bryant wrote:
At this point I wonder if we might be able to get away with no XML modifications since we know that we only want to attempt to run the helper when libvirt is running unprivileged.
I certainly don't expect there to be any changes to the XML for the purposes of supporting this QEMU bridge helper proxy. It should be automatically used when type=bridge or type=network on the <interface>, as a private implementation detail of libvirt not exposed to applications in the XML. When libvirt changes to separate its nework setup code out of libvirtd, then we will be able to transparently stop using the proxy helper.
I would greatly prefer that as well, as long as nobody is bothered by the potential upgrade problem I outlined in my previous mail - if there is config required outside libvirt to enable particular users to use the qemu helper, and if there is also extra config required to enable the capability for users when libvirt no longer requires the qemu helper, there could be cases where a working config would cease to work (until extra measures were taken) after an upgrade. If that situation is acceptable, then I also agree that no XML modifications is much better.
Quoting Jamie Strandboge <jamie@canonical.com>:
On Mon, 2012-07-09 at 10:22 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Tue, 2012-07-03 at 12:05 -0400, rmarwah@linux.vnet.ibm.com wrote:
Quoting Jamie Strandboge <jamie@canonical.com>:
On Fri, 2012-06-29 at 14:08 -0400, rmarwah@linux.vnet.ibm.com wrote:
From: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
This patch provides AppArmor policy updates for the QEMU bridge helper. The QEMU bridge helper is a SUID executable exec'd by QEMU that drops capabilities to CAP_NET_ADMIN and adds a tap device to a network bridge.
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com> --- examples/apparmor/libvirt-qemu | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu index 10cdd36..766a334 100644 --- a/examples/apparmor/libvirt-qemu +++ b/examples/apparmor/libvirt-qemu @@ -1,4 +1,4 @@ -# Last Modified: Mon Apr 5 15:11:27 2010 +# Last Modified: Fri Mar 9 14:43:22 2012
#include <abstractions/base> #include <abstractions/consoles> @@ -108,3 +108,22 @@ /bin/dash rmix, /bin/dd rmix, /bin/cat rmix, + + /usr/libexec/qemu-bridge-helper Cx, + # child profile for bridge helper process + profile /usr/libexec/qemu-bridge-helper { + #include <abstractions/base> + + capability setuid, + capability setgid, + capability setpcap, + capability net_admin, + + network inet stream, + + /dev/net/tun rw, + /etc/qemu/** r, + owner @{PROC}/*/status r, + + /usr/libexec/qemu-bridge-helper rmix, + }
Looking at the child profile itself, this seems fine.
However, the Cx transition makes it so that all libvirt-managed qemu processes are allowed to execute this setuid helper and I wonder if that is too much? Ie, a guest running under libvirt's NAT wouldn't need access to this helper at all. I wonder if it would be better to adjust virt-aa-helper to add this transition and child profile instead (the child profile could theoretically still be in apparmor/libvirt-qemu, but this is a bit messy)? Can we determine from the domain XML the circumstances when /usr/libexec/qemu-bridge-helper will be used? If so, virt-aa-helper could add the access only then. As a side-benefit, handling this in virt-aa-helper allows us at compile-time to adjust the path to qemu-bridge-helper to use the configured path to the binary (ie, some distros may not use /usr/libexec).
Thanks a lot reviewing the apparmor patch. We cannot detemine from the domain XML whether /usr/libexec/qemu-bridge-helper will be used or not because we cannot determine whether we are running as privileged user or not. Hmmm, that's too bad.
Is there a way we can specify the configured path to the binary in the current policy we have?
Not in a convenient way, no. The policy is intended as example policy anyway, and distributions are expected to modify this, so I don't think this alone is a blocker.
Right now the only way we can think of is that whenever in domain XML we see interface=bridge we set the policy for the qemu-bridge-helper even though we don't know if the qemu-bridge-helper is going to be used or not.
Ah, well, that would at least be somewhat better and IMHO, worthwhile.
Hi Jamie As we discussed earlier that we cannot determine from the domain XML whether the user is privileged or not to have the generation of the apparmor policy for qemu-bridge-helper. One main purpose of doing this was to get the adjustment of the path of the qemu-bridge-helper as some distros don't have /usr/libexec. So I tried to have the generation of the policies be done when we see interface = bridge in the domain XML. In the process of doing so it comes out that there is not a good way to determine the location of the qemu-bridge-helper other than the fact that it is hard-coded in the virt-aa-helper code. Also could not find a good way to have the generation of the child profile for qemu-bridge-helper apparmor policy. Since we have to hard-code the path of the executable I would suggest that we keep the apparmor policies for qemu-bridge-helper in libvirt-qemu only. Kindly let me know your thoughts. Regards Richa
-- Jamie Strandboge | http://www.canonical.com
participants (7)
-
Corey Bryant -
Daniel P. Berrange -
Jamie Strandboge -
Jim Fehlig -
Laine Stump -
Michal Privoznik -
rmarwah＠linux.vnet.ibm.com