[Qemu-devel] [PATCH for-2.6] Fix association of -drive & -iscsi args

[Qemu-devel] [PATCH for-2.6] Fix association of -drive & -iscsi args

[Daniel P. Berrange]

The iSCSI block driver is a bit strange in that it requires a
separate -iscsi arg to pass various values, instead of accepting
them directly with the -drive argument. Pino is working on fixing
that for 2.7:

  https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg02011.html

The problem with the current solution is that the iSCSI block driver
finds the -iscsi arg by using an ID value based on the iSCSI IQN.
Unfortunately most real world IQNs contain a ':' character which
is not valid in QEMU ID strings.

We would like a solution for 2.6 so that libvirt can fix its long
standing CVE-2015-5160 whereby passwords are exposed in the CLI args.

I previously suggested escaping the IQN invalid characters in this
patch:

  https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg02198.html

As well as being somewhat unpleasant John pointed out that it still
doesn't let us have a 1-1 mapping of -iscsi and -drive args.

This patch provides a different approach by letting the -drive arg
specify an explicit ID value via a new 'iscsi-id' property. If the
new prop is not specified, we fallback to the IQN, since we should
maintain compatibility in the unlikely event that someone is using
the current syntax.

Daniel P. Berrange (1):
  block: add an 'iscsi-id' value to match -drive with -iscsi opts

 block/iscsi.c   | 35 +++++++++++++++++++++++------------
 qemu-doc.texi   | 12 +++++++++---
 qemu-options.hx |  4 ++--
 3 files changed, 34 insertions(+), 17 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Daniel P. Berrange]

The iSCSI block driver has ability to lookup various options, in
particular authentication info, specified by the separate -iscsi
argument. It currently uses the iSCSI IQN as the ID value for this
lookup, however, this does not work for common iSCSI IQNs as they
contain characters such as ':' which are invalid for use as IDs.

This adds an optional 'iscsi-id' parameter to the iSCSI block
driver to allow an explicit ID string to be used to reference
the -iscsi arg. For example

 $QEMU \
   -iscsi id=my_initiator,user=fred,password-secret=sec0 \
   -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/iscsi.c   | 35 +++++++++++++++++++++++------------
 qemu-doc.texi   | 12 +++++++++---
 qemu-options.hx |  4 ++--
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 302baf8..4b7aa91 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1070,7 +1070,7 @@ retry:
     return 0;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+static void parse_chap(struct iscsi_context *iscsi, const char *id,
                        Error **errp)
 {
     QemuOptsList *list;
@@ -1085,7 +1085,7 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target,
         return;
     }
 
-    opts = qemu_opts_find(list, target);
+    opts = qemu_opts_find(list, id);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
@@ -1123,7 +1123,7 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target,
     g_free(secret);
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
+static void parse_header_digest(struct iscsi_context *iscsi, const char *id,
                                 Error **errp)
 {
     QemuOptsList *list;
@@ -1135,7 +1135,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target,
         return;
     }
 
-    opts = qemu_opts_find(list, target);
+    opts = qemu_opts_find(list, id);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
@@ -1161,7 +1161,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(const char *id)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1171,7 +1171,7 @@ static char *parse_initiator_name(const char *target)
 
     list = qemu_find_opts("iscsi");
     if (list) {
-        opts = qemu_opts_find(list, target);
+        opts = qemu_opts_find(list, id);
         if (!opts) {
             opts = QTAILQ_FIRST(&list->head);
         }
@@ -1195,7 +1195,7 @@ static char *parse_initiator_name(const char *target)
     return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
+static int parse_timeout(const char *id)
 {
     QemuOptsList *list;
     QemuOpts *opts;
@@ -1203,7 +1203,7 @@ static int parse_timeout(const char *target)
 
     list = qemu_find_opts("iscsi");
     if (list) {
-        opts = qemu_opts_find(list, target);
+        opts = qemu_opts_find(list, id);
         if (!opts) {
             opts = QTAILQ_FIRST(&list->head);
         }
@@ -1314,6 +1314,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "URL to the iscsi image",
         },
+        {
+            .name = "iscsi-id",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the -iscsi argument"
+        },
         { /* end of list */ }
     },
 };
@@ -1452,6 +1457,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *filename;
+    const char *iscsi_arg_id;
     int i, ret = 0, timeout = 0;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -1473,7 +1479,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
 
     memset(iscsilun, 0, sizeof(IscsiLun));
 
-    initiator_name = parse_initiator_name(iscsi_url->target);
+    iscsi_arg_id = qdict_get_try_str(bs->options, "iscsi-id");
+    if (!iscsi_arg_id) {
+        iscsi_arg_id = iscsi_url->target;
+    }
+
+    initiator_name = parse_initiator_name(iscsi_arg_id);
 
     iscsi = iscsi_create_context(initiator_name);
     if (iscsi == NULL) {
@@ -1499,7 +1510,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, iscsi_arg_id, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1515,7 +1526,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, iscsi_arg_id, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
@@ -1523,7 +1534,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(iscsi_arg_id);
 #if defined(LIBISCSI_API_VERSION) && LIBISCSI_API_VERSION >= 20150621
     iscsi_set_timeout(iscsi, timeout);
 #else
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 79141d3..ecfd277 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1018,7 +1018,7 @@ These can also be set via a configuration file
 
 Setting the target name allows different options for different targets
 @example
-[iscsi "iqn.target.name"]
+[iscsi "someid"]
   user = "CHAP username"
   password = "CHAP password"
   initiator-name = "iqn.qemu.test:my-initiator"
@@ -1026,18 +1026,24 @@ Setting the target name allows different options for different targets
   header-digest = "CRC32C"
 @end example
 
+The ``someid'' string is referenced from the -drive argument
+using the ``iscsi-id'' parameter. If ``iscsi-id'' is not set
+then it will try to use the iSCSI IQN value as the ID, however,
+since this often contains invalid characters, using an explicit
+ID is recommended.
 
 Howto use a configuration file to set iSCSI configuration options:
 @example
 cat >iscsi.conf <<EOF
-[iscsi]
+[iscsi "my_initiator"]
   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 \
+qemu-system-i386 \
+    -drive file=iscsi://127.0.0.1/iqn.qemu.test/1,driver=isci,iscsi-id=my_initiator \
     -readconfig iscsi.conf
 @end example
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 6106520..181df6f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2489,9 +2489,9 @@ iSCSI support is an optional feature of QEMU and only available when
 compiled and linked against libiscsi.
 ETEXI
 DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
-    "-iscsi [user=user][,password=password]\n"
+    "-iscsi [user=user][,password=password][,password-secret=id]\n"
     "       [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
-    "       [,initiator-name=initiator-iqn][,id=target-iqn]\n"
+    "       [,initiator-name=initiator-iqn][,id=id]\n"
     "       [,timeout=timeout]\n"
     "                iSCSI session parameters\n", QEMU_ARCH_ALL)
 STEXI
-- 
2.5.5

Re: [Qemu-devel] [PATCH for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Kevin Wolf]

Am 22.04.2016 um 12:24 hat Daniel P. Berrange geschrieben:
> The iSCSI block driver has ability to lookup various options, in
> particular authentication info, specified by the separate -iscsi
> argument. It currently uses the iSCSI IQN as the ID value for this
> lookup, however, this does not work for common iSCSI IQNs as they
> contain characters such as ':' which are invalid for use as IDs.
> 
> This adds an optional 'iscsi-id' parameter to the iSCSI block
> driver to allow an explicit ID string to be used to reference
> the -iscsi arg. For example
> 
>  $QEMU \
>    -iscsi id=my_initiator,user=fred,password-secret=sec0 \
>    -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

I would consider this a new feature rather than a fix appropriate for
-rc4.

Kevin

Re: [Qemu-devel] [PATCH for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Peter Lieven]

Am 22.04.2016 um 12:59 schrieb Kevin Wolf:
> Am 22.04.2016 um 12:24 hat Daniel P. Berrange geschrieben:
>> The iSCSI block driver has ability to lookup various options, in
>> particular authentication info, specified by the separate -iscsi
>> argument. It currently uses the iSCSI IQN as the ID value for this
>> lookup, however, this does not work for common iSCSI IQNs as they
>> contain characters such as ':' which are invalid for use as IDs.
>>
>> This adds an optional 'iscsi-id' parameter to the iSCSI block
>> driver to allow an explicit ID string to be used to reference
>> the -iscsi arg. For example
>>
>>  $QEMU \
>>    -iscsi id=my_initiator,user=fred,password-secret=sec0 \
>>    -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> I would consider this a new feature rather than a fix appropriate for
> -rc4.

+1

Its rather late and this might have some side effects that are not obvious.
If you need to specify different credentials for different targets you can stil
supply them in the iscsi URL:

iscsi://username:password@host/iqn/0

Peter

Re: [Qemu-devel] [PATCH for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Daniel P. Berrange]

On Fri, Apr 22, 2016 at 01:13:42PM +0200, Peter Lieven wrote:
> Am 22.04.2016 um 12:59 schrieb Kevin Wolf:
> > Am 22.04.2016 um 12:24 hat Daniel P. Berrange geschrieben:
> >> The iSCSI block driver has ability to lookup various options, in
> >> particular authentication info, specified by the separate -iscsi
> >> argument. It currently uses the iSCSI IQN as the ID value for this
> >> lookup, however, this does not work for common iSCSI IQNs as they
> >> contain characters such as ':' which are invalid for use as IDs.
> >>
> >> This adds an optional 'iscsi-id' parameter to the iSCSI block
> >> driver to allow an explicit ID string to be used to reference
> >> the -iscsi arg. For example
> >>
> >>  $QEMU \
> >>    -iscsi id=my_initiator,user=fred,password-secret=sec0 \
> >>    -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1
> >>
> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > I would consider this a new feature rather than a fix appropriate for
> > -rc4.
> 
> +1
> 
> Its rather late and this might have some side effects that are not obvious.
> If you need to specify different credentials for different targets you can stil
> supply them in the iscsi URL:
> 
> iscsi://username:password@host/iqn/0

Use of that syntax is why CVE-2015-5160 exists because it exposes the
password to any other process on the host which can see the QEMU argv.
-iscsi supports the new password-secret arg that lets us avoid that
flaw.

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 for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Kevin Wolf]

Am 22.04.2016 um 13:43 hat Daniel P. Berrange geschrieben:
> On Fri, Apr 22, 2016 at 01:13:42PM +0200, Peter Lieven wrote:
> > Am 22.04.2016 um 12:59 schrieb Kevin Wolf:
> > > Am 22.04.2016 um 12:24 hat Daniel P. Berrange geschrieben:
> > >> The iSCSI block driver has ability to lookup various options, in
> > >> particular authentication info, specified by the separate -iscsi
> > >> argument. It currently uses the iSCSI IQN as the ID value for this
> > >> lookup, however, this does not work for common iSCSI IQNs as they
> > >> contain characters such as ':' which are invalid for use as IDs.
> > >>
> > >> This adds an optional 'iscsi-id' parameter to the iSCSI block
> > >> driver to allow an explicit ID string to be used to reference
> > >> the -iscsi arg. For example
> > >>
> > >>  $QEMU \
> > >>    -iscsi id=my_initiator,user=fred,password-secret=sec0 \
> > >>    -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1
> > >>
> > >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > I would consider this a new feature rather than a fix appropriate for
> > > -rc4.
> > 
> > +1
> > 
> > Its rather late and this might have some side effects that are not obvious.
> > If you need to specify different credentials for different targets you can stil
> > supply them in the iscsi URL:
> > 
> > iscsi://username:password@host/iqn/0
> 
> Use of that syntax is why CVE-2015-5160 exists because it exposes the
> password to any other process on the host which can see the QEMU argv.
> -iscsi supports the new password-secret arg that lets us avoid that
> flaw.

-iscsi is a weird thing anyway. We should do things the usual way, with
a proper BlockdevOptionsIscsi QAPI structure. Introducing a new API in
2.6 when we know we'll deprecate it again in 2.7 doesn't seem to make
that much sense.

Plus, it's -rc4 now. The problem isn't a crash or a regression. It
merely means that you might need to wait for another release before you
can use iscsi. Pretty much the definition of a new feature.

Kevin

Re: [Qemu-devel] [PATCH for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Daniel P. Berrange]

On Fri, Apr 22, 2016 at 01:53:47PM +0200, Kevin Wolf wrote:
> Am 22.04.2016 um 13:43 hat Daniel P. Berrange geschrieben:
> > On Fri, Apr 22, 2016 at 01:13:42PM +0200, Peter Lieven wrote:
> > > Am 22.04.2016 um 12:59 schrieb Kevin Wolf:
> > > > Am 22.04.2016 um 12:24 hat Daniel P. Berrange geschrieben:
> > > >> The iSCSI block driver has ability to lookup various options, in
> > > >> particular authentication info, specified by the separate -iscsi
> > > >> argument. It currently uses the iSCSI IQN as the ID value for this
> > > >> lookup, however, this does not work for common iSCSI IQNs as they
> > > >> contain characters such as ':' which are invalid for use as IDs.
> > > >>
> > > >> This adds an optional 'iscsi-id' parameter to the iSCSI block
> > > >> driver to allow an explicit ID string to be used to reference
> > > >> the -iscsi arg. For example
> > > >>
> > > >>  $QEMU \
> > > >>    -iscsi id=my_initiator,user=fred,password-secret=sec0 \
> > > >>    -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1
> > > >>
> > > >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > > I would consider this a new feature rather than a fix appropriate for
> > > > -rc4.
> > > 
> > > +1
> > > 
> > > Its rather late and this might have some side effects that are not obvious.
> > > If you need to specify different credentials for different targets you can stil
> > > supply them in the iscsi URL:
> > > 
> > > iscsi://username:password@host/iqn/0
> > 
> > Use of that syntax is why CVE-2015-5160 exists because it exposes the
> > password to any other process on the host which can see the QEMU argv.
> > -iscsi supports the new password-secret arg that lets us avoid that
> > flaw.
> 
> -iscsi is a weird thing anyway. We should do things the usual way, with
> a proper BlockdevOptionsIscsi QAPI structure. Introducing a new API in
> 2.6 when we know we'll deprecate it again in 2.7 doesn't seem to make
> that much sense.
> 
> Plus, it's -rc4 now. The problem isn't a crash or a regression. It
> merely means that you might need to wait for another release before you
> can use iscsi. Pretty much the definition of a new feature.

Ok, i thought that would probably be the response, but I wanted to be able
to say I tried anyway, given it was for a libvirt security bug. We'll just
have to a wait a bit longer to fix it for iscsi.

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 for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Peter Maydell]

On 22 April 2016 at 12:55, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Fri, Apr 22, 2016 at 01:53:47PM +0200, Kevin Wolf wrote:
>> -iscsi is a weird thing anyway. We should do things the usual way, with
>> a proper BlockdevOptionsIscsi QAPI structure. Introducing a new API in
>> 2.6 when we know we'll deprecate it again in 2.7 doesn't seem to make
>> that much sense.
>>
>> Plus, it's -rc4 now. The problem isn't a crash or a regression. It
>> merely means that you might need to wait for another release before you
>> can use iscsi. Pretty much the definition of a new feature.
>
> Ok, i thought that would probably be the response, but I wanted to be able
> to say I tried anyway, given it was for a libvirt security bug. We'll just
> have to a wait a bit longer to fix it for iscsi.

OK, I have moved that item to "Not planned to be fixed for 2.6" in
the Planning page. Could you write a suitable note in the ChangeLog
page if you think it makes sense to do so, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Daniel P. Berrange]

On Fri, Apr 22, 2016 at 01:10:40PM +0100, Peter Maydell wrote:
> On 22 April 2016 at 12:55, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Fri, Apr 22, 2016 at 01:53:47PM +0200, Kevin Wolf wrote:
> >> -iscsi is a weird thing anyway. We should do things the usual way, with
> >> a proper BlockdevOptionsIscsi QAPI structure. Introducing a new API in
> >> 2.6 when we know we'll deprecate it again in 2.7 doesn't seem to make
> >> that much sense.
> >>
> >> Plus, it's -rc4 now. The problem isn't a crash or a regression. It
> >> merely means that you might need to wait for another release before you
> >> can use iscsi. Pretty much the definition of a new feature.
> >
> > Ok, i thought that would probably be the response, but I wanted to be able
> > to say I tried anyway, given it was for a libvirt security bug. We'll just
> > have to a wait a bit longer to fix it for iscsi.
> 
> OK, I have moved that item to "Not planned to be fixed for 2.6" in
> the Planning page. Could you write a suitable note in the ChangeLog
> page if you think it makes sense to do so, please?

Yep, will do.


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 for-2.6] block: add an 'iscsi-id' value to match -drive with -iscsi opts

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 22.04.2016 um 12:24 hat Daniel P. Berrange geschrieben:
>> The iSCSI block driver has ability to lookup various options, in
>> particular authentication info, specified by the separate -iscsi
>> argument. It currently uses the iSCSI IQN as the ID value for this
>> lookup, however, this does not work for common iSCSI IQNs as they
>> contain characters such as ':' which are invalid for use as IDs.
>> 
>> This adds an optional 'iscsi-id' parameter to the iSCSI block
>> driver to allow an explicit ID string to be used to reference
>> the -iscsi arg. For example
>> 
>>  $QEMU \
>>    -iscsi id=my_initiator,user=fred,password-secret=sec0 \
>>    -drive driver=iscsi,iscsi-id=my_initiator,file=iscsi://somehost/iqn/1
>> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

This patch is cleaner and safer than "block: convert iscsi target to a
valid ID for -iscsi arg lookup".

> I would consider this a new feature rather than a fix appropriate for
> -rc4.

I'd be willing to spin this as a fix for a design bug, but I agree with
Kevin, it missed the boat.  I might have supported it up until rc1 or
so.