Re: [Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

2015-09-14 Thread Eric Blake 
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.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

2015-09-13 Thread Peter Lieven 


> Am 14.09.2015 um 08:38 schrieb Fam Zheng :
> 
>> On Fri, 09/11 08:27, ronnie sahlberg wrote:
>>> On Fri, Sep 11, 2015 at 8:20 AM, Eric Blake  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 
 ---
 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 <> [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

Re: [Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

2015-09-13 Thread Fam Zheng 
On Fri, 09/11 08:27, ronnie sahlberg wrote:
> On Fri, Sep 11, 2015 at 8:20 AM, Eric Blake  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 
> >> ---
> >>  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 < [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

Re: [Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

2015-09-11 Thread ronnie sahlberg 
On Fri, Sep 11, 2015 at 8:20 AM, Eric Blake  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 
>> ---
>>  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 <
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

2015-09-11 Thread Eric Blake 
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 
> ---
>  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.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

2015-09-10 Thread Fam Zheng 
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 
---
 block/iscsi.c | 83 +--
 1 file changed, 64 insertions(+), 19 deletions(-)

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