[libvirt] Reworked the XML verification patch for svirt
Main goal of this patch it to verify data being written to xml and inform the user when he makes a mistake. Only able to verify static/dynamic in domain_conf. Added verify code to qemu_driver.c for model and potentially label.
On Wed, Apr 01, 2009 at 10:33:56AM -0400, Daniel J Walsh wrote:
Main goal of this patch it to verify data being written to xml and inform the user when he makes a mistake.
This sounds good, especially about the added regression test, but at some point we should try to hook up the RelaxNG validation code in libxml2 and install the various XML Schemas as part of libvirt(d) installation. Things like testing of strings lenght, allowed character ranges, etc are easier to write maintain and centralize in a schemas description than in accumulated c code checks. At least as long as the checks are not contextual it's IMHO an easier approach. Still patch makes sense to me, in the current setup, but maybe we should add a RNG validation layer as part of the next release. This implies: - making sure all schemas are complete - hooking up the RNG validation as part of libvirt entry point. I assume this could be made a non-fatal error at least until we get more confidence in schemas. - make sure the "make install" and packaging set up and pick the schemas. One of the things which would need special care is making sure the validation is done on the daemon side, not on the client when doing remote access. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Apr 01, 2009 at 10:33:56AM -0400, Daniel J Walsh wrote:
Main goal of this patch it to verify data being written to xml and inform the user when he makes a mistake.
Only able to verify static/dynamic in domain_conf.
Added verify code to qemu_driver.c for model and potentially label.
diff --git a/src/Makefile.am b/src/Makefile.am index d5aac11..8d53740 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -170,9 +170,9 @@ STORAGE_HELPER_DISK_SOURCES = \ SECURITY_DRIVER_SOURCES = \ security.h security.c
-SECURITY_DRIVER_SELINUX_SOURCES = \ - security_selinux.h security_selinux.c - +if WITH_SECDRIVER_SELINUX +SECURITY_DRIVER_SOURCES += security_selinux.h security_selinux.c +endif
NODE_DEVICE_DRIVER_SOURCES = \ node_device.c node_device.h @@ -390,9 +390,6 @@ endif libvirt_driver_security_la_SOURCES = $(SECURITY_DRIVER_SOURCES) noinst_LTLIBRARIES += libvirt_driver_security.la libvirt_la_LIBADD += libvirt_driver_security.la -if WITH_SECDRIVER_SELINUX -libvirt_driver_security_la_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) -endif
What's the purpose of this change ? The reason for keeping an explicit variable name SECURITY_DRIVER_SELINUX_SOURCES was that those sounds need to be added to EXTRA_DIST, even when the compilation of the selinux bits is turned off - so that 'make dist' always gets all files.
diff --git a/src/domain_conf.c b/src/domain_conf.c index 2696449..c9259db 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1859,29 +1859,44 @@ virSecurityLabelDefParseXML(virConnectPtr conn, if (virXPathNode(conn, "./seclabel", ctxt) == NULL) return 0;
+ p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->seclabel.model = p; + p = virXPathStringLimit(conn, "string(./seclabel/@type)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - if ((def->seclabel.type = virDomainSeclabelTypeFromString(p)) < 0) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security type")); goto error; + } + def->seclabel.type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); + if (def->seclabel.type < 0) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security type")); + goto error; + }
/* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit(conn, "string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - def->seclabel.model = p;
p = virXPathStringLimit(conn, "string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("security label is missing")); goto error; + } + def->seclabel.label = p; }
ACK, this looks fine
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a5f9f92..5c88009 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -248,6 +248,7 @@ free_qparam_set;
# security.h +virSecurityDriverVerify; virSecurityDriverStartup; virSecurityDriverInit; virSecurityDriverSetDOI; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b2974bb..cd48193 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2115,6 +2115,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def) < 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -3398,6 +3401,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def) < 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { virDomainObjUnlock(vm); diff --git a/src/security.c b/src/security.c index e2bd20a..72aadf4 100644 --- a/src/security.c +++ b/src/security.c @@ -28,6 +28,25 @@ static virSecurityDriverPtr security_drivers[] = { };
int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def) +{ + unsigned int i; + const virSecurityLabelDefPtr secdef = &def->seclabel; + + if (STREQ(secdef->model, "none")) + return 0; + + for (i = 0; security_drivers[i] != NULL ; i++) { + if (STREQ(security_drivers[i]->name, secdef->model)) { + return security_drivers[i]->domainSecurityVerify(conn, def); + } + }
A few whitespace problems here - HACKING file has some helpful emacs rules to avoid this
+ virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security model")); + return -1; +} + +int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name) { diff --git a/src/security.h b/src/security.h index 8cc2c17..396f47f 100644 --- a/src/security.h +++ b/src/security.h @@ -46,11 +46,14 @@ typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm); +typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn, + virDomainDefPtr def);
struct _virSecurityDriver { const char *name; virSecurityDriverProbe probe; virSecurityDriverOpen open; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainGenLabel domainGenSecurityLabel; @@ -71,6 +74,9 @@ struct _virSecurityDriver { int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name);
+int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def); + void virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 3, 4); diff --git a/src/security_selinux.c b/src/security_selinux.c index 1708d55..7b8e107 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 Red Hat, Inc. + * Copyright (C) 2008,2009 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> * * SELinux security driver. */ @@ -361,6 +364,20 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, }
static int +SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def) +{ + const virSecurityLabelDefPtr secdef = &def->seclabel; + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) { + if (security_check_context(secdef->label) != 0) { + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("Invalid security label %s"), secdef->label); + return -1; + } + } + return 0; +}
Some whitespace problem there too
+ +static int SELinuxSetSecurityLabel(virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm) @@ -406,6 +423,7 @@ virSecurityDriver virSELinuxSecurityDriver = { .name = SECURITY_SELINUX_NAME, .probe = SELinuxSecurityDriverProbe, .open = SELinuxSecurityDriverOpen, + .domainSecurityVerify = SELinuxSecurityVerify, .domainSetSecurityImageLabel = SELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, diff --git a/tests/.gitignore b/tests/.gitignore index 9d809c9..4f33d0b 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -15,6 +15,7 @@ nodedevxml2xmltest nodeinfotest statstest qparamtest +seclabeltest *.gcda *.gcno *.exe diff --git a/tests/Makefile.am b/tests/Makefile.am index 28b2737..48db913 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -54,7 +54,7 @@ EXTRA_DIST = \ nodedevschemadata
noinst_PROGRAMS = virshtest conftest \ - nodeinfotest statstest qparamtest + nodeinfotest statstest qparamtest seclabeltest
if WITH_XEN noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -97,6 +97,7 @@ EXTRA_DIST += $(test_scripts) TESTS = virshtest \ nodeinfotest \ statstest \ + seclabeltest \ qparamtest \ $(test_scripts)
@@ -203,6 +204,10 @@ statstest_SOURCES = \ statstest.c testutils.h testutils.c statstest_LDADD = $(LDADDS)
+seclabeltest_SOURCES = \ + seclabeltest.c +seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) + qparamtest_SOURCES = \ qparamtest.c testutils.h testutils.c qparamtest_LDADD = $(LDADDS) diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c new file mode 100644 index 0000000..f068a00 --- /dev/null +++ b/tests/seclabeltest.c @@ -0,0 +1,60 @@ +#include <config.h> + +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <errno.h> +#include "security.h" + +int +main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + int ret; + + const char *doi, *model; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup (&security_drv, "selinux"); + if (ret == -1) + { + fprintf (stderr, "Failed to start security driver"); + exit (-1); + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + model = virSecurityDriverGetModel (security_drv); + if (!model) + { + fprintf (stderr, "Failed to copy secModel model: %s", + strerror (errno)); + exit (-1); + } + + doi = virSecurityDriverGetDOI (security_drv); + if (!doi) + { + fprintf (stderr, "Failed to copy secModel DOI: %s", + strerror (errno)); + exit (-1); + } + + virConnectPtr conn; + conn = virConnectOpen (NULL); + if (conn == NULL) + { + fprintf (stderr, "First virConnectOpen() failed\n"); + exit (1); + } + virSecurityDriverSetDOI (conn, security_drv, "1"); + doi = virSecurityDriverGetDOI (security_drv); + if (strcmp (doi, "1") != 0)
Can you switch that one to STREQ Basically looks good aside from the whitespace bug/makefile change Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 04/01/2009 12:50 PM, Daniel P. Berrange wrote:
On Wed, Apr 01, 2009 at 10:33:56AM -0400, Daniel J Walsh wrote:
Main goal of this patch it to verify data being written to xml and inform the user when he makes a mistake.
Only able to verify static/dynamic in domain_conf.
Added verify code to qemu_driver.c for model and potentially label.
diff --git a/src/Makefile.am b/src/Makefile.am index d5aac11..8d53740 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -170,9 +170,9 @@ STORAGE_HELPER_DISK_SOURCES = \ SECURITY_DRIVER_SOURCES = \ security.h security.c
-SECURITY_DRIVER_SELINUX_SOURCES = \ - security_selinux.h security_selinux.c - +if WITH_SECDRIVER_SELINUX +SECURITY_DRIVER_SOURCES += security_selinux.h security_selinux.c +endif
NODE_DEVICE_DRIVER_SOURCES = \ node_device.c node_device.h @@ -390,9 +390,6 @@ endif libvirt_driver_security_la_SOURCES = $(SECURITY_DRIVER_SOURCES) noinst_LTLIBRARIES += libvirt_driver_security.la libvirt_la_LIBADD += libvirt_driver_security.la -if WITH_SECDRIVER_SELINUX -libvirt_driver_security_la_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) -endif
What's the purpose of this change ?
The reason for keeping an explicit variable name SECURITY_DRIVER_SELINUX_SOURCES was that those sounds need to be added to EXTRA_DIST, even when the compilation of the selinux bits is turned off - so that 'make dist' always gets all files.
diff --git a/src/domain_conf.c b/src/domain_conf.c index 2696449..c9259db 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1859,29 +1859,44 @@ virSecurityLabelDefParseXML(virConnectPtr conn, if (virXPathNode(conn, "./seclabel", ctxt) == NULL) return 0;
+ p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->seclabel.model = p; + p = virXPathStringLimit(conn, "string(./seclabel/@type)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - if ((def->seclabel.type = virDomainSeclabelTypeFromString(p))< 0) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security type")); goto error; + } + def->seclabel.type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); + if (def->seclabel.type< 0) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security type")); + goto error; + }
/* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags& VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit(conn, "string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - def->seclabel.model = p;
p = virXPathStringLimit(conn, "string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("security label is missing")); goto error; + } + def->seclabel.label = p; }
ACK, this looks fine
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a5f9f92..5c88009 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -248,6 +248,7 @@ free_qparam_set;
# security.h +virSecurityDriverVerify; virSecurityDriverStartup; virSecurityDriverInit; virSecurityDriverSetDOI; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b2974bb..cd48193 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2115,6 +2115,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def)< 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -3398,6 +3401,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def)< 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { virDomainObjUnlock(vm); diff --git a/src/security.c b/src/security.c index e2bd20a..72aadf4 100644 --- a/src/security.c +++ b/src/security.c @@ -28,6 +28,25 @@ static virSecurityDriverPtr security_drivers[] = { };
int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def) +{ + unsigned int i; + const virSecurityLabelDefPtr secdef =&def->seclabel; + + if (STREQ(secdef->model, "none")) + return 0; + + for (i = 0; security_drivers[i] != NULL ; i++) { + if (STREQ(security_drivers[i]->name, secdef->model)) { + return security_drivers[i]->domainSecurityVerify(conn, def); + } + }
A few whitespace problems here - HACKING file has some helpful emacs rules to avoid this
+ virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security model")); + return -1; +} + +int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name) { diff --git a/src/security.h b/src/security.h index 8cc2c17..396f47f 100644 --- a/src/security.h +++ b/src/security.h @@ -46,11 +46,14 @@ typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm); +typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn, + virDomainDefPtr def);
struct _virSecurityDriver { const char *name; virSecurityDriverProbe probe; virSecurityDriverOpen open; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainGenLabel domainGenSecurityLabel; @@ -71,6 +74,9 @@ struct _virSecurityDriver { int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name);
+int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def); + void virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 3, 4); diff --git a/src/security_selinux.c b/src/security_selinux.c index 1708d55..7b8e107 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 Red Hat, Inc. + * Copyright (C) 2008,2009 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> * * SELinux security driver. */ @@ -361,6 +364,20 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, }
static int +SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def) +{ + const virSecurityLabelDefPtr secdef =&def->seclabel; + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) { + if (security_check_context(secdef->label) != 0) { + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("Invalid security label %s"), secdef->label); + return -1; + } + } + return 0; +}
Some whitespace problem there too
+ +static int SELinuxSetSecurityLabel(virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm) @@ -406,6 +423,7 @@ virSecurityDriver virSELinuxSecurityDriver = { .name = SECURITY_SELINUX_NAME, .probe = SELinuxSecurityDriverProbe, .open = SELinuxSecurityDriverOpen, + .domainSecurityVerify = SELinuxSecurityVerify, .domainSetSecurityImageLabel = SELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, diff --git a/tests/.gitignore b/tests/.gitignore index 9d809c9..4f33d0b 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -15,6 +15,7 @@ nodedevxml2xmltest nodeinfotest statstest qparamtest +seclabeltest *.gcda *.gcno *.exe diff --git a/tests/Makefile.am b/tests/Makefile.am index 28b2737..48db913 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -54,7 +54,7 @@ EXTRA_DIST = \ nodedevschemadata
noinst_PROGRAMS = virshtest conftest \ - nodeinfotest statstest qparamtest + nodeinfotest statstest qparamtest seclabeltest
if WITH_XEN noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -97,6 +97,7 @@ EXTRA_DIST += $(test_scripts) TESTS = virshtest \ nodeinfotest \ statstest \ + seclabeltest \ qparamtest \ $(test_scripts)
@@ -203,6 +204,10 @@ statstest_SOURCES = \ statstest.c testutils.h testutils.c statstest_LDADD = $(LDADDS)
+seclabeltest_SOURCES = \ + seclabeltest.c +seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) + qparamtest_SOURCES = \ qparamtest.c testutils.h testutils.c qparamtest_LDADD = $(LDADDS) diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c new file mode 100644 index 0000000..f068a00 --- /dev/null +++ b/tests/seclabeltest.c @@ -0,0 +1,60 @@ +#include<config.h> + +#include<unistd.h> +#include<stdlib.h> +#include<stdio.h> +#include<string.h> +#include<errno.h> +#include "security.h" + +int +main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + int ret; + + const char *doi, *model; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup (&security_drv, "selinux"); + if (ret == -1) + { + fprintf (stderr, "Failed to start security driver"); + exit (-1); + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + model = virSecurityDriverGetModel (security_drv); + if (!model) + { + fprintf (stderr, "Failed to copy secModel model: %s", + strerror (errno)); + exit (-1); + } + + doi = virSecurityDriverGetDOI (security_drv); + if (!doi) + { + fprintf (stderr, "Failed to copy secModel DOI: %s", + strerror (errno)); + exit (-1); + } + + virConnectPtr conn; + conn = virConnectOpen (NULL); + if (conn == NULL) + { + fprintf (stderr, "First virConnectOpen() failed\n"); + exit (1); + } + virSecurityDriverSetDOI (conn, security_drv, "1"); + doi = virSecurityDriverGetDOI (security_drv); + if (strcmp (doi, "1") != 0)
Can you switch that one to STREQ
Basically looks good aside from the whitespace bug/makefile change
Regards, Daniel Try again...
Sorry forgot to do the make syntax-check, Fixed Whitespace. Removed Makefile patch. Use STREQ in seclabeltest.
On Wed, Apr 01, 2009 at 02:39:36PM -0400, Daniel J Walsh wrote:
Fixed Whitespace.
Removed Makefile patch.
Use STREQ in seclabeltest.
diff --git a/src/domain_conf.c b/src/domain_conf.c index 3d73414..0efa50f 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1859,29 +1859,44 @@ virSecurityLabelDefParseXML(virConnectPtr conn, if (virXPathNode(conn, "./seclabel", ctxt) == NULL) return 0;
+ p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->seclabel.model = p; + p = virXPathStringLimit(conn, "string(./seclabel/@type)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - if ((def->seclabel.type = virDomainSeclabelTypeFromString(p)) < 0) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security type")); goto error; + } + def->seclabel.type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); + if (def->seclabel.type < 0) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security type")); + goto error; + }
/* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit(conn, "string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - def->seclabel.model = p;
p = virXPathStringLimit(conn, "string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("security label is missing")); goto error; + } + def->seclabel.label = p; }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 78f093c..350a931 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -248,6 +248,7 @@ free_qparam_set;
# security.h +virSecurityDriverVerify; virSecurityDriverStartup; virSecurityDriverInit; virSecurityDriverSetDOI; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index fb857ee..8e0231b 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2115,6 +2115,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def) < 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -3398,6 +3401,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def) < 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { virDomainObjUnlock(vm); diff --git a/src/security.c b/src/security.c index e2bd20a..573895e 100644 --- a/src/security.c +++ b/src/security.c @@ -28,6 +28,25 @@ static virSecurityDriverPtr security_drivers[] = { };
int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def) +{ + unsigned int i; + const virSecurityLabelDefPtr secdef = &def->seclabel; + + if (STREQ(secdef->model, "none")) + return 0; + + for (i = 0; security_drivers[i] != NULL ; i++) { + if (STREQ(security_drivers[i]->name, secdef->model)) { + return security_drivers[i]->domainSecurityVerify(conn, def); + } + } + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security model")); + return -1; +} + +int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name) { diff --git a/src/security.h b/src/security.h index 8cc2c17..05bf88c 100644 --- a/src/security.h +++ b/src/security.h @@ -46,11 +46,14 @@ typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm); +typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn, + virDomainDefPtr def);
struct _virSecurityDriver { const char *name; virSecurityDriverProbe probe; virSecurityDriverOpen open; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainGenLabel domainGenSecurityLabel; @@ -71,6 +74,9 @@ struct _virSecurityDriver { int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name);
+int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def); + void virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 3, 4); diff --git a/src/security_selinux.c b/src/security_selinux.c index 091fe6e..abf975b 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 Red Hat, Inc. + * Copyright (C) 2008,2009 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> * * SELinux security driver. */ @@ -357,6 +360,20 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, }
static int +SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def) +{ + const virSecurityLabelDefPtr secdef = &def->seclabel; + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) { + if (security_check_context(secdef->label) != 0) { + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("Invalid security label %s"), secdef->label); + return -1; + } + } + return 0; +} + +static int SELinuxSetSecurityLabel(virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm) @@ -402,6 +419,7 @@ virSecurityDriver virSELinuxSecurityDriver = { .name = SECURITY_SELINUX_NAME, .probe = SELinuxSecurityDriverProbe, .open = SELinuxSecurityDriverOpen, + .domainSecurityVerify = SELinuxSecurityVerify, .domainSetSecurityImageLabel = SELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, diff --git a/tests/.gitignore b/tests/.gitignore index 9d809c9..4f33d0b 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -15,6 +15,7 @@ nodedevxml2xmltest nodeinfotest statstest qparamtest +seclabeltest *.gcda *.gcno *.exe diff --git a/tests/Makefile.am b/tests/Makefile.am index 28b2737..48db913 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -54,7 +54,7 @@ EXTRA_DIST = \ nodedevschemadata
noinst_PROGRAMS = virshtest conftest \ - nodeinfotest statstest qparamtest + nodeinfotest statstest qparamtest seclabeltest
if WITH_XEN noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -97,6 +97,7 @@ EXTRA_DIST += $(test_scripts) TESTS = virshtest \ nodeinfotest \ statstest \ + seclabeltest \ qparamtest \ $(test_scripts)
@@ -203,6 +204,10 @@ statstest_SOURCES = \ statstest.c testutils.h testutils.c statstest_LDADD = $(LDADDS)
+seclabeltest_SOURCES = \ + seclabeltest.c +seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) + qparamtest_SOURCES = \ qparamtest.c testutils.h testutils.c qparamtest_LDADD = $(LDADDS) diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c new file mode 100644 index 0000000..8805333 --- /dev/null +++ b/tests/seclabeltest.c @@ -0,0 +1,60 @@ +#include <config.h> + +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <errno.h> +#include "security.h" + +int +main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + int ret; + + const char *doi, *model; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup (&security_drv, "selinux"); + if (ret == -1) + { + fprintf (stderr, "Failed to start security driver"); + exit (-1); + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + model = virSecurityDriverGetModel (security_drv); + if (!model) + { + fprintf (stderr, "Failed to copy secModel model: %s", + strerror (errno)); + exit (-1); + } + + doi = virSecurityDriverGetDOI (security_drv); + if (!doi) + { + fprintf (stderr, "Failed to copy secModel DOI: %s", + strerror (errno)); + exit (-1); + } + + virConnectPtr conn; + conn = virConnectOpen (NULL); + if (conn == NULL) + { + fprintf (stderr, "First virConnectOpen() failed\n"); + exit (1); + } + virSecurityDriverSetDOI (conn, security_drv, "1"); + doi = virSecurityDriverGetDOI (security_drv); + if (STREQ(doi, "1")) + { + fprintf (stderr, "Failed to set secModel DOI: %s", strerror (errno)); + exit (1); + } + virConnectClose (conn); + return 0; +}
ACK, this looks good now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Apr 01, 2009 at 02:39:36PM -0400, Daniel J Walsh wrote:
Try again...
Sorry forgot to do the make syntax-check,
Fixed Whitespace.
Removed Makefile patch.
Use STREQ in seclabeltest.
diff --git a/src/domain_conf.c b/src/domain_conf.c index 3d73414..0efa50f 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1859,29 +1859,44 @@ virSecurityLabelDefParseXML(virConnectPtr conn, if (virXPathNode(conn, "./seclabel", ctxt) == NULL) return 0;
+ p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->seclabel.model = p; + p = virXPathStringLimit(conn, "string(./seclabel/@type)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - if ((def->seclabel.type = virDomainSeclabelTypeFromString(p)) < 0) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security type")); goto error; + } + def->seclabel.type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); + if (def->seclabel.type < 0) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security type")); + goto error; + }
/* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit(conn, "string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) - goto error; - def->seclabel.model = p;
p = virXPathStringLimit(conn, "string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("security label is missing")); goto error; + } + def->seclabel.label = p; }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 78f093c..350a931 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -248,6 +248,7 @@ free_qparam_set;
# security.h +virSecurityDriverVerify; virSecurityDriverStartup; virSecurityDriverInit; virSecurityDriverSetDOI; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index fb857ee..8e0231b 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2115,6 +2115,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def) < 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -3398,6 +3401,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
+ if (virSecurityDriverVerify(conn, def) < 0) + goto cleanup; + vm = virDomainFindByName(&driver->domains, def->name); if (vm) { virDomainObjUnlock(vm); diff --git a/src/security.c b/src/security.c index e2bd20a..573895e 100644 --- a/src/security.c +++ b/src/security.c @@ -28,6 +28,25 @@ static virSecurityDriverPtr security_drivers[] = { };
int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def) +{ + unsigned int i; + const virSecurityLabelDefPtr secdef = &def->seclabel; + + if (STREQ(secdef->model, "none")) + return 0; + + for (i = 0; security_drivers[i] != NULL ; i++) { + if (STREQ(security_drivers[i]->name, secdef->model)) { + return security_drivers[i]->domainSecurityVerify(conn, def); + } + } + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("invalid security model")); + return -1; +} + +int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name) { diff --git a/src/security.h b/src/security.h index 8cc2c17..05bf88c 100644 --- a/src/security.h +++ b/src/security.h @@ -46,11 +46,14 @@ typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm); +typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn, + virDomainDefPtr def);
struct _virSecurityDriver { const char *name; virSecurityDriverProbe probe; virSecurityDriverOpen open; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainGenLabel domainGenSecurityLabel; @@ -71,6 +74,9 @@ struct _virSecurityDriver { int virSecurityDriverStartup(virSecurityDriverPtr *drv, const char *name);
+int +virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def); + void virSecurityReportError(virConnectPtr conn, int code, const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 3, 4); diff --git a/src/security_selinux.c b/src/security_selinux.c index 091fe6e..abf975b 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008 Red Hat, Inc. + * Copyright (C) 2008,2009 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> * * SELinux security driver. */ @@ -357,6 +360,20 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, }
static int +SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def) +{ + const virSecurityLabelDefPtr secdef = &def->seclabel; + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) { + if (security_check_context(secdef->label) != 0) { + virSecurityReportError(conn, VIR_ERR_XML_ERROR, + _("Invalid security label %s"), secdef->label); + return -1; + } + } + return 0; +} + +static int SELinuxSetSecurityLabel(virConnectPtr conn, virSecurityDriverPtr drv, virDomainObjPtr vm) @@ -402,6 +419,7 @@ virSecurityDriver virSELinuxSecurityDriver = { .name = SECURITY_SELINUX_NAME, .probe = SELinuxSecurityDriverProbe, .open = SELinuxSecurityDriverOpen, + .domainSecurityVerify = SELinuxSecurityVerify, .domainSetSecurityImageLabel = SELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, diff --git a/tests/.gitignore b/tests/.gitignore index 9d809c9..4f33d0b 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -15,6 +15,7 @@ nodedevxml2xmltest nodeinfotest statstest qparamtest +seclabeltest *.gcda *.gcno *.exe diff --git a/tests/Makefile.am b/tests/Makefile.am index 28b2737..48db913 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -54,7 +54,7 @@ EXTRA_DIST = \ nodedevschemadata
noinst_PROGRAMS = virshtest conftest \ - nodeinfotest statstest qparamtest + nodeinfotest statstest qparamtest seclabeltest
if WITH_XEN noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -97,6 +97,7 @@ EXTRA_DIST += $(test_scripts) TESTS = virshtest \ nodeinfotest \ statstest \ + seclabeltest \ qparamtest \ $(test_scripts)
@@ -203,6 +204,10 @@ statstest_SOURCES = \ statstest.c testutils.h testutils.c statstest_LDADD = $(LDADDS)
+seclabeltest_SOURCES = \ + seclabeltest.c +seclabeltest_LDADD = ../src/libvirt_driver_security.la $(LDADDS) + qparamtest_SOURCES = \ qparamtest.c testutils.h testutils.c qparamtest_LDADD = $(LDADDS) diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c new file mode 100644 index 0000000..8805333 --- /dev/null +++ b/tests/seclabeltest.c @@ -0,0 +1,60 @@ +#include <config.h> + +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <errno.h> +#include "security.h" + +int +main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + int ret; + + const char *doi, *model; + virSecurityDriverPtr security_drv; + + ret = virSecurityDriverStartup (&security_drv, "selinux"); + if (ret == -1) + { + fprintf (stderr, "Failed to start security driver"); + exit (-1); + } + /* No security driver wanted to be enabled: just return */ + if (ret == -2) + return 0; + + model = virSecurityDriverGetModel (security_drv); + if (!model) + { + fprintf (stderr, "Failed to copy secModel model: %s", + strerror (errno)); + exit (-1); + } + + doi = virSecurityDriverGetDOI (security_drv); + if (!doi) + { + fprintf (stderr, "Failed to copy secModel DOI: %s", + strerror (errno)); + exit (-1); + } + + virConnectPtr conn; + conn = virConnectOpen (NULL); + if (conn == NULL) + { + fprintf (stderr, "First virConnectOpen() failed\n"); + exit (1); + } + virSecurityDriverSetDOI (conn, security_drv, "1"); + doi = virSecurityDriverGetDOI (security_drv); + if (STREQ(doi, "1")) + { + fprintf (stderr, "Failed to set secModel DOI: %s", strerror (errno)); + exit (1); + } + virConnectClose (conn); + return 0; +}
I've committed this patch now, but had to change the test case slightly. It is not possible to do a 'virConnectOpen' in the test suite because that will run real hypervisor driver with unpredictable results. So I've removed the code from the 'virConnectOpen' call onwards in the test case. I've got a project working on a integration test suite for libvirt, where we'll be able to test the security drivers in the context of the real functional QEMU driver. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel J Walsh -
Daniel P. Berrange -
Daniel Veillard