| Message ID | 1461320695-31372-2-git-send-email-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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
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
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
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
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(-)