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&lt;jmorris(a)namei.org&gt; > + * Dan Walsh&lt;dwalsh(a)redhat.com&gt; > * > * 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.

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(a)namei.org&gt; + * Dan Walsh <dwalsh(a)redhat.com&gt; * * 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; +}