Re: [libvirt] Reworked the XML verification patch for svirt

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