| Message ID | 1441951252-13439-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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
> 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
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.
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
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(-)