[libvirt] Reworked the XML verification patch for svirt
Daniel J Walsh
dwalsh at redhat.com
Wed Apr 1 18:39:36 UTC 2009
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 at namei.org>
>> + * Dan Walsh<dwalsh at 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.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: libvirt-svirt.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20090401/68c2addd/attachment-0001.ksh>
More information about the libvir-list
mailing list