Re: [Qemu-block] [PATCH 00/17] Framework for securely passing secrets to QEMU

[Stefan Hajnoczi]

On Mon, Oct 19, 2015 at 04:09:32PM +0100, Daniel P. Berrange wrote:
> There are a variety of places where QEMU needs to have access
> to passwords, encryption keys or similar kinds of secrets.
> 
>  - VNC / SPICE user passwords
>  - Curl block http / proxy passwords
>  - RBD auth password
>  - iSCSI CHAP password
>  - x509 private key password
>  - QCow/QCow2 encryption key
> 
> QEMU has a variety of ways of dealing with this problem, some
> good, some ugly, some bad.
> 
>  - The RBD block driver accepts the password in plaintext
>    via a private RBD config option. This is a pending CVE
> 
>     https://security-tracker.debian.org/tracker/CVE-2015-5160
> 
>  - The iSCSI driver accepts the password in plaintext as
>    a block driver option. This is the same as the RBD CVE
>    essentially, just a QEMU option, rather than a librbd
>    option
> 
>  - The VNC / SPICE servers only accept the passwords via
>    the QEMU monitor. This is secure, but it inconvenient
>    for adhoc developer usage where security of CLI args
>    does not matter.
> 
>  - QCow/QCow2 encryption keys can be provided by the monitor
>    but this is not available for qemu-img, qemu-io and
>    qemu-nbd. QEMU falls back to doing interactive
>    console prompting to get keys.
> 
>  - x509 privte key passwords are not supported at all by
>    QEMU which forces users to store their key in plaintext
>    on their host FS.
> 
>  - The CURL driver doesn't support HTTP auth at all
>    currently.
> 
> It is obvious there there is a wide variety of functionality
> in QEMU that needs access to "secrets". This need will only
> grow over time. We need to stop having everyone invent their
> own dangerous wheels and provide a standard mechanism for
> securely passing secrets to QEMU.
> 
> To this end, this series introduces a QCryptoSecret object
> class with short name "secret". All the places which needs
> passwords/keys are then converted to get their via this
> API, except VNC/SPICE which are a future exercise.
> 
> Example usage for creating secrets...
> 
> Direct password, insecure, for ad-hoc developer testing only
> 
>   $QEMU -object secret,id=sec0,data=letmein
> 
> Indirect password via a file, good for production
> 
>   echo -n "letmein" > mypasswd.txt
>   $QEMU -object secret,id=sec0,file=mypasswd.txt
> 
> The file based approach supports file descriptor passing,
> so mgmt apps can use that to dynamically add passwords to
> running QEMU.
> 
> There is a better way though, which is to use an encrypted
> secret. The intent here is that a mgmt app (like libvirt)
> will generate a random AES-256 key for each virtual machine
> it starts (saved in eg /var/run/libvirt/qemu/$GUEST.key)
> It can then use the direct password passing, but encrypt
> the data.
> 
>   $QEMU \
>     -object 
> secret,id=secmaster0,file=/var/run/libvirt/qemu/$GUEST.key,format=base64 \
>     -object secret,id=sec0,data=[base64 ciphertext],\
>                keyid=secmaster0,iv=[base64 initialization vector]
> 
> This means that the mgmt app does not need to worry about
> file descriptor passing at all. It can just use regular
> object properties, safe in the knowledge that the data is
> protected by a secret AES key shared only between QEMU
> and the mgmt app.
> 
> Use of encrypted secrets is not restricted to directly
> provided inline data. If the secret is stored in an
> external file, that can be encrypted too
> 
>   $QEMU \
>     -object 
> secret,id=secmaster0,file=/var/run/libvirt/qemu/$GUEST.key,format=base64 \
>     -object secret,id=sec0,file=/some/secret/file.aes,\
>                keyid=secmaster0,iv=[base64 initialization vector]
> 
> 
> 
> Example usage for referencing secrets...
> 
> CURL:
> 
>   $QEMU -object secret,id=sec0... \
>      -drive driver=http,url=http://example.com/someimg.qcow2,\
>               user=dan,passwordid=sec0
> 
>   $QEMU -object secret,id=sec0... -object secret,id=sec1 \
>      -drive driver=http,url=http://example.com/someimg.qcow2,\
>               user=dan,passwordid=sec0,proxyuser=dan,passwordid=sec1
> 
> iSCSI:
> 
>   $QEMU -object secret,id=sec0... \
>      -drive driver=iscsi,url=iscsi://example.com/target-foo/lun1,\
>              user=dan,passwordid=sec0
> 
> RBD:
> 
>   $QEMU -object secret,id=sec0... \
>      -drive driver=rbd,file=rbd:pool/image:id=myname,\
>              auth-supported-cephx,authsecret=sec0
> 
> QCow/Qcow2 encryption
> 
>   $QEMU -object secret,id=sec0... \
>      -drive file=someimage.qcow2,keyid=sec0
> 
> 
> Finally, this extends qemu-img, qemu-nbd and qemu-io. All of
> them gain a new '--object' parameter which provides the same
> functionality as '-object' with QEMU system emulators. This
> lets us create QCryptoSecret object instances in those tools
> 
> The tools also then get support for a new '--source IMG-OPTS'
> parameter to allow a full set of image options to be specified,
> instead of just separate hardcoded args for format + filename
> which they currently permit. This is probably the area I am
> least sure of. I struggled to understand what the "best
> practice" is for turning a QemuOpts into something you can
> use to instantiate block backends. So I may well have not
> done the right thing.
> 
> Towards the end I rip out the current encryption key handling
> from the block layer so all the hairy code for dealing
> with encrypted block devices disappears, and encryption
> can be a 100% private matter for the block driver internal
> impl.  This is obviously not backwards compatible, but we
> have been warning users we're dropping qcow2 encryption
> support for a while.
> 
> Finally I disable support for writing to encrypted qcow2
> files, but keep the ability to read them, so users can
> liberate data. Originally we were intending to fully
> delete encryption support, due to the burden it places
> on the internal boock API. Since I removed that burden
> I figured it is reasonable to keep read-only support
> around.
> 
> The only real missing thing is wiring up the VNC/SPICE
> servers. There is one complication here in that it is
> common to change the VNC/SPICE password at runtime, and
> I'm not sure what the best way to deal with this is.
> 
> There are two obvious choices
> 
>  a. Create a new secret, tell the VNC server to use
>     the new secret, delete the old secret. This will
>     need a new 'graphics_secret_update' command in
>     the monitor, to use alongside object_add/del.
> 
>  b. Allow the existing secret to be updated via some
>     new 'object_update' method, and internally notify
>     the VNC/SPICE server when the secret is updated.
>     This would probably need a new QOM interface
>     UserUpdatableObject to be defined, as an refinement
>     of UserCreatableObject.
> 
> Daniel P. Berrange (17):
>   crypto: add QCryptoSecret object class for password/key handling
>   crypto: add support for loading encrypted x509 keys
>   rbd: add support for getting password from QCryptoSecret object
>   curl: add support for HTTP authentication parameters
>   iscsi: add support for getting CHAP password via QCryptoSecret API
>   qcow: add a 'keyid' parameter to qcow options
>   qcow2: add a 'keyid' parameter to qcow2 options
>   qom: add user_creatable_add & user_creatable_del methods
>   qemu-img: add support for --object command line arg
>   qemu-nbd: add support for --object command line arg
>   qemu-io: add support for --object command line arg
>   qemu-io: allow specifying image as a set of options args
>   qemu-nbd: allow specifying image as a set of options args
>   qemu-img: allow specifying image as a set of options args
>   block: rip out all traces of password prompting
>   block: remove all encryption handling APIs
>   block: remove support for writing to qcow/qcow2 encrypted images
> 
>  block.c                         |  88 +----
>  block/curl.c                    |  66 ++++
>  block/iscsi.c                   |  24 +-
>  block/qapi.c                    |   2 +-
>  block/qcow.c                    | 122 +++++--
>  block/qcow2.c                   | 116 +++---
>  block/qcow2.h                   |   1 +
>  block/rbd.c                     |  42 +++
>  blockdev.c                      |  69 +---
>  crypto/Makefile.objs            |   1 +
>  crypto/secret.c                 | 513 ++++++++++++++++++++++++++
>  crypto/tlscredsx509.c           |  47 +++
>  hmp.c                           |  42 +--
>  hw/usb/dev-storage.c            |  32 --
>  include/block/block.h           |   5 +-
>  include/crypto/secret.h         | 139 +++++++
>  include/crypto/tlscredsx509.h   |   1 +
>  include/monitor/monitor.h       |  10 -
>  include/qemu/option.h           |   1 +
>  include/qemu/osdep.h            |   2 -
>  include/qom/object_interfaces.h |  31 ++
>  monitor.c                       |  65 ----
>  qapi/block-core.json            |  23 +-
>  qapi/crypto.json                |  14 +
>  qemu-img-cmds.hx                |  44 +--
>  qemu-img.c                      | 788 
> +++++++++++++++++++++++++++++++++++-----
>  qemu-img.texi                   |   8 +
>  qemu-io.c                       | 145 ++++++--
>  qemu-nbd.c                      | 142 +++++++-
>  qemu-nbd.texi                   |   7 +
>  qemu-options.hx                 |  84 ++++-
>  qmp.c                           |  83 +----
>  qom/object_interfaces.c         |  76 ++++
>  tests/.gitignore                |   1 +
>  tests/Makefile                  |   2 +
>  tests/qemu-iotests/087          |  20 +
>  tests/qemu-iotests/087.out      |  26 +-
>  tests/qemu-iotests/134          |  17 +-
>  tests/qemu-iotests/134.out      |  44 +--
>  tests/qemu-iotests/common.rc    |   4 +-
>  tests/test-crypto-secret.c      | 440 ++++++++++++++++++++++
>  util/oslib-posix.c              |  66 ----
>  util/oslib-win32.c              |  24 --
>  util/qemu-option.c              |   6 +
>  vl.c                            |   8 +-
>  45 files changed, 2740 insertions(+), 751 deletions(-)
>  create mode 100644 crypto/secret.c
>  create mode 100644 include/crypto/secret.h
>  create mode 100644 tests/test-crypto-secret.c

Acked-by: Stefan Hajnoczi <address@hidden>

i.e. makes sense to me but I haven't reviewed patches in detail