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

Re: [libvirt] [PATCH v2 6/7] storage: Support "chap" authentication for iscsi pool



On 07/15/2013 10:16 AM, Osier Yang wrote:
> On 10/07/13 03:10, John Ferlan wrote:
>> From: Osier Yang <jyang redhat com>

<...snip...>

>> diff --git a/src/storage/storage_backend_iscsi.c
>> b/src/storage/storage_backend_iscsi.c
>> index 0a4cd22..673ca16 100644
>> --- a/src/storage/storage_backend_iscsi.c
>> +++ b/src/storage/storage_backend_iscsi.c
>> @@ -32,6 +32,8 @@
>>   #include <unistd.h>
>>   #include <sys/stat.h>
>>   +#include "datatypes.h"
>> +#include "driver.h"
>>   #include "virerror.h"
>>   #include "storage_backend_scsi.h"
>>   #include "storage_backend_iscsi.h"
>> @@ -39,6 +41,7 @@
>>   #include "virlog.h"
>>   #include "virfile.h"
>>   #include "vircommand.h"
>> +#include "virobject.h"
>>   #include "virrandom.h"
>>   #include "virstring.h"
>>   @@ -545,9 +548,112 @@ cleanup:
>>       return ret;
>>   }
>>   +static int
>> +virStorageBackendISCSINodeUpdate(const char *portal,
>> +                                 const char *target,
>> +                                 const char *name,
>> +                                 const char *value)
>> +{
>> +     virCommandPtr cmd = NULL;
>> +     int status;
>> +     int ret = -1;
>> +
>> +     cmd = virCommandNewArgList(ISCSIADM,
>> +                                "--mode", "node",
>> +                                "--portal", portal,
>> +                                "--target", target,
>> +                                "--op", "update",
>> +                                "--name", name,
>> +                                "--value", value,
>> +                                NULL);
>> +
>> +    if (virCommandRun(cmd, &status) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Failed to update '%s' of node mode for
>> target '%s'"),
>> +                       name, target);
>> +        goto cleanup;
>> +    }
>> +
>> +    ret = 0;
>> +cleanup:
>> +    virCommandFree(cmd);
>> +    return ret;
>> +}
>> +
>> +static int
>> +virStorageBackendISCSISetAuth(const char *portal,
>> +                              virConnectPtr conn,
>> +                              virStoragePoolSourcePtr source)
>> +{
>> +    virSecretPtr secret = NULL;
>> +    unsigned char *secret_value = NULL;
>> +    const char *passwd = NULL;
>> +    virStoragePoolAuthChap chap;
>> +    int ret = -1;
>> +
>> +    if (source->authType == VIR_STORAGE_POOL_AUTH_NONE)
>> +        return 0;
>> +
>> +    if (source->authType != VIR_STORAGE_POOL_AUTH_CHAP) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("iscsi pool only supports 'chap' auth type"));
>> +        return -1;
>> +    }
>> +
>> +    chap = source->auth.chap;
>> +    if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET) {
>> +        if (chap.u.secret.uuidUsable)
>> +            secret = virSecretLookupByUUID(conn, chap.u.secret.uuid);
>> +        else
>> +            secret = virSecretLookupByUsage(conn,
>> VIR_SECRET_USAGE_TYPE_ISCSI,
>> +                                            chap.u.secret.usage);
>> +
>> +        if (secret) {
>> +            size_t secret_size;
>> +            secret_value =
>> +                conn->secretDriver->secretGetValue(secret,
>> &secret_size, 0,
>> +                                                  
>> VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>> +            if (!secret_value) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("could not get the value of the
>> secret "
>> +                                 "for username %s"), chap.login);
>> +                goto cleanup;
>> +            }
>> +        } else {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("username '%s' specified but secret not
>> found"),
>> +                           chap.login);
>> +            goto cleanup;
>> +        }
>> +        passwd = (const char *)secret_value;
>> +    } else if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) {
>> +        passwd = chap.u.passwd;
>> +    }
>> +
>> +    if (virStorageBackendISCSINodeUpdate(portal,
>> +                                         source->devices[0].path,
>> +                                         "node.session.auth.authmethod",
>> +                                         "CHAP") < 0 ||
>> +        virStorageBackendISCSINodeUpdate(portal,
>> +                                         source->devices[0].path,
>> +                                         "node.session.auth.username",
>> +                                         source->auth.chap.login) < 0 ||
>> +        virStorageBackendISCSINodeUpdate(portal,
>> +                                         source->devices[0].path,
>> +                                         "node.session.auth.password",
>> +                                         passwd) < 0)
>> +        goto cleanup;
>> +
>> +    ret = 0;
>> +
>> +cleanup:
>> +    virObjectUnref(secret);
>> +    VIR_FREE(secret_value);
>> +    return ret;
>> +}
>>     static char *
>> -virStorageBackendISCSIFindPoolSources(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>> +virStorageBackendISCSIFindPoolSources(virConnectPtr conn,
>>                                         const char *srcSpec,
>>                                         unsigned int flags)
>>   {
>> @@ -590,6 +696,9 @@
>> virStorageBackendISCSIFindPoolSources(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>>                                             &ntargets, &targets) < 0)
>>           goto cleanup;
>>   +    if (virStorageBackendISCSISetAuth(portal, conn, source) < 0)
>> +        goto cleanup;
>> +
> 
> i think the chap auth iscsi pool won't  be able to start with this change,
> findPoolSources is an api for discovering the pool sources. however,
> we want the chap auth are set up before connecting to the iscsi target
> when starting the pool, otherwise the pool starting will fail on
> authentication.
> note that startPool and findPoolSources are independant apis, they don't
> invoke each other.
> 
> with this change, if one wants to start the pool successfully, he has to
> invoke the findPoolSources first, this dependancy is incorrect. as an
> example, in virsh layer, one has to execute find-storage-pool-sources
> or find-storage-pool-sources-as before pool-start.
> 
> as an alternative way to have the non-null 'conn' for startPool, we can
> create a connection in storageDriverAutostart (like qemu driver does),
> which is the only place pass an null 'conn' to startPool,

While there is a v3 posted - this code hasn't differed there, so I'll
just use this opportunity to ask you questions...

A 'conn' to what? Should we assume qemu like nwfilter_driver.c does?

    if (!driverState->privileged)
        return 0;

   conn = virConnectOpen("qemu:///system");

Do we further restrict the StartPool code to ensure there is a
privileged qemu connection?

This was the decision point I had to make...


John

> 
> regards
> osier


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