[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4 2/3] qemu: Introduce new Secret AES API's




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 redhat com>
>> ---
>>  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 redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]