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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]