[PATCH v2 0/5] Refactor qemuDomainSecretAESSetup
As requested in: https://www.redhat.com/archives/libvir-list/2020-March/msg00377.html this is an incremental refactor of qemuDomainSecretAESSetup. Posted only as an individual refactor to omit re-sending the 26 other patches from the original series. Peter Krempa (5): qemuDomainSecretInfo: Register autoptr cleanup function qemuDomainSecretAESSetup: Automatically free non-secret locals qemuDomainSecretAESSetup: Allocate and return 'secinfo' here qemuDomainSecretAESSetup: Split out lookup of secret data Remove qemuDomainSecretInfoNew src/qemu/qemu_domain.c | 178 ++++++++++++++++++----------------------- src/qemu/qemu_domain.h | 2 + 2 files changed, 82 insertions(+), 98 deletions(-) -- 2.24.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 835321f54d..ad7ed3b9f0 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1042,6 +1042,8 @@ bool qemuDomainSupportsEncryptedSecret(qemuDomainObjPrivatePtr priv); void qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr secinfo) ATTRIBUTE_NONNULL(1); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainSecretInfo, qemuDomainSecretInfoFree); + void qemuDomainSecretInfoDestroy(qemuDomainSecretInfoPtr secinfo); void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) -- 2.24.1
Use g_autofree for the ciphertext and init vector as they are not secret and thus don't have to be cleared and use g_new0 to allocate the iv for parity. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7c962fb062..e33d3099d6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1536,11 +1536,11 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, { g_autoptr(virConnect) conn = virGetConnectSecret(); int ret = -1; - uint8_t *raw_iv = NULL; + g_autofree uint8_t *raw_iv = NULL; size_t ivlen = QEMU_DOMAIN_AES_IV_LEN; uint8_t *secret = NULL; size_t secretlen = 0; - uint8_t *ciphertext = NULL; + g_autofree uint8_t *ciphertext = NULL; size_t ciphertextlen = 0; if (!conn) @@ -1550,14 +1550,13 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, secinfo->s.aes.username = g_strdup(username); if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, isLuks))) - goto cleanup; + return -1; - if (VIR_ALLOC_N(raw_iv, ivlen) < 0) - goto cleanup; + raw_iv = g_new0(uint8_t, ivlen); /* Create a random initialization vector */ if (virRandomBytes(raw_iv, ivlen) < 0) - goto cleanup; + return -1; /* Encode the IV and save that since qemu will need it */ secinfo->s.aes.iv = g_base64_encode(raw_iv, ivlen); @@ -1583,9 +1582,7 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, ret = 0; cleanup: - VIR_DISPOSE_N(raw_iv, ivlen); VIR_DISPOSE_N(secret, secretlen); - VIR_DISPOSE_N(ciphertext, ciphertextlen); return ret; } -- 2.24.1
Rather than passing in an empty qemuDomainSecretInfoPtr allocate it in this function and return it. This is done by absorbing the check from qemuDomainSecretInfoNew and removing the internals of qemuDomainSecretInfoNew. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 53 ++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e33d3099d6..e83301d84e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1521,21 +1521,20 @@ qemuDomainSecretPlainSetup(qemuDomainSecretInfoPtr secinfo, * @seclookupdef: Pointer to seclookupdef data * @isLuks: True/False for is for luks (alias generation) * - * Taking a secinfo, fill in the AES specific information using the + * Encrypts a secret looked up via @seclookupdef for use with qemu. * - * Returns 0 on success, -1 on failure with error message + * Returns qemuDomainSecretInfoPtr filled with the necessary information. */ -static int +static qemuDomainSecretInfoPtr qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, - qemuDomainSecretInfoPtr secinfo, const char *srcalias, virSecretUsageType usageType, const char *username, virSecretLookupTypeDefPtr seclookupdef, bool isLuks) { + g_autoptr(qemuDomainSecretInfo) secinfo = NULL; g_autoptr(virConnect) conn = virGetConnectSecret(); - int ret = -1; g_autofree uint8_t *raw_iv = NULL; size_t ivlen = QEMU_DOMAIN_AES_IV_LEN; uint8_t *secret = NULL; @@ -1544,19 +1543,27 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, size_t ciphertextlen = 0; if (!conn) - return -1; + return NULL; + + if (!qemuDomainSupportsEncryptedSecret(priv)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("encrypted secrets are not supported")); + return NULL; + } + + secinfo = g_new0(qemuDomainSecretInfo, 1); secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES; secinfo->s.aes.username = g_strdup(username); if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, isLuks))) - return -1; + return NULL; raw_iv = g_new0(uint8_t, ivlen); /* Create a random initialization vector */ if (virRandomBytes(raw_iv, ivlen) < 0) - return -1; + return NULL; /* Encode the IV and save that since qemu will need it */ secinfo->s.aes.iv = g_base64_encode(raw_iv, ivlen); @@ -1564,13 +1571,13 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, /* Grab the unencoded secret */ if (virSecretGetSecretString(conn, seclookupdef, usageType, &secret, &secretlen) < 0) - goto cleanup; + goto error; if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN, raw_iv, ivlen, secret, secretlen, &ciphertext, &ciphertextlen) < 0) - goto cleanup; + goto error; /* Clear out the secret */ memset(secret, 0, secretlen); @@ -1579,11 +1586,11 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, secinfo->s.aes.ciphertext = g_base64_encode(ciphertext, ciphertextlen); - ret = 0; + return g_steal_pointer(&secinfo); - cleanup: + error: VIR_DISPOSE_N(secret, secretlen); - return ret; + return NULL; } @@ -1655,24 +1662,8 @@ qemuDomainSecretInfoNew(qemuDomainObjPrivatePtr priv, virSecretLookupTypeDefPtr lookupDef, bool isLuks) { - qemuDomainSecretInfoPtr secinfo = NULL; - - if (!qemuDomainSupportsEncryptedSecret(priv)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("encrypted secrets are not supported")); - return NULL; - } - - if (VIR_ALLOC(secinfo) < 0) - return NULL; - - if (qemuDomainSecretAESSetup(priv, secinfo, srcAlias, usageType, username, - lookupDef, isLuks) < 0) { - g_clear_pointer(&secinfo, qemuDomainSecretInfoFree); - return NULL; - } - - return secinfo; + return qemuDomainSecretAESSetup(priv, srcAlias, usageType, username, + lookupDef, isLuks); } -- 2.24.1
Split out the lookup of the secret from the secret driver into qemuDomainSecretAESSetupFromSecret so that we can also instantiate secret objects in qemu with data from other sources. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 87 ++++++++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e83301d84e..ba80bb67d2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1514,37 +1514,28 @@ qemuDomainSecretPlainSetup(qemuDomainSecretInfoPtr secinfo, /* qemuDomainSecretAESSetup: * @priv: pointer to domain private object - * @secinfo: Pointer to secret info - * @srcalias: Alias of the disk/hostdev used to generate the secret alias - * @usageType: The virSecretUsageType - * @username: username to use for authentication (may be NULL) - * @seclookupdef: Pointer to seclookupdef data - * @isLuks: True/False for is for luks (alias generation) + * @alias: alias of the secret + * @username: username to use (may be NULL) + * @secret: secret data + * @secretlen: length of @secret * - * Encrypts a secret looked up via @seclookupdef for use with qemu. + * Encrypts @secret for use with qemu. * * Returns qemuDomainSecretInfoPtr filled with the necessary information. */ static qemuDomainSecretInfoPtr qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, - const char *srcalias, - virSecretUsageType usageType, + const char *alias, const char *username, - virSecretLookupTypeDefPtr seclookupdef, - bool isLuks) + uint8_t *secret, + size_t secretlen) { g_autoptr(qemuDomainSecretInfo) secinfo = NULL; - g_autoptr(virConnect) conn = virGetConnectSecret(); g_autofree uint8_t *raw_iv = NULL; size_t ivlen = QEMU_DOMAIN_AES_IV_LEN; - uint8_t *secret = NULL; - size_t secretlen = 0; g_autofree uint8_t *ciphertext = NULL; size_t ciphertextlen = 0; - if (!conn) - return NULL; - if (!qemuDomainSupportsEncryptedSecret(priv)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("encrypted secrets are not supported")); @@ -1554,11 +1545,9 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, secinfo = g_new0(qemuDomainSecretInfo, 1); secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES; + secinfo->s.aes.alias = g_strdup(alias); secinfo->s.aes.username = g_strdup(username); - if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, isLuks))) - return NULL; - raw_iv = g_new0(uint8_t, ivlen); /* Create a random initialization vector */ @@ -1568,29 +1557,61 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, /* Encode the IV and save that since qemu will need it */ secinfo->s.aes.iv = g_base64_encode(raw_iv, ivlen); - /* Grab the unencoded secret */ - if (virSecretGetSecretString(conn, seclookupdef, usageType, - &secret, &secretlen) < 0) - goto error; - if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN, raw_iv, ivlen, secret, secretlen, &ciphertext, &ciphertextlen) < 0) - goto error; - - /* Clear out the secret */ - memset(secret, 0, secretlen); + return NULL; /* Now encode the ciphertext and store to be passed to qemu */ secinfo->s.aes.ciphertext = g_base64_encode(ciphertext, ciphertextlen); return g_steal_pointer(&secinfo); +} + + +/** + * qemuDomainSecretAESSetupFromSecret: + * @priv: pointer to domain private object + * @srcalias: Alias of the disk/hostdev used to generate the secret alias + * @usageType: The virSecretUsageType + * @username: username to use for authentication (may be NULL) + * @seclookupdef: Pointer to seclookupdef data + * @isLuks: True/False for is for luks (alias generation) + * + * Looks up a secret in the secret driver based on @usageType and @seclookupdef + * and builds qemuDomainSecretInfoPtr from it. + */ +static qemuDomainSecretInfoPtr +qemuDomainSecretAESSetupFromSecret(qemuDomainObjPrivatePtr priv, + const char *srcalias, + virSecretUsageType usageType, + const char *username, + virSecretLookupTypeDefPtr seclookupdef, + bool isLuks) +{ + g_autoptr(virConnect) conn = virGetConnectSecret(); + qemuDomainSecretInfoPtr secinfo; + g_autofree char *alias = NULL; + uint8_t *secret = NULL; + size_t secretlen = 0; + + if (!conn) + return NULL; + + if (!(alias = qemuDomainGetSecretAESAlias(srcalias, isLuks))) + return NULL; + + if (virSecretGetSecretString(conn, seclookupdef, usageType, + &secret, &secretlen) < 0) + return NULL; + + secinfo = qemuDomainSecretAESSetup(priv, alias, username, secret, secretlen); - error: VIR_DISPOSE_N(secret, secretlen); - return NULL; + + return secinfo; } @@ -1662,8 +1683,8 @@ qemuDomainSecretInfoNew(qemuDomainObjPrivatePtr priv, virSecretLookupTypeDefPtr lookupDef, bool isLuks) { - return qemuDomainSecretAESSetup(priv, srcAlias, usageType, username, - lookupDef, isLuks); + return qemuDomainSecretAESSetupFromSecret(priv, srcAlias, usageType, username, + lookupDef, isLuks); } -- 2.24.1
Replace it by a direct call to qemuDomainSecretAESSetupFromSecret. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 53 +++++++++++------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ba80bb67d2..f74a1b6fb3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1661,33 +1661,6 @@ qemuDomainSecretInfoNewPlain(virSecretUsageType usageType, } -/* qemuDomainSecretInfoNew: - * @priv: pointer to domain private object - * @srcAlias: Alias base to use for TLS object - * @usageType: Secret usage type - * @username: username - * @looupDef: lookup def describing secret - * @isLuks: boolean for luks lookup - * - * Helper function to create a secinfo to be used for secinfo consumers. This - * sets up encrypted data to be used with qemu's 'secret' object. - * - * Returns @secinfo on success, NULL on failure. Caller is responsible - * to eventually free @secinfo. - */ -static qemuDomainSecretInfoPtr -qemuDomainSecretInfoNew(qemuDomainObjPrivatePtr priv, - const char *srcAlias, - virSecretUsageType usageType, - const char *username, - virSecretLookupTypeDefPtr lookupDef, - bool isLuks) -{ - return qemuDomainSecretAESSetupFromSecret(priv, srcAlias, usageType, username, - lookupDef, isLuks); -} - - /** * qemuDomainSecretInfoTLSNew: * @priv: pointer to domain private object @@ -1714,9 +1687,9 @@ qemuDomainSecretInfoTLSNew(qemuDomainObjPrivatePtr priv, } seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; - return qemuDomainSecretInfoNew(priv, srcAlias, - VIR_SECRET_USAGE_TYPE_TLS, NULL, - &seclookupdef, false); + return qemuDomainSecretAESSetupFromSecret(priv, srcAlias, + VIR_SECRET_USAGE_TYPE_TLS, + NULL, &seclookupdef, false); } @@ -1806,11 +1779,11 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv, src->auth->username, &src->auth->seclookupdef); } else { - srcPriv->secinfo = qemuDomainSecretInfoNew(priv, authalias, - usageType, - src->auth->username, - &src->auth->seclookupdef, - false); + srcPriv->secinfo = qemuDomainSecretAESSetupFromSecret(priv, authalias, + usageType, + src->auth->username, + &src->auth->seclookupdef, + false); } if (!srcPriv->secinfo) @@ -1818,11 +1791,11 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv, } if (hasEnc) { - if (!(srcPriv->encinfo = - qemuDomainSecretInfoNew(priv, encalias, - VIR_SECRET_USAGE_TYPE_VOLUME, NULL, - &src->encryption->secrets[0]->seclookupdef, - true))) + if (!(srcPriv->encinfo = qemuDomainSecretAESSetupFromSecret(priv, encalias, + VIR_SECRET_USAGE_TYPE_VOLUME, + NULL, + &src->encryption->secrets[0]->seclookupdef, + true))) return -1; } -- 2.24.1
On a Monday in 2020, Peter Krempa wrote:
As requested in:
https://www.redhat.com/archives/libvir-list/2020-March/msg00377.html
this is an incremental refactor of qemuDomainSecretAESSetup. Posted only as an individual refactor to omit re-sending the 26 other patches from the original series.
Peter Krempa (5): qemuDomainSecretInfo: Register autoptr cleanup function qemuDomainSecretAESSetup: Automatically free non-secret locals qemuDomainSecretAESSetup: Allocate and return 'secinfo' here qemuDomainSecretAESSetup: Split out lookup of secret data Remove qemuDomainSecretInfoNew
src/qemu/qemu_domain.c | 178 ++++++++++++++++++----------------------- src/qemu/qemu_domain.h | 2 + 2 files changed, 82 insertions(+), 98 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa