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" ?
> + *
> + * 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.
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.
> +
> + 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.
> + 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].
> + /* 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.
> + 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.
> +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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list