[libvirt] [PATCH 0/7] Introduce sVirt to LXC driver
This patch series adds support for sVirt to the LXC driver. In this series, all LXC guests continue to run unconfined by default. The app has to explicitly request sVirt confinement for the guest. This is to ensure backwards compatibility with existing deployments. Since we won't auto-relabel filesystem trees, it is not possible to turn it on by default, as we previously did with QEMU
From: "Daniel P. Berrange" <berrange@redhat.com> Add a virFileTouch API which ensures that a file will always exist, even if zero length * src/lxc/lxc_container.c, src/util/virfile.h: virFileTouch --- src/util/virfile.c | 21 +++++++++++++++++++++ src/util/virfile.h | 2 ++ 2 files changed, 23 insertions(+), 0 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index cbc3fcc..e6b469c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -390,3 +390,24 @@ cleanup: } return ret; } + + +int virFileTouch(const char *path, mode_t mode) +{ + int fd = -1; + + if ((fd = open(path, O_WRONLY | O_CREAT, mode)) < 0) { + virReportSystemError(errno, _("cannot create file '%s'"), + path); + return -1; + } + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("cannot save file '%s'"), + path); + VIR_FORCE_CLOSE(fd); + return -1; + } + + return 0; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index a6e6597..7be15b5 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -74,4 +74,6 @@ int virFileRewrite(const char *path, virFileRewriteFunc rewrite, void *opaque); +int virFileTouch(const char *path, mode_t mode); + #endif /* __VIR_FILES_H */ -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Add a virFileTouch API which ensures that a file will always exist, even if zero length
* src/lxc/lxc_container.c, src/util/virfile.h: virFileTouch
This line is not quite accurate,
--- src/util/virfile.c | 21 +++++++++++++++++++++ src/util/virfile.h | 2 ++ 2 files changed, 23 insertions(+), 0 deletions(-)
to this diffstat (drop lxc_container). Otherwise, looks like a useful API.
diff --git a/src/util/virfile.c b/src/util/virfile.c index cbc3fcc..e6b469c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -390,3 +390,24 @@ cleanup: } return ret; } + + +int virFileTouch(const char *path, mode_t mode) +{ + int fd = -1; + + if ((fd = open(path, O_WRONLY | O_CREAT, mode)) < 0) { + virReportSystemError(errno, _("cannot create file '%s'"), + path); + return -1;
Should we be using virFileOpenAs() for the sake of touching a file with different permissions on a root-squash NFS server? In particular, I wonder if any of qemu_driver.c could reuse this function, if it were to use virFileOpenAs() - for example, creating a qemu snapshot requires that a file already exist and be empty before making the monitor call. ACK - any change to use virFileOpenAs can come as a later patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The current setup code for LXC is bind mounting /dev/pts/ptmx ontop of a character device /dev/ptmx. This is denied by SELinux policy and is just wrong. The target of a bind mount should just be a plain file * src/lxc/lxc_container.c: Don't bind /dev/pts/ptmx onto a char device --- src/lxc/lxc_container.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 1e7803a..3fb7d06 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -555,18 +555,23 @@ static int lxcContainerPopulateDevices(char **ttyPaths, size_t nttyPaths) } } - dev_t dev = makedev(LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX); - if (mknod("/dev/ptmx", S_IFCHR, dev) < 0 || - chmod("/dev/ptmx", 0666)) { - virReportSystemError(errno, "%s", - _("Failed to make device /dev/ptmx")); - return -1; - } - if (access("/dev/pts/ptmx", W_OK) == 0) { + /* We have private devpts capability, so bind that */ + if (virFileTouch("/dev/ptmx", 0666) < 0) + return -1; + if (mount("/dev/pts/ptmx", "/dev/ptmx", "ptmx", MS_BIND, NULL) < 0) { virReportSystemError(errno, "%s", - _("Failed to bind-mount /dev/ptmx to /dev/pts/ptmx")); + _("Failed to bind /dev/pts/ptmx on to /dev/ptmx")); + return -1; + } + } else { + /* Legacy devpts, so we need to just use shared one */ + dev_t dev = makedev(LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX); + if (mknod("/dev/ptmx", S_IFCHR, dev) < 0 || + chmod("/dev/ptmx", 0666)) { + virReportSystemError(errno, "%s", + _("Failed to make device /dev/ptmx")); return -1; } } -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The current setup code for LXC is bind mounting /dev/pts/ptmx ontop of a character device /dev/ptmx. This is denied by SELinux
s/ontop/on top/
policy and is just wrong. The target of a bind mount should just be a plain file
* src/lxc/lxc_container.c: Don't bind /dev/pts/ptmx onto a char device --- src/lxc/lxc_container.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Revert parsing changes: commit 302fe95ffa1bc5f1c61c0beb31a1adfbc38c668e Author: Eric Blake <eblake@redhat.com> Date: Wed Jan 4 16:01:24 2012 -0700 seclabel: fix regression in libvirtd restart commit b43432931aef92325920953ff92beabfbe5224c8 Author: Eric Blake <eblake@redhat.com> Date: Thu Dec 22 17:47:50 2011 -0700 seclabel: allow a seclabel override on a disk src These two commits changed the sec label parsing code so that the same code dealt with both the VM level sec label, and the per device label. Unfortunately, as we add more options to the VM level sec label, the logic required to use the same parsing code for the per device label becomes unintelligable. * src/conf/domain_conf.c: Remove support for parsing per device sec labels --- src/conf/domain_conf.c | 190 ++++++++++++----------------------------------- 1 files changed, 49 insertions(+), 141 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 180dd2b..8db674b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2011 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -798,15 +798,6 @@ virSecurityLabelDefClear(virSecurityLabelDefPtr def) VIR_FREE(def->baselabel); } -static void -virSecurityLabelDefFree(virSecurityLabelDefPtr def) -{ - if (!def) - return; - virSecurityLabelDefClear(def); - VIR_FREE(def); -} - void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) { int ii; @@ -876,7 +867,6 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->serial); VIR_FREE(def->src); - virSecurityLabelDefFree(def->seclabel); VIR_FREE(def->dst); VIR_FREE(def->driverName); VIR_FREE(def->driverType); @@ -2527,33 +2517,31 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) return 0; } -/* Parse the portion of a SecurityLabel that is common to both the - * top-level <seclabel> and to a per-device override. - * default_seclabel is NULL for top-level, or points to the top-level - * when parsing an override. */ static int -virSecurityLabelDefParseXMLHelper(virSecurityLabelDefPtr def, - xmlNodePtr node, - xmlXPathContextPtr ctxt, - virSecurityLabelDefPtr default_seclabel, - unsigned int flags) +virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, + xmlXPathContextPtr ctxt, + unsigned int flags) { char *p; - xmlNodePtr save_ctxt = ctxt->node; - int ret = -1; - int type = default_seclabel ? default_seclabel->type : def->type; - ctxt->node = node; + if (virXPathNode("./seclabel", ctxt) == NULL) + return 0; - /* Can't use overrides if top-level doesn't allow relabeling. */ - if (default_seclabel && default_seclabel->norelabel) { - virDomainReportError(VIR_ERR_XML_ERROR, "%s", - _("label overrides require relabeling to be " - "enabled at the domain level")); - goto cleanup; + p = virXPathStringLimit("string(./seclabel/@type)", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing security type")); + goto error; } - - p = virXPathStringLimit("string(./@relabel)", + def->type = virDomainSeclabelTypeFromString(p); + VIR_FREE(p); + if (def->type < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("invalid security type")); + goto error; + } + p = virXPathStringLimit("string(./seclabel/@relabel)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p != NULL) { if (STREQ(p, "yes")) { @@ -2564,77 +2552,38 @@ virSecurityLabelDefParseXMLHelper(virSecurityLabelDefPtr def, virDomainReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), p); VIR_FREE(p); - goto cleanup; + goto error; } VIR_FREE(p); - if (!default_seclabel && - type == VIR_DOMAIN_SECLABEL_DYNAMIC && + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && def->norelabel) { - virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("dynamic label type must use resource " - "relabeling")); - goto cleanup; + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("dynamic label type must use resource relabeling")); + goto error; } } else { - if (!default_seclabel && type == VIR_DOMAIN_SECLABEL_STATIC) + if (def->type == VIR_DOMAIN_SECLABEL_STATIC) def->norelabel = true; else def->norelabel = false; } /* Only parse label, if using static labels, or - * if the 'live' VM XML is requested, or if this is a device override + * if the 'live' VM XML is requested */ - if (type == VIR_DOMAIN_SECLABEL_STATIC || - !(flags & VIR_DOMAIN_XML_INACTIVE) || - (default_seclabel && !def->norelabel)) { - p = virXPathStringLimit("string(./label[1])", + if (def->type == VIR_DOMAIN_SECLABEL_STATIC || + !(flags & VIR_DOMAIN_XML_INACTIVE)) { + p = virXPathStringLimit("string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL && !(default_seclabel && def->norelabel)) { + if (p == NULL) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); - goto cleanup; + goto error; } def->label = p; } - ret = 0; -cleanup: - ctxt->node = save_ctxt; - return ret; -} - -/* Parse the top-level <seclabel>, if present. */ -static int -virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, - xmlXPathContextPtr ctxt, - unsigned int flags) -{ - char *p; - xmlNodePtr node = virXPathNode("./seclabel", ctxt); - - if (node == NULL) - return 0; - - p = virXPathStringLimit("string(./seclabel/@type)", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security type")); - goto error; - } - def->type = virDomainSeclabelTypeFromString(p); - VIR_FREE(p); - if (def->type < 0) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("invalid security type")); - goto error; - } - - if (virSecurityLabelDefParseXMLHelper(def, node, ctxt, NULL, flags) < 0) - goto error; - /* Only parse imagelabel, if requested live XML with relabeling */ if (!def->norelabel && !(flags & VIR_DOMAIN_XML_INACTIVE)) { @@ -2760,7 +2709,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr node, xmlXPathContextPtr ctxt, virBitmapPtr bootMap, - virSecurityLabelDefPtr default_seclabel, unsigned int flags) { virDomainDiskDefPtr def; @@ -3068,16 +3016,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } - /* If source is present, check for an optional seclabel override. */ - if (source) { - xmlNodePtr seclabel = virXPathNode("./source/seclabel", ctxt); - if (seclabel && - (VIR_ALLOC(def->seclabel) < 0 || - virSecurityLabelDefParseXMLHelper(def->seclabel, seclabel, ctxt, - default_seclabel, flags) < 0)) - goto error; - } - if (target == NULL) { virDomainReportError(VIR_ERR_NO_TARGET, source ? "%s" : NULL, source); @@ -6400,8 +6338,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, if (xmlStrEqual(node->name, BAD_CAST "disk")) { dev->type = VIR_DOMAIN_DEVICE_DISK; if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, - NULL, &def->seclabel, - flags))) + NULL, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { dev->type = VIR_DOMAIN_DEVICE_LEASE; @@ -7503,7 +7440,6 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, nodes[i], ctxt, bootMap, - &def->seclabel, flags); if (!disk) goto error; @@ -9807,32 +9743,23 @@ virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def, if (!sectype) goto cleanup; - if (def->model && - def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && !def->baselabel && (flags & VIR_DOMAIN_XML_INACTIVE)) { /* This is the default for inactive xml, so nothing to output. */ } else { - virBufferAddLit(buf, "<seclabel"); - if (def->model) - virBufferAsprintf(buf, " type='%s' model='%s'", - sectype, def->model); - virBufferAsprintf(buf, " relabel='%s'", + virBufferAsprintf(buf, "<seclabel type='%s' model='%s' relabel='%s'>\n", + sectype, def->model, def->norelabel ? "no" : "yes"); - if (def->label || def->baselabel) { - virBufferAddLit(buf, ">\n"); - virBufferEscapeString(buf, " <label>%s</label>\n", - def->label); - if (!def->norelabel) - virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", - def->imagelabel); - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) - virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", - def->baselabel); - virBufferAddLit(buf, "</seclabel>\n"); - } else { - virBufferAddLit(buf, "/>\n"); - } + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + if (!def->norelabel) + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", + def->imagelabel); + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", + def->baselabel); + virBufferAddLit(buf, "</seclabel>\n"); } ret = 0; cleanup: @@ -9952,36 +9879,17 @@ virDomainDiskDefFormat(virBufferPtr buf, def->startupPolicy) { switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: - virBufferAddLit(buf, " <source"); + virBufferAsprintf(buf," <source"); if (def->src) virBufferEscapeString(buf, " file='%s'", def->src); if (def->startupPolicy) virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - if (def->seclabel) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 8); - if (virSecurityLabelDefFormat(buf, def->seclabel, flags) < 0) - return -1; - virBufferAdjustIndent(buf, -8); - virBufferAddLit(buf, " </source>\n"); - } else { - virBufferAddLit(buf, "/>\n"); - } + virBufferAsprintf(buf, "/>\n"); break; case VIR_DOMAIN_DISK_TYPE_BLOCK: - if (def->src && def->seclabel) { - virBufferEscapeString(buf, " <source dev='%s'>\n", - def->src); - virBufferAdjustIndent(buf, 8); - if (virSecurityLabelDefFormat(buf, def->seclabel, flags) < 0) - return -1; - virBufferAdjustIndent(buf, -8); - virBufferAddLit(buf, " </source>\n"); - } else { - virBufferEscapeString(buf, " <source dev='%s'/>\n", - def->src); - } + virBufferEscapeString(buf, " <source dev='%s'/>\n", + def->src); break; case VIR_DOMAIN_DISK_TYPE_DIR: virBufferEscapeString(buf, " <source dir='%s'/>\n", -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Revert parsing changes:
commit 302fe95ffa1bc5f1c61c0beb31a1adfbc38c668e Author: Eric Blake <eblake@redhat.com> Date: Wed Jan 4 16:01:24 2012 -0700
seclabel: fix regression in libvirtd restart
commit b43432931aef92325920953ff92beabfbe5224c8 Author: Eric Blake <eblake@redhat.com> Date: Thu Dec 22 17:47:50 2011 -0700
seclabel: allow a seclabel override on a disk src
These two commits changed the sec label parsing code so that the same code dealt with both the VM level sec label, and the per device label. Unfortunately, as we add more options to the VM level sec label, the logic required to use the same parsing code for the per device label becomes unintelligable.
s/unintelligable/unintelligible/
* src/conf/domain_conf.c: Remove support for parsing per device sec labels --- src/conf/domain_conf.c | 190 ++++++++++++----------------------------------- 1 files changed, 49 insertions(+), 141 deletions(-)
With this patch applied, we get a testsuite failure. But I guess that is to be expected - we reverted functionality and won't be restoring it until 4/7. I can live with the 'git bisect' weakness. ACK, assuming I like 4/7. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> This re-introduces parsing & formatting for per device seclabels. There is a new virDomainDeviceSeclabelPtr struct and corresponding APIs for parsing/formatting. --- src/conf/domain_conf.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 12 +++++- 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8db674b..4ae417e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -798,6 +798,17 @@ virSecurityLabelDefClear(virSecurityLabelDefPtr def) VIR_FREE(def->baselabel); } + +static void +virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def) +{ + if (!def) + return; + VIR_FREE(def->label); + VIR_FREE(def); +} + + void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) { int ii; @@ -876,6 +887,8 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) virStorageEncryptionFree(def->encryption); virDomainDeviceInfoClear(&def->info); + virSecurityDeviceLabelDefFree(def->seclabel); + for (i = 0 ; i < def->nhosts ; i++) virDomainDiskHostDefFree(&def->hosts[i]); VIR_FREE(def->hosts); @@ -2628,6 +2641,54 @@ error: return -1; } + +static int +virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr *def, + virSecurityLabelDefPtr vmDef, + xmlXPathContextPtr ctxt) +{ + char *p; + + *def = NULL; + + if (virXPathNode("./seclabel", ctxt) == NULL) + return 0; + + if (VIR_ALLOC(*def) < 0) { + virReportOOMError(); + return -1; + } + + p = virXPathStringLimit("string(./seclabel/@relabel)", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p != NULL) { + if (STREQ(p, "yes")) { + (*def)->norelabel = false; + } else if (STREQ(p, "no")) { + (*def)->norelabel = true; + } else { + virDomainReportError(VIR_ERR_XML_ERROR, + _("invalid security relabel value %s"), p); + VIR_FREE(p); + VIR_FREE(*def); + return -1; + } + VIR_FREE(p); + } else { + if (vmDef->type == VIR_DOMAIN_SECLABEL_STATIC) + (*def)->norelabel = true; + else + (*def)->norelabel = false; + } + + p = virXPathStringLimit("string(./seclabel/label[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + (*def)->label = p; + + return 0; +} + + /* Parse the XML definition for a lease */ static virDomainLeaseDefPtr @@ -2709,9 +2770,11 @@ virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr node, xmlXPathContextPtr ctxt, virBitmapPtr bootMap, + virSecurityLabelDefPtr vmSeclabel, unsigned int flags) { virDomainDiskDefPtr def; + xmlNodePtr sourceNode = NULL; xmlNodePtr cur, child; xmlNodePtr save_ctxt = ctxt->node; char *type = NULL; @@ -2766,6 +2829,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, if ((source == NULL && hosts == NULL) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { + sourceNode = cur; + switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: source = virXMLPropString(cur, "file"); @@ -3016,6 +3081,17 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } + /* If source is present, check for an optional seclabel override. */ + if (sourceNode) { + xmlNodePtr saved_node = ctxt->node; + ctxt->node = sourceNode; + if (virSecurityDeviceLabelDefParseXML(&def->seclabel, + vmSeclabel, + ctxt) < 0) + goto error; + ctxt->node = saved_node; + } + if (target == NULL) { virDomainReportError(VIR_ERR_NO_TARGET, source ? "%s" : NULL, source); @@ -6338,7 +6414,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, if (xmlStrEqual(node->name, BAD_CAST "disk")) { dev->type = VIR_DOMAIN_DEVICE_DISK; if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, - NULL, flags))) + NULL, &def->seclabel, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { dev->type = VIR_DOMAIN_DEVICE_LEASE; @@ -7440,6 +7516,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, nodes[i], ctxt, bootMap, + &def->seclabel, flags); if (!disk) goto error; @@ -9767,6 +9844,19 @@ cleanup: } +static void +virSecurityDeviceLabelDefFormat(virBufferPtr buf, + virSecurityDeviceLabelDefPtr def) +{ + virBufferAsprintf(buf, "<seclabel relabel='%s'>\n", + def->norelabel ? "no" : "yes"); + if (def->label) + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + virBufferAddLit(buf, "</seclabel>\n"); +} + + static int virDomainLeaseDefFormat(virBufferPtr buf, virDomainLeaseDefPtr def) @@ -9879,17 +9969,34 @@ virDomainDiskDefFormat(virBufferPtr buf, def->startupPolicy) { switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: - virBufferAsprintf(buf," <source"); + virBufferAddLit(buf," <source"); if (def->src) virBufferEscapeString(buf, " file='%s'", def->src); if (def->startupPolicy) virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virBufferAsprintf(buf, "/>\n"); + if (def->seclabel) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 8); + virSecurityDeviceLabelDefFormat(buf, def->seclabel); + virBufferAdjustIndent(buf, -8); + virBufferAddLit(buf, " </source>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } break; case VIR_DOMAIN_DISK_TYPE_BLOCK: - virBufferEscapeString(buf, " <source dev='%s'/>\n", + virBufferEscapeString(buf, " <source dev='%s'", def->src); + if (def->seclabel) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 8); + virSecurityDeviceLabelDefFormat(buf, def->seclabel); + virBufferAdjustIndent(buf, -8); + virBufferAddLit(buf, " </source>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } break; case VIR_DOMAIN_DISK_TYPE_DIR: virBufferEscapeString(buf, " <source dir='%s'/>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3d5d4f8..25aae62 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -180,6 +180,16 @@ struct _virSecurityLabelDef { bool norelabel; }; + +/* Security configuration for domain */ +typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef; +typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr; +struct _virSecurityDeviceLabelDef { + char *label; /* image label string */ + bool norelabel; +}; + + typedef struct _virDomainHostdevOrigStates virDomainHostdevOrigStates; typedef virDomainHostdevOrigStates *virDomainHostdevOrigStatesPtr; struct _virDomainHostdevOrigStates { @@ -358,7 +368,7 @@ struct _virDomainDiskDef { int device; int bus; char *src; - virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr seclabel; char *dst; int protocol; int nhosts; -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This re-introduces parsing & formatting for per device seclabels. There is a new virDomainDeviceSeclabelPtr struct and corresponding APIs for parsing/formatting. --- src/conf/domain_conf.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 12 +++++- 2 files changed, 122 insertions(+), 5 deletions(-)
Complex enough that I agree with your decision to separate the revert from the new implementation, even if it means a potential for a failed 'git bisect'.
+static int +virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr *def, + virSecurityLabelDefPtr vmDef, + xmlXPathContextPtr ctxt) +{ + char *p; + + *def = NULL; + + if (virXPathNode("./seclabel", ctxt) == NULL) + return 0;
Missing a check here that we aren't trying to override when the top-level seclabel forbids it; something to replace this hunk removed in 3/7: - /* Can't use overrides if top-level doesn't allow relabeling. */ - if (default_seclabel && default_seclabel->norelabel) { - virDomainReportError(VIR_ERR_XML_ERROR, "%s", - _("label overrides require relabeling to be " - "enabled at the domain level")); - goto cleanup; except that it would now be looking at 'vmDef->norelabel' instead of 'default_seclabel && default_seclabel->norelabel'.
+ + if (VIR_ALLOC(*def) < 0) { + virReportOOMError(); + return -1; + } + + p = virXPathStringLimit("string(./seclabel/@relabel)", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p != NULL) { + if (STREQ(p, "yes")) { + (*def)->norelabel = false; + } else if (STREQ(p, "no")) { + (*def)->norelabel = true; + } else { + virDomainReportError(VIR_ERR_XML_ERROR, + _("invalid security relabel value %s"), p); + VIR_FREE(p); + VIR_FREE(*def); + return -1; + } + VIR_FREE(p); + } else { + if (vmDef->type == VIR_DOMAIN_SECLABEL_STATIC) + (*def)->norelabel = true; + else + (*def)->norelabel = false;
The code in 3/7 was defaulting (*def)->norelabel to false, regardless of the top-level setting (that is, the only way to get a device label to set norelabel is with an explicit attribute, so that a device label of: <seclabel> <label>...</label> </seclabel> is then valid shorthand for: <seclabel relabel='yes'> <label>...</label> </seclabel>
+ } + + p = virXPathStringLimit("string(./seclabel/label[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + (*def)->label = p;
Do we want to be saving this label if norelabel is true (that is, if the user passes: <seclabel relabel='no'> <label>...</label> </seclabel> do we reject that as invalid, or silently ignore the useless <label>, instead of your behavior of accepting it)?
@@ -9767,6 +9844,19 @@ cleanup: }
+static void +virSecurityDeviceLabelDefFormat(virBufferPtr buf, + virSecurityDeviceLabelDefPtr def) +{ + virBufferAsprintf(buf, "<seclabel relabel='%s'>\n", + def->norelabel ? "no" : "yes"); + if (def->label) + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + virBufferAddLit(buf, "</seclabel>\n");
With norelabel, this would generate the odd-looking: <seclabel relabel='no'> </seclabel> How about instead doing: virBufferAsprintf(buf, "<seclabel relabel='%s'", def->norelabel ? "no" : "yes"); if (def->label) { virBufferAddLit(buf, ">\n"); virBufferEscapeString(buf, " <label>%s</label>\n", def->label); virBufferAddLit(buf, "</seclabel>\n"); } else { virBufferAddLit(buf, "/>\n"); } Close, but I'd feel a bit more comfortable seeing a v2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/23/2012 03:38 PM, Eric Blake wrote:
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This re-introduces parsing & formatting for per device seclabels. There is a new virDomainDeviceSeclabelPtr struct and corresponding APIs for parsing/formatting. --- src/conf/domain_conf.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 12 +++++- 2 files changed, 122 insertions(+), 5 deletions(-)
Complex enough that I agree with your decision to separate the revert from the new implementation, even if it means a potential for a failed 'git bisect'.
Alas, this still fails testing: 78) QEMU XML-2-XML seclabel-dynamic-override ... Offset 574 Expect [/] Actual [> </seclabel] ... FAILED
Do we want to be saving this label if norelabel is true (that is, if the user passes:
<seclabel relabel='no'> <label>...</label> </seclabel>
do we reject that as invalid, or silently ignore the useless <label>, instead of your behavior of accepting it)?
@@ -9767,6 +9844,19 @@ cleanup: }
+static void +virSecurityDeviceLabelDefFormat(virBufferPtr buf, + virSecurityDeviceLabelDefPtr def) +{ + virBufferAsprintf(buf, "<seclabel relabel='%s'>\n", + def->norelabel ? "no" : "yes"); + if (def->label) + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + virBufferAddLit(buf, "</seclabel>\n");
With norelabel, this would generate the odd-looking:
<seclabel relabel='no'> </seclabel>
How about instead doing:
virBufferAsprintf(buf, "<seclabel relabel='%s'", def->norelabel ? "no" : "yes"); if (def->label) { virBufferAddLit(buf, ">\n"); virBufferEscapeString(buf, " <label>%s</label>\n", def->label); virBufferAddLit(buf, "</seclabel>\n"); } else { virBufferAddLit(buf, "/>\n"); }
In fact, that's necessary to get the test to pass again. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Curently security labels can be of type 'dynamic' or 'static'. If no security label is given, then 'dynamic' is assumed. The current code takes advantage of this default, and avoids even saving <seclabel> elements with type='dynamic' to disk. This means if you temporarily change security driver, the guests can all still start. With the introduce of sVirt to LXC though, there needs to be a new default of 'none' to allow unconfined LXC containers. This patch introduces two new security label types - default: the host configuration decides whether to run the guest with type 'none' or 'dynamic' at guest start - none: the guest will run unconfined by security policy The 'none' label type will obviously be undesirable for some deployments, so a new qemu.conf option allows a host admin to mandate confined guests. It is also possible to turn off default confinement security_default_confined = 1|0 (default == 1) security_require_confined = 1|0 (default == 0) * src/conf/domain_conf.c, src/conf/domain_conf.h: Add new seclabel types * src/security/security_manager.c, src/security/security_manager.h: Set default sec label types * src/security/security_selinux.c: Handle 'none' seclabel type * src/qemu/qemu.conf, src/qemu/qemu_conf.c, src/qemu/qemu_conf.h, src/qemu/libvirtd_qemu.aug: New security config options * src/qemu/qemu_driver.c: Tell security driver about default config --- src/conf/domain_conf.c | 81 +++++++++++++++++++++++---------------- src/conf/domain_conf.h | 2 + src/qemu/libvirtd_qemu.aug | 2 + src/qemu/qemu.conf | 8 ++++ src/qemu/qemu_conf.c | 11 +++++ src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 7 +++- src/security/security_manager.c | 51 ++++++++++++++++++++++-- src/security/security_manager.h | 8 +++- src/security/security_selinux.c | 28 ++++++++++---- 10 files changed, 152 insertions(+), 48 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4ae417e..a2b7430 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -551,6 +551,8 @@ VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST, "unknown") VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, + "default", + "none", "dynamic", "static") @@ -2547,13 +2549,15 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, "%s", _("missing security type")); goto error; } + def->type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); - if (def->type < 0) { + if (def->type <= 0) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("invalid security type")); goto error; } + p = virXPathStringLimit("string(./seclabel/@relabel)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p != NULL) { @@ -2574,13 +2578,23 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, "%s", _("dynamic label type must use resource relabeling")); goto error; } + if (def->type == VIR_DOMAIN_SECLABEL_NONE && + !def->norelabel) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("dynamic label type must use not resource relabeling")); + goto error; + } } else { - if (def->type == VIR_DOMAIN_SECLABEL_STATIC) + if (def->type == VIR_DOMAIN_SECLABEL_STATIC || + def->type == VIR_DOMAIN_SECLABEL_NONE) def->norelabel = true; else def->norelabel = false; } + if (def->type == VIR_DOMAIN_SECLABEL_NONE) + return 0; + /* Only parse label, if using static labels, or * if the 'live' VM XML is requested */ @@ -9810,37 +9824,41 @@ virDomainLifecycleDefFormat(virBufferPtr buf, } -static int -virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def, - unsigned int flags) +static void +virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def) { const char *sectype = virDomainSeclabelTypeToString(def->type); - int ret = -1; if (!sectype) - goto cleanup; + return; - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && - !def->baselabel && - (flags & VIR_DOMAIN_XML_INACTIVE)) { - /* This is the default for inactive xml, so nothing to output. */ - } else { - virBufferAsprintf(buf, "<seclabel type='%s' model='%s' relabel='%s'>\n", - sectype, def->model, - def->norelabel ? "no" : "yes"); - virBufferEscapeString(buf, " <label>%s</label>\n", - def->label); - if (!def->norelabel) - virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", - def->imagelabel); - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) - virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", - def->baselabel); - virBufferAddLit(buf, "</seclabel>\n"); + if (def->type == VIR_DOMAIN_SECLABEL_DEFAULT) + return; + + virBufferAsprintf(buf, "<seclabel type='%s'", + sectype); + if (def->model) + virBufferEscapeString(buf, " model='%s' ", def->model); + + virBufferAsprintf(buf, " relabel='%s'", + def->norelabel ? "no" : "yes"); + + if (def->type == VIR_DOMAIN_SECLABEL_NONE) { + virBufferAddLit(buf, "/>\n"); + return; } - ret = 0; -cleanup: - return ret; + + virBufferAddLit(buf, ">\n"); + + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + if (!def->norelabel) + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", + def->imagelabel); + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", + def->baselabel); + virBufferAddLit(buf, "</seclabel>\n"); } @@ -11811,12 +11829,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, " </devices>\n"); - if (def->seclabel.model) { - virBufferAdjustIndent(buf, 2); - if (virSecurityLabelDefFormat(buf, &def->seclabel, flags) < 0) - goto cleanup; - virBufferAdjustIndent(buf, -2); - } + virBufferAdjustIndent(buf, 2); + virSecurityLabelDefFormat(buf, &def->seclabel); + virBufferAdjustIndent(buf, -2); if (def->namespaceData && def->ns.format) { if ((def->ns.format)(buf, def->namespaceData) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25aae62..d3a1175 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -162,6 +162,8 @@ struct _virDomainDeviceInfo { }; enum virDomainSeclabelType { + VIR_DOMAIN_SECLABEL_DEFAULT, + VIR_DOMAIN_SECLABEL_NONE, VIR_DOMAIN_SECLABEL_DYNAMIC, VIR_DOMAIN_SECLABEL_STATIC, diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 078e9c4..f6cec1f 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -33,6 +33,8 @@ module Libvirtd_qemu = | bool_entry "vnc_sasl" | str_entry "vnc_sasl_dir" | str_entry "security_driver" + | bool_entry "security_default_confined" + | bool_entry "security_require_confined" | str_entry "user" | str_entry "group" | bool_entry "dynamic_ownership" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 4ec5e6c..d32a4c3 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -138,6 +138,14 @@ # # security_driver = "selinux" +# If set to non-zero, then the default security labelling +# will make guests confined. If set to zero, then guests +# will be unconfined by default. Defaults to 1 +# security_default_confined = 1 + +# If set to non-zero, then attempts to create unconfined +# guests will be blocked. Defaults to zero. +# security_require_confined = 1 # The user ID for QEMU processes run by the system instance. #user = "root" diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bc0a646..e95c7a5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -75,6 +75,8 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, int i; /* Setup critical defaults */ + driver->securityDefaultConfined = true; + driver->securityRequireConfined = false; driver->dynamicOwnership = 1; driver->clearEmulatorCapabilities = 1; @@ -195,6 +197,15 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } } + p = virConfGetValue (conf, "security_default_confined"); + CHECK_TYPE ("security_default_confined", VIR_CONF_LONG); + if (p) driver->securityDefaultConfined = p->l; + + p = virConfGetValue (conf, "security_require_confined"); + CHECK_TYPE ("security_require_confined", VIR_CONF_LONG); + if (p) driver->securityRequireConfined = p->l; + + p = virConfGetValue (conf, "vnc_sasl"); CHECK_TYPE ("vnc_sasl", VIR_CONF_LONG); if (p) driver->vncSASL = p->l; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d8d7915..fb98ef2 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -115,6 +115,8 @@ struct qemud_driver { virDomainEventStatePtr domainEventState; char *securityDriverName; + bool securityDefaultConfined; + bool securityRequireConfined; virSecurityManagerPtr securityManager; char *saveImageFormat; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 712f1fc..b697f43 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -208,7 +208,10 @@ static int qemuSecurityInit(struct qemud_driver *driver) { virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, - driver->allowDiskFormatProbing); + driver->allowDiskFormatProbing, + driver->securityDefaultConfined, + driver->securityRequireConfined); + if (!mgr) goto error; @@ -216,6 +219,8 @@ qemuSecurityInit(struct qemud_driver *driver) virSecurityManagerPtr dac = virSecurityManagerNewDAC(driver->user, driver->group, driver->allowDiskFormatProbing, + driver->securityDefaultConfined, + driver->securityRequireConfined, driver->dynamicOwnership); if (!dac) goto error; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 2e4956a..d0bafae 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -36,10 +36,14 @@ struct _virSecurityManager { virSecurityDriverPtr drv; bool allowDiskFormatProbing; + bool defaultConfined; + bool requireConfined; }; static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr drv, - bool allowDiskFormatProbing) + bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined) { virSecurityManagerPtr mgr; @@ -50,6 +54,8 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr mgr->drv = drv; mgr->allowDiskFormatProbing = allowDiskFormatProbing; + mgr->defaultConfined = defaultConfined; + mgr->requireConfined = requireConfined; if (drv->open(mgr) < 0) { virSecurityManagerFree(mgr); @@ -64,7 +70,9 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, - virSecurityManagerGetAllowDiskFormatProbing(primary)); + virSecurityManagerGetAllowDiskFormatProbing(primary), + virSecurityManagerGetDefaultConfined(primary), + virSecurityManagerGetRequireConfined(primary)); if (!mgr) return NULL; @@ -78,11 +86,15 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, gid_t group, bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined, bool dynamicOwnership) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, - allowDiskFormatProbing); + allowDiskFormatProbing, + defaultConfined, + requireConfined); if (!mgr) return NULL; @@ -95,13 +107,18 @@ virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, } virSecurityManagerPtr virSecurityManagerNew(const char *name, - bool allowDiskFormatProbing) + bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined) { virSecurityDriverPtr drv = virSecurityDriverLookup(name); if (!drv) return NULL; - return virSecurityManagerNewDriver(drv, allowDiskFormatProbing); + return virSecurityManagerNewDriver(drv, + allowDiskFormatProbing, + defaultConfined, + requireConfined); } @@ -149,6 +166,16 @@ bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr) return mgr->allowDiskFormatProbing; } +bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr) +{ + return mgr->defaultConfined; +} + +bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) +{ + return mgr->requireConfined; +} + int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virDomainDiskDefPtr disk) @@ -248,6 +275,20 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) { + if (vm->seclabel.type == VIR_DOMAIN_SECLABEL_DEFAULT) { + if (mgr->defaultConfined) + vm->seclabel.type = VIR_DOMAIN_SECLABEL_DYNAMIC; + else + vm->seclabel.type = VIR_DOMAIN_SECLABEL_NONE; + } + + if ((vm->seclabel.type == VIR_DOMAIN_SECLABEL_NONE) && + mgr->requireConfined) { + virSecurityReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unconfined guests are not allowed on this host")); + return -1; + } + if (mgr->drv->domainGenSecurityLabel) return mgr->drv->domainGenSecurityLabel(mgr, vm); diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 6731d59..32c8c3b 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -32,7 +32,9 @@ typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; virSecurityManagerPtr virSecurityManagerNew(const char *name, - bool allowDiskFormatProbing); + bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined); virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, virSecurityManagerPtr secondary); @@ -40,6 +42,8 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, gid_t group, bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined, bool dynamicOwnership); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); @@ -49,6 +53,8 @@ void virSecurityManagerFree(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr); +bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr); +bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr); int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c2dceca..ade36f9 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -170,6 +170,7 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, int c1 = 0; int c2 = 0; context_t ctx = NULL; + const char *range; if ((def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) && !def->seclabel.baselabel && @@ -200,7 +201,8 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return rc; } - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) { + switch (def->seclabel.type) { + case VIR_DOMAIN_SECLABEL_STATIC: if (!(ctx = context_new(def->seclabel.label)) ) { virReportSystemError(errno, _("unable to allocate socket security context '%s'"), @@ -208,13 +210,15 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return rc; } - const char *range = context_range_get(ctx); + range = context_range_get(ctx); if (!range || !(mcs = strdup(range))) { virReportOOMError(); goto cleanup; } - } else { + break; + + case VIR_DOMAIN_SECLABEL_DYNAMIC: do { c1 = virRandom(1024); c2 = virRandom(1024); @@ -246,12 +250,20 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, _("cannot generate selinux context for %s"), mcs); goto cleanup; } + break; + + case VIR_DOMAIN_SECLABEL_NONE: + /* no op */ + break; } - def->seclabel.imagelabel = SELinuxGenNewContext(default_image_context, mcs); - if (!def->seclabel.imagelabel) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot generate selinux context for %s"), mcs); - goto cleanup; + + if (!def->seclabel.norelabel) { + def->seclabel.imagelabel = SELinuxGenNewContext(default_image_context, mcs); + if (!def->seclabel.imagelabel) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto cleanup; + } } if (!def->seclabel.model && -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Curently security labels can be of type 'dynamic' or 'static'. If no security label is given, then 'dynamic' is assumed. The current code takes advantage of this default, and avoids even saving <seclabel> elements with type='dynamic' to disk. This means if you temporarily change security driver, the guests can all still start.
With the introduce of sVirt to LXC though, there needs to be
s/introduce/introduction/
a new default of 'none' to allow unconfined LXC containers.
This patch introduces two new security label types
- default: the host configuration decides whether to run the guest with type 'none' or 'dynamic' at guest start - none: the guest will run unconfined by security policy
The 'none' label type will obviously be undesirable for some deployments, so a new qemu.conf option allows a host admin to mandate confined guests. It is also possible to turn off default confinement
security_default_confined = 1|0 (default == 1) security_require_confined = 1|0 (default == 0)
* src/conf/domain_conf.c, src/conf/domain_conf.h: Add new seclabel types * src/security/security_manager.c, src/security/security_manager.h: Set default sec label types * src/security/security_selinux.c: Handle 'none' seclabel type * src/qemu/qemu.conf, src/qemu/qemu_conf.c, src/qemu/qemu_conf.h, src/qemu/libvirtd_qemu.aug: New security config options * src/qemu/qemu_driver.c: Tell security driver about default config
Fails 'make syntax-check': po_check --- ./po/POTFILES.in +++ ./po/POTFILES.in @@ -89,6 +89,7 @@ src/security/security_apparmor.c src/security/security_dac.c src/security/security_driver.c +src/security/security_manager.c src/security/security_selinux.c src/security/virt-aa-helper.c src/storage/parthelper.c maint.mk: you have changed the set of files with translatable diagnostics; apply the above patch Fails 'make check': seclabeltest.c: In function 'main': seclabeltest.c:16:5: error: too few arguments to function 'virSecurityManagerNew' ../src/security/security_manager.h:34:23: note: declared here I had to squash this in to get tests to pass: diff --git i/po/POTFILES.in w/po/POTFILES.in index ca1db70..9b699bd 100644 --- i/po/POTFILES.in +++ w/po/POTFILES.in @@ -89,6 +89,7 @@ src/secret/secret_driver.c src/security/security_apparmor.c src/security/security_dac.c src/security/security_driver.c +src/security/security_manager.c src/security/security_selinux.c src/security/virt-aa-helper.c src/storage/parthelper.c diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 07080ce..91b95e2 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -9852,7 +9852,7 @@ virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def) virBufferAsprintf(buf, "<seclabel type='%s'", sectype); if (def->model) - virBufferEscapeString(buf, " model='%s' ", def->model); + virBufferEscapeString(buf, " model='%s'", def->model); virBufferAsprintf(buf, " relabel='%s'", def->norelabel ? "no" : "yes"); diff --git i/tests/seclabeltest.c w/tests/seclabeltest.c index 5d87789..fca76b9 100644 --- i/tests/seclabeltest.c +++ w/tests/seclabeltest.c @@ -13,7 +13,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) virSecurityManagerPtr mgr; const char *doi, *model; - mgr = virSecurityManagerNew(NULL, false); + mgr = virSecurityManagerNew(NULL, false, true, false); if (mgr == NULL) { fprintf (stderr, "Failed to start security driver"); exit (-1);
--- src/conf/domain_conf.c | 81 +++++++++++++++++++++++---------------- src/conf/domain_conf.h | 2 + src/qemu/libvirtd_qemu.aug | 2 + src/qemu/qemu.conf | 8 ++++ src/qemu/qemu_conf.c | 11 +++++ src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 7 +++- src/security/security_manager.c | 51 ++++++++++++++++++++++-- src/security/security_manager.h | 8 +++- src/security/security_selinux.c | 28 ++++++++++---- 10 files changed, 152 insertions(+), 48 deletions(-)
You also need to list the new XML in docs/schemas/domaincommon.rng, document it in docs/formatdomain.html.in, and add tests for the new XML being parsed correctly. I don't feel comfortable acking with that much missing, so I'll need a v2; but I can proceed to give code review.
@@ -2547,13 +2549,15 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, "%s", _("missing security type")); goto error; } + def->type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); - if (def->type < 0) { + if (def->type <= 0) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("invalid security type"));
This explicitly rejects type='default', was that intentional (where you _want_ the user to omit the attribute to get the default)? It matches what we do elsewhere, but that does mean that you have to be careful in documenting it this way (type='none|dynamic|static' or omitted type to form the four possibilities).
@@ -2574,13 +2578,23 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, "%s", _("dynamic label type must use resource relabeling")); goto error; } + if (def->type == VIR_DOMAIN_SECLABEL_NONE && + !def->norelabel) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("dynamic label type must use not resource relabeling"));
Bad grammar in the error message, but even with that fixed, it still doesn't make sense compared to the if conditional. I think you meant something more like: "resource relabeling not compatible with 'none' label type"
+ goto error; + } } else { - if (def->type == VIR_DOMAIN_SECLABEL_STATIC) + if (def->type == VIR_DOMAIN_SECLABEL_STATIC || + def->type == VIR_DOMAIN_SECLABEL_NONE) def->norelabel = true; else def->norelabel = false; }
+ if (def->type == VIR_DOMAIN_SECLABEL_NONE) + return 0;
So we explicitly ignore any <label> sub-elements with type of none; I can live with that.
@@ -9810,37 +9824,41 @@ virDomainLifecycleDefFormat(virBufferPtr buf, }
-static int -virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def, - unsigned int flags) +static void +virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def)
I like that you are dropping dependency on the flags argument. I will note that this means that 'virsh save-image-edit' might end up changing a save image file created pre-patch into a new form post-patch even when there are no changes made by the user (but that's not the first time we've done that). But as long as we aren't changing the output from the input _from the same version of libvirt_, I'm okay if upgrading the libvirt version causes a rewrite when editing an older save image.
{ const char *sectype = virDomainSeclabelTypeToString(def->type); - int ret = -1;
if (!sectype) - goto cleanup; + return;
- if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && - !def->baselabel && - (flags & VIR_DOMAIN_XML_INACTIVE)) { - /* This is the default for inactive xml, so nothing to output. */
The old way omitted output if you use the defaults, but still generated output if you do this as short-hand input: <seclabel> <baselabel>...</baselabel> </seclabel> for the intended: <seclabel type='dynamic' model='selinux' relabel='yes'> <baselabel>...</baselabel> </seclabel>
- } else { - virBufferAsprintf(buf, "<seclabel type='%s' model='%s' relabel='%s'>\n", - sectype, def->model, - def->norelabel ? "no" : "yes"); - virBufferEscapeString(buf, " <label>%s</label>\n", - def->label); - if (!def->norelabel) - virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", - def->imagelabel); - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) - virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", - def->baselabel); - virBufferAddLit(buf, "</seclabel>\n"); + if (def->type == VIR_DOMAIN_SECLABEL_DEFAULT) + return;
...whereas your new code fails to look for the presence of <baselabel>, and would silently lose the user's input. I argue that the bug is in your parse routine, and not your output routine. That is, if the user omitted type, but then provides a <baselabel>, you need to explicitly set type to dynamic rather than leaving it at default, so that the output routine can blindly omit output when type is default precisely because <baselabel> no longer maps to default.
+ + virBufferAsprintf(buf, "<seclabel type='%s'", + sectype); + if (def->model) + virBufferEscapeString(buf, " model='%s' ", def->model);
Lose the trailing space. Also, you can exploit the fact that virBufferEscapeString does a NULL check for you, and lose the 'if (def->model)' with no change in output.
+ + virBufferAsprintf(buf, " relabel='%s'", + def->norelabel ? "no" : "yes"); + + if (def->type == VIR_DOMAIN_SECLABEL_NONE) { + virBufferAddLit(buf, "/>\n"); + return; } - ret = 0; -cleanup: - return ret; + + virBufferAddLit(buf, ">\n"); + + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + if (!def->norelabel) + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", + def->imagelabel); + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", + def->baselabel); + virBufferAddLit(buf, "</seclabel>\n");
This can result in: <seclabel type='dynamic' model='selinux' relabel='yes'> </seclabel> for an inactive domain. Do we want to go to any effort to shorten this to: <seclabel type='dynamic' model='selinux' relabel='yes'/>
+++ b/src/qemu/qemu.conf @@ -138,6 +138,14 @@ # # security_driver = "selinux"
+# If set to non-zero, then the default security labelling
Do we want the US spelling of labeling here?
+# will make guests confined. If set to zero, then guests +# will be unconfined by default. Defaults to 1 +# security_default_confined = 1 + +# If set to non-zero, then attempts to create unconfined +# guests will be blocked. Defaults to zero. +# security_require_confined = 1
Consistency argues that we should use the digit rather than the name of the number, and end with a full stop, as in: Defaults to 1. Defaults to 0. (that is, both variable descriptions need a tweak, but for different reasons)
@@ -195,6 +197,15 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } }
+ p = virConfGetValue (conf, "security_default_confined"); + CHECK_TYPE ("security_default_confined", VIR_CONF_LONG); + if (p) driver->securityDefaultConfined = p->l; + + p = virConfGetValue (conf, "security_require_confined"); + CHECK_TYPE ("security_require_confined", VIR_CONF_LONG); + if (p) driver->securityRequireConfined = p->l;
Conversion of long to boolean. I guess this is copy-and-paste from other places doing this, and also that since we require C99 it will be safe. But in C89 projects, the gnulib replacement for <stdbool.h> means that this is non-portable, even though it should work just fine with C99 bool. (That is, the gnulib replacement bool can compile (bool)256L to false instead of the desired true, depending on your platform and compiler). No need to fix it in your patch (if we do anything, it should be a global style scrub of all offenders in this file in one go).
@@ -246,12 +250,20 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, _("cannot generate selinux context for %s"), mcs); goto cleanup; } + break; + + case VIR_DOMAIN_SECLABEL_NONE: + /* no op */ + break;
Might be safer to put a default case that errors out, so that we catch any future additions. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> For the sake of backwards compat, LXC guests are *not* confined by default. This is because it is not practical to dynamically relabel containers using large filesystem trees. Applications can create confined containers though, by giving suitable XML configs * src/Makefile.am: Link libvirt_lxc to security drivers * src/lxc/libvirtd_lxc.aug, src/lxc/lxc_conf.h, src/lxc/lxc_conf.c, src/lxc/lxc.conf, src/lxc/test_libvirtd_lxc.aug: Config file handling for security driver * src/lxc/lxc_driver.c: Wire up security driver functions * src/lxc/lxc_controller.c: Add a '--security' flag to specify which security driver to activate * src/lxc/lxc_container.c, src/lxc/lxc_container.h: Set the process label just before exec'ing init. --- src/Makefile.am | 13 +++ src/lxc/libvirtd_lxc.aug | 15 +++- src/lxc/lxc.conf | 18 ++++ src/lxc/lxc_conf.c | 62 ++++++++++++-- src/lxc/lxc_conf.h | 8 ++- src/lxc/lxc_container.c | 9 ++- src/lxc/lxc_container.h | 2 + src/lxc/lxc_controller.c | 28 +++++- src/lxc/lxc_driver.c | 198 ++++++++++++++++++++++++++++++++++++++++- src/lxc/test_libvirtd_lxc.aug | 2 + 10 files changed, 340 insertions(+), 15 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 0a1221a..a4688a1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1501,7 +1501,14 @@ libvirt_lxc_SOURCES = \ $(DOMAIN_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ + $(SECURITY_DRIVER_SOURCES) \ $(NWFILTER_PARAM_CONF_SOURCES) +if WITH_SECDRIVER_SELINUX +libvirt_lxc_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) +endif +if WITH_SECDRIVER_APPARMOR +libvirt_lxc_SOURCES += $(SECURITY_DRIVER_APPARMOR_SOURCES) +endif libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(AM_LDFLAGS) libvirt_lxc_LDADD = $(CAPNG_LIBS) $(YAJL_LIBS) \ $(LIBXML_LIBS) $(NUMACTL_LIBS) $(THREAD_LIBS) \ @@ -1511,6 +1518,9 @@ libvirt_lxc_LDADD = $(CAPNG_LIBS) $(YAJL_LIBS) \ if WITH_DTRACE libvirt_lxc_LDADD += probes.o endif +if WITH_SECDRIVER_SELINUX +libvirt_lxc_LDADD += $(SELINUX_LIBS) +endif libvirt_lxc_CFLAGS = \ $(LIBPARTED_CFLAGS) \ $(NUMACTL_CFLAGS) \ @@ -1523,6 +1533,9 @@ if HAVE_LIBBLKID libvirt_lxc_CFLAGS += $(BLKID_CFLAGS) libvirt_lxc_LDADD += $(BLKID_LIBS) endif +if WITH_SECDRIVER_SELINUX +libvirt_lxc_CFLAGS += $(SELINUX_CFLAGS) +endif endif endif EXTRA_DIST += $(LXC_CONTROLLER_SOURCES) diff --git a/src/lxc/libvirtd_lxc.aug b/src/lxc/libvirtd_lxc.aug index 10f25e4..be6402c 100644 --- a/src/lxc/libvirtd_lxc.aug +++ b/src/lxc/libvirtd_lxc.aug @@ -7,13 +7,26 @@ module Libvirtd_lxc = let value_sep = del /[ \t]*=[ \t]*/ " = " let indent = del /[ \t]*/ "" + let array_sep = del /,[ \t\n]*/ ", " + let array_start = del /\[[ \t\n]*/ "[ " + let array_end = del /\]/ "]" + + let str_val = del /\"/ "\"" . store /[^\"]*/ . del /\"/ "\"" let bool_val = store /0|1/ + let int_val = store /[0-9]+/ + let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ "" + let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end + let str_entry (kw:string) = [ key kw . value_sep . str_val ] let bool_entry (kw:string) = [ key kw . value_sep . bool_val ] - + let int_entry (kw:string) = [ key kw . value_sep . int_val ] + let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ] (* Config entry grouped by function - same order as example config *) let log_entry = bool_entry "log_with_libvirtd" + | str_entry "security_driver" + | bool_entry "security_default_confined" + | bool_entry "security_require_confined" (* Each enty in the config is one of the following three ... *) let entry = log_entry diff --git a/src/lxc/lxc.conf b/src/lxc/lxc.conf index 7a5066f..ca3f4c6 100644 --- a/src/lxc/lxc.conf +++ b/src/lxc/lxc.conf @@ -11,3 +11,21 @@ # This is disabled by default, uncomment below to enable it. # # log_with_libvirtd = 1 + + +# The default security driver is SELinux. If SELinux is disabled +# on the host, then the security driver will automatically disable +# itself. If you wish to disable QEMU SELinux security driver while +# leaving SELinux enabled for the host in general, then set this +# to 'none' instead. +# +# security_driver = "selinux" + +# If set to non-zero, then the default security labelling +# will make guests confined. If set to zero, then guests +# will be unconfined by default. Defaults to zero +# security_default_confined = 1 + +# If set to non-zero, then attempts to create unconfined +# guests will be blocked. Defaults to zero. +# security_require_confined = 1 \ No newline at end of file diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index e842736..72547c4 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -49,7 +49,7 @@ static int lxcDefaultConsoleType(const char *ostype ATTRIBUTE_UNUSED) /* Functions */ -virCapsPtr lxcCapsInit(void) +virCapsPtr lxcCapsInit(lxc_driver_t *driver) { struct utsname utsname; virCapsPtr caps; @@ -127,8 +127,30 @@ virCapsPtr lxcCapsInit(void) /* LXC Requires an emulator in the XML */ virCapabilitiesSetEmulatorRequired(caps); + if (driver) { + /* Security driver data */ + const char *doi, *model; + + doi = virSecurityManagerGetDOI(driver->securityManager); + model = virSecurityManagerGetModel(driver->securityManager); + if (STRNEQ(model, "none")) { + if (!(caps->host.secModel.model = strdup(model))) + goto no_memory; + if (!(caps->host.secModel.doi = strdup(doi))) + goto no_memory; + } + + VIR_DEBUG("Initialized caps for security driver \"%s\" with " + "DOI \"%s\"", model, doi); + } else { + VIR_INFO("No driver, not initializing security driver"); + } + return caps; +no_memory: + virReportOOMError(); + error: virCapabilitiesFree(caps); return NULL; @@ -140,6 +162,9 @@ int lxcLoadDriverConfig(lxc_driver_t *driver) virConfPtr conf; virConfValuePtr p; + driver->securityDefaultConfined = false; + driver->securityRequireConfined = false; + /* Set the container configuration directory */ if ((driver->configDir = strdup(LXC_CONFIG_DIR)) == NULL) goto no_memory; @@ -161,14 +186,39 @@ int lxcLoadDriverConfig(lxc_driver_t *driver) if (!conf) goto done; +#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ + lxcError(VIR_ERR_INTERNAL_ERROR, \ + "%s: %s: expected type " #typ, \ + filename, (name)); \ + virConfFree(conf); \ + return -1; \ + } + p = virConfGetValue(conf, "log_with_libvirtd"); - if (p) { - if (p->type != VIR_CONF_LONG) - VIR_WARN("lxcLoadDriverConfig: invalid setting: log_with_libvirtd"); - else - driver->log_libvirtd = p->l; + CHECK_TYPE ("log_with_libvirtd", VIR_CONF_LONG); + if (p) driver->log_libvirtd = p->l; + + p = virConfGetValue (conf, "security_driver"); + CHECK_TYPE ("security_driver", VIR_CONF_STRING); + if (p && p->str) { + if (!(driver->securityDriverName = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } } + p = virConfGetValue (conf, "security_default_confined"); + CHECK_TYPE ("security_default_confined", VIR_CONF_LONG); + if (p) driver->securityDefaultConfined = p->l; + + p = virConfGetValue (conf, "security_require_confined"); + CHECK_TYPE ("security_require_confined", VIR_CONF_LONG); + if (p) driver->securityRequireConfined = p->l; + + +#undef CHECK_TYPE + virConfFree(conf); done: diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index b124330..ebdc173 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -33,6 +33,7 @@ # include "capabilities.h" # include "threads.h" # include "cgroup.h" +# include "security/security_manager.h" # include "configmake.h" # define LXC_CONFIG_DIR SYSCONFDIR "/libvirt/lxc" @@ -57,6 +58,11 @@ struct __lxc_driver { virDomainEventStatePtr domainEventState; + char *securityDriverName; + bool securityDefaultConfined; + bool securityRequireConfined; + virSecurityManagerPtr securityManager; + /* Mapping of 'char *uuidstr' -> virConnectPtr * of guests which will be automatically killed * when the virConnectPtr is closed*/ @@ -64,7 +70,7 @@ struct __lxc_driver { }; int lxcLoadDriverConfig(lxc_driver_t *driver); -virCapsPtr lxcCapsInit(void); +virCapsPtr lxcCapsInit(lxc_driver_t *driver); # define lxcError(code, ...) \ virReportErrorHelper(VIR_FROM_LXC, code, __FILE__, \ diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 3fb7d06..4df0b55 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -91,6 +91,7 @@ typedef char lxc_message_t; typedef struct __lxc_child_argv lxc_child_argv_t; struct __lxc_child_argv { virDomainDefPtr config; + virSecurityManagerPtr securityDriver; unsigned int nveths; char **veths; int monitor; @@ -1273,6 +1274,10 @@ static int lxcContainerChild( void *data ) goto cleanup; } + VIR_DEBUG("Setting up security labeling"); + if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(ttyPath); @@ -1335,6 +1340,7 @@ const char *lxcContainerGetAlt32bitArch(const char *arch) * Returns PID of container on success or -1 in case of error */ int lxcContainerStart(virDomainDefPtr def, + virSecurityManagerPtr securityDriver, unsigned int nveths, char **veths, int control, @@ -1346,7 +1352,8 @@ int lxcContainerStart(virDomainDefPtr def, int cflags; int stacksize = getpagesize() * 4; char *stack, *stacktop; - lxc_child_argv_t args = { def, nveths, veths, control, + lxc_child_argv_t args = { def, securityDriver, + nveths, veths, control, ttyPaths, nttyPaths, handshakefd}; /* allocate a stack for the container */ diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h index ffeda5e..77fb9b2 100644 --- a/src/lxc/lxc_container.h +++ b/src/lxc/lxc_container.h @@ -25,6 +25,7 @@ # define LXC_CONTAINER_H # include "lxc_conf.h" +# include "security/security_manager.h" enum { LXC_CONTAINER_FEATURE_NET = (1 << 0), @@ -49,6 +50,7 @@ int lxcContainerSendContinue(int control); int lxcContainerWaitForContinue(int control); int lxcContainerStart(virDomainDefPtr def, + virSecurityManagerPtr securityDriver, unsigned int nveths, char **veths, int control, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index bb936ee..6bc54b7 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1267,6 +1267,7 @@ cleanup: static int lxcControllerRun(virDomainDefPtr def, + virSecurityManagerPtr securityDriver, unsigned int nveths, char **veths, int monitor, @@ -1421,6 +1422,7 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; if ((container = lxcContainerStart(def, + securityDriver, nveths, veths, control[1], @@ -1529,11 +1531,13 @@ int main(int argc, char *argv[]) { "veth", 1, NULL, 'v' }, { "console", 1, NULL, 'c' }, { "handshakefd", 1, NULL, 's' }, + { "security", 1, NULL, 'S' }, { "help", 0, NULL, 'h' }, { 0, 0, 0, 0 }, }; int *ttyFDs = NULL; size_t nttyFDs = 0; + virSecurityManagerPtr securityDriver = NULL; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || @@ -1545,7 +1549,7 @@ int main(int argc, char *argv[]) while (1) { int c; - c = getopt_long(argc, argv, "dn:v:m:c:s:h", + c = getopt_long(argc, argv, "dn:v:m:c:s:h:S:", options, NULL); if (c == -1) @@ -1593,6 +1597,14 @@ int main(int argc, char *argv[]) } break; + case 'S': + if (!(securityDriver = virSecurityManagerNew(optarg, false, false, false))) { + fprintf(stderr, "Cannot create security manager '%s'", + optarg); + goto cleanup; + } + break; + case 'h': case '?': fprintf(stderr, "\n"); @@ -1605,12 +1617,20 @@ int main(int argc, char *argv[]) fprintf(stderr, " -c FD, --console FD\n"); fprintf(stderr, " -v VETH, --veth VETH\n"); fprintf(stderr, " -s FD, --handshakefd FD\n"); + fprintf(stderr, " -S NAME, --security NAME\n"); fprintf(stderr, " -h, --help\n"); fprintf(stderr, "\n"); goto cleanup; } } + if (securityDriver == NULL) { + if (!(securityDriver = virSecurityManagerNew("none", false, false, false))) { + fprintf(stderr, "%s: cannot initialize nop security manager", argv[0]); + goto cleanup; + } + } + if (name == NULL) { fprintf(stderr, "%s: missing --name argument for configuration\n", argv[0]); @@ -1630,7 +1650,7 @@ int main(int argc, char *argv[]) virEventRegisterDefaultImpl(); - if ((caps = lxcCapsInit()) == NULL) + if ((caps = lxcCapsInit(NULL)) == NULL) goto cleanup; if ((configFile = virDomainConfigFile(LXC_STATE_DIR, @@ -1696,10 +1716,10 @@ int main(int argc, char *argv[]) goto cleanup; } - rc = lxcControllerRun(def, nveths, veths, monitor, client, + rc = lxcControllerRun(def, securityDriver, + nveths, veths, monitor, client, ttyFDs, nttyFDs, handshakefd); - cleanup: if (def) virPidFileDelete(LXC_STATE_DIR, def->name); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3baff19..0cd1ffd 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -440,6 +440,9 @@ static virDomainPtr lxcDomainDefine(virConnectPtr conn, const char *xml) VIR_DOMAIN_XML_INACTIVE))) goto cleanup; + if (virSecurityManagerVerify(driver->securityManager, def) < 0) + goto cleanup; + if ((dupVM = virDomainObjIsDuplicate(&driver->domains, def, 0)) < 0) goto cleanup; @@ -1425,7 +1428,21 @@ static int lxcMonitorClient(lxc_driver_t * driver, return -1; } - if ((fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + if (virSecurityManagerSetSocketLabel(driver->securityManager, vm->def) < 0) { + VIR_ERROR(_("Failed to set security context for monitor for %s"), + vm->def->name); + goto error; + } + + fd = socket(PF_UNIX, SOCK_STREAM, 0); + + if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) < 0) { + VIR_ERROR(_("Failed to clear security context for monitor for %s"), + vm->def->name); + goto error; + } + + if (fd < 0) { virReportSystemError(errno, "%s", _("Failed to create client socket")); goto error; @@ -1468,6 +1485,16 @@ static int lxcVmTerminate(lxc_driver_t *driver, return -1; } + virSecurityManagerRestoreAllLabel(driver->securityManager, + vm->def, false); + virSecurityManagerReleaseLabel(driver->securityManager, vm->def); + /* Clear out dynamically assigned labels */ + if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { + VIR_FREE(vm->def->seclabel.model); + VIR_FREE(vm->def->seclabel.label); + VIR_FREE(vm->def->seclabel.imagelabel); + } + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) == 0) { rc = virCgroupKillPainfully(group); if (rc < 0) { @@ -1598,6 +1625,12 @@ lxcBuildControllerCmd(lxc_driver_t *driver, virCommandAddArgFormat(cmd, "%d", ttyFDs[i]); virCommandPreserveFD(cmd, ttyFDs[i]); } + + if (driver->securityDriverName) { + virCommandAddArg(cmd, "--security"); + virCommandAddArg(cmd, driver->securityDriverName); + } + virCommandAddArg(cmd, "--handshake"); virCommandAddArgFormat(cmd, "%d", handshakefd); virCommandAddArg(cmd, "--background"); @@ -1792,6 +1825,24 @@ static int lxcVmStart(virConnectPtr conn, virReportOOMError(); goto cleanup; } + + /* If you are using a SecurityDriver with dynamic labelling, + then generate a security label for isolation */ + VIR_DEBUG("Generating domain security label (if required)"); + if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DEFAULT) + vm->def->seclabel.type = VIR_DOMAIN_SECLABEL_NONE; + + if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) { + virDomainAuditSecurityLabel(vm, false); + goto cleanup; + } + virDomainAuditSecurityLabel(vm, true); + + VIR_DEBUG("Setting domain security labels"); + if (virSecurityManagerSetAllLabel(driver->securityManager, + vm->def, NULL) < 0) + goto cleanup; + for (i = 0 ; i < vm->def->nconsoles ; i++) ttyFDs[i] = -1; @@ -1947,6 +1998,16 @@ cleanup: if (rc != 0) { VIR_FORCE_CLOSE(priv->monitor); virDomainConfVMNWFilterTeardown(vm); + + virSecurityManagerRestoreAllLabel(driver->securityManager, + vm->def, false); + virSecurityManagerReleaseLabel(driver->securityManager, vm->def); + /* Clear out dynamically assigned labels */ + if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { + VIR_FREE(vm->def->seclabel.model); + VIR_FREE(vm->def->seclabel.label); + VIR_FREE(vm->def->seclabel.imagelabel); + } } for (i = 0 ; i < nttyFDs ; i++) VIR_FORCE_CLOSE(ttyFDs[i]); @@ -2071,6 +2132,9 @@ lxcDomainCreateAndStart(virConnectPtr conn, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; + if (virSecurityManagerVerify(driver->securityManager, def) < 0) + goto cleanup; + if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) goto cleanup; @@ -2115,6 +2179,105 @@ cleanup: } +static int lxcDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) +{ + lxc_driver_t *driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + lxcDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + + memset(seclabel, 0, sizeof(*seclabel)); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + lxcError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (!virDomainVirtTypeToString(vm->def->virtType)) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("unknown virt type in domain definition '%d'"), + vm->def->virtType); + goto cleanup; + } + + /* + * Theoretically, the pid can be replaced during this operation and + * return the label of a different process. If atomicity is needed, + * further validation will be required. + * + * Comment from Dan Berrange: + * + * Well the PID as stored in the virDomainObjPtr can't be changed + * because you've got a locked object. The OS level PID could have + * exited, though and in extreme circumstances have cycled through all + * PIDs back to ours. We could sanity check that our PID still exists + * after reading the label, by checking that our FD connecting to the + * LXC monitor hasn't seen SIGHUP/ERR on poll(). + */ + if (virDomainObjIsActive(vm)) { + if (virSecurityManagerGetProcessLabel(driver->securityManager, + vm->def, vm->pid, seclabel) < 0) { + lxcError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to get security label")); + goto cleanup; + } + } + + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + lxcDriverUnlock(driver); + return ret; +} + +static int lxcNodeGetSecurityModel(virConnectPtr conn, + virSecurityModelPtr secmodel) +{ + lxc_driver_t *driver = conn->privateData; + char *p; + int ret = 0; + + lxcDriverLock(driver); + memset(secmodel, 0, sizeof(*secmodel)); + + /* NULL indicates no driver, which we treat as + * success, but simply return no data in *secmodel */ + if (driver->caps->host.secModel.model == NULL) + goto cleanup; + + p = driver->caps->host.secModel.model; + if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("security model string exceeds max %d bytes"), + VIR_SECURITY_MODEL_BUFLEN-1); + ret = -1; + goto cleanup; + } + strcpy(secmodel->model, p); + + p = driver->caps->host.secModel.doi; + if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("security DOI string exceeds max %d bytes"), + VIR_SECURITY_DOI_BUFLEN-1); + ret = -1; + goto cleanup; + } + strcpy(secmodel->doi, p); + +cleanup: + lxcDriverUnlock(driver); + return ret; +} + + static int lxcDomainEventRegister(virConnectPtr conn, virConnectDomainEventCallback callback, @@ -2363,6 +2526,10 @@ lxcReconnectVM(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) lxcMonitorEvent, vm, NULL)) < 0) goto error; + + if (virSecurityManagerReserveLabel(driver->securityManager, + vm->def, vm->pid) < 0) + goto error; } else { vm->def->id = -1; VIR_FORCE_CLOSE(priv->monitor); @@ -2379,6 +2546,27 @@ error: } +static int +lxcSecurityInit(lxc_driver_t *driver) +{ + virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, + false, + driver->securityDefaultConfined, + driver->securityRequireConfined); + if (!mgr) + goto error; + + driver->securityManager = mgr; + + return 0; + +error: + VIR_ERROR(_("Failed to initialize security drivers")); + virSecurityManagerFree(mgr); + return -1; +} + + static int lxcStartup(int privileged) { char *ld; @@ -2439,7 +2627,10 @@ static int lxcStartup(int privileged) if (lxcLoadDriverConfig(lxc_driver) < 0) goto cleanup; - if ((lxc_driver->caps = lxcCapsInit()) == NULL) + if (lxcSecurityInit(lxc_driver) < 0) + goto cleanup; + + if ((lxc_driver->caps = lxcCapsInit(lxc_driver)) == NULL) goto cleanup; lxc_driver->caps->privateDataAllocFunc = lxcDomainObjPrivateAlloc; @@ -2531,6 +2722,7 @@ static int lxcShutdown(void) lxcProcessAutoDestroyShutdown(lxc_driver); virCapabilitiesFree(lxc_driver->caps); + virSecurityManagerFree(lxc_driver->securityManager); VIR_FREE(lxc_driver->configDir); VIR_FREE(lxc_driver->autostartDir); VIR_FREE(lxc_driver->stateDir); @@ -3859,6 +4051,8 @@ static virDriver lxcDriver = { .domainGetBlkioParameters = lxcDomainGetBlkioParameters, /* 0.9.8 */ .domainGetInfo = lxcDomainGetInfo, /* 0.4.2 */ .domainGetState = lxcDomainGetState, /* 0.9.2 */ + .domainGetSecurityLabel = lxcDomainGetSecurityLabel, /* 0.9.4 */ + .nodeGetSecurityModel = lxcNodeGetSecurityModel, /* 0.9.4 */ .domainGetXMLDesc = lxcDomainGetXMLDesc, /* 0.4.2 */ .listDefinedDomains = lxcListDefinedDomains, /* 0.4.2 */ .numOfDefinedDomains = lxcNumDefinedDomains, /* 0.4.2 */ diff --git a/src/lxc/test_libvirtd_lxc.aug b/src/lxc/test_libvirtd_lxc.aug index e757b82..f8544a0 100644 --- a/src/lxc/test_libvirtd_lxc.aug +++ b/src/lxc/test_libvirtd_lxc.aug @@ -13,6 +13,7 @@ module Test_libvirtd_lxc = # This is disabled by default, uncomment below to enable it. # log_with_libvirtd = 1 +security_driver = \"selinux\" " test Libvirtd_lxc.lns get conf = @@ -29,3 +30,4 @@ log_with_libvirtd = 1 { "#comment" = "This is disabled by default, uncomment below to enable it." } { "#comment" = "" } { "log_with_libvirtd" = "1" } +{ "security_driver" = "selinux" } -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
For the sake of backwards compat, LXC guests are *not* confined by default. This is because it is not practical to dynamically relabel containers using large filesystem trees. Applications can create confined containers though, by giving suitable XML configs
* src/Makefile.am: Link libvirt_lxc to security drivers * src/lxc/libvirtd_lxc.aug, src/lxc/lxc_conf.h, src/lxc/lxc_conf.c, src/lxc/lxc.conf, src/lxc/test_libvirtd_lxc.aug: Config file handling for security driver * src/lxc/lxc_driver.c: Wire up security driver functions * src/lxc/lxc_controller.c: Add a '--security' flag to specify which security driver to activate * src/lxc/lxc_container.c, src/lxc/lxc_container.h: Set the process label just before exec'ing init. ---
+++ b/src/lxc/lxc.conf @@ -11,3 +11,21 @@ # This is disabled by default, uncomment below to enable it. # # log_with_libvirtd = 1 + + +# The default security driver is SELinux. If SELinux is disabled +# on the host, then the security driver will automatically disable +# itself. If you wish to disable QEMU SELinux security driver while +# leaving SELinux enabled for the host in general, then set this +# to 'none' instead. +# +# security_driver = "selinux" + +# If set to non-zero, then the default security labelling
Same question as 5/7 about whether to prefer US spelling of labeling.
+# will make guests confined. If set to zero, then guests +# will be unconfined by default. Defaults to zero +# security_default_confined = 1 + +# If set to non-zero, then attempts to create unconfined +# guests will be blocked. Defaults to zero.
Consistency - one description ended with '.', the other did not. Back to the 5/7 question of whether this should be spelled out as 'zero' or listed as '0'.
+# security_require_confined = 1 \ No newline at end of file
'make syntax-check' wasn't happy: prohibit_empty_lines_at_EOF src/lxc/lxc.conf maint.mk: empty line(s) or no newline at EOF
@@ -1598,6 +1625,12 @@ lxcBuildControllerCmd(lxc_driver_t *driver, virCommandAddArgFormat(cmd, "%d", ttyFDs[i]); virCommandPreserveFD(cmd, ttyFDs[i]); } + + if (driver->securityDriverName) { + virCommandAddArg(cmd, "--security"); + virCommandAddArg(cmd, driver->securityDriverName); + }
Is it worth the shorter: if (driver->securityDriverName) virCommandAddArgPair(cmd, "--security", driver->securityDriverName);
+ +static int lxcNodeGetSecurityModel(virConnectPtr conn, + virSecurityModelPtr secmodel) +{
+ + p = driver->caps->host.secModel.model; + if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("security model string exceeds max %d bytes"), + VIR_SECURITY_MODEL_BUFLEN-1); + ret = -1; + goto cleanup; + } + strcpy(secmodel->model, p);
Rather than doing length checks and then strcpy, wouldn't it be better to use virStrncpy? (Twice in this function).
@@ -3859,6 +4051,8 @@ static virDriver lxcDriver = { .domainGetBlkioParameters = lxcDomainGetBlkioParameters, /* 0.9.8 */ .domainGetInfo = lxcDomainGetInfo, /* 0.4.2 */ .domainGetState = lxcDomainGetState, /* 0.9.2 */ + .domainGetSecurityLabel = lxcDomainGetSecurityLabel, /* 0.9.4 */ + .nodeGetSecurityModel = lxcNodeGetSecurityModel, /* 0.9.4 */
You've been sitting on this series for a while, now :) 0.9.10, not 0.9.4. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> To allow the container to access /dev and /dev/pts when under sVirt, set an explicit mount option. Also set a max size on the /dev mount to prevent DOS on memory usage * src/lxc/lxc_container.c: Set /dev mount context * src/lxc/lxc_controller.c: Set /dev/pts mount context --- src/lxc/lxc_container.c | 75 +++++++++++++++++++++++++++++++++++---------- src/lxc/lxc_controller.c | 43 +++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 4df0b55..3c8e0e3 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -36,6 +36,10 @@ #include <unistd.h> #include <mntent.h> +#if HAVE_SELINUX +# include <selinux/selinux.h> +#endif + /* Yes, we want linux private one, for _syscall2() macro */ #include <linux/unistd.h> @@ -419,7 +423,6 @@ err: static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) { const struct { - bool onlyPivotRoot; bool needPrefix; const char *src; const char *dst; @@ -433,16 +436,19 @@ static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) * mount point in the main OS becomes readonly too which is not what * we want. Hence some things have two entries here. */ - { true, false, "devfs", "/dev", "tmpfs", "mode=755", MS_NOSUID }, - { false, false, "proc", "/proc", "proc", NULL, MS_NOSUID|MS_NOEXEC|MS_NODEV }, - { false, false, "/proc/sys", "/proc/sys", NULL, NULL, MS_BIND }, - { false, false, "/proc/sys", "/proc/sys", NULL, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY }, - { false, true, "/sys", "/sys", NULL, NULL, MS_BIND }, - { false, true, "/sys", "/sys", NULL, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY }, - { false, true, "/selinux", "/selinux", NULL, NULL, MS_BIND }, - { false, true, "/selinux", "/selinux", NULL, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY }, + { false, "proc", "/proc", "proc", NULL, MS_NOSUID|MS_NOEXEC|MS_NODEV }, + { false, "/proc/sys", "/proc/sys", NULL, NULL, MS_BIND }, + { false, "/proc/sys", "/proc/sys", NULL, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY }, + { true, "/sys", "/sys", NULL, NULL, MS_BIND }, + { true, "/sys", "/sys", NULL, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY }, + { true, "/selinux", "/selinux", NULL, NULL, MS_BIND }, + { true, "/selinux", "/selinux", NULL, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY }, }; int i, rc = -1; + char *opts = NULL; +#if HAVE_SELINUX + security_context_t con; +#endif VIR_DEBUG("Mounting basic filesystems %s pivotRoot=%d", NULLSTR(srcprefix), pivotRoot); @@ -450,10 +456,8 @@ static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) char *src = NULL; const char *srcpath = NULL; - VIR_DEBUG("Consider %s onlyPivotRoot=%d", - mnts[i].src, mnts[i].onlyPivotRoot); - if (mnts[i].onlyPivotRoot && !pivotRoot) - continue; + VIR_DEBUG("Process %s -> %s", + mnts[i].src, mnts[i].dst); if (virFileMakePath(mnts[i].dst) < 0) { virReportSystemError(errno, @@ -474,8 +478,10 @@ static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) /* Skip if mount doesn't exist in source */ if ((srcpath[0] == '/') && - (access(srcpath, R_OK) < 0)) + (access(srcpath, R_OK) < 0)) { + VIR_FREE(src); continue; + } VIR_DEBUG("Mount %s on %s type=%s flags=%x, opts=%s", srcpath, mnts[i].dst, mnts[i].type, mnts[i].mflags, mnts[i].opts); @@ -489,15 +495,50 @@ static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) VIR_FREE(src); } + if (pivotRoot) { +#if HAVE_SELINUX + if (getfilecon("/", &con) < 0 && + errno != ENOTSUP) { + virReportSystemError(errno, "%s", + _("Failed to query file context on /")); + goto cleanup; + } else { +#endif + /* + * tmpfs is limited to 64kb, since we only have device nodes in there + * and don't want to DOS the entire OS RAM usage + */ + if (virAsprintf(&opts, "mode=755,size=65536%%%s%s%s", + con ? ",context=\"" : "", + con ? (const char *)con : "", + con ? "\"" : "") < 0) { + virReportOOMError(); + goto cleanup; + } +#if HAVE_SELINUX + } +#endif + + VIR_DEBUG("Mount devfs on /dev type=tmpfs flags=%x, opts=%s", + MS_NOSUID, opts); + if (mount("devfs", "/dev", "tmpfs", MS_NOSUID, opts) < 0) { + virReportSystemError(errno, + _("Failed to mount %s on %s type %s"), + "devfs", "/dev", "tmpfs"); + goto cleanup; + } + } + rc = 0; cleanup: VIR_DEBUG("rc=%d", rc); + VIR_FREE(opts); return rc; } -static int lxcContainerMountDevFS(virDomainFSDefPtr root) +static int lxcContainerMountFSDevPTS(virDomainFSDefPtr root) { char *devpts = NULL; int rc = -1; @@ -1069,8 +1110,8 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, if (lxcContainerMountBasicFS("/.oldroot", true) < 0) return -1; - /* Mounts /dev and /dev/pts */ - if (lxcContainerMountDevFS(root) < 0) + /* Mounts /dev/pts */ + if (lxcContainerMountFSDevPTS(root) < 0) return -1; /* Populates device nodes in /dev/ */ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 6bc54b7..edd9dca 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -52,6 +52,9 @@ # define NUMA_VERSION1_COMPATIBILITY 1 # include <numa.h> #endif +#if HAVE_SELINUX +# include <selinux/selinux.h> +#endif #include "virterror_internal.h" #include "logging.h" @@ -1339,6 +1342,10 @@ lxcControllerRun(virDomainDefPtr def, * marked as shared */ if (root) { +#if HAVE_SELINUX + security_context_t con; +#endif + char *opts; VIR_DEBUG("Setting up private /dev/pts"); if (!virFileExists(root->src)) { @@ -1373,16 +1380,42 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } - /* XXX should we support gid=X for X!=5 for distros which use - * a different gid for tty? */ - VIR_DEBUG("Mounting 'devpts' on %s", devpts); - if (mount("devpts", devpts, "devpts", 0, - "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) { +#if HAVE_SELINUX + if (getfilecon(root->src, &con) < 0 && + errno != ENOTSUP) { + virReportSystemError(errno, + _("Failed to query file context on %s"), + root->src); + goto cleanup; + } else { +#endif + /* + * tmpfs is limited to 64kb, since we only have device nodes in there + * and don't want to DOS the entire OS RAM usage + */ + /* XXX should we support gid=X for X!=5 for distros which use + * a different gid for tty? */ + if (virAsprintf(&opts, "newinstance,ptmxmode=0666,mode=0620,gid=5%s%s%s", + con ? ",context=\"" : "", + con ? (const char *)con : "", + con ? "\"" : "") < 0) { + virReportOOMError(); + goto cleanup; + } +#if HAVE_SELINUX + } +#endif + + VIR_DEBUG("Mount devpts on %s type=tmpfs flags=%x, opts=%s", + devpts, MS_NOSUID, opts); + if (mount("devpts", devpts, "devpts", MS_NOSUID, opts) < 0) { + VIR_FREE(opts); virReportSystemError(errno, _("Failed to mount devpts on %s"), devpts); goto cleanup; } + VIR_FREE(opts); if (access(devptmx, R_OK) < 0) { VIR_WARN("Kernel does not support private devpts, using shared devpts"); -- 1.7.7.5
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
To allow the container to access /dev and /dev/pts when under sVirt, set an explicit mount option. Also set a max size on the /dev mount to prevent DOS on memory usage
* src/lxc/lxc_container.c: Set /dev mount context * src/lxc/lxc_controller.c: Set /dev/pts mount context --- src/lxc/lxc_container.c | 75 +++++++++++++++++++++++++++++++++++---------- src/lxc/lxc_controller.c | 43 +++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 22 deletions(-)
+ } else { +#endif + /* + * tmpfs is limited to 64kb, since we only have device nodes in there + * and don't want to DOS the entire OS RAM usage + */ + if (virAsprintf(&opts, "mode=755,size=65536%%%s%s%s",
Ouch. size=65536% is _not_ what you want; you either want size=65536 or something like size=10%.
+ con ? ",context=\"" : "", + con ? (const char *)con : "", + con ? "\"" : "") < 0) {
I would have split this: if (virAsprintf(&opts, "mode=755,size=65536") < 0 || (con && virAsprintf(&opts, ",context=\"%s\"", (const char *)con) < 0)) {
+ virReportOOMError(); + goto cleanup; + } +#if HAVE_SELINUX + } +#endif
You don't need this second #if. That is, instead of writing: #if HAVE_SELINUX if (condition) { goto cleanup; } else { #endif stuff; #if HAVE_SELINUX } #endif I would have done: #if HAVE_SELINUX if (condition) { goto cleanup; } #endif stuff;
@@ -1373,16 +1380,42 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; }
- /* XXX should we support gid=X for X!=5 for distros which use - * a different gid for tty? */ - VIR_DEBUG("Mounting 'devpts' on %s", devpts); - if (mount("devpts", devpts, "devpts", 0, - "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) { +#if HAVE_SELINUX + if (getfilecon(root->src, &con) < 0 && + errno != ENOTSUP) { + virReportSystemError(errno, + _("Failed to query file context on %s"), + root->src); + goto cleanup; + } else { +#endif + /* + * tmpfs is limited to 64kb, since we only have device nodes in there + * and don't want to DOS the entire OS RAM usage + */
Is this comment really relative to the devpts mount point?
+ /* XXX should we support gid=X for X!=5 for distros which use + * a different gid for tty? */ + if (virAsprintf(&opts, "newinstance,ptmxmode=0666,mode=0620,gid=5%s%s%s", + con ? ",context=\"" : "", + con ? (const char *)con : "", + con ? "\"" : "") < 0) { + virReportOOMError(); + goto cleanup; + } +#if HAVE_SELINUX + } +#endif
Same formatting nit about not needing a second #if. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/24/2012 01:21 PM, Eric Blake wrote:
+ if (virAsprintf(&opts, "mode=755,size=65536%%%s%s%s", + con ? ",context=\"" : "", + con ? (const char *)con : "", + con ? "\"" : "") < 0) {
I would have split this:
if (virAsprintf(&opts, "mode=755,size=65536") < 0 || (con && virAsprintf(&opts, ",context=\"%s\"", (const char *)con) < 0)) {
Never mind - that doesn't work; likewise, I don't think we have any guarantees about self-modifying strings such as: if (virAsprintf(&opts, "mode=755,size=65536") < 0 || (con && virAsprintf(&opts, "%s,context=\"%s\"", opts, (const char *)con) < 0)) { I guess I was thinking virBufferAsprintf, where appending is indeed easier than doing it in one shot. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jan 11, 2012 at 04:33:29PM +0000, Daniel P. Berrange wrote:
This patch series adds support for sVirt to the LXC driver. In this series, all LXC guests continue to run unconfined by default. The app has to explicitly request sVirt confinement for the guest. This is to ensure backwards compatibility with existing deployments. Since we won't auto-relabel filesystem trees, it is not possible to turn it on by default, as we previously did with QEMU
Any takers for review ? I really want to see this in this release. 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 :|
participants (2)
-
Daniel P. Berrange -
Eric Blake