iscsi: Add chap and "initiator-name" etc as per drive options
diff mbox

Message ID 1441951252-13439-1-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Sept. 11, 2015, 6 a.m. UTC
Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
specify iscsi connection parameters, unfortunately it doesn't work with
qemu-img.

This patch adds per drive options to iscsi driver so that at least
qemu-img can use the "json:{...}" filename magic.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/iscsi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 19 deletions(-)

Comments

Eric Blake Sept. 11, 2015, 3:20 p.m. UTC | #1
On 09/11/2015 12:00 AM, Fam Zheng wrote:
> Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
> specify iscsi connection parameters, unfortunately it doesn't work with
> qemu-img.
> 
> This patch adds per drive options to iscsi driver so that at least
> qemu-img can use the "json:{...}" filename magic.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/iscsi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 19 deletions(-)

It would be nice to also add a matching BlockdevOptionsIscsi to
qapi/block-core.json, to allow setting these structured options from
QMP.  Separate patch is fine, but we need to do the work for ALL of the
remaining block devices eventually, and now that you are structuring the
command line is a good time to think about it.


>  static void iscsi_nop_timed_event(void *opaque)
> @@ -1229,6 +1253,27 @@ static QemuOptsList runtime_opts = {
>              .name = "filename",
>              .type = QEMU_OPT_STRING,
>              .help = "URL to the iscsi image",
> +        },{
> +            .name = "user",
> +            .type = QEMU_OPT_STRING,
> +            .help = "username for CHAP authentication to target",
> +        },{
> +            .name = "password",
> +            .type = QEMU_OPT_STRING,
> +            .help = "password for CHAP authentication to target",
> +        },{

Also, this requires passing the password in the command line. We
_really_ need to solve the problem of allowing the password to be passed
via a fd or other QMP command, rather than on the command line.
ronnie sahlberg Sept. 11, 2015, 3:27 p.m. UTC | #2
On Fri, Sep 11, 2015 at 8:20 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/11/2015 12:00 AM, Fam Zheng wrote:
>> Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
>> specify iscsi connection parameters, unfortunately it doesn't work with
>> qemu-img.
>>
>> This patch adds per drive options to iscsi driver so that at least
>> qemu-img can use the "json:{...}" filename magic.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  block/iscsi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 64 insertions(+), 19 deletions(-)
>
> It would be nice to also add a matching BlockdevOptionsIscsi to
> qapi/block-core.json, to allow setting these structured options from
> QMP.  Separate patch is fine, but we need to do the work for ALL of the
> remaining block devices eventually, and now that you are structuring the
> command line is a good time to think about it.
>
>
>>  static void iscsi_nop_timed_event(void *opaque)
>> @@ -1229,6 +1253,27 @@ static QemuOptsList runtime_opts = {
>>              .name = "filename",
>>              .type = QEMU_OPT_STRING,
>>              .help = "URL to the iscsi image",
>> +        },{
>> +            .name = "user",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "username for CHAP authentication to target",
>> +        },{
>> +            .name = "password",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "password for CHAP authentication to target",
>> +        },{
>
> Also, this requires passing the password in the command line. We
> _really_ need to solve the problem of allowing the password to be passed
> via a fd or other QMP command, rather than on the command line.


Passing via command line is evil. It should still be possible to pass
all this via a config file to qemu :

"""
...
Howto use a configuration file to set iSCSI configuration options:
@example
cat >iscsi.conf <<EOF
[iscsi "iqn.target.name"]
  user = "me"
  password = "my password"
  initiator-name = "iqn.qemu.test:my-initiator"
  header-digest = "CRC32C"
EOF

qemu-system-i386 -drive file=iscsi://127.0.0.1/iqn.qemu.test/1 \
    -readconfig iscsi.conf
@end example
...
"""





>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Fam Zheng Sept. 14, 2015, 6:38 a.m. UTC | #3
On Fri, 09/11 08:27, ronnie sahlberg wrote:
> On Fri, Sep 11, 2015 at 8:20 AM, Eric Blake <eblake@redhat.com> wrote:
> > On 09/11/2015 12:00 AM, Fam Zheng wrote:
> >> Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
> >> specify iscsi connection parameters, unfortunately it doesn't work with
> >> qemu-img.
> >>
> >> This patch adds per drive options to iscsi driver so that at least
> >> qemu-img can use the "json:{...}" filename magic.
> >>
> >> Signed-off-by: Fam Zheng <famz@redhat.com>
> >> ---
> >>  block/iscsi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--------------
> >>  1 file changed, 64 insertions(+), 19 deletions(-)
> >
> > It would be nice to also add a matching BlockdevOptionsIscsi to
> > qapi/block-core.json, to allow setting these structured options from
> > QMP.  Separate patch is fine, but we need to do the work for ALL of the
> > remaining block devices eventually, and now that you are structuring the
> > command line is a good time to think about it.
> >
> >
> >>  static void iscsi_nop_timed_event(void *opaque)
> >> @@ -1229,6 +1253,27 @@ static QemuOptsList runtime_opts = {
> >>              .name = "filename",
> >>              .type = QEMU_OPT_STRING,
> >>              .help = "URL to the iscsi image",
> >> +        },{
> >> +            .name = "user",
> >> +            .type = QEMU_OPT_STRING,
> >> +            .help = "username for CHAP authentication to target",
> >> +        },{
> >> +            .name = "password",
> >> +            .type = QEMU_OPT_STRING,
> >> +            .help = "password for CHAP authentication to target",
> >> +        },{
> >
> > Also, this requires passing the password in the command line. We
> > _really_ need to solve the problem of allowing the password to be passed
> > via a fd or other QMP command, rather than on the command line.
> 
> 
> Passing via command line is evil. It should still be possible to pass
> all this via a config file to qemu :
> 
> """
> ...
> Howto use a configuration file to set iSCSI configuration options:
> @example
> cat >iscsi.conf <<EOF
> [iscsi "iqn.target.name"]
>   user = "me"
>   password = "my password"
>   initiator-name = "iqn.qemu.test:my-initiator"
>   header-digest = "CRC32C"
> EOF
> 
> qemu-system-i386 -drive file=iscsi://127.0.0.1/iqn.qemu.test/1 \
>     -readconfig iscsi.conf
> @end example
> ...
> """

I agree passing password with clear text command line is bad, but -readconfig
doesn't work for qemu-img and qemu-io.  Any idea how to make that work?

Fam
Peter Lieven Sept. 14, 2015, 6:50 a.m. UTC | #4
> Am 14.09.2015 um 08:38 schrieb Fam Zheng <famz@redhat.com>:
> 
>> On Fri, 09/11 08:27, ronnie sahlberg wrote:
>>> On Fri, Sep 11, 2015 at 8:20 AM, Eric Blake <eblake@redhat.com> wrote:
>>>> On 09/11/2015 12:00 AM, Fam Zheng wrote:
>>>> Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
>>>> specify iscsi connection parameters, unfortunately it doesn't work with
>>>> qemu-img.
>>>> 
>>>> This patch adds per drive options to iscsi driver so that at least
>>>> qemu-img can use the "json:{...}" filename magic.
>>>> 
>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>> block/iscsi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--------------
>>>> 1 file changed, 64 insertions(+), 19 deletions(-)
>>> 
>>> It would be nice to also add a matching BlockdevOptionsIscsi to
>>> qapi/block-core.json, to allow setting these structured options from
>>> QMP.  Separate patch is fine, but we need to do the work for ALL of the
>>> remaining block devices eventually, and now that you are structuring the
>>> command line is a good time to think about it.
>>> 
>>> 
>>>> static void iscsi_nop_timed_event(void *opaque)
>>>> @@ -1229,6 +1253,27 @@ static QemuOptsList runtime_opts = {
>>>>             .name = "filename",
>>>>             .type = QEMU_OPT_STRING,
>>>>             .help = "URL to the iscsi image",
>>>> +        },{
>>>> +            .name = "user",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "username for CHAP authentication to target",
>>>> +        },{
>>>> +            .name = "password",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +            .help = "password for CHAP authentication to target",
>>>> +        },{
>>> 
>>> Also, this requires passing the password in the command line. We
>>> _really_ need to solve the problem of allowing the password to be passed
>>> via a fd or other QMP command, rather than on the command line.
>> 
>> 
>> Passing via command line is evil. It should still be possible to pass
>> all this via a config file to qemu :
>> 
>> """
>> ...
>> Howto use a configuration file to set iSCSI configuration options:
>> @example
>> cat >iscsi.conf <<EOF
>> [iscsi "iqn.target.name"]
>>  user = "me"
>>  password = "my password"
>>  initiator-name = "iqn.qemu.test:my-initiator"
>>  header-digest = "CRC32C"
>> EOF
>> 
>> qemu-system-i386 -drive file=iscsi://127.0.0.1/iqn.qemu.test/1 \
>>    -readconfig iscsi.conf
>> @end example
>> ...
>> """
> 
> I agree passing password with clear text command line is bad, but -readconfig
> doesn't work for qemu-img and qemu-io.  Any idea how to make that work?

you can pass the secrets via environment variables (see libiscsi readme).

Peter
Eric Blake Sept. 14, 2015, 3:04 p.m. UTC | #5
On 09/14/2015 12:50 AM, Peter Lieven wrote:

>>>> It would be nice to also add a matching BlockdevOptionsIscsi to
>>>> qapi/block-core.json, to allow setting these structured options from
>>>> QMP.  Separate patch is fine, but we need to do the work for ALL of the
>>>> remaining block devices eventually, and now that you are structuring the
>>>> command line is a good time to think about it.
>>>>
>>>>

>>> Passing via command line is evil. It should still be possible to pass
>>> all this via a config file to qemu :
>>>

>>
>> I agree passing password with clear text command line is bad, but -readconfig
>> doesn't work for qemu-img and qemu-io.  Any idea how to make that work?
> 
> you can pass the secrets via environment variables (see libiscsi readme).

Environment variables are no more secure than command line parameters -
both are visible via ps to other processes, and hence relatively
insecure.  We need a way to pass secrets over a file descriptor, whether
that file descriptor be a config file, or whether it be a pipe.

Patch
diff mbox

diff --git a/block/iscsi.c b/block/iscsi.c
index 5002916..9efb9ec 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1011,7 +1011,9 @@  retry:
     return 0;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+static void parse_chap(struct iscsi_context *iscsi,
+                       QemuOpts *img_opts,
+                       const char *target,
                        Error **errp)
 {
     QemuOptsList *list;
@@ -1025,19 +1027,22 @@  static void parse_chap(struct iscsi_context *iscsi, const char *target,
     }
 
     opts = qemu_opts_find(list, target);
-    if (opts == NULL) {
+    if (!opts) {
         opts = QTAILQ_FIRST(&list->head);
-        if (!opts) {
-            return;
-        }
     }
 
-    user = qemu_opt_get(opts, "user");
+    user = qemu_opt_get(img_opts, "user");
+    if (!user && opts) {
+        user = qemu_opt_get(opts, "user");
+    }
     if (!user) {
         return;
     }
 
-    password = qemu_opt_get(opts, "password");
+    password = qemu_opt_get(img_opts, "password");
+    if (!password && opts) {
+        password = qemu_opt_get(opts, "password");
+    }
     if (!password) {
         error_setg(errp, "CHAP username specified but no password was given");
         return;
@@ -1048,13 +1053,20 @@  static void parse_chap(struct iscsi_context *iscsi, const char *target,
     }
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
+static void parse_header_digest(struct iscsi_context *iscsi,
+                                QemuOpts *img_opts,
+                                const char *target,
                                 Error **errp)
 {
     QemuOptsList *list;
     QemuOpts *opts;
     const char *digest = NULL;
 
+    digest = qemu_opt_get(img_opts, "header-digest");
+    if (digest) {
+        goto found;
+    }
+
     list = qemu_find_opts("iscsi");
     if (!list) {
         return;
@@ -1073,6 +1085,7 @@  static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
         return;
     }
 
+found:
     if (!strcmp(digest, "CRC32C")) {
         iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_CRC32C);
     } else if (!strcmp(digest, "NONE")) {
@@ -1086,7 +1099,7 @@  static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
     }
 }
 
-static char *parse_initiator_name(const char *target)
+static char *parse_initiator_name(QemuOpts *img_opts, const char *target)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1094,6 +1107,11 @@  static char *parse_initiator_name(const char *target)
     char *iscsi_name;
     UuidInfo *uuid_info;
 
+    name = qemu_opt_get(img_opts, "initiator-name");
+    if (name) {
+        return g_strdup(name);
+    }
+
     list = qemu_find_opts("iscsi");
     if (list) {
         opts = qemu_opts_find(list, target);
@@ -1120,12 +1138,17 @@  static char *parse_initiator_name(const char *target)
     return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
+static int parse_timeout(QemuOpts *img_opts, const char *target)
 {
     QemuOptsList *list;
     QemuOpts *opts;
     const char *timeout;
 
+    timeout = qemu_opt_get(img_opts, "iscsi");
+    if (timeout) {
+        goto out;
+    }
+
     list = qemu_find_opts("iscsi");
     if (list) {
         opts = qemu_opts_find(list, target);
@@ -1134,13 +1157,14 @@  static int parse_timeout(const char *target)
         }
         if (opts) {
             timeout = qemu_opt_get(opts, "timeout");
-            if (timeout) {
-                return atoi(timeout);
-            }
         }
     }
-
-    return 0;
+out:
+    if (timeout) {
+        return atoi(timeout);
+    } else {
+        return 0;
+    }
 }
 
 static void iscsi_nop_timed_event(void *opaque)
@@ -1229,6 +1253,27 @@  static QemuOptsList runtime_opts = {
             .name = "filename",
             .type = QEMU_OPT_STRING,
             .help = "URL to the iscsi image",
+        },{
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+            .help = "username for CHAP authentication to target",
+        },{
+            .name = "password",
+            .type = QEMU_OPT_STRING,
+            .help = "password for CHAP authentication to target",
+        },{
+            .name = "header-digest",
+            .type = QEMU_OPT_STRING,
+            .help = "HeaderDigest setting. "
+                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
+        },{
+            .name = "initiator-name",
+            .type = QEMU_OPT_STRING,
+            .help = "Initiator iqn name to use when connecting",
+        },{
+            .name = "timeout",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Request timeout in seconds (default 0 = no timeout)",
         },
         { /* end of list */ }
     },
@@ -1390,7 +1435,7 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    initiator_name = parse_initiator_name(opts, iscsi_url->target);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
@@ -1416,7 +1461,7 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* check if we got CHAP username/password via the options */
-    parse_chap(iscsi, iscsi_url->target, &local_err);
+    parse_chap(iscsi, opts, iscsi_url->target, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1432,7 +1477,7 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
 
     /* check if we got HEADER_DIGEST via the options */
-    parse_header_digest(iscsi, iscsi_url->target, &local_err);
+    parse_header_digest(iscsi, opts, iscsi_url->target, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1440,7 +1485,7 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* timeout handling is broken in libiscsi before 1.15.0 */
-    timeout = parse_timeout(iscsi_url->target);
+    timeout = parse_timeout(opts, iscsi_url->target);
 #if defined(LIBISCSI_API_VERSION) && LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else