[libvirt] [PATCH v4 0/3] Add AES Secret Object support (for RBD only)

v3: http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html Changes since v3 (by patch)... 1. Separate out the mock for virRandomBytes. I realize this is not the desired state, but figured to at least be prepared for what would be coming as a followup to: http://www.redhat.com/archives/libvir-list/2016-May/msg00735.html that I'd at least adjust the code to follow using a different random number generator (use increasing i instead of just 0xff for all) 2. Changes from code review... Change name from IV to AES, plus others qemuDomainSecretAESSetup is variable based on HAVE_GNUTLS_CIPHER_ENCRYPT Changes to utilize virStringEncodeBase64 Moved qemuDomainSecretSetup into this version (and made i 3. Split out mock (into patch 1) Comment cleanup Still unresolved from code review is a review comment in qemuBuildDriveStr regarding where the desire movement was. There's a very specific order to where the "password-secret=%s," can be placed. It just wasn't clear from the review comment where exactly it was expected to "move" those lines John Ferlan (3): tests: Add mock for virRandomBytes [NOT TO BE PUSHED] qemu: Introduce new Secret AES API's qemu: Utilize qemu secret objects for RBD auth/secret configure.ac | 1 + src/qemu/qemu_alias.c | 23 +++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 122 +++++++++++- src/qemu/qemu_domain.c | 206 ++++++++++++++++++++- ...muxml2argv-disk-drive-network-rbd-auth-AES.args | 31 ++++ ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml | 42 +++++ tests/qemuxml2argvmock.c | 31 +++- tests/qemuxml2argvtest.c | 3 + 9 files changed, 451 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml -- 2.5.5

A different model will be posted soon... Essentially this is way to test/show that a not so random key and iv will generate a consistent value. This version uses the setting of the key to the array index rather than -1 for every entry. Thus a key/iv would be 0x000102030405... etc. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuxml2argvmock.c | 31 ++++++++++++++++++++++++++++++- tests/qemuxml2argvtest.c | 1 + 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..dade748 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -30,6 +30,13 @@ #include "virstring.h" #include "virtpm.h" #include "virutil.h" +#include "virrandom.h" +#ifdef WITH_GNUTLS +# include <gnutls/gnutls.h> +# if HAVE_GNUTLS_CRYPTO_H +# include <gnutls/crypto.h> +# endif +#endif #include <time.h> #include <unistd.h> @@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, { /* nada */ } + +int +virRandomBytes(unsigned char *buf, + size_t buflen) +{ + size_t i; + + for (i = 0; i < buflen; i++) + buf[i] = i; + + return 0; +} + +#ifdef WITH_GNUTLS +int +gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED, + void *data, + size_t len) +{ + return virRandomBytes(data, len); +#endif +} diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db42f0b..25caca7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -26,6 +26,7 @@ # include "virstring.h" # include "storage/storage_driver.h" # include "virmock.h" +# include "virrandom.h" # include "testutilsqemu.h" -- 2.5.5

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 + * + * 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")); + 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) +{ +#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; + + 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"), + virStorageNetProtocolTypeToString(protocol)); + return -1; + } + + if (!(secinfo->s.aes.alias = qemuDomainGetAESKeyAlias(srcalias))) + return -1; + + /* Create a random initialization vector */ + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_AES_IV_LEN))) + return -1; + + /* Encode the IV and save that since qemu will need it */ + if (!(secinfo->s.aes.iv = virStringEncodeBase64(raw_iv, + QEMU_DOMAIN_AES_IV_LEN))) + goto cleanup; + + /* Grab the unencoded secret */ + 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; + + /* 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); + 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 +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

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" ?
+ * + * 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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

On Wed, May 18, 2016 at 18:45:21 -0400, John Ferlan wrote:
On 05/18/2016 03:16 AM, Peter Krempa wrote:
On Tue, May 17, 2016 at 12:36:08 -0400, John Ferlan wrote:
[...]
+ + /* 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? ;-)
Almost. It'd at least switch me from "this looks wrong" to "oh, something interresting is going on (also it's expected)".
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...
I just tried to point out that there are certain places where comments are actually needed and that is really one of them. The rest doesn't hurt but is obvious compared to this magic part.
+ /* 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;
Also while changing this. s/iv_key/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
Oh right. I chose to clear the second argument too in VIR_DISPOSE_N so it has to be an L-value. Don't bother changing this then. IV is still going to be put on the command line.

On Tue, May 17, 2016 at 12:36:08 -0400, John Ferlan wrote: [...]
+/* qemuDomainGetAESKeyAlias: + * + * Generate and return an initialization vector 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")); + return NULL; + } + + ignore_value(virAsprintf(&alias, "%s-aesKey0", srcalias));
So this will be part of the following command line: -object secret,id=virtio-disk0-aesKey0,\ data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 The object added represents the secret for a given disk, not the AES key or anything else. The secret is encrypted using the AES key which has alias 'masterKey0'. I'm thinking that something along "virtio-disk0-secret0" might be a better match. Peter

https://bugzilla.redhat.com/show_bug.cgi?id=1182074 If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for RBD volumes instead of passing the base64 encoded secret on the command line. The goal is to make AES secrets the default and have no user interaction required in order to allow using the AES mechanism. If the mechanism is not available, then fall back to the current plain mechanism using a base64 encoded secret. New API's: qemu_command.c: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the AES secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense. qemuBuildSecretAESCommandLine (private) Generate and add to the command line the -object secret for the AES secret. This will be required for the subsequent RBD reference to the object. qemuBuildDiskSecinfoCommandLine (private) Handle adding the AES secret object. Adjustments: Command Building: Adjust the qemuBuildRBDSecinfoURI API's in order to generate the specific command options for an AES secret, such as: -object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ mon_host=mon1.example.org\:6321,password-secret=$alias,... where the 'id=' value is the secret object alias generated by concatenating the disk alias and "-aesKey0". The 'keyid= $masterKey' is the master key shared with qemu, and the -drive syntax will reference that alias as the 'password-secret'. For the -drive syntax, the 'id=myname' is kept to define the username, while the 'key=$base64 encoded secret' is removed. While according to the syntax described for qemu commit '60390a21' or as seen in the email archive: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html it is possible to pass a plaintext password via a file, the qemu commit 'ac1d8878' describes the more feature rich 'keyid=' option based upon the shared masterKey. qemu_domain.c Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and qemuDomainSecretHostdevPrepare to setup the secret rather than assuming plain secret setup. Add test for checking/comparing output. NB: For hotplug, since the hotplug code doesn't add command line arguments, passing the encoded secret directly to the monitor will suffice. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 122 ++++++++++++++++++++- src/qemu/qemu_domain.c | 16 +-- ...muxml2argv-disk-drive-network-rbd-auth-AES.args | 31 ++++++ ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml | 42 +++++++ tests/qemuxml2argvtest.c | 2 + 5 files changed, 204 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a2fa1d..e2e0b3c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -616,6 +616,110 @@ qemuNetworkDriveGetPort(int protocol, } +/** + * qemuBuildSecretInfoProps: + * @secinfo: pointer to the secret info object + * @type: returns a pointer to a character string for object name + * @props: json properties to return + * + * Build the JSON properties for the secret info type. + * + * Returns 0 on success with the filled in JSON property; otherwise, + * returns -1 on failure error message set. + */ +static int +qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, + const char **type, + virJSONValuePtr *propsret) +{ + int ret = -1; + char *keyid = NULL; + virJSONValuePtr props = NULL; + + *type = "secret"; + + if (!(keyid = qemuDomainGetMasterKeyAlias())) + return -1; + + if (virJSONValueObjectCreate(&props, + "s:data", secinfo->s.aes.ciphertext, + "s:keyid", keyid, + "s:iv", secinfo->s.aes.iv, + "s:format", "base64", NULL) < 0) + goto cleanup; + + *propsret = props; + props = NULL; + ret = 0; + + cleanup: + VIR_FREE(keyid); + virJSONValueFree(props); + + return ret; +} + + +/** + * qemuBuildSecretAESCommandLine: + * @cmd: the command to modify + * @secinfo: pointer to the secret info object + * + * If the secinfo is available and associated with an AES secret, + * then format the command line for the secret object. This object + * will be referenced by the device that needs/uses it, so it needs + * to be in place first. + * + * Returns 0 on success, -1 w/ error message on failure + */ +static int +qemuBuildSecretAESCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + int ret = -1; + virJSONValuePtr props = NULL; + const char *type; + char *tmp = NULL; + + if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) + return -1; + + if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.aes.alias, + props))) + goto cleanup; + + virCommandAddArgList(cmd, "-object", tmp, NULL); + ret = 0; + + cleanup: + virJSONValueFree(props); + VIR_FREE(tmp); + + return ret; +} + + +/* qemuBuildDiskSecinfoCommandLine: + * @cmd: Pointer to the command string + * @secinfo: Pointer to a possible secinfo + * + * Add the secret object for the disks that will be using it to perform + * their authentication. + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + /* Not necessary for non AES secrets */ + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_AES) + return 0; + + return qemuBuildSecretAESCommandLine(cmd, secinfo); +} + + /* qemuBuildGeneralSecinfoURI: * @uri: Pointer to the URI structure to add to * @secinfo: Pointer to the secret info data (if present) @@ -697,6 +801,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, break; case VIR_DOMAIN_SECRET_INFO_TYPE_AES: + virBufferEscape(buf, '\\', ":", ":id=%s:auth_supported=cephx\\;none", + secinfo->s.aes.username); + break; + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -1094,6 +1202,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, char *source = NULL; int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1175,7 +1284,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; } - if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0) + if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0) goto error; if (source && @@ -1227,6 +1336,12 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, qemuBufferEscapeComma(&opt, source); virBufferAddLit(&opt, ","); + if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD && + secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + virBufferAsprintf(&opt, "password-secret=%s,", + secinfo->s.aes.alias); + } + if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) virBufferAsprintf(&opt, "format=%s,", @@ -1906,6 +2021,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk = def->disks[i]; bool withDeviceArg = false; bool deviceFlagMasked = false; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; /* Unless we have -device, then USB disks need special handling */ @@ -1948,6 +2065,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, break; } + if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); /* Unfortunately it is not possible to use diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index edc3ac7..ac3ecd1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1052,7 +1052,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn ATTRIBUTE_UNUSED, * * Returns 0 on success, -1 on failure */ -static int ATTRIBUTE_UNUSED +static int qemuDomainSecretSetup(virConnectPtr conn, qemuDomainObjPrivatePtr priv, qemuDomainSecretInfoPtr secinfo, @@ -1102,7 +1102,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) */ int qemuDomainSecretDiskPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, + qemuDomainObjPrivatePtr priv, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; @@ -1119,8 +1119,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol, - src->auth) < 0) + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + src->protocol, src->auth) < 0) goto error; diskPriv->secinfo = secinfo; @@ -1163,7 +1163,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) */ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, + qemuDomainObjPrivatePtr priv, virDomainHostdevDefPtr hostdev) { virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; @@ -1184,9 +1184,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretPlainSetup(conn, secinfo, - VIR_STORAGE_NET_PROTOCOL_ISCSI, - iscsisrc->auth) < 0) + if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias, + VIR_STORAGE_NET_PROTOCOL_ISCSI, + iscsisrc->auth) < 0) goto error; hostdevPriv->secinfo = secinfo; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args new file mode 100644 index 0000000..ef9c968 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object secret,id=virtio-disk0-aesKey0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ +password-secret=virtio-disk0-aesKey0,format=raw,if=none,id=drive-virtio-disk0' \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml new file mode 100644 index 0000000..ac2e942 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml @@ -0,0 +1,42 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 25caca7..db2f781 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -776,6 +776,8 @@ mymain(void) DO_TEST("disk-drive-network-rbd", NONE); DO_TEST("disk-drive-network-sheepdog", NONE); DO_TEST("disk-drive-network-rbd-auth", NONE); + DO_TEST("disk-drive-network-rbd-auth-AES", + QEMU_CAPS_OBJECT_SECRET); DO_TEST("disk-drive-network-rbd-ipv6", NONE); DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); DO_TEST("disk-drive-no-boot", -- 2.5.5

On Tue, May 17, 2016 at 12:36:09 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1182074
If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for RBD volumes instead of passing the base64 encoded secret on the command line.
The goal is to make AES secrets the default and have no user interaction required in order to allow using the AES mechanism. If the mechanism is not available, then fall back to the current plain mechanism using a base64 encoded secret.
New API's:
qemu_command.c: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the AES secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense.
qemuBuildSecretAESCommandLine (private) Generate and add to the command line the -object secret for the AES secret. This will be required for the subsequent RBD reference to the object.
qemuBuildDiskSecinfoCommandLine (private) Handle adding the AES secret object.
Adjustments:
Command Building: Adjust the qemuBuildRBDSecinfoURI API's in order to generate the specific command options for an AES secret, such as:
-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ mon_host=mon1.example.org\:6321,password-secret=$alias,...
where the 'id=' value is the secret object alias generated by concatenating the disk alias and "-aesKey0". The 'keyid= $masterKey' is the master key shared with qemu, and the -drive syntax will reference that alias as the 'password-secret'. For the -drive syntax, the 'id=myname' is kept to define the username, while the 'key=$base64 encoded secret' is removed.
While according to the syntax described for qemu commit '60390a21' or as seen in the email archive:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
it is possible to pass a plaintext password via a file, the qemu commit 'ac1d8878' describes the more feature rich 'keyid=' option based upon the shared masterKey.
qemu_domain.c Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and qemuDomainSecretHostdevPrepare to setup the secret rather than assuming plain secret setup.
Add test for checking/comparing output.
NB: For hotplug, since the hotplug code doesn't add command line arguments, passing the encoded secret directly to the monitor will suffice.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 122 ++++++++++++++++++++- src/qemu/qemu_domain.c | 16 +-- ...muxml2argv-disk-drive-network-rbd-auth-AES.args | 31 ++++++ ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml | 42 +++++++ tests/qemuxml2argvtest.c | 2 + 5 files changed, 204 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a2fa1d..e2e0b3c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -616,6 +616,110 @@ qemuNetworkDriveGetPort(int protocol, }
+/** + * qemuBuildSecretInfoProps: + * @secinfo: pointer to the secret info object + * @type: returns a pointer to a character string for object name + * @props: json properties to return + * + * Build the JSON properties for the secret info type. + * + * Returns 0 on success with the filled in JSON property; otherwise, + * returns -1 on failure error message set. + */ +static int +qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, + const char **type, + virJSONValuePtr *propsret) +{ + int ret = -1; + char *keyid = NULL; + virJSONValuePtr props = NULL; + + *type = "secret"; + + if (!(keyid = qemuDomainGetMasterKeyAlias())) + return -1; + + if (virJSONValueObjectCreate(&props,
Using 'propsret' directly avoids the temp variable.
+ "s:data", secinfo->s.aes.ciphertext, + "s:keyid", keyid, + "s:iv", secinfo->s.aes.iv, + "s:format", "base64", NULL) < 0) + goto cleanup; + + *propsret = props; + props = NULL; + ret = 0; + + cleanup: + VIR_FREE(keyid); + virJSONValueFree(props);
This is dead code. props will either be NULL if virJSONValueObjectCreate succeeds and thus is returned or will be NULL if virJSONValueObjectCreate fails as no other code is jumping to cleanup.
+ + return ret; +} + + +/** + * qemuBuildSecretAESCommandLine:
qemuBuildObjectSecretCommandLine perhaps? At this point it doesn't matter which algorithm was used since that is encoded in secinfo and qemuBuildSecretInfoProps should do the correct thing encoding the type right away. This code is building the object command line for a object type = secret.
+ * @cmd: the command to modify + * @secinfo: pointer to the secret info object + * + * If the secinfo is available and associated with an AES secret, + * then format the command line for the secret object. This object + * will be referenced by the device that needs/uses it, so it needs + * to be in place first. + * + * Returns 0 on success, -1 w/ error message on failure + */ +static int +qemuBuildSecretAESCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + int ret = -1; + virJSONValuePtr props = NULL; + const char *type; + char *tmp = NULL; + + if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) + return -1; + + if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.aes.alias, + props))) + goto cleanup; + + virCommandAddArgList(cmd, "-object", tmp, NULL); + ret = 0; + + cleanup: + virJSONValueFree(props); + VIR_FREE(tmp); + + return ret; +} + + +/* qemuBuildDiskSecinfoCommandLine: + * @cmd: Pointer to the command string + * @secinfo: Pointer to a possible secinfo + * + * Add the secret object for the disks that will be using it to perform + * their authentication. + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + /* Not necessary for non AES secrets */ + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_AES) + return 0; + + return qemuBuildSecretAESCommandLine(cmd, secinfo); +} + + /* qemuBuildGeneralSecinfoURI: * @uri: Pointer to the URI structure to add to * @secinfo: Pointer to the secret info data (if present) @@ -697,6 +801,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, break;
case VIR_DOMAIN_SECRET_INFO_TYPE_AES: + virBufferEscape(buf, '\\', ":", ":id=%s:auth_supported=cephx\\;none", + secinfo->s.aes.username); + break; + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -1094,6 +1202,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, char *source = NULL; int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1175,7 +1284,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; }
- if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0) + if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0) goto error;
if (source && @@ -1227,6 +1336,12 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, qemuBufferEscapeComma(&opt, source); virBufferAddLit(&opt, ",");
+ if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
Looks like this point will not be specific for RBD but rather for anything using the secret object to store the password so the first part of the condition should not be necessary.
+ secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + virBufferAsprintf(&opt, "password-secret=%s,", + secinfo->s.aes.alias); + } +
Peter

On Tue, May 17, 2016 at 12:36:06 -0400, John Ferlan wrote:
v3:
http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html
Changes since v3 (by patch)...
1. Separate out the mock for virRandomBytes. I realize this is not the desired state, but figured to at least be prepared for what would be coming as a followup to:
http://www.redhat.com/archives/libvir-list/2016-May/msg00735.html
Looking at it back I think that patch is okay by itself. Adding the mock for virRandomBytes to deal with UUID generation is going to be worth a separate patch with separate justification to take care of different parts of the test suite. Peter

On 05/18/2016 02:55 AM, Peter Krempa wrote:
On Tue, May 17, 2016 at 12:36:06 -0400, John Ferlan wrote:
v3:
http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html
Changes since v3 (by patch)...
1. Separate out the mock for virRandomBytes. I realize this is not the desired state, but figured to at least be prepared for what would be coming as a followup to:
http://www.redhat.com/archives/libvir-list/2016-May/msg00735.html
Looking at it back I think that patch is okay by itself. Adding the mock for virRandomBytes to deal with UUID generation is going to be worth a separate patch with separate justification to take care of different parts of the test suite.
OK - I can make it official; however, I was trying to figure out a way to be able to add a test to ensure that the "mocked" function was called from the tests... That whole environment is BFM (as I'm sure Michal can now attest). I do have something that can compile, but it gets a nefarious warning about the resulting image/library not being portable at build time. I can show you the code. The purpose for the code was in the followup patch where vircrypto.c is modified was to be able to have vircrypto.c call the mocked virRandomBytes. Right now I just have code that will fill in the masterkey and iv with a known value. It "proves" that the encryption code "works". John

On Wed, May 18, 2016 at 19:08:01 -0400, John Ferlan wrote: [...]
1. Separate out the mock for virRandomBytes. I realize this is not the desired state, but figured to at least be prepared for what would be coming as a followup to:
http://www.redhat.com/archives/libvir-list/2016-May/msg00735.html
Looking at it back I think that patch is okay by itself. Adding the mock for virRandomBytes to deal with UUID generation is going to be worth a separate patch with separate justification to take care of different parts of the test suite.
OK - I can make it official; however, I was trying to figure out a way to be able to add a test to ensure that the "mocked" function was called from the tests... That whole environment is BFM (as I'm sure Michal can now attest).
Erm I think you misunderstood. I was referring to my patch that you've linked above. I wanted to point out that the patch as-is is fine for fixing the mocking in this case and that if we want to moc virRandomBytes to be used for UUID generation and such is stuff for a separate patch. Peter
participants (2)
-
John Ferlan
-
Peter Krempa