[libvirt] [PATCH 0/6] Improve security driver support for LXC
This series aims to improve the SELinux security driver support for LXC. First it allows the SELinux driver to load different configuration for LXC, vs SELinux. Second it makes the LXC code use the security drivers for figuring out mount options. The code was originally written by Dan Walsh, but split into a 6 part patch series & made to follow coding standards by myself.
From: Daniel Walsh <dwalsh@redhat.com> To allow the security drivers to apply different configuration information per hypervisor, pass the virtualization driver name into the security manager constructor. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_conf.h | 2 ++ src/lxc/lxc_controller.c | 8 ++++++-- src/lxc/lxc_driver.c | 7 ++++--- src/qemu/qemu_driver.c | 10 +++++++--- src/security/security_apparmor.c | 2 +- src/security/security_dac.c | 2 +- src/security/security_driver.c | 5 +++-- src/security/security_driver.h | 5 +++-- src/security/security_manager.c | 18 ++++++++++++++++-- src/security/security_manager.h | 5 ++++- src/security/security_nop.c | 2 +- src/security/security_selinux.c | 2 +- src/security/security_stack.c | 2 +- tests/seclabeltest.c | 2 +- 14 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index ebdc173..cc279b2 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -36,6 +36,8 @@ # include "security/security_manager.h" # include "configmake.h" +# define LXC_DRIVER_NAME "LXC" + # define LXC_CONFIG_DIR SYSCONFDIR "/libvirt/lxc" # define LXC_STATE_DIR LOCALSTATEDIR "/run/libvirt/lxc" # define LXC_LOG_DIR LOCALSTATEDIR "/log/libvirt/lxc" diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 26b3115..1292751 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1723,7 +1723,9 @@ int main(int argc, char *argv[]) break; case 'S': - if (!(securityDriver = virSecurityManagerNew(optarg, false, false, false))) { + if (!(securityDriver = virSecurityManagerNew(optarg, + LXC_DRIVER_NAME, + false, false, false))) { fprintf(stderr, "Cannot create security manager '%s'", optarg); goto cleanup; @@ -1750,7 +1752,9 @@ int main(int argc, char *argv[]) } if (securityDriver == NULL) { - if (!(securityDriver = virSecurityManagerNew("none", false, false, false))) { + if (!(securityDriver = virSecurityManagerNew("none", + LXC_DRIVER_NAME, + false, false, false))) { fprintf(stderr, "%s: cannot initialize nop security manager", argv[0]); goto cleanup; } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 03783ff..42d1d94 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2533,7 +2533,8 @@ error: static int lxcSecurityInit(lxc_driver_t *driver) { - virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, + virSecurityManagerPtr mgr = virSecurityManagerNew(LXC_DRIVER_NAME, + driver->securityDriverName, false, driver->securityDefaultConfined, driver->securityRequireConfined); @@ -3851,7 +3852,7 @@ static virNWFilterCallbackDriver lxcCallbackDriver = { /* Function Tables */ static virDriver lxcDriver = { .no = VIR_DRV_LXC, - .name = "LXC", + .name = LXC_DRIVER_NAME, .open = lxcOpen, /* 0.4.2 */ .close = lxcClose, /* 0.4.2 */ .version = lxcVersion, /* 0.4.6 */ @@ -3915,7 +3916,7 @@ static virDriver lxcDriver = { }; static virStateDriver lxcStateDriver = { - .name = "LXC", + .name = LXC_DRIVER_NAME, .initialize = lxcStartup, .cleanup = lxcShutdown, .active = lxcActive, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2bec617..aed1daa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -95,6 +95,8 @@ #define VIR_FROM_THIS VIR_FROM_QEMU +#define QEMU_DRIVER_NAME "QEMU" + #define QEMU_NB_MEM_PARAM 3 #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 @@ -213,6 +215,7 @@ static int qemuSecurityInit(struct qemud_driver *driver) { virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, + QEMU_DRIVER_NAME, driver->allowDiskFormatProbing, driver->securityDefaultConfined, driver->securityRequireConfined); @@ -221,7 +224,8 @@ qemuSecurityInit(struct qemud_driver *driver) goto error; if (driver->privileged) { - virSecurityManagerPtr dac = virSecurityManagerNewDAC(driver->user, + virSecurityManagerPtr dac = virSecurityManagerNewDAC(QEMU_DRIVER_NAME, + driver->user, driver->group, driver->allowDiskFormatProbing, driver->securityDefaultConfined, @@ -12784,7 +12788,7 @@ cleanup: static virDriver qemuDriver = { .no = VIR_DRV_QEMU, - .name = "QEMU", + .name = QEMU_DRIVER_NAME, .open = qemudOpen, /* 0.2.0 */ .close = qemudClose, /* 0.2.0 */ .supports_feature = qemudSupportsFeature, /* 0.5.0 */ @@ -12975,7 +12979,7 @@ qemuVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED, } static virNWFilterCallbackDriver qemuCallbackDriver = { - .name = "QEMU", + .name = QEMU_DRIVER_NAME, .vmFilterRebuild = qemuVMFilterRebuild, .vmDriverLock = qemuVMDriverLock, .vmDriverUnlock = qemuVMDriverUnlock, diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 8f8b200..d638d1f 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -328,7 +328,7 @@ AppArmorSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, /* Called on libvirtd startup to see if AppArmor is available */ static int -AppArmorSecurityManagerProbe(void) +AppArmorSecurityManagerProbe(const char *virtDriver ATTRIBUTE_UNUSED) { char *template = NULL; int rc = SECURITY_DRIVER_DISABLE; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e71dc20..8201022 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -65,7 +65,7 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, } static virSecurityDriverStatus -virSecurityDACProbe(void) +virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) { return SECURITY_DRIVER_ENABLE; } diff --git a/src/security/security_driver.c b/src/security/security_driver.c index fd2c01a..39736cf 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -37,7 +37,8 @@ static virSecurityDriverPtr security_drivers[] = { &virSecurityDriverNop, /* Must always be last, since it will always probe */ }; -virSecurityDriverPtr virSecurityDriverLookup(const char *name) +virSecurityDriverPtr virSecurityDriverLookup(const char *name, + const char *virtDriver) { virSecurityDriverPtr drv = NULL; int i; @@ -51,7 +52,7 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name) STRNEQ(tmp->name, name)) continue; - switch (tmp->probe()) { + switch (tmp->probe(virtDriver)) { case SECURITY_DRIVER_ENABLE: VIR_DEBUG("Probed name=%s", tmp->name); drv = tmp; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index f0ace1c..d24304c 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -31,7 +31,7 @@ typedef enum { typedef struct _virSecurityDriver virSecurityDriver; typedef virSecurityDriver *virSecurityDriverPtr; -typedef virSecurityDriverStatus (*virSecurityDriverProbe) (void); +typedef virSecurityDriverStatus (*virSecurityDriverProbe) (const char *virtDriver); typedef int (*virSecurityDriverOpen) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverClose) (virSecurityManagerPtr mgr); @@ -125,6 +125,7 @@ struct _virSecurityDriver { virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; }; -virSecurityDriverPtr virSecurityDriverLookup(const char *name); +virSecurityDriverPtr virSecurityDriverLookup(const char *name, + const char *virtDriver); #endif /* __VIR_SECURITY_H__ */ diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 0a43458..e0dd165 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -38,9 +38,11 @@ struct _virSecurityManager { bool allowDiskFormatProbing; bool defaultConfined; bool requireConfined; + const char *virtDriver; }; static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr drv, + const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined) @@ -56,6 +58,7 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr mgr->allowDiskFormatProbing = allowDiskFormatProbing; mgr->defaultConfined = defaultConfined; mgr->requireConfined = requireConfined; + mgr->virtDriver = virtDriver; if (drv->open(mgr) < 0) { virSecurityManagerFree(mgr); @@ -70,6 +73,7 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, + virSecurityManagerGetDriver(primary), virSecurityManagerGetAllowDiskFormatProbing(primary), virSecurityManagerGetDefaultConfined(primary), virSecurityManagerGetRequireConfined(primary)); @@ -83,7 +87,8 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, return mgr; } -virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, +virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, + uid_t user, gid_t group, bool allowDiskFormatProbing, bool defaultConfined, @@ -92,6 +97,7 @@ virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, + virtDriver, allowDiskFormatProbing, defaultConfined, requireConfined); @@ -107,11 +113,12 @@ virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, } virSecurityManagerPtr virSecurityManagerNew(const char *name, + const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined) { - virSecurityDriverPtr drv = virSecurityDriverLookup(name); + virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver); if (!drv) return NULL; @@ -136,6 +143,7 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, } return virSecurityManagerNewDriver(drv, + virtDriver, allowDiskFormatProbing, defaultConfined, requireConfined); @@ -162,6 +170,12 @@ void virSecurityManagerFree(virSecurityManagerPtr mgr) } const char * +virSecurityManagerGetDriver(virSecurityManagerPtr mgr) +{ + return mgr->virtDriver; +} + +const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) { if (mgr->drv->getDOI) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 32c8c3b..ca27bc6 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -32,6 +32,7 @@ typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; virSecurityManagerPtr virSecurityManagerNew(const char *name, + const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined); @@ -39,7 +40,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, virSecurityManagerPtr secondary); -virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, +virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, + uid_t user, gid_t group, bool allowDiskFormatProbing, bool defaultConfined, @@ -50,6 +52,7 @@ void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); void virSecurityManagerFree(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr); diff --git a/src/security/security_nop.c b/src/security/security_nop.c index c3bd426..e979b54 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -21,7 +21,7 @@ #include "security_nop.h" -static virSecurityDriverStatus virSecurityDriverProbeNop(void) +static virSecurityDriverStatus virSecurityDriverProbeNop(const char *virtDriver ATTRIBUTE_UNUSED) { return SECURITY_DRIVER_ENABLE; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 1e27e10..4bd33a5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -346,7 +346,7 @@ err: static int -SELinuxSecurityDriverProbe(void) +SELinuxSecurityDriverProbe(const char *virtDriver ATTRIBUTE_UNUSED) { return is_selinux_enabled() ? SECURITY_DRIVER_ENABLE : SECURITY_DRIVER_DISABLE; } diff --git a/src/security/security_stack.c b/src/security/security_stack.c index c82865f..2eab38c 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -49,7 +49,7 @@ void virSecurityStackSetSecondary(virSecurityManagerPtr mgr, } static virSecurityDriverStatus -virSecurityStackProbe(void) +virSecurityStackProbe(const char *virtDriver ATTRIBUTE_UNUSED) { return SECURITY_DRIVER_ENABLE; } diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index fca76b9..2f65ec1 100644 --- a/tests/seclabeltest.c +++ b/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, true, false); + mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false); if (mgr == NULL) { fprintf (stderr, "Failed to start security driver"); exit (-1); -- 1.7.10.1
On Fri, May 11, 2012 at 11:10:01AM +0100, Daniel P. Berrange wrote:
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 03783ff..42d1d94 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2533,7 +2533,8 @@ error: static int lxcSecurityInit(lxc_driver_t *driver) { - virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, + virSecurityManagerPtr mgr = virSecurityManagerNew(LXC_DRIVER_NAME, + driver->securityDriverName,
Yes I have the 2 args the wrong way around here. I have fixed this locally, but won't repost unless there are other flaws identified in this patch. 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 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
To allow the security drivers to apply different configuration information per hypervisor, pass the virtualization driver name into the security manager constructor.
ACK
From: Daniel Walsh <dwalsh@redhat.com> The AppArmour driver does not currently have support for LXC so ensure that when probing, it claims to be disabled Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_apparmor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index d638d1f..2d05fd0 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -328,7 +328,7 @@ AppArmorSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, /* Called on libvirtd startup to see if AppArmor is available */ static int -AppArmorSecurityManagerProbe(const char *virtDriver ATTRIBUTE_UNUSED) +AppArmorSecurityManagerProbe(const char *virtDriver) { char *template = NULL; int rc = SECURITY_DRIVER_DISABLE; @@ -336,6 +336,9 @@ AppArmorSecurityManagerProbe(const char *virtDriver ATTRIBUTE_UNUSED) if (use_apparmor() < 0) return rc; + if (virtDriver && STREQ(virtDriver, "LXC")) + return rc; + /* see if template file exists */ if (virAsprintf(&template, "%s/TEMPLATE", APPARMOR_DIR "/libvirt") == -1) { -- 1.7.10.1
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
The AppArmour driver does not currently have support for LXC so ensure that when probing, it claims to be disabled
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- src/security/security_apparmor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index d638d1f..2d05fd0 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -328,7 +328,7 @@ AppArmorSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED,
/* Called on libvirtd startup to see if AppArmor is available */ static int -AppArmorSecurityManagerProbe(const char *virtDriver ATTRIBUTE_UNUSED) +AppArmorSecurityManagerProbe(const char *virtDriver) { char *template = NULL; int rc = SECURITY_DRIVER_DISABLE; @@ -336,6 +336,9 @@ AppArmorSecurityManagerProbe(const char *virtDriver ATTRIBUTE_UNUSED) if (use_apparmor()< 0) return rc;
+ if (virtDriver&& STREQ(virtDriver, "LXC")) + return rc; + /* see if template file exists */ if (virAsprintf(&template, "%s/TEMPLATE", APPARMOR_DIR "/libvirt") == -1) { ACK
On Fri, 2012-05-11 at 22:39 -0400, Stefan Berger wrote:
+ if (virtDriver&& STREQ(virtDriver, "LXC")) + return rc; +
ACK once changed to: if (virtDriver && STREQ(virtDriver, "LXC")) ... -- Jamie Strandboge | http://www.canonical.com
On Mon, May 14, 2012 at 08:06:03AM -0500, Jamie Strandboge wrote:
On Fri, 2012-05-11 at 22:39 -0400, Stefan Berger wrote:
+ if (virtDriver&& STREQ(virtDriver, "LXC")) + return rc; +
ACK once changed to: if (virtDriver && STREQ(virtDriver, "LXC"))
The patch is already like that actually - it was Stefan's mail client mangling whitespace in his reply that made it look odd 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 :|
From: Daniel Walsh <dwalsh@redhat.com> Currently the SELinux driver stores its state in a set of global variables. This switches it to use a private data struct instead. This will enable different instances to have their own data. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 173 ++++++++++++++++++++++++--------------- 1 file changed, 106 insertions(+), 67 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 4bd33a5..7202e71 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2011 Red Hat, Inc. + * Copyright (C) 2008-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -33,12 +33,30 @@ #include "storage_file.h" #include "virfile.h" #include "virrandom.h" +#include "util.h" +#include "conf.h" #define VIR_FROM_THIS VIR_FROM_SECURITY -static char default_domain_context[1024]; -static char default_content_context[1024]; -static char default_image_context[1024]; +#define MAX_CONTEXT 1024 + +typedef struct _virSecuritySELinuxData virSecuritySELinuxData; +typedef virSecuritySELinuxData *virSecuritySELinuxDataPtr; + +typedef struct _virSecuritySELinuxCallbackData virSecuritySELinuxCallbackData; +typedef virSecuritySELinuxCallbackData *virSecuritySELinuxCallbackDataPtr; + +struct _virSecuritySELinuxData { + char *domain_context; + char *file_context; + char *content_context; +}; + +struct _virSecuritySELinuxCallbackData { + virSecurityManagerPtr manager; + virSecurityLabelDefPtr secdef; +}; + #define SECURITY_SELINUX_VOID_DOI "0" #define SECURITY_SELINUX_NAME "selinux" @@ -109,60 +127,51 @@ err: } static int -SELinuxInitialize(void) +SELinuxInitialize(virSecurityManagerPtr mgr) { - char *ptr = NULL; - int fd = 0; + char *ptr; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); - fd = open(selinux_virtual_domain_context_path(), O_RDONLY); - if (fd < 0) { + if (virFileReadAll(selinux_virtual_domain_context_path(), MAX_CONTEXT, &(data->domain_context)) < 0) { virReportSystemError(errno, - _("cannot open SELinux virtual domain context file '%s'"), + _("cannot read SELinux virtual domain context file '%s'"), selinux_virtual_domain_context_path()); - return -1; + goto error; } - if (saferead(fd, default_domain_context, sizeof(default_domain_context)) < 0) { - virReportSystemError(errno, - _("cannot read SELinux virtual domain context file %s"), - selinux_virtual_domain_context_path()); - VIR_FORCE_CLOSE(fd); - return -1; - } - VIR_FORCE_CLOSE(fd); - - ptr = strchrnul(default_domain_context, '\n'); - *ptr = '\0'; - - if ((fd = open(selinux_virtual_image_context_path(), O_RDONLY)) < 0) { - virReportSystemError(errno, - _("cannot open SELinux virtual image context file %s"), - selinux_virtual_image_context_path()); - return -1; - } + ptr = strchrnul(data->domain_context, '\n'); + if (ptr) + *ptr = '\0'; - if (saferead(fd, default_image_context, sizeof(default_image_context)) < 0) { + if (virFileReadAll(selinux_virtual_image_context_path(), 2*MAX_CONTEXT, &(data->file_context)) < 0) { virReportSystemError(errno, _("cannot read SELinux virtual image context file %s"), selinux_virtual_image_context_path()); - VIR_FORCE_CLOSE(fd); - return -1; + goto error; } - VIR_FORCE_CLOSE(fd); - ptr = strchrnul(default_image_context, '\n'); - if (*ptr == '\n') { + ptr = strchrnul(data->file_context, '\n'); + if (ptr && *ptr == '\n') { *ptr = '\0'; - strcpy(default_content_context, ptr+1); - ptr = strchrnul(default_content_context, '\n'); - if (*ptr == '\n') + data->content_context = strdup(ptr+1); + if (!data->content_context) + goto error; + ptr = strchrnul(data->content_context, '\n'); + if (ptr && *ptr == '\n') *ptr = '\0'; } + return 0; + +error: + VIR_FREE(data->domain_context); + VIR_FREE(data->file_context); + VIR_FREE(data->content_context); + return -1; } static int -SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +SELinuxGenSecurityLabel(virSecurityManagerPtr mgr, virDomainDefPtr def) { int rc = -1; @@ -172,7 +181,9 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, int c2 = 0; context_t ctx = NULL; const char *range; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + VIR_DEBUG("SELinuxGenSecurityLabel %s", virSecurityManagerGetDriver(mgr)); if ((def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) && !def->seclabel.baselabel && def->seclabel.model) { @@ -202,6 +213,8 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return rc; } + VIR_DEBUG("SELinuxGenSecurityLabel %d", def->seclabel.type); + switch (def->seclabel.type) { case VIR_DOMAIN_SECLABEL_STATIC: if (!(ctx = context_new(def->seclabel.label)) ) { @@ -245,7 +258,7 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, def->seclabel.label = SELinuxGenNewContext(def->seclabel.baselabel ? def->seclabel.baselabel : - default_domain_context, mcs); + data->domain_context, mcs); if (! def->seclabel.label) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("cannot generate selinux context for %s"), mcs); @@ -264,13 +277,11 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, 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; - } + def->seclabel.imagelabel = SELinuxGenNewContext(data->file_context, mcs); + if (!def->seclabel.imagelabel) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto cleanup; } if (!def->seclabel.model && @@ -344,22 +355,39 @@ err: } - static int -SELinuxSecurityDriverProbe(const char *virtDriver ATTRIBUTE_UNUSED) +SELinuxSecurityDriverProbe(const char *virtDriver) { - return is_selinux_enabled() ? SECURITY_DRIVER_ENABLE : SECURITY_DRIVER_DISABLE; + if (!is_selinux_enabled()) + return SECURITY_DRIVER_DISABLE; + + if (virtDriver && STREQ(virtDriver, "LXC") && + !virFileExists(selinux_lxc_contexts_path())) + return SECURITY_DRIVER_DISABLE; + + return SECURITY_DRIVER_ENABLE; } + static int -SELinuxSecurityDriverOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +SELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { - return SELinuxInitialize(); + return SELinuxInitialize(mgr); } + static int -SELinuxSecurityDriverClose(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +SELinuxSecurityDriverClose(virSecurityManagerPtr mgr) { + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + + if (!data) + return 0; + + VIR_FREE(data->domain_context); + VIR_FREE(data->file_context); + VIR_FREE(data->content_context); + return 0; } @@ -405,6 +433,7 @@ SELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, strcpy(sec->label, (char *) ctx); freecon(ctx); + VIR_DEBUG("SELinuxGetSecurityProcessLabel %s", sec->label); sec->enforcing = security_getenforce(); if (sec->enforcing == -1) { virReportSystemError(errno, "%s", @@ -639,8 +668,10 @@ SELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, size_t depth, void *opaque) { - const virSecurityLabelDefPtr secdef = opaque; + virSecuritySELinuxCallbackDataPtr cbdata = opaque; + const virSecurityLabelDefPtr secdef = cbdata->secdef; int ret; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(cbdata->manager); if (disk->seclabel && disk->seclabel->norelabel) return 0; @@ -649,17 +680,18 @@ SELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, disk->seclabel->label) { ret = SELinuxSetFilecon(path, disk->seclabel->label); } else if (depth == 0) { + if (disk->shared) { - ret = SELinuxSetFileconOptional(path, default_image_context); + ret = SELinuxSetFileconOptional(path, data->file_context); } else if (disk->readonly) { - ret = SELinuxSetFileconOptional(path, default_content_context); + ret = SELinuxSetFileconOptional(path, data->content_context); } else if (secdef->imagelabel) { ret = SELinuxSetFileconOptional(path, secdef->imagelabel); } else { ret = 0; } } else { - ret = SELinuxSetFileconOptional(path, default_content_context); + ret = SELinuxSetFileconOptional(path, data->content_context); } if (ret == 1 && !disk->seclabel) { /* If we failed to set a label, but virt_use_nfs let us @@ -680,10 +712,13 @@ SELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk) { - const virSecurityLabelDefPtr secdef = &def->seclabel; + virSecuritySELinuxCallbackData cbdata; + cbdata.secdef = &def->seclabel; + cbdata.manager = mgr; + bool allowDiskFormatProbing = virSecurityManagerGetAllowDiskFormatProbing(mgr); - if (secdef->norelabel) + if (cbdata.secdef->norelabel) return 0; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) @@ -700,7 +735,7 @@ SELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, true, -1, -1, /* current process uid:gid */ SELinuxSetSecurityFileLabel, - secdef); + &cbdata); } @@ -1119,6 +1154,7 @@ SELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr, { /* TODO: verify DOI */ const virSecurityLabelDefPtr secdef = &def->seclabel; + VIR_DEBUG("SELinuxSetSecurityProcessLabel %s", secdef->label); if (def->seclabel.label == NULL) return 0; @@ -1300,9 +1336,11 @@ SELinuxSetSecurityChardevCallback(virDomainDefPtr def, static int SELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, virDomainSmartcardDefPtr dev, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { const char *database; + virSecurityManagerPtr mgr = opaque; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); switch (dev->type) { case VIR_DOMAIN_SMARTCARD_TYPE_HOST: @@ -1312,7 +1350,7 @@ SELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return SELinuxSetFilecon(database, default_content_context); + return SELinuxSetFilecon(database, data->content_context); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return SELinuxSetSecurityChardevLabel(def, &dev->data.passthru); @@ -1333,6 +1371,7 @@ SELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path) { + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); const virSecurityLabelDefPtr secdef = &def->seclabel; int i; @@ -1368,19 +1407,19 @@ SELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, if (virDomainSmartcardDefForeach(def, true, SELinuxSetSecuritySmartcardCallback, - NULL) < 0) + mgr) < 0) return -1; if (def->os.kernel && - SELinuxSetFilecon(def->os.kernel, default_content_context) < 0) + SELinuxSetFilecon(def->os.kernel, data->content_context) < 0) return -1; if (def->os.initrd && - SELinuxSetFilecon(def->os.initrd, default_content_context) < 0) + SELinuxSetFilecon(def->os.initrd, data->content_context) < 0) return -1; if (stdin_path) { - if (SELinuxSetFilecon(stdin_path, default_content_context) < 0 && + if (SELinuxSetFilecon(stdin_path, data->content_context) < 0 && virStorageFileIsSharedFSType(stdin_path, VIR_STORAGE_FILE_SHFS_NFS) != 1) return -1; @@ -1403,7 +1442,7 @@ SELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, } virSecurityDriver virSecurityDriverSELinux = { - 0, + sizeof(virSecuritySELinuxData), SECURITY_SELINUX_NAME, SELinuxSecurityDriverProbe, SELinuxSecurityDriverOpen, -- 1.7.10.1
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Currently the SELinux driver stores its state in a set of global variables. This switches it to use a private data struct instead. This will enable different instances to have their own data.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- +SELinuxInitialize(virSecurityManagerPtr mgr) { [...] - ptr = strchrnul(default_image_context, '\n'); - if (*ptr == '\n') { + ptr = strchrnul(data->file_context, '\n'); + if (ptr&& *ptr == '\n') { *ptr = '\0'; - strcpy(default_content_context, ptr+1); - ptr = strchrnul(default_content_context, '\n'); - if (*ptr == '\n') + data->content_context = strdup(ptr+1); + if (!data->content_context) + goto error;
virReportOOMError ?
@@ -264,13 +277,11 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, 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; - } + def->seclabel.imagelabel = SELinuxGenNewContext(data->file_context, mcs); + if (!def->seclabel.imagelabel) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto cleanup; }
There was this check if (!def->seclabel.norelabel) that's now gone. Was this removed by accident? ACK with nit fixed.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 05/11/2012 10:43 PM, Stefan Berger wrote:
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Currently the SELinux driver stores its state in a set of global variables. This switches it to use a private data struct instead. This will enable different instances to have their own data.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- +SELinuxInitialize(virSecurityManagerPtr mgr) { [...] - ptr = strchrnul(default_image_context, '\n'); - if (*ptr == '\n') { + ptr = strchrnul(data->file_context, '\n'); + if (ptr&& *ptr == '\n') { *ptr = '\0'; - strcpy(default_content_context, ptr+1); - ptr = strchrnul(default_content_context, '\n'); - if (*ptr == '\n') + data->content_context = strdup(ptr+1); + if (!data->content_context) + goto error;
virReportOOMError ?
@@ -264,13 +277,11 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, 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; - } + def->seclabel.imagelabel = SELinuxGenNewContext(data->file_context, mcs); + if (!def->seclabel.imagelabel) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto cleanup; }
There was this check if (!def->seclabel.norelabel) that's now gone. Was this removed by accident?
ACK with nit fixed.
norelabel indicates that the Physical disk files/images should not be relabeled. When we create a tmpfs file system lxc containers always need to set an initial label on them. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk+uM7oACgkQrlYvE4MpobOFPACfZ/tDVzatSSoGkVUDEzICFmPE +1IAoNg7FX9wknCvZWFc9e7eLpN5SrZR =RQi1 -----END PGP SIGNATURE-----
On 05/12/2012 05:56 AM, Daniel J Walsh wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/11/2012 10:43 PM, Stefan Berger wrote:
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Currently the SELinux driver stores its state in a set of global variables. This switches it to use a private data struct instead. This will enable different instances to have their own data.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- +SELinuxInitialize(virSecurityManagerPtr mgr) { [...] - ptr = strchrnul(default_image_context, '\n'); - if (*ptr == '\n') { + ptr = strchrnul(data->file_context, '\n'); + if (ptr&& *ptr == '\n') { *ptr = '\0'; - strcpy(default_content_context, ptr+1); - ptr = strchrnul(default_content_context, '\n'); - if (*ptr == '\n') + data->content_context = strdup(ptr+1); + if (!data->content_context) + goto error; virReportOOMError ?
@@ -264,13 +277,11 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, 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; - } + def->seclabel.imagelabel = SELinuxGenNewContext(data->file_context, mcs); + if (!def->seclabel.imagelabel) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto cleanup; }
There was this check if (!def->seclabel.norelabel) that's now gone. Was this removed by accident?
ACK with nit fixed.
norelabel indicates that the Physical disk files/images should not be relabeled. When we create a tmpfs file system lxc containers always need to set an initial label on them.
Now I wonder whether this flag should not be enforced in case of non-tmpfs file system (LXC) and should be enforced in other cases (Qemu). Maybe we need to pass a flag to this function indicating tmpfs ? Stefan
On Fri, May 11, 2012 at 10:43:38PM -0400, Stefan Berger wrote:
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Currently the SELinux driver stores its state in a set of global variables. This switches it to use a private data struct instead. This will enable different instances to have their own data.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- +SELinuxInitialize(virSecurityManagerPtr mgr) { [...] - ptr = strchrnul(default_image_context, '\n'); - if (*ptr == '\n') { + ptr = strchrnul(data->file_context, '\n'); + if (ptr&& *ptr == '\n') { *ptr = '\0'; - strcpy(default_content_context, ptr+1); - ptr = strchrnul(default_content_context, '\n'); - if (*ptr == '\n') + data->content_context = strdup(ptr+1); + if (!data->content_context) + goto error;
virReportOOMError ?
@@ -264,13 +277,11 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, 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; - } + def->seclabel.imagelabel = SELinuxGenNewContext(data->file_context, mcs); + if (!def->seclabel.imagelabel) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot generate selinux context for %s"), mcs); + goto cleanup; }
There was this check if (!def->seclabel.norelabel) that's now gone. Was this removed by accident?
Yes & no. It was intentionally removed, but it should have been done in a separate patch, rather than this one. I'll remove this behaviour change & re-submit in a seprate patch. 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 :|
From: Daniel Walsh <dwalsh@redhat.com> The SELinux policy for LXC uses a different confinguration file to the traditional svirt one. Thus we need to load /etc/selinux/targeted/contexts/lxc_contexts which contains something like this: process = "system_u:system_r:svirt_lxc_net_t:s0" file = "system_u:object_r:svirt_lxc_file_t:s0" content = "system_u:object_r:virt_var_lib_t:s0" cleverly designed to be parsable by virConfPtr Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 80 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7202e71..dd6aee9 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -126,8 +126,73 @@ err: return newcontext; } + static int -SELinuxInitialize(virSecurityManagerPtr mgr) +SELinuxLXCInitialize(virSecurityManagerPtr mgr) +{ + virConfValuePtr scon = NULL; + virConfValuePtr tcon = NULL; + virConfValuePtr dcon = NULL; + virConfPtr selinux_conf; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + + selinux_conf = virConfReadFile(selinux_lxc_contexts_path(), 0); + if (!selinux_conf) { + virReportSystemError(errno, + _("cannot open SELinux lxc contexts file '%s'"), + selinux_lxc_contexts_path()); + return -1; + } + + scon = virConfGetValue(selinux_conf, "process"); + if (! scon || scon->type != VIR_CONF_STRING || (! scon->str)) { + virReportSystemError(errno, + _("cannot read 'process' value from selinux lxc contexts file '%s'"), + selinux_lxc_contexts_path()); + goto error; + } + + tcon = virConfGetValue(selinux_conf, "file"); + if (! tcon || tcon->type != VIR_CONF_STRING || (! tcon->str)) { + virReportSystemError(errno, + _("cannot read 'file' value from selinux lxc contexts file '%s'"), + selinux_lxc_contexts_path()); + goto error; + } + + dcon = virConfGetValue(selinux_conf, "content"); + if (! dcon || dcon->type != VIR_CONF_STRING || (! dcon->str)) { + virReportSystemError(errno, + _("cannot read 'file' value from selinux lxc contexts file '%s'"), + selinux_lxc_contexts_path()); + goto error; + } + + data->domain_context = strdup(scon->str); + data->file_context = strdup(tcon->str); + data->content_context = strdup(dcon->str); + if (!data->domain_context || + !data->file_context || + !data->content_context) { + virReportSystemError(errno, + _("cannot allocate memory for LXC SELinux contexts '%s'"), + selinux_lxc_contexts_path()); + goto error; + } + virConfFree(selinux_conf); + return 0; + +error: + virConfFree(selinux_conf); + VIR_FREE(data->domain_context); + VIR_FREE(data->file_context); + VIR_FREE(data->content_context); + return -1; +} + + +static int +SELinuxQEMUInitialize(virSecurityManagerPtr mgr) { char *ptr; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); @@ -170,6 +235,19 @@ error: return -1; } + +static int +SELinuxInitialize(virSecurityManagerPtr mgr) +{ + VIR_DEBUG("SELinuxInitialize %s", virSecurityManagerGetDriver(mgr)); + if (STREQ(virSecurityManagerGetDriver(mgr), "LXC")) { + return SELinuxLXCInitialize(mgr); + } else { + return SELinuxQEMUInitialize(mgr); + } +} + + static int SELinuxGenSecurityLabel(virSecurityManagerPtr mgr, virDomainDefPtr def) -- 1.7.10.1
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
The SELinux policy for LXC uses a different confinguration file
s/confinguration/configuration/
to the traditional svirt one. Thus we need to load
s/to/than/
/etc/selinux/targeted/contexts/lxc_contexts which contains something like this:
process = "system_u:system_r:svirt_lxc_net_t:s0" file = "system_u:object_r:svirt_lxc_file_t:s0" content = "system_u:object_r:virt_var_lib_t:s0"
cleverly designed to be parsable by virConfPtr
Signed-off-by: Daniel P. Berrange<berrange@redhat.com>
ACK
From: Daniel Walsh <dwalsh@redhat.com> Some security drivers require special options to be passed to the mount system call. Add a security driver API for handling this data. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 7 +++++ src/security/security_driver.c | 3 ++- src/security/security_driver.h | 4 +++ src/security/security_manager.c | 14 +++++++++- src/security/security_manager.h | 3 ++- src/security/security_nop.c | 7 +++++ src/security/security_selinux.c | 56 +++++++++++++++++++++++++++++++++++++++ src/security/security_stack.c | 6 +++++ 9 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4f34d25..11e254a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -961,6 +961,7 @@ virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerVerify; +virSecurityManagerGetMountOptions; # sexpr.h sexpr_append; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 8201022..470861d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -717,6 +717,11 @@ virSecurityDACSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +static char *virSecurityDACGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { + return NULL; +} + virSecurityDriver virSecurityDriverDAC = { sizeof(virSecurityDACData), "virDAC", @@ -754,4 +759,6 @@ virSecurityDriver virSecurityDriverDAC = { virSecurityDACRestoreSavedStateLabel, virSecurityDACSetImageFDLabel, + + virSecurityDACGetMountOptions, }; diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 39736cf..0f21d7a 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 Red Hat, Inc. + * Copyright (C) 2008-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -8,6 +8,7 @@ * * Authors: * James Morris <jmorris@namei.org> + * Dan Walsh <dwalsh@redhat.com> * */ #include <config.h> diff --git a/src/security/security_driver.h b/src/security/security_driver.h index d24304c..c68615d 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -86,6 +86,8 @@ typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetImageFDLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, int fd); +typedef char *(*virSecurityDomainGetMountOptions) (virSecurityManagerPtr mgr, + virDomainDefPtr def); struct _virSecurityDriver { size_t privateDataLen; @@ -123,6 +125,8 @@ struct _virSecurityDriver { virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel; virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; + + virSecurityDomainGetMountOptions domainGetSecurityMountOptions; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index e0dd165..ece39cd 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -149,7 +149,6 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, requireConfined); } - void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) { /* This accesses the memory just beyond mgr, which was allocated @@ -423,3 +422,16 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; } + +char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + if (mgr->drv->domainGetSecurityMountOptions) + return mgr->drv->domainGetSecurityMountOptions(mgr, vm); + +/* + I don't think this is an error, these should be optional + virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +*/ + return NULL; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index ca27bc6..f0bf60d 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -107,5 +107,6 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr, int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, int fd); - +char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, + virDomainDefPtr vm); #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_nop.c b/src/security/security_nop.c index e979b54..b62daf5 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -164,6 +164,11 @@ static int virSecurityDomainSetFDLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UN return 0; } +static char *virSecurityDomainGetMountOptionsNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { + return NULL; +} + virSecurityDriver virSecurityDriverNop = { 0, "none", @@ -200,4 +205,6 @@ virSecurityDriver virSecurityDriverNop = { virSecurityDomainRestoreSavedStateLabelNop, virSecurityDomainSetFDLabelNop, + + virSecurityDomainGetMountOptionsNop, }; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index dd6aee9..f7bc567 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1519,6 +1519,60 @@ SELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return SELinuxFSetFilecon(fd, secdef->imagelabel); } +static char *genImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def) { + const virSecurityLabelDefPtr secdef = &def->seclabel; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + const char *range; + context_t ctx = NULL; + char *label = NULL; + const char *mcs = NULL; + + if (secdef->label) { + ctx = context_new(secdef->label); + if (!ctx) { + virReportOOMError(); + goto cleanup; + } + range = context_range_get(ctx); + if (range) { + mcs = strdup(range); + if (!mcs) { + virReportOOMError(); + goto cleanup; + } + label = SELinuxGenNewContext(data->file_context, mcs); + if (!label) { + virReportOOMError(); + goto cleanup; + } + } + } + +cleanup: + context_free(ctx); + VIR_FREE(mcs); + return label; +} + +static char *SELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr, + virDomainDefPtr def) { + char *opts = NULL; + const virSecurityLabelDefPtr secdef = &def->seclabel; + + if (! secdef->imagelabel) + secdef->imagelabel = genImageLabel(mgr,def); + + if (secdef->imagelabel) { + virAsprintf(&opts, + ",context=\"%s\"", + (const char*) secdef->imagelabel); + } + + VIR_DEBUG("SELinuxGetSecurityMountOptions imageLabel %s", secdef->imagelabel); + return opts; +} + virSecurityDriver virSecurityDriverSELinux = { sizeof(virSecuritySELinuxData), SECURITY_SELINUX_NAME, @@ -1555,4 +1609,6 @@ virSecurityDriver virSecurityDriverSELinux = { SELinuxRestoreSavedStateLabel, SELinuxSetImageFDLabel, + + SELinuxGetSecurityMountOptions, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 2eab38c..6ecd099 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -403,6 +403,10 @@ virSecurityStackSetImageFDLabel(virSecurityManagerPtr mgr, return rc; } +static char *virSecurityStackGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { + return NULL; +} virSecurityDriver virSecurityDriverStack = { sizeof(virSecurityStackData), @@ -440,4 +444,6 @@ virSecurityDriver virSecurityDriverStack = { virSecurityStackRestoreSavedStateLabel, virSecurityStackSetImageFDLabel, + + virSecurityStackGetMountOptions, }; -- 1.7.10.1
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Some security drivers require special options to be passed to the mount system call. Add a security driver API for handling this data.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- @@ -423,3 +422,16 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; } + +char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + if (mgr->drv->domainGetSecurityMountOptions) + return mgr->drv->domainGetSecurityMountOptions(mgr, vm); + +/* + I don't think this is an error, these should be optional + virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +*/
Indentation -- or is my email program distorting it? ACK
On Fri, May 11, 2012 at 10:43:50PM -0400, Stefan Berger wrote:
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Some security drivers require special options to be passed to the mount system call. Add a security driver API for handling this data.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> --- @@ -423,3 +422,16 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; } + +char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + if (mgr->drv->domainGetSecurityMountOptions) + return mgr->drv->domainGetSecurityMountOptions(mgr, vm); + +/* + I don't think this is an error, these should be optional + virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +*/
Indentation -- or is my email program distorting it?
This is a genuine whitespace bug. 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 :|
From: Daniel Walsh <dwalsh@redhat.com> Instead of hardcoding use of SELinux contexts in the LXC driver, switch over to using the official security driver API. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 54 ++++++++++++++++++---------------------------- src/lxc/lxc_controller.c | 26 +++++----------------- src/lxc/lxc_driver.c | 1 + 3 files changed, 27 insertions(+), 54 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 0636eab..ca5696d 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -36,10 +36,6 @@ #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> @@ -426,7 +422,10 @@ err: } -static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) +static int lxcContainerMountBasicFS(virDomainDefPtr def, + const char *srcprefix, + bool pivotRoot, + virSecurityManagerPtr securityDriver) { const struct { bool needPrefix; @@ -454,9 +453,6 @@ static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) }; 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); @@ -504,28 +500,15 @@ static int lxcContainerMountBasicFS(const char *srcprefix, bool pivotRoot) } if (pivotRoot) { -#if HAVE_SELINUX - if (getfilecon("/", &con) < 0 && - errno != ENOTSUP) { - virReportSystemError(errno, "%s", - _("Failed to query file context on /")); - goto cleanup; - } -#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 HAVE_SELINUX - if (con) - ignore_value(virAsprintf(&opts, - "mode=755,size=65536,context=\"%s\"", - (const char *)con)); - else -#endif - opts = strdup("mode=755,size=65536"); - + char *mount_options = virSecurityManagerGetMountOptions(securityDriver, def); + ignore_value(virAsprintf(&opts, + "mode=755,size=65536%s",(mount_options ? mount_options : ""))); + VIR_FREE(mount_options); if (!opts) { virReportOOMError(); goto cleanup; @@ -1130,14 +1113,15 @@ cleanup: static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, virDomainFSDefPtr root, char **ttyPaths, - size_t nttyPaths) + size_t nttyPaths, + virSecurityManagerPtr securityDriver) { /* Gives us a private root, leaving all parent OS mounts on /.oldroot */ if (lxcContainerPivotRoot(root) < 0) return -1; /* Mounts the core /proc, /sys, etc filesystems */ - if (lxcContainerMountBasicFS("/.oldroot", true) < 0) + if (lxcContainerMountBasicFS(vmDef, "/.oldroot", true, securityDriver) < 0) return -1; /* Mounts /dev/pts */ @@ -1162,7 +1146,8 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, /* Nothing mapped to /, we're using the main root, but with extra stuff mapped in */ -static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef) +static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef, + virSecurityManagerPtr securityDriver) { VIR_DEBUG("def=%p", vmDef); /* @@ -1181,7 +1166,7 @@ static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef) return -1; /* Mounts the core /proc, /sys, etc filesystems */ - if (lxcContainerMountBasicFS(NULL, false) < 0) + if (lxcContainerMountBasicFS(vmDef, NULL, false, securityDriver) < 0) return -1; VIR_DEBUG("Mounting completed"); @@ -1211,15 +1196,16 @@ static int lxcContainerResolveSymlinks(virDomainDefPtr vmDef) static int lxcContainerSetupMounts(virDomainDefPtr vmDef, virDomainFSDefPtr root, char **ttyPaths, - size_t nttyPaths) + size_t nttyPaths, + virSecurityManagerPtr securityDriver) { if (lxcContainerResolveSymlinks(vmDef) < 0) return -1; if (root) - return lxcContainerSetupPivotRoot(vmDef, root, ttyPaths, nttyPaths); + return lxcContainerSetupPivotRoot(vmDef, root, ttyPaths, nttyPaths, securityDriver); else - return lxcContainerSetupExtraMounts(vmDef); + return lxcContainerSetupExtraMounts(vmDef, securityDriver); } @@ -1330,7 +1316,9 @@ static int lxcContainerChild( void *data ) goto cleanup; } - if (lxcContainerSetupMounts(vmDef, root, argv->ttyPaths, argv->nttyPaths) < 0) + if (lxcContainerSetupMounts(vmDef, root, + argv->ttyPaths, argv->nttyPaths, + argv->securityDriver) < 0) goto cleanup; if (!virFileExists(vmDef->os.init)) { diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1292751..b262259 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -52,9 +52,6 @@ # define NUMA_VERSION1_COMPATIBILITY 1 # include <numa.h> #endif -#if HAVE_SELINUX -# include <selinux/selinux.h> -#endif #include "virterror_internal.h" #include "logging.h" @@ -1385,6 +1382,7 @@ lxcControllerRun(virDomainDefPtr def, size_t nloopDevs = 0; int *loopDevs = NULL; size_t i; + char *mount_options = NULL; if (VIR_ALLOC_N(containerTtyFDs, nttyFDs) < 0) { virReportOOMError(); @@ -1436,11 +1434,7 @@ lxcControllerRun(virDomainDefPtr def, * marked as shared */ if (root) { -#if HAVE_SELINUX - security_context_t con; -#else - bool con = false; -#endif + mount_options = virSecurityManagerGetMountOptions(securityDriver, def); char *opts; VIR_DEBUG("Setting up private /dev/pts"); @@ -1476,21 +1470,10 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } -#if HAVE_SELINUX - if (getfilecon(root->src, &con) < 0 && - errno != ENOTSUP) { - virReportSystemError(errno, - _("Failed to query file context on %s"), - root->src); - goto cleanup; - } -#endif /* 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) { + if (virAsprintf(&opts, "newinstance,ptmxmode=0666,mode=0620,gid=5%s", + (mount_options ? mount_options : "")) < 0) { virReportOOMError(); goto cleanup; } @@ -1607,6 +1590,7 @@ lxcControllerRun(virDomainDefPtr def, monitor = client = -1; cleanup: + VIR_FREE(mount_options); VIR_FREE(devptmx); VIR_FREE(devpts); VIR_FORCE_CLOSE(control[0]); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 42d1d94..1cbb839 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2533,6 +2533,7 @@ error: static int lxcSecurityInit(lxc_driver_t *driver) { + VIR_INFO("lxcSecurityInit %s", driver->securityDriverName); virSecurityManagerPtr mgr = virSecurityManagerNew(LXC_DRIVER_NAME, driver->securityDriverName, false, -- 1.7.10.1
On 05/11/2012 06:10 AM, Daniel P. Berrange wrote:
From: Daniel Walsh<dwalsh@redhat.com>
Instead of hardcoding use of SELinux contexts in the LXC driver, switch over to using the official security driver API.
Signed-off-by: Daniel P. Berrange<berrange@redhat.com> ---
ACK
participants (4)
-
Daniel J Walsh -
Daniel P. Berrange -
Jamie Strandboge -
Stefan Berger