[Qemu-devel] [PATCH v2 0/3] Use QCryptoSecret for block device passwords

[Qemu-devel] [PATCH v2 0/3] Use QCryptoSecret for block device passwords

[Daniel P. Berrange]

The RBD, Curl and iSCSI block device drivers all need the ability
to accept a password to authenticate with the remote network storage
server. Currently RBD and iSCSI both just take the password in clear
text as part of the block parameters which is insecure (passwords are
visible in the process listing), while Curl doesn't support auth at
all.

This series updates all three drivers so that they use the recently
merged QCryptoSecret API for getting passwords. Each driver gains
a 'passwordid' property that can be set to provide the ID of a
QCryptoSecret object instance, which in turn provides the actual
password data.

These patches were previously sent as part of a larger series:

  https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html

The QCryptoSecret object support was already merged, and the additions
to qemu-img/qemu-io/qemu-nbd I'll submit separately since there is no
strict dependancy between those additions and these patches.

Daniel P. Berrange (3):
  rbd: add support for getting password from QCryptoSecret object
  curl: add support for HTTP authentication parameters
  iscsi: add support for getting CHAP password via QCryptoSecret API

 block/curl.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/iscsi.c | 24 +++++++++++++++++++++-
 block/rbd.c   | 47 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+), 1 deletion(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/3] rbd: add support for getting password from QCryptoSecret object

[Daniel P. Berrange]

Currently RBD passwords must be provided on the command line
via

  $QEMU -drive file=rbd:pool/image:id=myname:\
               key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
               auth_supported=cephx

This is insecure because the key is visible in the OS process
listing.

This adds support for an 'passwordid' parameter in the RBD
parameters that can be used with the QCryptoSecret object to
provide the password via a file:

  echo "QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=" > poolkey.b64
  $QEMU -object secret,id=secret0,file=poolkey.b64,format=base64 \
        -drive driver=rbd,filename=rbd:pool/image:id=myname:\
               auth_supported=cephx,passwordid=secret0

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/rbd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index a60a19d..dc4db0a 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -16,6 +16,7 @@
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
+#include "crypto/secret.h"
 
 #include <rbd/librbd.h>
 
@@ -228,6 +229,27 @@ static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
     return NULL;
 }
 
+
+static int qemu_rbd_set_auth(rados_t cluster, const char *passwordid,
+                             Error **errp)
+{
+    if (passwordid == 0) {
+        return 0;
+    }
+
+    gchar *secret = qcrypto_secret_lookup_as_base64(passwordid,
+                                                    errp);
+    if (!secret) {
+        return -1;
+    }
+
+    rados_conf_set(cluster, "key", secret);
+    g_free(secret);
+
+    return 0;
+}
+
+
 static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
                              bool only_read_conf_file,
                              Error **errp)
@@ -299,10 +321,13 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     char conf[RBD_MAX_CONF_SIZE];
     char clientname_buf[RBD_MAX_CONF_SIZE];
     char *clientname;
+    const char *passwordid;
     rados_t cluster;
     rados_ioctx_t io_ctx;
     int ret;
 
+    passwordid = qemu_opt_get(opts, "passwordid");
+
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
                            snap_buf, sizeof(snap_buf),
                            name, sizeof(name),
@@ -350,6 +375,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         return -EIO;
     }
 
+    if (qemu_rbd_set_auth(cluster, passwordid, errp) < 0) {
+        rados_shutdown(cluster);
+        return -EIO;
+    }
+
     if (rados_connect(cluster) < 0) {
         error_setg(errp, "error connecting");
         rados_shutdown(cluster);
@@ -423,6 +453,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Specification of the rbd image",
         },
+        {
+            .name = "passwordid",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret providing the password",
+        },
         { /* end of list */ }
     },
 };
@@ -436,6 +471,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     char conf[RBD_MAX_CONF_SIZE];
     char clientname_buf[RBD_MAX_CONF_SIZE];
     char *clientname;
+    const char *passwordid;
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *filename;
@@ -450,6 +486,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     filename = qemu_opt_get(opts, "filename");
+    passwordid = qemu_opt_get(opts, "passwordid");
 
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
                            snap_buf, sizeof(snap_buf),
@@ -488,6 +525,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
+    if (qemu_rbd_set_auth(s->cluster, passwordid, errp) < 0) {
+        r = -EIO;
+        goto failed_shutdown;
+    }
+
     /*
      * Fallback to more conservative semantics if setting cache
      * options fails. Ignore errors from setting rbd_cache because the
@@ -919,6 +961,11 @@ static QemuOptsList qemu_rbd_create_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "RBD object size"
         },
+        {
+            .name = "passwordid",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret providing the password",
+        },
         { /* end of list */ }
     }
 };
-- 
2.5.0

[Qemu-devel] [PATCH v2 2/3] curl: add support for HTTP authentication parameters

[Daniel P. Berrange]

If connecting to a web server which has authentication
turned on, QEMU gets a 401 as curl has not been configured
with any authentication credentials.

This adds 4 new parameters to the curl block driver
options, username, passwordid, proxyusername and
proxypasswordid. Passwords are provided using the
recently added 'secret' object type

 $QEMU \
     -object secret,id=sec0,filename=/home/berrange/example.pw \
     -object secret,id=sec1,filename=/home/berrange/proxy.pw \
     -drive driver=http,url=http://example.com/some.img,\
            username=dan,passwordid=sec0,\
            proxyusername=dan,passwordid=sec1

Of course it is possible to use the same secret for both the
proxy & server passwords if desired, or omit the proxy auth
details, or the server auth details as required.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/curl.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 8994182..0552abf 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -26,6 +26,7 @@
 #include "block/block_int.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qstring.h"
+#include "crypto/secret.h"
 #include <curl/curl.h>
 
 // #define DEBUG_CURL
@@ -77,6 +78,10 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
 #define CURL_BLOCK_OPT_TIMEOUT "timeout"
 #define CURL_BLOCK_OPT_COOKIE    "cookie"
+#define CURL_BLOCK_OPT_USERNAME "username"
+#define CURL_BLOCK_OPT_PASSWORDID "passwordid"
+#define CURL_BLOCK_OPT_PROXY_USERNAME "proxyusername"
+#define CURL_BLOCK_OPT_PROXY_PASSWORDID "proxypasswordid"
 
 struct BDRVCURLState;
 
@@ -119,6 +124,10 @@ typedef struct BDRVCURLState {
     char *cookie;
     bool accept_range;
     AioContext *aio_context;
+    char *username;
+    char *password;
+    char *proxyusername;
+    char *proxypassword;
 } BDRVCURLState;
 
 static void curl_clean_state(CURLState *s);
@@ -418,6 +427,21 @@ static CURLState *curl_init_state(BlockDriverState *bs, BDRVCURLState *s)
         curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg);
         curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1);
 
+        if (s->username) {
+            curl_easy_setopt(state->curl, CURLOPT_USERNAME, s->username);
+        }
+        if (s->password) {
+            curl_easy_setopt(state->curl, CURLOPT_PASSWORD, s->password);
+        }
+        if (s->proxyusername) {
+            curl_easy_setopt(state->curl,
+                             CURLOPT_PROXYUSERNAME, s->proxyusername);
+        }
+        if (s->proxypassword) {
+            curl_easy_setopt(state->curl,
+                             CURLOPT_PROXYPASSWORD, s->proxypassword);
+        }
+
         /* Restrict supported protocols to avoid security issues in the more
          * obscure protocols.  For example, do not allow POP3/SMTP/IMAP see
          * CVE-2013-0249.
@@ -524,10 +548,31 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Pass the cookie or list of cookies with each request"
         },
+        {
+            .name = CURL_BLOCK_OPT_USERNAME,
+            .type = QEMU_OPT_STRING,
+            .help = "Username for HTTP auth"
+        },
+        {
+            .name = CURL_BLOCK_OPT_PASSWORDID,
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret used as password for HTTP auth",
+        },
+        {
+            .name = CURL_BLOCK_OPT_PROXY_USERNAME,
+            .type = QEMU_OPT_STRING,
+            .help = "Username for HTTP proxy auth"
+        },
+        {
+            .name = CURL_BLOCK_OPT_PROXY_PASSWORDID,
+            .type = QEMU_OPT_STRING,
+            .help = "ID of secret used as password for HTTP proxy auth",
+        },
         { /* end of list */ }
     },
 };
 
+
 static int curl_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
@@ -538,6 +583,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     const char *file;
     const char *cookie;
     double d;
+    const char *passwordid;
 
     static int inited = 0;
 
@@ -579,6 +625,26 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
         goto out_noclean;
     }
 
+    s->username = g_strdup(qemu_opt_get(opts, CURL_BLOCK_OPT_USERNAME));
+    passwordid = qemu_opt_get(opts, CURL_BLOCK_OPT_PASSWORDID);
+
+    if (passwordid) {
+        s->password = qcrypto_secret_lookup_as_utf8(passwordid, errp);
+        if (!s->password) {
+            goto out_noclean;
+        }
+    }
+
+    s->proxyusername = g_strdup(
+        qemu_opt_get(opts, CURL_BLOCK_OPT_PROXY_USERNAME));
+    passwordid = qemu_opt_get(opts, CURL_BLOCK_OPT_PROXY_PASSWORDID);
+    if (passwordid) {
+        s->proxypassword = qcrypto_secret_lookup_as_utf8(passwordid, errp);
+        if (!s->proxypassword) {
+            goto out_noclean;
+        }
+    }
+
     if (!inited) {
         curl_global_init(CURL_GLOBAL_ALL);
         inited = 1;
-- 
2.5.0

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

[Daniel P. Berrange]

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 <berrange@redhat.com>
---
 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);
+        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. "
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 1/3] rbd: add support for getting password from QCryptoSecret object

[Josh Durgin]

On 12/21/2015 06:59 AM, Daniel P. Berrange wrote:
> Currently RBD passwords must be provided on the command line
> via
>
>    $QEMU -drive file=rbd:pool/image:id=myname:\
>                 key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
>                 auth_supported=cephx
>
> This is insecure because the key is visible in the OS process
> listing.
>
> This adds support for an 'passwordid' parameter in the RBD
> parameters that can be used with the QCryptoSecret object to
> provide the password via a file:
>
>    echo "QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=" > poolkey.b64
>    $QEMU -object secret,id=secret0,file=poolkey.b64,format=base64 \
>          -drive driver=rbd,filename=rbd:pool/image:id=myname:\
>                 auth_supported=cephx,passwordid=secret0
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Looks good to me, thanks!

Reviewed-by: Josh Durgin <jdurgin@redhat.com>

> ---
>   block/rbd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index a60a19d..dc4db0a 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -16,6 +16,7 @@
>   #include "qemu-common.h"
>   #include "qemu/error-report.h"
>   #include "block/block_int.h"
> +#include "crypto/secret.h"
>
>   #include <rbd/librbd.h>
>
> @@ -228,6 +229,27 @@ static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
>       return NULL;
>   }
>
> +
> +static int qemu_rbd_set_auth(rados_t cluster, const char *passwordid,
> +                             Error **errp)
> +{
> +    if (passwordid == 0) {
> +        return 0;
> +    }
> +
> +    gchar *secret = qcrypto_secret_lookup_as_base64(passwordid,
> +                                                    errp);
> +    if (!secret) {
> +        return -1;
> +    }
> +
> +    rados_conf_set(cluster, "key", secret);
> +    g_free(secret);
> +
> +    return 0;
> +}
> +
> +
>   static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
>                                bool only_read_conf_file,
>                                Error **errp)
> @@ -299,10 +321,13 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>       char conf[RBD_MAX_CONF_SIZE];
>       char clientname_buf[RBD_MAX_CONF_SIZE];
>       char *clientname;
> +    const char *passwordid;
>       rados_t cluster;
>       rados_ioctx_t io_ctx;
>       int ret;
>
> +    passwordid = qemu_opt_get(opts, "passwordid");
> +
>       if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                              snap_buf, sizeof(snap_buf),
>                              name, sizeof(name),
> @@ -350,6 +375,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>           return -EIO;
>       }
>
> +    if (qemu_rbd_set_auth(cluster, passwordid, errp) < 0) {
> +        rados_shutdown(cluster);
> +        return -EIO;
> +    }
> +
>       if (rados_connect(cluster) < 0) {
>           error_setg(errp, "error connecting");
>           rados_shutdown(cluster);
> @@ -423,6 +453,11 @@ static QemuOptsList runtime_opts = {
>               .type = QEMU_OPT_STRING,
>               .help = "Specification of the rbd image",
>           },
> +        {
> +            .name = "passwordid",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
>           { /* end of list */ }
>       },
>   };
> @@ -436,6 +471,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>       char conf[RBD_MAX_CONF_SIZE];
>       char clientname_buf[RBD_MAX_CONF_SIZE];
>       char *clientname;
> +    const char *passwordid;
>       QemuOpts *opts;
>       Error *local_err = NULL;
>       const char *filename;
> @@ -450,6 +486,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>
>       filename = qemu_opt_get(opts, "filename");
> +    passwordid = qemu_opt_get(opts, "passwordid");
>
>       if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                              snap_buf, sizeof(snap_buf),
> @@ -488,6 +525,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>           }
>       }
>
> +    if (qemu_rbd_set_auth(s->cluster, passwordid, errp) < 0) {
> +        r = -EIO;
> +        goto failed_shutdown;
> +    }
> +
>       /*
>        * Fallback to more conservative semantics if setting cache
>        * options fails. Ignore errors from setting rbd_cache because the
> @@ -919,6 +961,11 @@ static QemuOptsList qemu_rbd_create_opts = {
>               .type = QEMU_OPT_SIZE,
>               .help = "RBD object size"
>           },
> +        {
> +            .name = "passwordid",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
>           { /* end of list */ }
>       }
>   };
>

Re: [Qemu-devel] [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 <berrange@redhat.com>
> ---
>  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. "
> 

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

[Daniel P. Berrange]

On Mon, Dec 21, 2015 at 04:58:41PM +0100, Paolo Bonzini wrote:
> 
> 
> 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 <berrange@redhat.com>
> > ---
> >  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.

The password is passed into libiscsi using

   iscsi_set_initiator_username_pwd()

which expects a NULL terminated string. This gives us a choice of
clamping the data to 7-bit ascii only, or using utf-8. We can't
pass it 8-bit data, as that can contain embedded NULs. So IIUC
using utf-8 is best thing here.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 0/3] Use QCryptoSecret for block device passwords

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> The RBD, Curl and iSCSI block device drivers all need the ability
> to accept a password to authenticate with the remote network storage
> server. Currently RBD and iSCSI both just take the password in clear
> text as part of the block parameters which is insecure (passwords are
> visible in the process listing), while Curl doesn't support auth at
> all.
>
> This series updates all three drivers so that they use the recently
> merged QCryptoSecret API for getting passwords. Each driver gains
> a 'passwordid' property that can be set to provide the ID of a
> QCryptoSecret object instance, which in turn provides the actual
> password data.

I'd find password-id easier to read than passwordid.

> These patches were previously sent as part of a larger series:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html
>
> The QCryptoSecret object support was already merged, and the additions
> to qemu-img/qemu-io/qemu-nbd I'll submit separately since there is no
> strict dependancy between those additions and these patches.

Re: [Qemu-devel] [PATCH v2 0/3] Use QCryptoSecret for block device passwords

[Daniel P. Berrange]

On Mon, Jan 11, 2016 at 04:12:12PM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > The RBD, Curl and iSCSI block device drivers all need the ability
> > to accept a password to authenticate with the remote network storage
> > server. Currently RBD and iSCSI both just take the password in clear
> > text as part of the block parameters which is insecure (passwords are
> > visible in the process listing), while Curl doesn't support auth at
> > all.
> >
> > This series updates all three drivers so that they use the recently
> > merged QCryptoSecret API for getting passwords. Each driver gains
> > a 'passwordid' property that can be set to provide the ID of a
> > QCryptoSecret object instance, which in turn provides the actual
> > password data.
> 
> I'd find password-id easier to read than passwordid.

Sure, works for me - I'm ambivalent either way, so will update it.

> > These patches were previously sent as part of a larger series:
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04365.html
> >
> > The QCryptoSecret object support was already merged, and the additions
> > to qemu-img/qemu-io/qemu-nbd I'll submit separately since there is no
> > strict dependancy between those additions and these patches.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|