From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36177) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aB39b-0007SY-0r for qemu-devel@nongnu.org; Mon, 21 Dec 2015 11:17:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aB39Z-0001ax-UB for qemu-devel@nongnu.org; Mon, 21 Dec 2015 11:17:42 -0500 Date: Mon, 21 Dec 2015 16:17:34 +0000 From: "Daniel P. Berrange" Message-ID: <20151221161734.GG5316@redhat.com> References: <1450709966-2998-1-git-send-email-berrange@redhat.com> <1450709966-2998-4-git-send-email-berrange@redhat.com> <567821B1.9060907@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <567821B1.9060907@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 3/3] iscsi: add support for getting CHAP password via QCryptoSecret API Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , Josh Durgin , qemu-block@nongnu.org, Jeff Cody , qemu-devel@nongnu.org, Ronnie Sahlberg On Mon, Dec 21, 2015 at 04:58:41PM +0100, Paolo Bonzini wrote: > > > On 21/12/2015 15:59, Daniel P. Berrange wrote: > > The iSCSI driver currently accepts the CHAP password in plain text > > as a block driver property. This change adds a new "passwordid" > > property that accepts the ID of a QCryptoSecret instance. > > > > $QEMU \ > > -object secret,id=sec0,filename=/home/berrange/example.pw \ > > -drive driver=iscsi,url=iscsi://example.com/target-foo/lun1,\ > > user=dan,passwordid=sec0 > > > > Signed-off-by: Daniel P. Berrange > > --- > > block/iscsi.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/block/iscsi.c b/block/iscsi.c > > index bd1f1bf..96fa3e1 100644 > > --- a/block/iscsi.c > > +++ b/block/iscsi.c > > @@ -39,6 +39,7 @@ > > #include "sysemu/sysemu.h" > > #include "qmp-commands.h" > > #include "qapi/qmp/qstring.h" > > +#include "crypto/secret.h" > > > > #include > > #include > > @@ -1075,6 +1076,8 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target, > > QemuOpts *opts; > > const char *user = NULL; > > const char *password = NULL; > > + const char *passwordid; > > + char *secret = NULL; > > > > list = qemu_find_opts("iscsi"); > > if (!list) { > > @@ -1094,8 +1097,20 @@ static void parse_chap(struct iscsi_context *iscsi, const char *target, > > return; > > } > > > > + passwordid = qemu_opt_get(opts, "passwordid"); > > password = qemu_opt_get(opts, "password"); > > - if (!password) { > > + if (passwordid && password) { > > + error_setg(errp, "'password' and 'passwordid' properties are " > > + "mutually exclusive"); > > + return; > > + } > > + if (passwordid) { > > + secret = qcrypto_secret_lookup_as_utf8(passwordid, errp); > > I'm not sure about the UTF-8 part (it should be binary), but I think we > discussed this already. Apart from this, the patch is okay. The password is passed into libiscsi using iscsi_set_initiator_username_pwd() which expects a NULL terminated string. This gives us a choice of clamping the data to 7-bit ascii only, or using utf-8. We can't pass it 8-bit data, as that can contain embedded NULs. So IIUC using utf-8 is best thing here. 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 :|