From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1ILt-0004FC-1a for qemu-devel@nongnu.org; Tue, 24 Nov 2015 13:30:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1ILp-0005st-Qj for qemu-devel@nongnu.org; Tue, 24 Nov 2015 13:30:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35235) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1ILp-0005sG-Gp for qemu-devel@nongnu.org; Tue, 24 Nov 2015 13:30:01 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 85F6965654 for ; Tue, 24 Nov 2015 18:30:00 +0000 (UTC) References: <1448377362-18117-1-git-send-email-berrange@redhat.com> <1448377362-18117-5-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <5654ACA3.6030907@redhat.com> Date: Tue, 24 Nov 2015 11:29:55 -0700 MIME-Version: 1.0 In-Reply-To: <1448377362-18117-5-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UHPd9BmrUX8bW93GB1gKHDxrg6SIrDwLI" Subject: Re: [Qemu-devel] [PATCH v2 4/5] crypto: add QCryptoSecret object class for password/key handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --UHPd9BmrUX8bW93GB1gKHDxrg6SIrDwLI Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/24/2015 08:02 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. >=20 > 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, dictionary.com says it's always 'ad hoc' (since it is literally two Latin words), even though the two are always spoken together in English. And although 'ad-hoc' is starting to gain some traction in modern usage, squashing it to 'adhoc' just feels like a typo. > it is fine to provide the secrets directly without encryption > so this is not explicitly forbidden. >=20 > 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. >=20 > It also makes life easier for people who are scripting the > management of QEMU, for whom FD passing is significantly more > complex. >=20 > Providing data inline (insecure, only for adhoc dev testing) Of course, if we touch it up in one place, you should use consistent spelling throughout :) >=20 > $QEMU -object secret,id=3Dsec0,data=3Dletmein >=20 > Providing data indirectly in raw format >=20 > printf "letmein" > mypasswd.txt > $QEMU -object secret,id=3Dsec0,file=3Dmypasswd.txt >=20 > Providing data indirectly in base64 format >=20 > $QEMU -object secret,id=3Dsec0,file=3Dmykey.b64,format=3Dbase64 >=20 > Providing data with encryption >=20 > $QEMU -object secret,id=3Dmaster0,file=3Dmykey.b64,format=3Dbase64 \ > -object secret,id=3Dsec0,data=3D[base64 ciphertext],\ > keyid=3Dmaster0,iv=3D[base64 IV],format=3Dbase64 >=20 > Note that 'format' here refers to the format of the ciphertext > data. The decrypted data must always be in raw byte format. >=20 > More examples are shown in the updated docs. >=20 > Signed-off-by: Daniel P. Berrange > --- > +static void > +qcrypto_secret_load_data(QCryptoSecret *secret, > + uint8_t **output, > + size_t *outputlen, > + Error **errp) > +{ > + int fd; > + char *data =3D NULL; > + size_t offset =3D 0; > + size_t length =3D 0; > + > + *output =3D NULL; > + *outputlen =3D 0; > + > + if (secret->file) { > + if (secret->data) { > + error_setg(errp, > + "'file' and 'data' are mutually exclusive"); Is it worth trying to use a qapi flat union to make the mutual exclusion inherent in the type definition, rather than something we have to enforce manually? (I've got more experience with qapi than with Object, so my question may be nonsensical) > + return; > + } > + fd =3D qemu_open(secret->file, O_RDONLY); > + if (fd < 0) { > + error_setg_errno(errp, errno, > + "Unable to open %s", secret->file); Using error_setg_file_open() makes for a consistent message on open() failure. > + return; > + } > + while (length < (1024 * 1024)) { /* Limit secrets to 1 MB */ > + if ((length - offset) < 1024) { > + length +=3D 1024; > + data =3D g_renew(char, data, length); > + } > + ssize_t ret =3D read(fd, data + offset, length - offset); > + if (ret =3D=3D 0) { > + break; > + } > + if (ret < 0) { > + error_setg_errno(errp, errno, > + "Unable to read from %s", secret->fil= e); Does glib have a convenience function for reading contents of a file? > + close(fd); > + g_free(data); > + return; > + } > + offset +=3D ret; > + } > + close(fd); > + if (offset) { > + /* Even though data is raw 8-bit, so may contain > + * arbitrary NULs, ensure it is explicitly NUL > + * terminated */ > + *output =3D g_renew(uint8_t, data, offset + 1); > + (*output)[offset] =3D '\0'; > + *outputlen =3D offset; > + } else { > + error_setg(errp, "Secret file %s is empty", > + secret->file); > + g_free(data); > + } > + } else if (secret->data) { > + *outputlen =3D strlen(secret->data); > + *output =3D g_new(uint8_t, *outputlen + 1); > + memcpy(*output, secret->data, *outputlen + 1); > + } else { > + error_setg(errp, "Either 'file' or 'data' must be provided"); > + } > +} > + > + > +static void qcrypto_secret_decrypt(QCryptoSecret *secret, > + const uint8_t *input, > + size_t inputlen, > + uint8_t **output, > + size_t *outputlen, > + Error **errp) > +{ > + uint8_t *key =3D NULL, *ciphertext =3D NULL, *iv =3D NULL; > + size_t keylen, ciphertextlen, ivlen; > + QCryptoCipher *aes =3D NULL; > + uint8_t *plaintext =3D NULL; > + > + *output =3D NULL; > + *outputlen =3D 0; > + > + if (qcrypto_secret_lookup(secret->keyid, > + &key, &keylen, > + errp) < 0) { > + goto cleanup; > + } > + > + if (keylen !=3D 32) { > + error_setg(errp, "Key should be 32 bytes in length"); > + goto cleanup; > + } > + > + if (!secret->iv) { > + error_setg(errp, "IV is required to decrypt secret"); > + goto cleanup; > + } > + > + iv =3D (uint8_t *)g_base64_decode(secret->iv, &ivlen); Shouldn't this be using qbase64_decode()? > +++ 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. > + * > + * Providing data directly, insecurely (suitable for > + * adhoc developer testing only) Another instance. > +++ 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.6 > +## > +{ 'enum': 'QCryptoSecretFormat', > + 'prefix': 'QCRYPTO_SECRET_FORMAT', > + 'data': ['raw', 'base64']} This part is fine. > diff --git a/qemu-options.hx b/qemu-options.hx > index 0eea4ee..41c0e59 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3677,6 +3677,83 @@ Dump the network traffic on netdev @var{dev} to = the file specified by > The file format is libpcap, so it can be analyzed with tools such as t= cpdump > or Wireshark. > =20 > +@item -object secret,id=3D@var{id},data=3D@var{string},format=3D@var{r= aw|base64}[,keyid=3D@var{secretid},iv=3D@var{string}] > +@item -object secret,id=3D@var{id},file=3D@var{filename},format=3D@var= {raw|base64}[,keyid=3D@var{secretid},iv=3D@var{string}] > + > +Defines a secret to store a password, encryption key, or some other se= nsitive > +data. The senstive data can either be passed directly via the @var{dat= a} s/senstive/sensitive/ > +parameter, or indirectly via the @var{file} parameter. Using the @var{= data} > +parameter is insecure unless the sensitive data is encrypted. > + Overall looks fairly reasonable. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --UHPd9BmrUX8bW93GB1gKHDxrg6SIrDwLI Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWVKyjAAoJEKeha0olJ0NqCfYH/3HF35K4MNaxMulX04PA6iYK p3HwgRkjIIanAbBx+7r/9p5U29iBRA6LOaU29jRP3s8574MDOrYxiIkX734VXtjD BiyzttwxN1w2wt30nvYG8oxApXeKWgWNlnnZbYW1AO+DHteE/kgm0n75GEfGVjzS ag95UkPK5r7pEHrWQ4ube5HxDOxeiRqM01zvCYy3ygpx20hPx35IXKqAATfLK4eF 514pgXpX0KwiQ+FuCt6ddg2zrC3TO1bWWrosJ/zBzi7FysZ0q8ylclxxK2dTO3OV gnGh0jwRkXiJD9zLUwfWrZhvo2kIxPIo+xtgfpBuGbK/+X5cwDqiipZyDLW++c4= =kIY6 -----END PGP SIGNATURE----- --UHPd9BmrUX8bW93GB1gKHDxrg6SIrDwLI--