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

Re: [libvirt] [PATCH] Fix configuration of QEMU security drivers



On Thu, Aug 30, 2012 at 01:37:01AM +0100, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> If no 'security_driver' config option was set, then the code
> just loaded the 'dac' security driver. This is a regression
> on previous behaviour, where we would probe for a possible
> security driver. ie default to SELinux if available.
> 
> This changes things so that it 'security_driver' is not set,
> we once again do probing. For simplicity we also always
> create the stack driver, even if there is only one driver
> active.
> 
> The desired semantics are:
> 
>  - security_driver not set
>      -> probe for selinux/apparmour/nop
>      -> auto-add DAC driver
>  - security_driver set to a string
>      -> add that one driver
>      -> auto-add DAC driver
>  - security_driver set to a list
>      -> add all drivers in list
>      -> auto-add DAC driver
> 
> It is not allowed, or possible to specify 'dac' in the
> security_driver config param, since that is always
> enabled.
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  src/qemu/qemu_driver.c          | 135 +++++++++++++---------------------------
>  src/security/security_manager.c |   8 ++-
>  src/security/security_stack.c   |  38 ++++-------
>  src/security/security_stack.h   |   8 ---
>  4 files changed, 61 insertions(+), 128 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9a25253..374349a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -249,119 +249,70 @@ static int
>  qemuSecurityInit(struct qemud_driver *driver)
>  {
>      char **names;
> -    char *primary = NULL;
>      virSecurityManagerPtr mgr = NULL;
> -    virSecurityManagerPtr nested = NULL;
>      virSecurityManagerPtr stack = NULL;
>      bool hasDAC = false;
>  
> -    /* set the name of the primary security driver */
> -    if (driver->securityDriverNames)
> -        primary = driver->securityDriverNames[0];
> -
> -    /* add primary security driver */
> -    if ((primary == NULL && driver->privileged) ||
> -        STREQ_NULLABLE(primary, "dac")) {
> -        if (!driver->privileged) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("DAC security driver usable only when "
> -                             "running privileged (as root)"));
> -            goto error;
> -        }
> -
> -        mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> -                                       driver->user,
> -                                       driver->group,
> -                                       driver->allowDiskFormatProbing,
> -                                       driver->securityDefaultConfined,
> -                                       driver->securityRequireConfined,
> -                                       driver->dynamicOwnership);
> -        hasDAC = true;
> -    } else {
> -        mgr = virSecurityManagerNew(primary,
> -                                    QEMU_DRIVER_NAME,
> -                                    driver->allowDiskFormatProbing,
> -                                    driver->securityDefaultConfined,
> -                                    driver->securityRequireConfined);
> -    }
> -
> -    if (!mgr)
> -        goto error;
> -
> -    /* We need a stack to group the security drivers if:
> -     * - additional drivers are provived in configuration
> -     * - the primary driver isn't DAC and we are running privileged
> -     */
> -    if ((driver->privileged && !hasDAC) ||
> -        (driver->securityDriverNames && driver->securityDriverNames[1])) {
> -        if (!(stack = virSecurityManagerNewStack(mgr)))
> -            goto error;
> -        mgr = stack;
> -    }
> -
> -    /* Loop through additional driver names and add them as nested */
>      if (driver->securityDriverNames) {
> -        names = driver->securityDriverNames + 1;
> +        names = driver->securityDriverNames;
>          while (names && *names) {
> -            if (STREQ("dac", *names)) {
> -                /* A DAC driver has specific parameters */
> -                if (!driver->privileged) {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                                   _("DAC security driver usable only when "
> -                                     "running privileged (as root)"));
> -                    goto error;
> -                }
> -
> -                nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> -                                                  driver->user,
> -                                                  driver->group,
> -                                                  driver->allowDiskFormatProbing,
> -                                                  driver->securityDefaultConfined,
> -                                                  driver->securityRequireConfined,
> -                                                  driver->dynamicOwnership);
> +            if (STREQ("dac", *names))
>                  hasDAC = true;
> -            } else {
> -                nested = virSecurityManagerNew(*names,
> -                                               QEMU_DRIVER_NAME,
> -                                               driver->allowDiskFormatProbing,
> -                                               driver->securityDefaultConfined,
> -                                               driver->securityRequireConfined);
> -            }
> -
> -            if (!nested)
> -                goto error;
>  
> -            if (virSecurityManagerStackAddNested(stack, nested))
> +            if (!(mgr = virSecurityManagerNew(*names,
> +                                              QEMU_DRIVER_NAME,
> +                                              driver->allowDiskFormatProbing,
> +                                              driver->securityDefaultConfined,
> +                                              driver->securityRequireConfined)))
>                  goto error;
> -
> -            nested = NULL;
> +            if (!stack) {
> +                if (!(stack = virSecurityManagerNewStack(mgr)))
> +                    goto error;
> +            } else {
> +                if (virSecurityManagerStackAddNested(stack, mgr) < 0)
> +                    goto error;
> +            }
> +            mgr = NULL;
>              names++;
>          }
> -    }
> -
> -    if (driver->privileged && !hasDAC) {
> -        if (!(nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> -                                                driver->user,
> -                                                driver->group,
> -                                                driver->allowDiskFormatProbing,
> -                                                driver->securityDefaultConfined,
> -                                                driver->securityRequireConfined,
> -                                                driver->dynamicOwnership)))
> +    } else {
> +        if (!(mgr = virSecurityManagerNew(NULL,
> +                                          QEMU_DRIVER_NAME,
> +                                          driver->allowDiskFormatProbing,
> +                                          driver->securityDefaultConfined,
> +                                          driver->securityRequireConfined)))
>              goto error;
> -
> -        if (virSecurityManagerStackAddNested(stack, nested))
> +        if (!(stack = virSecurityManagerNewStack(mgr)))
>              goto error;
> +        mgr = NULL;
> +    }
>  
> -        nested = NULL;
> +    if (!hasDAC && driver->privileged) {
> +        if (!(mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> +                                             driver->user,
> +                                             driver->group,
> +                                             driver->allowDiskFormatProbing,
> +                                             driver->securityDefaultConfined,
> +                                             driver->securityRequireConfined,
> +                                             driver->dynamicOwnership)))
> +            goto error;
> +        if (!stack) {
> +            if (!(stack = virSecurityManagerNewStack(mgr)))
> +                goto error;
> +        } else {
> +            if (virSecurityManagerStackAddNested(stack, mgr) < 0)
> +                goto error;
> +        }
> +        mgr = NULL;
>      }
>  
> -    driver->securityManager = mgr;
> +    driver->securityManager = stack;
>      return 0;
>  
>  error:
>      VIR_ERROR(_("Failed to initialize security drivers"));
> +    virSecurityManagerFree(stack);
>      virSecurityManagerFree(mgr);
> -    virSecurityManagerFree(nested);
>      return -1;
>  }
>  
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 0e106d5..367f7ad 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -49,6 +49,12 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr
>  {
>      virSecurityManagerPtr mgr;
>  
> +    VIR_DEBUG("drv=%p (%s) virtDriver=%s allowDiskFormatProbing=%d "
> +              "defaultConfined=%d requireConfined=%d",
> +              drv, drv->name, virtDriver,
> +              allowDiskFormatProbing, defaultConfined,
> +              requireConfined);
> +
>      if (VIR_ALLOC_VAR(mgr, char, drv->privateDataLen) < 0) {
>          virReportOOMError();
>          return NULL;
> @@ -80,7 +86,7 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary)
>      if (!mgr)
>          return NULL;
>  
> -    virSecurityStackAddPrimary(mgr, primary);
> +    virSecurityStackAddNested(mgr, primary);
>  
>      return mgr;
>  }
> diff --git a/src/security/security_stack.c b/src/security/security_stack.c
> index 7dcd626..0eb7e76 100644
> --- a/src/security/security_stack.c
> +++ b/src/security/security_stack.c
> @@ -38,35 +38,31 @@ struct _virSecurityStackItem {
>  };
>  
>  struct _virSecurityStackData {
> -    virSecurityManagerPtr primary;
>      virSecurityStackItemPtr itemsHead;
>  };
>  
>  int
> -virSecurityStackAddPrimary(virSecurityManagerPtr mgr,
> -                           virSecurityManagerPtr primary)
> -{
> -    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> -    if (virSecurityStackAddNested(mgr, primary) < 0)
> -        return -1;
> -    priv->primary = primary;
> -    return 0;
> -}
> -
> -int
>  virSecurityStackAddNested(virSecurityManagerPtr mgr,
>                            virSecurityManagerPtr nested)
>  {
>      virSecurityStackItemPtr item = NULL;
>      virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr tmp;
> +
> +    tmp = priv->itemsHead;
> +    while (tmp && tmp->next)
> +        tmp = tmp->next;
>  
>      if (VIR_ALLOC(item) < 0) {
>          virReportOOMError();
>          return -1;
>      }
>      item->securityManager = nested;
> -    item->next = priv->itemsHead;
> -    priv->itemsHead = item;
> +    if (tmp)
> +        tmp->next = item;
> +    else
> +        priv->itemsHead = item;
> +
>      return 0;
>  }
>  
> @@ -74,19 +70,7 @@ virSecurityManagerPtr
>  virSecurityStackGetPrimary(virSecurityManagerPtr mgr)
>  {
>      virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> -    return (priv->primary) ? priv->primary : priv->itemsHead->securityManager;
> -}
> -
> -void virSecurityStackSetPrimary(virSecurityManagerPtr mgr,
> -                                virSecurityManagerPtr primary)
> -{
> -    virSecurityStackAddPrimary(mgr, primary);
> -}
> -
> -void virSecurityStackSetSecondary(virSecurityManagerPtr mgr,
> -                                  virSecurityManagerPtr secondary)
> -{
> -    virSecurityStackAddNested(mgr, secondary);
> +    return priv->itemsHead->securityManager;
>  }
>  
>  static virSecurityDriverStatus
> diff --git a/src/security/security_stack.h b/src/security/security_stack.h
> index 6898c03..5bb3be7 100644
> --- a/src/security/security_stack.h
> +++ b/src/security/security_stack.h
> @@ -27,19 +27,11 @@ extern virSecurityDriver virSecurityDriverStack;
>  
>  
>  int
> -virSecurityStackAddPrimary(virSecurityManagerPtr mgr,
> -                           virSecurityManagerPtr primary);
> -int
>  virSecurityStackAddNested(virSecurityManagerPtr mgr,
>                            virSecurityManagerPtr nested);
>  virSecurityManagerPtr
>  virSecurityStackGetPrimary(virSecurityManagerPtr mgr);
>  
> -void virSecurityStackSetPrimary(virSecurityManagerPtr mgr,
> -                                virSecurityManagerPtr primary);
> -void virSecurityStackSetSecondary(virSecurityManagerPtr mgr,
> -                                  virSecurityManagerPtr secondary);
> -
>  virSecurityManagerPtr*
>  virSecurityStackGetNested(virSecurityManagerPtr mgr);

  ACK, tested, works for me, so pushed !

    thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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