Re: [Qemu-devel] The unholy encrypted image key mess

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> I can't see why QMP commands would ever want to create in state NEEDKEY.
> We could easily avoid it there: give QMP commands creating
> BlockDriverStates an optional password parameter, fail the command if
> the BDS is encrypted and the password parameter is missing.

Fully agree.

The change command kind-of makes sense for encrypted block devices
specified on the command line.  By passing the password via monitor
command it can't be captured easily (unlike for example passwords
specified on the command line which show up in ps listings).

> For HMP, we need to make up our minds how to do passwords.
> 
> The current way is to tie NEEDKEY to "guest paused".  I hate that.

I'd still tend to simply remove (or disable for 2.0, remove later on)
encryption support from qemu (keep in qemu-img).

Then worry how to do it right once we add a sane encrypted format.

> >> The final funny is device model usb-storage (another one that
> >> desperately needs to be buried deep).  Its init() callback
> >> usb_msd_initfn_storage() tries to be cute when it detects state NEEDKEY.
> >> 
> >> If it's running in monitor context (say in HMP/QMP command device_add),
> >> it attempts to ask for a key.  In HMP context, it unplugs itself when
> >> this fails (I think).  In QMP context, it behaves similar to change: it
> >> works, but you get a "DeviceEncrypted" error, and the backend remains in
> >> state NEEDKEY.
> >> 
> >> If it's not running in monitor context, it clears autostart.  No idea
> >> why it does that, and I'm not sure it has any effect.  Opening an
> >> encrypted image clears autostart already, in blockdev_init().

IIRC to allow the user specify the password before starting the guest.
And, yes, if blockdev_init does that anyway it most likely is not
needed.

> Can we get rid of this, and make usb-storage behave like any other
> hot-pluggable block device model?

The dance is needed for the legacy usb_add monitor command, which
creates blockdev + usb-storage in one go.

If we go zap encryption altogether we can obviously simply remove that.
Otherwise it should be possible to move that logic into the legacy usb
init function (usb_msd_init) so it will never ever kick in with -device.

cheers,
  Gerd