[libvirt] [PATCH 06/11] storage: Support "chap" authentication for iscsi pool

[Osier Yang]

On 07/06/13 00:31, John Ferlan wrote:
> On 05/28/2013 02:39 AM, Osier Yang wrote:
>> Though the XML for "chap" authentication with plain "password"
>> was introduced long ago, the function was never implemented.
>> This patch completes it.
>>
>> There are two types of CHAP configurations supported for iSCSI
>> authentication:
>>    * Initiator Authentication
>>      Forward, one-way; The initiator is authenticated by the target.
>>
>>    * Target Authentication
>>      Reverse, Bi-directional, mutual, two-way; The target is authenticated
>>      by the initiator; This method also requires Initiator Authentication
>>
>> This only supports the "Initiator Authentication". (I don't have any
>> enterprise iSCSI env for testing, only have a iSCSI target setup with
>> tgtd, which doesn't support "Target Authentication").
>>
>> "Discovery authentication" is not supported by tgt yet too. So this only
>> setup the session authentication by executing 3 iscsiadm commands, E.g:
>>
>> % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \
>>    "node.session.auth.authmethod" -v "CHAP" --op update
>>
>> % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \
>>    "node.session.auth.username" -v "Jim" --op update
>>
>> % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \
>>    "node.session.auth.password" -v "Jimsecret" --op update
>> ---
>>   src/storage/storage_backend_iscsi.c | 68 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>
>> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
>> index ad38ab2..6a17b5a 100644
>> --- a/src/storage/storage_backend_iscsi.c
>> +++ b/src/storage/storage_backend_iscsi.c
>> @@ -713,6 +713,68 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>       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(virStoragePoolDefPtr def,
>> +                              const char *portal,
> [1] Any reason to not pass portal first like other API's? Not that matters..

No reason, will change. :-)


>
>> +                              const char *target)
>> +{
>> +    if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE)
>> +        return 0;
>> +
>> +    if (def->source.authType != VIR_STORAGE_POOL_AUTH_CHAP) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("iscsi pool only supports 'chap' auth type"));
>> +        return -1;
>> +    }
>> +
>> +    if (virStorageBackendISCSINodeUpdate(portal,
>> +                                         target,
>> +                                         "node.session.auth.authmethod",
>> +                                         "CHAP") < 0 ||
>> +        virStorageBackendISCSINodeUpdate(portal,
>> +                                         target,
>> +                                         "node.session.auth.username",
>> +                                         def->source.auth.chap.login) < 0 ||
>> +        virStorageBackendISCSINodeUpdate(portal,
>> +                                         target,
>> +                                         "node.session.auth.password",
>> +                                         def->source.auth.chap.u.passwd) < 0)
> In 04/11, you added def->source.auth.type setting/checking which I don't
> see here... Or is there some magic happening where auth.u.secret can
> take the place of passwd...

It's handled in next patch. Commit log should be enough to clarify this
patch only handles plain password:

<...>
Though the XML for "chap" authentication with plain "password" was 
introduced long ago, the function was never implemented. This patch 
completes it.
</...>

>
>
>> +        return -1;
>> +
>> +    return 0;
>> +}
>>   
>>   static int
>>   virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>> @@ -745,6 +807,7 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>       if ((session = virStorageBackendISCSISession(pool, 1)) == NULL) {
>>           if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL)
>>               goto cleanup;
>> +
>>           /*
>>            * iscsiadm doesn't let you login to a target, unless you've
>>            * first issued a 'sendtargets' command to the portal :-(
>> @@ -754,6 +817,11 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>                                                 NULL, NULL) < 0)
>>               goto cleanup;
>>   
>> +        if (virStorageBackendISCSISetAuth(pool->def,
>> +                                          portal,
> [1] Why not follow other code which passes portal first...

Will change.

> Also why pass
> pool->def and pool->def->source.devices[0].path - it's not like you
> couldn't grab "source.devices[0].path" from the passed pool->def

Sounds reasonable, I will see if there is requirement for it, if not
will change too.

Osier