Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Daniel P. Berrange]

On Wed, Mar 11, 2015 at 09:55:16AM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> writes:
> 
> > On Tue, Mar 10, 2015 at 06:26:38PM +0100, Markus Armbruster wrote:
> >> RFC because the series only covers open [PATCH 1], but not create.
> >> Also missing: make qemu-img print a warning when it creates an
> >> encrypted image.  Finally, some of the material in the cover letter
> >> should be worked into the commit messages.
> >> 
> >> We've steered users away from QCOW/QCOW2 encryption for a while,
> >> because it's a flawed design (commit 136cd19 Describe flaws in
> >> qcow/qcow2 encryption in the docs).
> >> 
> >> In addition to flawed crypto, we have comically bad usability, and
> >> plain old bugs.  Let me show you.
> >
> >
> >> = Usability issues =
> >
> >> == Confusing startup ==
> >> == Incorrect passwords not caught ==
> >> == Need to stop the guest to add an encrypted image ==
> >> == Use without key is not always caught ==
> >> == QMP device_add of usb-storage fails when it shouldn't ==
> >
> > So there's really two separate root cuase problems we're facing
> > here. One of the usability issues is inherant artifact of the
> > qcow design. The other 4 issues are all due to the badly designed
> > block driver / monitor key management approach.
> 
> Yup.  The latter is fixable, but not worth fixing as long as the
> underlying crypto isn't worth using.
> 
> >> If people become sufficiently interested in encrypted images to
> >> contribute a cryptographically sane implementation for QCOW2 (or
> >> whatever other format), then rewriting the necessary support around it
> >> from scratch will likely be easier and yield better results than
> >> fixing up the existing mess.
> >> 
> >> Let's drop the mess and move on.  Keep qemu-img convert working, of
> >> course, to let users rescue their data.
> >
> > Once I've got through the current work i'm doing on TLS support
> > for migrate/nbd/chardev/etc, my intention is to work on adding
> > support for the LUKS format to QEMU. We really need this natively
> > in OpenStack since we're increasingly using the QEMU native client
> > for nbd, iscsi, nfs, etc but at the same time don't want to sacrifice
> > encryption which we currently do via LUKS. It is probably a good
> > 4-6 months though before I get on to working on this.
> 
> Great!
> 
> > I agree with all your points about the usability being fubar. This
> > clearly needs to be fixed for encryption support to be viable in
> > QEMU, regardless of the actual encryption format used.
> >
> > I guess my question is whether it is worth trying to fix the blockdev
> > integration part of things now, or to rip it out now and reimplement
> > it from scratch later ?  I think I probably agree with killing it
> > now, since it might actually make doing a sensible impl easier later
> > on.
> 
> In your shoes, I'd certainly prefer to drop the current mess and start
> over.  Not just because I think it'll make the work easier, but also
> because I'd want to break out of the back-compat strait-jacket, to be
> free to create the best user interface I can.
> 
> > And lets assume we do eventually have a fixed blockdev layer and a
> > sane LUKS encryption driver, would we still want to kill off qcow2
> > encryption ?  Given the way subpar encryption is being actively
> > attacked by everyone & their dog, I think mandatory retirement of
> > qcow2 encryption is a good idea sooner rather than later.
> 
> I strongly believe retiring it is a public service.
> 
> Except for qemu-img.  I'm prepared to keep it there for a long time,
> maybe forever.
> 
> > My only concern here is whether we've given users enough prior
> > warning. While we added that doc change a year ago, what are the
> > odds that anyone has actually read those docs & noticed the warning.
> > Should we have one major release where we log a deprecation warning
> > on stderr, informing users of an explicit timeframe for its removal,
> > before we actually use the big hammer of disabling it permanently ?
> 
> I'm fine with that.  In fact, I could agree to pretty much any
> deprecation schedule, as long as we start it *now*.

IIUC, the 2.3.0 major branch is targetted for end of this month,
so we could put in code that issues a deprecation warning on
stderr for 2.3.0, and then delete all this stuff when GIT master
opens for 2.4.0 development ?

> 
> > FWIW, I could see an improved interaction scheme working as follows
> >
> > First, introduce a new monitor command for setting named passwords,
> >
> >     add_key mykey1 SECRETDATA
> >
> > Now, extend the blockdev_add so that you can provide key names
> > by adding
> >
> >     'keyname': 'mykey1'
> >
> > as a parameter in the json args.
> 
> Can you explain why that's better than sticking 'key': SECRETDATA right
> into blockdev-add's arguments?

Just have a small preference to keep passwords separated from the
rest of the data, so when logging the stuff for debug purposes we
don't compromise people's passwords quite so readily. It is more
straightforward for us to mask out the passwords if we can just
match on the command name, and not have to try to grok the specific
field in a large set of args.  Also in terms of cold startup, it
is not desirable to have the password directly included in the
args to -drive or equiv, as that's visible in process listings.

> > If an attempt is made to add a blockdev without having provided a
> > key, the attempt should just fail. This avoids all the insanity
> > around delayed opening of files, as well as avoiding need to stop
> > the guest to add devices.
> 
> Yes, we need to exterminate the awkward and dangerous intermediate state
> "image opened, but lacks decryption key".  This series avoids it (except
> in qemu-img, but there it's safely confined to img_open()).  Future work
> should not bring it back.
> 
> > For cold plug, have a command line arg '--add-keys prompt' to
> > indicate the user should be prompted on TTY to enter keys, which
> > is good for interactive usage. For managed usage we could allow
> > '--add-keys fd=FDNUM' and just read keys from the file descriptor.
> 
> I guess sugared command line like "-hda geheim.qcow2" should simply
> prompt for the key right in the tty.  None of this nonsense with not
> starting the guest, and having "cont" prompt in the monitor.

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 :|