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

[John Ferlan]

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..

> +                              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...


> +        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... 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

John

> +                                          pool->def->source.devices[0].path) < 0)
> +            goto cleanup;
> +
>          if (virStorageBackendISCSIConnection(portal,
>                                               pool->def->source.initiator.iqn,
>                                               pool->def->source.devices[0].path,
>