[libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's
John Ferlan
jferlan at redhat.com
Wed May 11 17:09:27 UTC 2016
On 05/11/2016 08:47 AM, Peter Krempa wrote:
> On Thu, May 05, 2016 at 15:00:40 -0400, John Ferlan wrote:
>> New APIs:
>>
>> qemuDomainGetIVKeyAlias:
>> Generate/return the secret object alias for an initialization
>> vector (IV) secret info type. This will be saved in the secret
>> info block. This will be called from qemuDomainSecretIVSetup.
>>
>> 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.
>>
>> qemuDomainSecretIVSetup: (private)
>> This API handles the details of the generation of the IV 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.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> configure.ac | 1 +
>> src/qemu/qemu_alias.c | 23 +++++++++
>> src/qemu/qemu_alias.h | 2 +
>> src/qemu/qemu_domain.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 163 insertions(+)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 88e2e20..3cabd5e 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1264,6 +1264,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 ade2033..de9a74f 100644
>> --- a/src/qemu/qemu_alias.c
>> +++ b/src/qemu/qemu_alias.c
>> @@ -556,3 +556,26 @@ qemuDomainGetMasterKeyAlias(void)
>>
>> return alias;
>> }
>> +
>> +
>> +/* qemuDomainGetIVKeyAlias:
>> + *
>> + * Generate and return an initialization vector alias
>> + *
>> + * Returns NULL or a string containing the IV key alias
>> + */
>> +char *
>> +qemuDomainGetIVKeyAlias(const char *srcalias)
>> +{
>> + char *alias;
>> +
>> + if (!srcalias) {
>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("secret iv alias requires valid source alias"));
>> + return NULL;
>> + }
>> +
>> + ignore_value(virAsprintf(&alias, "%s-ivKey0", srcalias));
>> +
>> + return alias;
>> +}
>> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
>> index 2d7bc9b..435747e 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 *qemuDomainGetIVKeyAlias(const char *srcalias);
>> +
>> #endif /* __QEMU_ALIAS_H__*/
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index cae356c..b174caa 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -893,6 +893,143 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
>> }
>>
>>
>> +/* qemuDomainSecretHaveEncypt:
>> + *
>> + * Returns true if we can support the encryption code, false otherwise
>> + */
>> +static bool ATTRIBUTE_UNUSED
>> +qemuDomainSecretHaveEncrypt(void)
>> +{
>> +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
>> + return true;
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> +
>> +/* qemuDomainSecretIVSetup:
>> + * @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 initialization vector information
>> + *
>> + * Returns 0 on success, -1 on failure with error message
>> + */
>> +static int ATTRIBUTE_UNUSED
>> +qemuDomainSecretIVSetup(virConnectPtr conn,
>> + qemuDomainObjPrivatePtr priv,
>> + qemuDomainSecretInfoPtr secinfo,
>> + const char *srcalias,
>> + virStorageNetProtocol protocol,
>> + virStorageAuthDefPtr authdef)
>> +{
>> + int ret = -1;
>> + int rc;
>> + uint8_t *raw_iv = NULL;
>> + char *secret = NULL;
>> + uint8_t *ciphertext = NULL;
>> + size_t secretlen = 0, ciphertextlen = 0, paddinglen;
>
> Please one variable per line.
>
OK
>> + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
>
> Rather than changing to RBD below I think the decision should be done in
> a single swithch.
>
OK - a switch it is.
>> + const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
>> + gnutls_cipher_hd_t handle = NULL;
>> + gnutls_datum_t enc_key;
>> + gnutls_datum_t iv_key;
>
> This function is compiled even without gnutls.
>
True, but it's not called unless qemuDomainSecretHaveEncrypt returns
true, which of course is the "next" patch only because I was trying to
keep the changes to a minimum. I left it this way since "theoretically
speaking" something could be added.
Shall I assume you'd prefer a function for each? That means a function
with a bunch of ATTRIBUTE_UNUSED for each argument, a virReportError,
and a -1 return.
>> +
>> + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
>> + if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
>> + return -1;
>> +
>> + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
>> + secretType = VIR_SECRET_USAGE_TYPE_CEPH;
>> +
>> + if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
>> + return -1;
>> +
>> + /* Create a random initialization vector */
>> + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
>> + return -1;
>> +
>> + /* Encode the IV and save that since qemu will need it */
>
> So the initialization vector or IV is a specific of the used cipher. I
> don't think we should use it in the names for the secret type.
>
> Either the name of the cipher should be used or just "secret" if it's
> unlikey that qemu would add new ciphers.
>
The IV is just a 16 byte random key and yes, currently generated by
either the gnutls_rnd if available; otherwise, by virRandomBytes if not.
I'm not sure it matters "how" it was generated since we pass it in on
the command line in an argument "iv=" for the secret. of course for
this implementation, we're pretty much guaranteed that gnutls_rnd
generated the iv.
Which "iv" name do you want changed? The union or one in the struct? I
think you want the union name changed, but I'm not 100% sure - so better
to ask. Based on your comment that means you'd rather see soemthing like
"_qemuDomainSecretAES_256_CBC" instead of "_qemuDomainSecretIV"? And
then use "secret" instead of "iv" in the union?
>> + base64_encode_alloc((const char *)raw_iv, QEMU_DOMAIN_IV_KEY_LEN,
>> + &secinfo->s.iv.iv);
>> + if (!secinfo->s.iv.iv) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to encode initialization vector"));
>> + goto cleanup;
>> + }
>> +
>> + /* Grab the unencoded secret */
>> + if (!(secret = virSecretGetSecretString(conn, protocolstr, false,
>
> In qemuDomainSecretPlainSetup the secret is requested encoded for RBD,
> thus it was possible to use non ascii keys ...
>
Ug... Looks like virSecretGetSecretString needs another parameter to
return the secretlen and as Dan points out the uint8_t * instead of char
*. That'll add more patches to the pile...
>> + authdef, secretType)))
>> + goto cleanup;
>> +
>> + /* Allocate a padded buffer */
>> + secretlen = strlen(secret);
>
> ... which won't work with this.
>
>> + paddinglen = 16 - (secretlen % 16);
>
> Magic constants?
>
Black magic constants!
>> + ciphertextlen = secretlen + paddinglen;
>
> VIR_ROUND_UP
>
Ok sure...
>> + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
>> + goto cleanup;
>> + memcpy(ciphertext, secret, secretlen);
>> + memset(ciphertext + secretlen, paddinglen, paddinglen);
>
> You are padding it by the last byte of the padding size? Is this really
> desired?
>
What I read on this for this encryption algorithm is that the size needs
to be 16 byte aligned and that the "filler" bytes need to be the size of
the padding. Look at the qemu source for qcrypto_secret_decrypt.
>> +
>> + /* clear secret so that it doesn't hang around */
>> + memset(secret, 0, strlen(secret));
>
> You do it again in cleanup. Is it really necessary?
>
I can remove this one.
John
>> +
>> + /* Initialize the gnutls cipher */
>> + enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN;
>> + enc_key.data = priv->masterKey;
>> + iv_key.size = QEMU_DOMAIN_IV_KEY_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 base64 encode the ciphertext and store to be passed to qemu */
>> + base64_encode_alloc((const char *)ciphertext, ciphertextlen,
>> + &secinfo->s.iv.ciphertext);
>> + if (!secinfo->s.iv.ciphertext) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to encode ciphertext"));
>> + goto cleanup;
>> + }
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> +
>> + /* Clear and free the raw_iv, plaintext secret, and ciphertext */
>> + memset(raw_iv, 0, QEMU_DOMAIN_IV_KEY_LEN);
>> + VIR_FREE(raw_iv);
>> +
>> + memset(secret, 0, secretlen);
>> + VIR_FREE(secret);
>> +
>> + memset(ciphertext, 0, ciphertextlen);
>> + VIR_FREE(ciphertext);
>> +
>> + return ret;
>
> Peter
>
More information about the libvir-list
mailing list