Re: [Qemu-block] [PATCH v2 3/3] iscsi: add support for getting CHAP password via QCryptoSecret API

[Daniel P. Berrange]

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 <address@hidden>
> > ---
> >  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 <iscsi/iscsi.h>
> >  #include <iscsi/scsi-lowlevel.h>
> > @@ -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 :|