Re: [Qemu-block] [Qemu-devel] [PATCH WIP 01/30] crypto: add QCryptoSecret object class for password/key handling

[Eric Blake]

On 11/20/2015 11:04 AM, Daniel P. Berrange wrote:
> Introduce a new QCryptoSecret object class which will be used
> for providing passwords and keys to other objects which need
> sensitive credentials.
> 
> The new object can provide secret values directly as properties,
> or indirectly via a file. The latter includes support for file
> descriptor passing syntax on UNIX platforms. Ordinarily passing
> secret values directly as properties is insecure, since they
> are visible in process listings, or in log files showing the
> CLI args / QMP commands. It is possible to use AES-256-CBC to
> encrypt the secret values though, in which case all that is
> visible is the ciphertext.  For adhoc developer testing though,
> it is fine to provide the secrets directly without encryption
> so this is not explicitly forbidden.
> 
> The anticipated scenario is that libvirtd will create a random
> master key per QEMU instance (eg /var/run/libvirt/qemu/$VMNAME.key)
> and will use that key to encrypt all passwords it provides to
> QEMU via '-object secret,....'.  This avoids the need for libvirt
> (or other mgmt apps) to worry about file descriptor passing.
> 
> It also makes life easier for people who are scripting the
> management of QEMU, for whom FD passing is significantly more
> complex.
> 
> Providing data inline (insecure, only for adhoc dev tetsing)

s/tetsing/testing/

> 
>   $QEMU -object secret,id=sec0,data=letmein
> 
> Providing data indirectly in raw format
> 
>   echo -n "letmein" > mypasswd.txt

'echo -n' is non-portable (just this month, there was a user complaining
that gentoo disables it on dash, even though upstream dash supports it).
 Might be better as:

printf "letmein" > mypasswd.txt

>   $QEMU -object secret,id=sec0,file=mypasswd.txt
> 
> Providing data indirectly in base64 format
> 
>   $QEMU -object secret,id=sec0,file=mykey.b64,format=base64
> 
> Providing data with encyption

s/encyption/encryption/

> 
>   $QEMU -object secret,id=master0,file=mykey.b64,format=base64 \
>         -object secret,id=sec0,data=[base64 ciphertext],\
>                  keyid=master0,iv=[base64 IV],format=base64
> 
> Note that 'format' here refers to the format of the ciphertext
> data. The decrypted data must always be in raw byte format.
> 
> More examples are shown in the updated docs.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  crypto/Makefile.objs       |   1 +
>  crypto/secret.c            | 567 
> +++++++++++++++++++++++++++++++++++++++++++++
>  include/crypto/secret.h    | 148 ++++++++++++
>  qapi/crypto.json           |  14 ++
>  qemu-options.hx            |  78 +++++++
>  tests/.gitignore           |   1 +
>  tests/Makefile             |   2 +
>  tests/test-crypto-secret.c | 446 +++++++++++++++++++++++++++++++++++
>  8 files changed, 1257 insertions(+)
>  create mode 100644 crypto/secret.c
>  create mode 100644 include/crypto/secret.h
>  create mode 100644 tests/test-crypto-secret.c

Just focusing on the interface on this pass:


> +
> +static const char *base64_valid_chars =
> +    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=";
> +
> +static int
> +qcrypto_secret_validate_base64(const uint8_t *input,
> +                               size_t inputlen,
> +                               Error **errp)

Don't we already have base64 utility methods available?

> +++ b/include/crypto/secret.h

> +/**
> + * QCryptoSecret:
> + *
> + * The QCryptoSecret object provides storage of secrets,
> + * which may be user passwords, encryption keys or any
> + * other kind of sensitive data that is represented as
> + * a sequence of bytes.
> + *
> + * The sensitive data associated with the secret can
> + * be provided directly via the 'data' property, or
> + * indirectly via the 'file' property. In the latter
> + * case there is support for file descriptor passing
> + * via the usual /dev/fdset/NN syntax that QEMU uses.
> + *
> + * The data for a secret can be provided in two formats,
> + * either as a UTF-8 string (the default), or as base64
> + * encoded 8-bit binary data. The latter is appropriate
> + * for  raw encrypton keys, while the former is appropriate

s/for  raw/for raw/
s/encrypton/encryption/

> + * for user entered passwords.
> + *
> + * The data may be optionally encrypted with AES-256-CBC,
> + * and the decryption key provided by another
> + * QCryptoSecret instance identified by the 'keyid'
> + * property. When passing sensitive data directly
> + * via the 'data' property it is strongly recommended
> + * to use the AES encryption facility to prevent the
> + * sensitive data being exposed in the process listing
> + * or system log files.
> + *
> + * Providing data directly, insecurely (suitable for
> + * adhoc developer testing only)
> + *
> + *  $QEMU -object secret,id=sec0,data=letmein
> + *
> + * Providing data indirectly:
> + *
> + *  # echo -n "letmein" > password.txt

same comment as on commit message

> + *  # $QEMU \
> + *      -object secret,id=sec0,file=password.txt
> + *
> + * Using a master encryption key with data.
> + *
> + * The master key needs to be created as 32 secure
> + * random bytes (optionally base64 encoded)
> + *
> + *  # openssl rand -base64 32 > key.b64
> + *  # KEY=$(base64 -d key.b64 | hexdump  -v -e '/1 "%02X"')
> + *
> + * Each secret to be encrypted needs to have a random
> + * initialization vector generated. These do not need
> + * to be kept secret
> + *
> + *  # openssl rand -base64 16 > iv.b64
> + *  # IV=$(base64 -d iv.b64 | hexdump  -v -e '/1 "%02X"')
> + *
> + * A secret to be defined can now be encrypted
> + *
> + *  # SECRET=$(echo -n "letmein" |
> + *             openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
> + *
> + * When launching QEMU, create a master secret pointing
> + * to key.b64 and specify that to be used to decrypt
> + * the user password
> + *
> + *  # $QEMU \
> + *      -object secret,id=secmaster0,format=base64,file=key.b64 \
> + *      -object secret,id=sec0,keyid=secmaster0,format=base64,\
> + *          data=$SECRET,iv=$(<iv.b64)
> + *
> + * When encrypting, the data can still be provided via an
> + * external file, in which case it is possible to use either
> + * raw binary data, or base64 encoded. This example uses
> + * raw format
> + *
> + *  # echo -n "letmein" |
> + *       openssl enc -aes-256-cbc -K $KEY -iv $IV -o pw.aes
> + *  # $QEMU \
> + *      -object secret,id=secmaster0,format=base64,file=key.b64 \
> + *      -object secret,id=sec0,keyid=secmaster0,\
> + *          file=pw.aes,iv=$(<iv.b64)
> + *
> + * Note that the ciphertext can be in either raw or base64
> + * format, as indicated by the 'format' parameter, but the
> + * plaintext resulting from decryption is expected to always
> + * be in raw format.
> + */


> +++ b/qapi/crypto.json
> @@ -19,3 +19,17 @@
>  { 'enum': 'QCryptoTLSCredsEndpoint',
>    'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
>    'data': ['client', 'server']}
> +
> +
> +##
> +# QCryptoSecretFormat:
> +#
> +# The data format that the secret is provided in
> +#
> +# @raw: raw bytes. When encoded in JSON only valid UTF-8 sequences can be 
> used
> +# @base64: arbitrary base64 encoded binary data
> +# Since: 2.5

You've missed 2.5.  Probably need to tweak the whole series to call out 2.6.

> +##
> +{ 'enum': 'QCryptoSecretFormat',
> +  'prefix': 'QCRYPTO_SECRET_FORMAT',
> +  'data': ['raw', 'base64']}
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0eea4ee..dd3f7f8 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3670,6 +3670,7 @@ queue @var{all|rx|tx} is an option that can be applied 
> to any netfilter.
>  @option{tx}: the filter is attached to the transmit queue of the netdev,
>               where it will receive packets sent by the netdev.
>  
> +
>  @item -object 
> filter-dump,address@hidden,address@hidden,address@hidden,address@hidden

Why the added blank line here?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature