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@xxxxxxxxx>
> + * Dan Walsh <dwalsh@xxxxxxxxxx>
> *
> * 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list