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

Re: [libvirt] [PATCH 5/7] Update QEMU driver to support multiple security drivers



On 21.05.2012 15:39, Marcelo Cerri wrote:
> ---
>  src/driver.h            |    8 ++-
>  src/qemu/qemu_conf.c    |   33 ++++++++
>  src/qemu/qemu_conf.h    |    1 +
>  src/qemu/qemu_driver.c  |  196 ++++++++++++++++++++++++++++++++++++++---------
>  src/qemu/qemu_process.c |   53 +++++++++----
>  5 files changed, 236 insertions(+), 55 deletions(-)

Changes to driver.h looks suspicious :) ...

> 
> diff --git a/src/driver.h b/src/driver.h
> index 03d249b..ca4927f 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -305,11 +305,14 @@ typedef int
>                                           int maplen);
>  typedef int
>          (*virDrvDomainGetMaxVcpus)	(virDomainPtr domain);
> -
>  typedef int
> -        (*virDrvDomainGetSecurityLabel)	(virDomainPtr domain,
> +        (*virDrvDomainGetSecurityLabel) (virDomainPtr domain,
>                                           virSecurityLabelPtr seclabel);
>  typedef int
> +        (*virDrvDomainGetSecurityLabelList) (virDomainPtr domain,
> +                                         virSecurityLabelPtr seclabels,
> +                                         int nseclabels);
> +typedef int
>          (*virDrvNodeGetSecurityModel)	(virConnectPtr conn,
>                                           virSecurityModelPtr secmodel);
>  typedef int
> @@ -911,6 +914,7 @@ struct _virDriver {
>      virDrvDomainGetVcpus		domainGetVcpus;
>      virDrvDomainGetMaxVcpus		domainGetMaxVcpus;
>      virDrvDomainGetSecurityLabel     domainGetSecurityLabel;
> +    virDrvDomainGetSecurityLabelList     domainGetSecurityLabelList;
>      virDrvNodeGetSecurityModel  nodeGetSecurityModel;
>      virDrvDomainGetXMLDesc		domainGetXMLDesc;
>      virDrvConnectDomainXMLFromNative domainXMLFromNative;

... but reasonable after all. Although it may be better to save
formatting cleanups for another patch - leaving us with more easier to
understand git bisects in the future.

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 88a04bc..5cc2071 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -202,6 +202,39 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
>          }
>      }
>  
> +    p = virConfGetValue (conf, "additional_security_drivers");
> +    CHECK_TYPE ("additional_security_driver", VIR_CONF_STRING);
> +    if (p && p->str) {
> +        char *it, *tok;
> +        size_t len;
> +
> +        for (len = 1, it = p->str; *it; it++)
> +            len++;
> +        if (VIR_ALLOC_N(driver->additionalSecurityDriverNames, len + 1) < 0) {
> +            virReportOOMError();
> +            virConfFree(conf);
> +            return -1;
> +        }

I'd say drop this ^^ ...

> +
> +        i = 0;
> +        tok = it = p->str;
> +        while (1) {
> +            if (*it == ',' || *it == '\0') {
> +                driver->additionalSecurityDriverNames[i] = strndup(tok, it - tok);

... and expand this array only if needed.

> +                if (driver->additionalSecurityDriverNames == NULL) {
> +                    virReportOOMError();
> +                    virConfFree(conf);
> +                    return -1;
> +                }
> +                tok = it + 1;
> +                i++;
> +            }
> +            if (*it == '\0')
> +                break;
> +            it++;
> +        }
> +    }
> +


>      p = virConfGetValue (conf, "security_default_confined");
>      CHECK_TYPE ("security_default_confined", VIR_CONF_LONG);
>      if (p) driver->securityDefaultConfined = p->l;
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 482e6d3..a5867b6 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -117,6 +117,7 @@ struct qemud_driver {
>      virDomainEventStatePtr domainEventState;
>  
>      char *securityDriverName;
> +    char **additionalSecurityDriverNames;
>      bool securityDefaultConfined;
>      bool securityRequireConfined;
>      virSecurityManagerPtr securityManager;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index efbc421..39d9eee 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -214,36 +214,84 @@ 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;
> +    virSecurityManagerPtr mgr, nested, stack;
> +
> +    /* Create primary driver */
> +    mgr = virSecurityManagerNew(driver->securityDriverName,
> +                                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->additionalSecurityDriverNames) {
> +        stack = virSecurityManagerNewStack(mgr);
> +        if (!stack)
>              goto error;
> +        mgr = stack;
> +    }
> +
> +    /* Loop through additional driver names and add a secudary driver to each
> +     * one */
> +    if (driver->additionalSecurityDriverNames) {
> +        names = driver->additionalSecurityDriverNames;
> +        while (names && *names) {
> +            if (STREQ("dac", *names)) {
> +                /* A DAC driver has specic 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))
> +                goto error;
> +            names++;
> +        }
> +    }
>  
> -        if (!(driver->securityManager = virSecurityManagerNewStack(mgr,
> -                                                                   dac))) {
> -
> -            virSecurityManagerFree(dac);
> -            goto error;
> +    if (driver->privileged) {
> +        /* When a DAC driver is required, check if there is already one in the
> +         * additional drivers */
> +        names = driver->additionalSecurityDriverNames;
> +        while (names && *names) {
> +            if (STREQ("dac", *names)) {
> +               break;
> +            }
> +            names++;
> +        }
> +        /* If there isn't a DAC driver, create a new one and add it to the stack
> +         * manager */
> +        if (names == NULL || *names == NULL) {
> +            nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> +                                              driver->user,
> +                                              driver->group,
> +                                              driver->allowDiskFormatProbing,
> +                                              driver->securityDefaultConfined,
> +                                              driver->securityRequireConfined,
> +                                              driver->dynamicOwnership);
> +            if (nested == NULL)
> +                goto error;
> +            if (virSecurityManagerStackAddNested(stack, nested))
> +                goto error;
>          }
> -    } else {
> -        driver->securityManager = mgr;
>      }
>  
> +    driver->securityManager = mgr;
>      return 0;
>  
>  error:
> @@ -257,7 +305,11 @@ static virCapsPtr
>  qemuCreateCapabilities(virCapsPtr oldcaps,
>                         struct qemud_driver *driver)
>  {
> +    size_t i;
>      virCapsPtr caps;
> +    virSecurityManagerPtr *sec_managers = NULL;
> +    /* Security driver data */
> +    const char *doi, *model;
>  
>      /* Basic host arch / guest machine capabilities */
>      if (!(caps = qemuCapsInit(oldcaps))) {
> @@ -282,26 +334,38 @@ qemuCreateCapabilities(virCapsPtr oldcaps,
>          goto err_exit;
>      }
>  
> -    /* Security driver data */
> -    const char *doi, *model;
> +    /* access sec drivers and create a sec model to each one */
> +    sec_managers = virSecurityManagerGetNested(driver->securityManager);
> +    if (sec_managers == NULL) {
> +        goto err_exit;
> +    }
> +
> +    /* calculate length */
> +    for (i = 0; sec_managers[i]; i++)
> +        ;
> +    caps->host.nsecModels = i;
>  
> -    doi = virSecurityManagerGetDOI(driver->securityManager);
> -    model = virSecurityManagerGetModel(driver->securityManager);
> -    if (STRNEQ(model, "none")) {
> -        if (!(caps->host.secModel.model = strdup(model)))
> +    if (VIR_ALLOC_N(caps->host.secModels, caps->host.nsecModels) < 0)
> +        goto no_memory;
> +
> +    for (i = 0; sec_managers[i]; i++) {
> +        doi = virSecurityManagerGetDOI(sec_managers[i]);
> +        model = virSecurityManagerGetModel(sec_managers[i]);
> +        if (!(caps->host.secModels[i].model = strdup(model)))
>              goto no_memory;
> -        if (!(caps->host.secModel.doi = strdup(doi)))
> +        if (!(caps->host.secModels[i].doi = strdup(doi)))
>              goto no_memory;
> +        VIR_DEBUG("Initialized caps for security driver \"%s\" with "
> +                  "DOI \"%s\"", model, doi);
>      }
> -
> -    VIR_DEBUG("Initialized caps for security driver \"%s\" with "
> -              "DOI \"%s\"", model, doi);
> +    VIR_FREE(sec_managers);
>  
>      return caps;
>  
>  no_memory:
>      virReportOOMError();
>  err_exit:
> +    VIR_FREE(sec_managers);
>      virCapabilitiesFree(caps);
>      return NULL;
>  }
> @@ -3958,6 +4022,63 @@ cleanup:
>      return ret;
>  }
>  
> +static int qemudDomainGetSecurityLabelList(virDomainPtr dom,
> +                                           virSecurityLabelPtr seclabel,
> +                                           int nseclabels)
> +{
> +    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    int i, ret = -1;
> +
> +    qemuDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +

Do we really want to hold driver locked over the whole API?

> +    memset(seclabel, 0, sizeof(*seclabel));
> +
> +    if (!vm) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(dom->uuid, uuidstr);
> +        qemuReportError(VIR_ERR_NO_DOMAIN,
> +                        _("no domain with matching uuid '%s'"), uuidstr);
> +        goto cleanup;
> +    }
> +
> +    if (!virDomainVirtTypeToString(vm->def->virtType)) {
> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("unknown virt type in domain definition '%d'"),
> +                        vm->def->virtType);
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * Check the comment in qemudDomainGetSecurityLabel function.
> +     */
> +    if (virDomainObjIsActive(vm)) {
> +        virSecurityManagerPtr* mgrs = virSecurityManagerGetNested(
> +                                            driver->securityManager);
> +        if (!mgrs)
> +            goto cleanup;
> +
> +        for (i = 0; mgrs[i] && i < nseclabels; i++) {
> +            if (virSecurityManagerGetProcessLabel(mgrs[i], vm->def, vm->pid,
> +                                                  seclabel) < 0) {
> +                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                                "%s", _("Failed to get security label"));
> +                VIR_FREE(mgrs);
> +                goto cleanup;
> +            }
> +        }
> +        VIR_FREE(mgrs);
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    qemuDriverUnlock(driver);
> +    return ret;
> +}
>  static int qemudNodeGetSecurityModel(virConnectPtr conn,
>                                       virSecurityModelPtr secmodel)
>  {
> @@ -3968,12 +4089,12 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn,
>      qemuDriverLock(driver);
>      memset(secmodel, 0, sizeof(*secmodel));
>  
> -    /* NULL indicates no driver, which we treat as
> -     * success, but simply return no data in *secmodel */
> -    if (driver->caps->host.secModel.model == NULL)
> +    /* We treat no driver as success, but simply return no data in *secmodel */
> +    if (driver->caps->host.nsecModels == 0 ||
> +        driver->caps->host.secModels[0].model == NULL)
>          goto cleanup;
>  
> -    p = driver->caps->host.secModel.model;
> +    p = driver->caps->host.secModels[0].model;
>      if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("security model string exceeds max %d bytes"),
> @@ -3983,7 +4104,7 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn,
>      }
>      strcpy(secmodel->model, p);
>  
> -    p = driver->caps->host.secModel.doi;
> +    p = driver->caps->host.secModels[0].doi;
>      if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("security DOI string exceeds max %d bytes"),
> @@ -13004,6 +13125,7 @@ static virDriver qemuDriver = {
>      .domainGetVcpus = qemudDomainGetVcpus, /* 0.4.4 */
>      .domainGetMaxVcpus = qemudDomainGetMaxVcpus, /* 0.4.4 */
>      .domainGetSecurityLabel = qemudDomainGetSecurityLabel, /* 0.6.1 */
> +    .domainGetSecurityLabelList = qemudDomainGetSecurityLabelList, /* ? */
>      .nodeGetSecurityModel = qemudNodeGetSecurityModel, /* 0.6.1 */
>      .domainGetXMLDesc = qemuDomainGetXMLDesc, /* 0.2.0 */
>      .domainXMLFromNative = qemuDomainXMLFromNative, /* 0.6.4 */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 58ba5bf..59c7e0d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3979,12 +3979,12 @@ void qemuProcessStop(struct qemud_driver *driver,
>      virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
>  
>      /* Clear out dynamically assigned labels */
> -    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> -        if (!vm->def->seclabel.baselabel)
> -            VIR_FREE(vm->def->seclabel.model);
> -        VIR_FREE(vm->def->seclabel.label);
> +    for (i = 0; i < vm->def->nseclabels; i++) {
> +        if (vm->def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> +            VIR_FREE(vm->def->seclabels[i]->label);
> +        }
> +        VIR_FREE(vm->def->seclabels[i]->imagelabel);
>      }
> -    VIR_FREE(vm->def->seclabel.imagelabel);
>  
>      virDomainDefClearDeviceAliases(vm->def);
>      if (!priv->persistentAddrs) {
> @@ -4088,6 +4088,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
>                        virDomainChrSourceDefPtr monConfig,
>                        bool monJSON)
>  {
> +    size_t i;
>      char ebuf[1024];
>      int logfile = -1;
>      char *timestamp;
> @@ -4095,6 +4096,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
>      bool running = true;
>      virDomainPausedReason reason;
>      virSecurityLabelPtr seclabel = NULL;
> +    virSecurityLabelDefPtr seclabeldef = NULL;
> +    virSecurityManagerPtr* sec_managers;
> +    const char *model;
>  
>      VIR_DEBUG("Beginning VM attach process");
>  
> @@ -4127,17 +4131,35 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto no_memory;
>  
>      VIR_DEBUG("Detect security driver config");
> -    vm->def->seclabel.type = VIR_DOMAIN_SECLABEL_STATIC;
> -    if (VIR_ALLOC(seclabel) < 0)
> -        goto no_memory;
> -    if (virSecurityManagerGetProcessLabel(driver->securityManager,
> -                                          vm->def, vm->pid, seclabel) < 0)
> +    sec_managers = virSecurityManagerGetNested(driver->securityManager);
> +    if (sec_managers == NULL) {
>          goto cleanup;
> -    if (driver->caps->host.secModel.model &&
> -        !(vm->def->seclabel.model = strdup(driver->caps->host.secModel.model)))
> -        goto no_memory;
> -    if (!(vm->def->seclabel.label = strdup(seclabel->label)))
> -        goto no_memory;
> +    }
> +
> +    for (i = 0; sec_managers[i]; i++) {
> +        model = virSecurityManagerGetModel(sec_managers[i]);
> +        seclabeldef = virDomainDefGetSecurityLabelDef(vm->def, model);
> +        if (seclabeldef == NULL) {
> +            goto cleanup;
> +        }
> +        seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC;
> +        if (VIR_ALLOC(seclabel) < 0)
> +            goto no_memory;
> +        if (virSecurityManagerGetProcessLabel(driver->securityManager,
> +                                              vm->def, vm->pid, seclabel) < 0)
> +            goto cleanup;
> +
> +        //if (driver->caps->host.secModel.model &&
> +        //    !(seclabeldef.model = strdup(driver->caps->host.secModel.model)))
> +        //    goto no_memory;
> +        if (!(seclabeldef->model = strdup(model)))
> +            goto no_memory;
> +
> +        if (!(seclabeldef->label = strdup(seclabel->label)))
> +            goto no_memory;
> +        VIR_FREE(seclabel);
> +        seclabel = NULL;
> +    }
>  
>      VIR_DEBUG("Creating domain log file");
>      if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0)
> @@ -4256,7 +4278,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto cleanup;
>  
>      VIR_FORCE_CLOSE(logfile);
> -    VIR_FREE(seclabel);
>  
>      return 0;
>  


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