[libvirt] [PATCH v2 4/5] Support for multiple default security drivers in QEMU config

[Michal Privoznik]

On 18.07.2012 03:28, Marcelo Cerri wrote:
> This patch replaces the key "security_driver" in QEMU config by
> "security_drivers", which accepts a list of default drivers. If
> "security_drivers" can't be found, libvirt will use "security_driver" to
> ensure that it will remain compatible with older version of the config
> file.
> ---
>  src/qemu/libvirtd_qemu.aug         |    2 +-
>  src/qemu/qemu.conf                 |    2 +-
>  src/qemu/qemu_conf.c               |   42 ++++++++++++++-
>  src/qemu/qemu_conf.h               |    2 +-
>  src/qemu/qemu_driver.c             |   97 ++++++++++++++++++++++++++++--------
>  src/qemu/test_libvirtd_qemu.aug.in |    2 +-
>  6 files changed, 119 insertions(+), 28 deletions(-)
> 
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index 683aadb..fab97d7 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -39,7 +39,7 @@ module Libvirtd_qemu =
>                   | str_entry  "spice_tls_x509_cert_dir"
>                   | str_entry "spice_password"
>  
> -   let security_entry = str_entry "security_driver"
> +   let security_entry = str_entry "security_drivers"
>                   | bool_entry "security_default_confined"
>                   | bool_entry "security_require_confined"
>                   | str_entry "user"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index ed4683c..ffb03f8 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -146,7 +146,7 @@
>  # leaving SELinux enabled for the host in general, then set this
>  # to 'none' instead.
>  #
> -#security_driver = "selinux"
> +#security_drivers = "selinux"

No, we can't do that. Changing this would silently discard
any values set by users. Moreover, separating values by comma -
- that's yet another way for multiple values passing. 
Can't we just re-use:

  cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuset", "cpuacct" ]

that is making these two syntactically and semantically correct:

  security_driver = "selinux"
  security_driver = [ "selinux", "apparmor", "YetAnotherSecurtyDriver" ]

Having said that - the rest of the patch is pointless to review and need rework after we settle down on design.
But I'll point out one obvious error anyway.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1b02f28..f01566b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -220,36 +220,91 @@ qemuAutostartDomains(struct qemud_driver *driver)
>  static int
>  qemuSecurityInit(struct qemud_driver *driver)
>  {
> -    virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName,
> -                                                      QEMU_DRIVER_NAME,
> -                                                      driver->allowDiskFormatProbing,
> -                                                      driver->securityDefaultConfined,
> -                                                      driver->securityRequireConfined);
> +    char **names;
> +    char *primary;
> +    virSecurityManagerPtr mgr, nested, stack;

Stack may be use uninitialized ...

>  
> +    if (driver->securityDriverNames == NULL)
> +        primary = NULL;
> +    else
> +        primary = driver->securityDriverNames[0];
> +
> +    /* Create primary driver */
> +    mgr = virSecurityManagerNew(primary,
> +                                QEMU_DRIVER_NAME,
> +                                driver->allowDiskFormatProbing,
> +                                driver->securityDefaultConfined,
> +                                driver->securityRequireConfined);
>      if (!mgr)
>          goto error;
>  
> -    if (driver->privileged) {
> -        virSecurityManagerPtr dac = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> -                                                             driver->user,
> -                                                             driver->group,
> -                                                             driver->allowDiskFormatProbing,
> -                                                             driver->securityDefaultConfined,
> -                                                             driver->securityRequireConfined,
> -                                                             driver->dynamicOwnership);
> -        if (!dac)
> +    /* If a DAC driver is required or additional drivers are provived, a stack
> +     * driver should be create to group them all */
> +    if (driver->privileged ||
> +        (driver->securityDriverNames && driver->securityDriverNames[1])) {
> +        stack = virSecurityManagerNewStack(mgr);
> +        if (!stack)
>              goto error;
> +        mgr = stack;
> +    }
> +
> +    /* Loop through additional driver names and add a secudary driver to each
> +     * one */
> +    if (driver->securityDriverNames) {
> +        names = driver->securityDriverNames + 1;
> +        while (names && *names) {
> +            if (STREQ("dac", *names)) {
> +                /* A DAC driver has specific parameters */
> +                nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> +                                                  driver->user,
> +                                                  driver->group,
> +                                                  driver->allowDiskFormatProbing,
> +                                                  driver->securityDefaultConfined,
> +                                                  driver->securityRequireConfined,
> +                                                  driver->dynamicOwnership);
> +            } else {
> +                nested = virSecurityManagerNew(*names,
> +                                               QEMU_DRIVER_NAME,
> +                                               driver->allowDiskFormatProbing,
> +                                               driver->securityDefaultConfined,
> +                                               driver->securityRequireConfined);
> +            }
> +            if (nested == NULL)
> +                goto error;
> +            if (virSecurityManagerStackAddNested(stack, nested))

... here.

> +                goto error;
> +            names++;
> +        }
> +    }


Michal