Re: [Qemu-block] [PATCH v2 3/3] iscsi: add support for getting CHAP password via QCryptoSecret API

[Paolo Bonzini]

On 21/12/2015 15:59, Daniel P. Berrange wrote:
> The iSCSI driver currently accepts the CHAP password in plain text
> as a block driver property. This change adds a new "passwordid"
> property that accepts the ID of a QCryptoSecret instance.
> 
>   $QEMU \
>      -object secret,id=sec0,filename=/home/berrange/example.pw \
>      -drive driver=iscsi,url=iscsi://example.com/target-foo/lun1,\
>             user=dan,passwordid=sec0
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  block/iscsi.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index bd1f1bf..96fa3e1 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -39,6 +39,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qmp-commands.h"
>  #include "qapi/qmp/qstring.h"
> +#include "crypto/secret.h"
>  
>  #include <iscsi/iscsi.h>
>  #include <iscsi/scsi-lowlevel.h>
> @@ -1075,6 +1076,8 @@ static void parse_chap(struct iscsi_context *iscsi, 
> const char *target,
>      QemuOpts *opts;
>      const char *user = NULL;
>      const char *password = NULL;
> +    const char *passwordid;
> +    char *secret = NULL;
>  
>      list = qemu_find_opts("iscsi");
>      if (!list) {
> @@ -1094,8 +1097,20 @@ static void parse_chap(struct iscsi_context *iscsi, 
> const char *target,
>          return;
>      }
>  
> +    passwordid = qemu_opt_get(opts, "passwordid");
>      password = qemu_opt_get(opts, "password");
> -    if (!password) {
> +    if (passwordid && password) {
> +        error_setg(errp, "'password' and 'passwordid' properties are "
> +                   "mutually exclusive");
> +        return;
> +    }
> +    if (passwordid) {
> +        secret = qcrypto_secret_lookup_as_utf8(passwordid, errp);

I'm not sure about the UTF-8 part (it should be binary), but I think we
discussed this already.  Apart from this, the patch is okay.

Paolo

> +        if (!secret) {
> +            return;
> +        }
> +        password = secret;
> +    } else if (!password) {
>          error_setg(errp, "CHAP username specified but no password was 
> given");
>          return;
>      }
> @@ -1103,6 +1118,8 @@ static void parse_chap(struct iscsi_context *iscsi, 
> const char *target,
>      if (iscsi_set_initiator_username_pwd(iscsi, user, password)) {
>          error_setg(errp, "Failed to set initiator username and password");
>      }
> +
> +    g_free(secret);
>  }
>  
>  static void parse_header_digest(struct iscsi_context *iscsi, const char 
> *target,
> @@ -1848,6 +1865,11 @@ static QemuOptsList qemu_iscsi_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "password for CHAP authentication to target",
>          },{
> +            .name = "passwordid",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of the secret providing password for CHAP "
> +                    "authentication to target",
> +        },{
>              .name = "header-digest",
>              .type = QEMU_OPT_STRING,
>              .help = "HeaderDigest setting. "
>