On 05/18/2016 03:16 AM, Peter Krempa wrote:
> On Tue, May 17, 2016 at 12:36:08 -0400, John Ferlan wrote:
>> New APIs:
>>
>> qemuDomainGetAESKeyAlias:
>> Generate/return the secret object alias for an AES Secret Info type.
>> This will be called from qemuDomainSecretAESSetup.
>>
>> qemuDomainSecretHaveEncrypt: (private)
>> Boolean function to determine whether the underly encryption
>> API is available. This function will utilize a similar mechanism
>> as the 'gnutls_rnd' did in configure.ac.
>>
>> qemuDomainSecretAESSetup: (private)
>> This API handles the details of the generation of the AES secret
>> and saves the pieces that need to be passed to qemu in order for
>> the secret to be decrypted. The encrypted secret based upon the
>> domain master key, an initialization vector (16 byte random value),
>> and the stored secret. Finally, the requirement from qemu is the IV
>> and encrypted secret are to be base64 encoded. They can be passed
>> either directly or within a file. This implementation chooses
>> to pass directly rather than a file.
>>
>> If the gnutls_cipher_init is not available, then an alternate API
>> returning -1 has been created.
>>
>> qemuDomainSecretSetup: (private)
>> Shim to call either the AES or Plain Setup functions based upon
>> whether AES secrets are possible (we have the encryption API) or not,
>> we have secrets, and of course if the protocol source is RBD.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> configure.ac | 1 +
>> src/qemu/qemu_alias.c | 23 ++++++
>> src/qemu/qemu_alias.h | 2 +
>> src/qemu/qemu_domain.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 217 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 378069d..10fbd20 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1261,6 +1261,7 @@ if test "x$with_gnutls" != "xno"; then
>> ]])
>>
>> AC_CHECK_FUNCS([gnutls_rnd])
>> + AC_CHECK_FUNCS([gnutls_cipher_encrypt])
>>
>> CFLAGS="$old_CFLAGS"
>> LIBS="$old_LIBS"
>> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
>> index cb102ec..bd1a694 100644
>> --- a/src/qemu/qemu_alias.c
>> +++ b/src/qemu/qemu_alias.c
>> @@ -482,3 +482,26 @@ qemuDomainGetMasterKeyAlias(void)
>>
>> return alias;
>> }
>> +
>> +
>> +/* qemuDomainGetAESKeyAlias:
>> + *
>> + * Generate and return an initialization vector alias
>
> This comment doesn't make sense. Perhaps "encrypted secret key alias" ?
>
(*forehead smack*) - I searched for all IV occurrences, but forgot to do
the same for 'iv' and 'initialization vector'
>> + *
>> + * Returns NULL or a string containing the AES key alias
>> + */
>> +char *
>> +qemuDomainGetAESKeyAlias(const char *srcalias)
>> +{
>> + char *alias;
>> +
>> + if (!srcalias) {
>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("secret iv alias requires valid source alias"));
>
> ... same here.
>
>> + return NULL;
>> + }
>> +
>> + ignore_value(virAsprintf(&alias, "%s-aesKey0", srcalias));
>> +
>> + return alias;
>> +}
>> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
>> index 2d7bc9b..c31643d 100644
>> --- a/src/qemu/qemu_alias.h
>> +++ b/src/qemu/qemu_alias.h
>> @@ -69,4 +69,6 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk);
>>
>> char *qemuDomainGetMasterKeyAlias(void);
>>
>> +char *qemuDomainGetAESKeyAlias(const char *srcalias);
>> +
>> #endif /* __QEMU_ALIAS_H__*/
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 0733872..edc3ac7 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -47,7 +47,6 @@
>> #include "virprocess.h"
>> #include "virrandom.h"
>> #include "secret_util.h"
>> -#include "base64.h"
>> #ifdef WITH_GNUTLS
>> # include <gnutls/gnutls.h>
>> # if HAVE_GNUTLS_CRYPTO_H
>> @@ -884,6 +883,197 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
>> }
>>
>>
>> +/* qemuDomainSecretHaveEncrypt:
>> + *
>> + * Returns true if we can support the encryption code, false otherwise
>> + */
>> +static bool
>> +qemuDomainSecretHaveEncrypt(void)
>
> [3]
>
>> +{
>> +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
>> + return true;
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> +
>> +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
>> +/* qemuDomainSecretAESSetup:
>> + * @conn: Pointer to connection
>> + * @priv: pointer to domain private object
>> + * @secinfo: Pointer to secret info
>> + * @srcalias: Alias of the disk/hostdev used to generate the secret alias
>> + * @protocol: Protocol for secret
>> + * @authdef: Pointer to auth data
>> + *
>> + * Taking a secinfo, fill in the AES specific information using the
>> + * gnutls_cipher_encrypt API algorithm for GNUTLS_CIPHER_AES_256_CBC
>> + *
>> + * Returns 0 on success, -1 on failure with error message
>> + */
>> +static int
>> +qemuDomainSecretAESSetup(virConnectPtr conn,
>> + qemuDomainObjPrivatePtr priv,
>> + qemuDomainSecretInfoPtr secinfo,
>> + const char *srcalias,
>> + virStorageNetProtocol protocol,
>> + virStorageAuthDefPtr authdef)
>> +{
>> + int ret = -1;
>> + int rc;
>> + uint8_t *raw_iv = NULL;
>> + uint8_t *secret = NULL;
>> + uint8_t *ciphertext = NULL;
>> + size_t secretlen = 0;
>> + size_t ciphertextlen = 0;
>> + size_t i;
>> + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
>> + gnutls_cipher_hd_t handle = NULL;
>> + gnutls_datum_t enc_key;
>> + gnutls_datum_t iv_key;
>
> If you split out the gnutls stuff into a util function (in
> util/vircrypto.c which sounds lile the right place ) that will do the
> AES encryption you will not need to taint the qemu code with a lot of
> conditionally compiled code.
OK - that's fine... patches forthcoming...
>
> In addition you could also split out the helper that is using gnutls to
> generate better key since it seems that [2] is not really qemu
> specific. [3] can be added there too as a witness that we can encrypt
> stuff.
>
[2] isn't qemu specific, but the base64 encoded value is required to be
supplied on the command line so this code will need to know what it is.
>> +
>> + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES;
>> + if (VIR_STRDUP(secinfo->s.aes.username, authdef->username) < 0)
>> + return -1;
>> +
>> + switch ((virStorageNetProtocol)protocol) {
>> + case VIR_STORAGE_NET_PROTOCOL_RBD:
>> + secretType = VIR_SECRET_USAGE_TYPE_CEPH;
>> + break;
>> +
>> + case VIR_STORAGE_NET_PROTOCOL_NONE:
>> + case VIR_STORAGE_NET_PROTOCOL_NBD:
>> + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
>> + case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
>> + case VIR_STORAGE_NET_PROTOCOL_ISCSI:
>> + case VIR_STORAGE_NET_PROTOCOL_HTTP:
>> + case VIR_STORAGE_NET_PROTOCOL_HTTPS:
>> + case VIR_STORAGE_NET_PROTOCOL_FTP:
>> + case VIR_STORAGE_NET_PROTOCOL_FTPS:
>> + case VIR_STORAGE_NET_PROTOCOL_TFTP:
>> + case VIR_STORAGE_NET_PROTOCOL_LAST:
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("unsupported protocol '%s' for initialization vector"),
>
> This error message doesn't make sense at all. "protocol %s can't be used
> with encrypted secrets" perhaps.
>
Right ... changed...
>> + virStorageNetProtocolTypeToString(protocol));
>> + return -1;
>> + }
>> +
>> + if (!(secinfo->s.aes.alias = qemuDomainGetAESKeyAlias(srcalias)))
>> + return -1;
>> +
>> + /* Create a random initialization vector */
>
> [1]
>
>> + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_AES_IV_LEN)))
>
> [2]
>
>> + return -1;
>> +
>> + /* Encode the IV and save that since qemu will need it */
>
> [1]
>
>> + if (!(secinfo->s.aes.iv = virStringEncodeBase64(raw_iv,
>> + QEMU_DOMAIN_AES_IV_LEN)))
>> + goto cleanup;
>> +
>> + /* Grab the unencoded secret */
>
> [1]
>
>> + if (virSecretGetSecretString(conn, authdef, secretType,
>> + &secret, &secretlen) < 0)
>> + goto cleanup;
>> +
>> + /* Allocate a padded buffer */
>> + ciphertextlen = VIR_ROUND_UP(secretlen, 16);
>> + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
>> + goto cleanup;
>> + memcpy(ciphertext, secret, secretlen);
>> + for (i = secretlen; i < ciphertextlen; i++)
>> + ciphertext[i] = ciphertextlen - secretlen;
>
> The memset here was okay along with this. What I've asked for is to add
> a comment for one of the few non-obvious parts of the code once you
> didn't spare comments for the obvious stuff [1].
>
So the comment "/* this is black magic */" is sufficient, right? ;-)
I can be less verbose on the [1] comments though. They are derived when
I'm putting together the code so I don't forget a step...
>> + /* Initialize the gnutls cipher */
>> + enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN;
>> + enc_key.data = priv->masterKey;
>> + iv_key.size = QEMU_DOMAIN_AES_IV_LEN;
>> + iv_key.data = raw_iv;
>> + if ((rc = gnutls_cipher_init(&handle, GNUTLS_CIPHER_AES_256_CBC,
>> + &enc_key, &iv_key)) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to initialize cipher: '%s'"),
>> + gnutls_strerror(rc));
>> + goto cleanup;
>> + }
>> +
>> + /* Encrypt the secret and free the memory for cipher operations */
>> + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
>> + gnutls_cipher_deinit(handle);
>> + if (rc < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to encrypt the secret: '%s'"),
>> + gnutls_strerror(rc));
>> + goto cleanup;
>> + }
>> +
>> + /* Now encode the ciphertext and store to be passed to qemu */
>> + if (!(secinfo->s.aes.ciphertext = virStringEncodeBase64(ciphertext,
>> + ciphertextlen)))
>> + goto cleanup;
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> + VIR_FREE(raw_iv);
>
> Since you are wiping the ciphertext you can wipe the IV too.
>
I think I tried that, but VIR_DISPOSE_N didn't seem to be happy with
receiving QEMU_DOMAIN_AES_IV_LEN
>> + VIR_DISPOSE_N(secret, secretlen);
>> + VIR_DISPOSE_N(ciphertext, ciphertextlen);
>> +
>> + return ret;
>> +}
>> +#else
>> +static int
>> +qemuDomainSecretAESSetup(virConnectPtr conn ATTRIBUTE_UNUSED,
>> + qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
>> + qemuDomainSecretInfoPtr secinfo ATTRIBUTE_UNUSED,
>> + const char *srcalias ATTRIBUTE_UNUSED,
>> + virStorageNetProtocol protocol ATTRIBUTE_UNUSED,
>> + virStorageAuthDefPtr authdef ATTRIBUTE_UNUSED)
>> +{
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("gnutls_cipher_encrypt unsupported"));
>> + return -1;
>> +}
>> +#endif
>> +
>> +
>> +/* qemuDomainSecretSetup:
>> + * @conn: Pointer to connection
>> + * @priv: pointer to domain private object
>> + * @secinfo: Pointer to secret info
>> + * @srcalias: Alias of the disk/hostdev used to generate the secret alias
>> + * @protocol: Protocol for secret
>> + * @authdef: Pointer to auth data
>> + *
>> + * If we have the encryption API present and can support a secret object, then
>> + * build the AES secret; otherwise, build the Plain secret. This is the magic
>> + * decision point for utilizing the AES secrets for an RBD disk. For now iSCSI
>> + * disks and hostdevs will not be able to utilize this mechanism.
>> + *
>> + * Returns 0 on success, -1 on failure
>> + */
>> +static int ATTRIBUTE_UNUSED
>
> If you add this function in a separate patch and it will call just
> qemuDomainSecretPlainSetup at that point you will not have to make it
> unused here and the next patch can merge the few simpler functions in
> one take.
>
Sure - just a different way of getting there...
John
>> +qemuDomainSecretSetup(virConnectPtr conn,
>> + qemuDomainObjPrivatePtr priv,
>> + qemuDomainSecretInfoPtr secinfo,
>> + const char *srcalias,
>> + virStorageNetProtocol protocol,
>> + virStorageAuthDefPtr authdef)
>> +{
>> + if (qemuDomainSecretHaveEncrypt() &&
>> + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
>> + protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
>> + if (qemuDomainSecretAESSetup(conn, priv, secinfo,
>> + srcalias, protocol, authdef) < 0)
>> + return -1;
>> + } else {
>> + if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0)
>> + return -1;
>> + }
>> + return 0;
>> +}
>> +
>> +
>> /* qemuDomainSecretDiskDestroy:
>> * @disk: Pointer to a disk definition
>> *
>> --
>> 2.5.5
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list