On Tue, Dec 09, 2025 at 01:22:31 +0530, Arun Menon via Devel wrote:
> Now that we have the functionality to provide the secrets driver
> with an encryption key through a configuration file or using system
> credentials, and the newly introduced attribute to iterate over the
> encryption schemes, we can use the key to save and load secrets.
>
> Encrypt all secrets that are going to be saved on the disk if the
> 'secrets_encryption_key' path is set in the secret.conf file OR
> if a valid systemd generated credential exists.
>
> While loading secrets, identify the decryption method by matching the file
> extension of the stored secret against the known encryptionSchemeType values.
> If no matching scheme is found, the secret is skipped. If the encryption
> key is changed across restarts, then also the secret driver will fail to load
> the secrets from the disk that were encrypted with the former key.
>
> Signed-off-by: Arun Menon <[email protected]>
> ---
> src/conf/virsecretobj.c | 166 ++++++++++++++++++++++++++++++-------
> src/conf/virsecretobj.h | 18 +++-
> src/secret/secret_driver.c | 25 ++++--
> 3 files changed, 169 insertions(+), 40 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index a3dd7983bb..60a1323625 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -31,6 +31,10 @@
> #include "virhash.h"
> #include "virlog.h"
> #include "virstring.h"
> +#include "virsecret.h"
> +#include "virrandom.h"
> +#include "vircrypto.h"
> +#include "virsecureerase.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECRET
>
> @@ -45,6 +49,16 @@ struct _virSecretObj {
> size_t value_size;
> };
>
> +typedef struct _virSecretSchemeInfo {
> + const char *suffix;
> + virCryptoCipher cipher;
> +} virSecretSchemeInfo;
> +
> +virSecretSchemeInfo schemeInfo[] = {
> + { ".base64", -1 },
> + { ".aes256cbc", VIR_CRYPTO_CIPHER_AES256CBC },
If you order the entries in this struct the other way around you'll not
have to use VIR_SECRET_ENCRYPTION_SCHEME_LAST - 1 everywhere.
In fact you won't even need the virSecretEncryptionScheme type at all.
> +};
> +
> static virClass *virSecretObjClass;
> static virClass *virSecretObjListClass;
> static void virSecretObjDispose(void *obj);
> @@ -323,7 +337,8 @@ virSecretObj *
> virSecretObjListAdd(virSecretObjList *secrets,
> virSecretDef **newdef,
> const char *configDir,
> - virSecretDef **oldDef)
> + virSecretDef **oldDef,
> + bool encryptData)
> {
> virSecretObj *obj;
> virSecretDef *objdef;
> @@ -363,6 +378,8 @@ virSecretObjListAdd(virSecretObjList *secrets,
> } else {
> /* No existing secret with same UUID,
> * try look for matching usage instead */
> + const char *secretSuffix = ".base64";
> + g_autofree char *encryptionSchemeSuffix = NULL;
> if ((obj = virSecretObjListFindByUsageLocked(secrets,
> (*newdef)->usage_type,
> (*newdef)->usage_id))) {
> @@ -379,10 +396,17 @@ virSecretObjListAdd(virSecretObjList *secrets,
> goto cleanup;
>
> /* Generate the possible configFile and secretValueFile strings
> - * using the configDir, uuidstr, and appropriate suffix
> + * using the configDir, uuidstr, and appropriate suffix.
> + * By default, the latest encryption cipher will be used to encrypt
> secrets.
> */
> +
> + if (encryptData) {
> + encryptionSchemeSuffix = g_strdup_printf(".%s",
> virSecretEncryptionSchemeTypeToString(VIR_SECRET_ENCRYPTION_SCHEME_LAST- 1));
use schemeinfo[0].suffix and you won't have to copy these any more.
> + } else {
> + encryptionSchemeSuffix = g_strdup(secretSuffix);
> + }
> if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
> ".xml")) ||
> - !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr,
> ".base64")))
> + !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr,
> encryptionSchemeSuffix)))
> goto cleanup;
>
> if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> @@ -682,15 +706,40 @@ virSecretObjSaveConfig(virSecretObj *obj)
>
>
> int
> -virSecretObjSaveData(virSecretObj *obj)
> +virSecretObjSaveData(virSecretObj *obj,
> + bool encryptData,
> + uint8_t *secretsEncryptionKey,
> + size_t secretsKeyLen)
> {
> g_autofree char *base64 = NULL;
> + g_autofree uint8_t *secret = NULL;
> + g_autofree uint8_t *encryptedValue = NULL;
> + size_t encryptedValueLen = 0;
> + size_t secretLen = 0;
> + uint8_t iv[16] = { 0 };
>
> if (!obj->value)
> return 0;
>
> - base64 = g_base64_encode(obj->value, obj->value_size);
> -
> + if (encryptData && secretsEncryptionKey) {
> + if (virRandomBytes(iv, sizeof(iv)) < 0) {
> + return -1;
> + }
> + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_LAST-1,
Umm so this is a 3rd distinct way how you specify AES in this series.
use the entry from the struct instead so that there isn't multiple
places that'll need to be changed if a different one is to be picked.
> + secretsEncryptionKey, secretsKeyLen,
> + iv, sizeof(iv),
> + (uint8_t *)obj->value, obj->value_size,
> + &encryptedValue, &encryptedValueLen) < 0) {
> + return -1;
> + }
> + secretLen = sizeof(iv) + encryptedValueLen;
> + secret = g_new0(uint8_t, secretLen);
> + memcpy(secret, iv, sizeof(iv));
> + memcpy(secret + sizeof(iv), encryptedValue, encryptedValueLen);
> + base64 = g_base64_encode(secret, secretLen);
> + } else {
> + base64 = g_base64_encode(obj->value, obj->value_size);
> + }
> if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) <
> 0)
> return -1;
>
[...]
> @@ -807,11 +862,24 @@ virSecretLoadValidateUUID(virSecretDef *def,
>
>
> static int
> -virSecretLoadValue(virSecretObj *obj)
> +virSecretLoadValue(virSecretObj *obj,
> + uint8_t *secretsEncryptionKey,
> + size_t secretsKeyLen)
> {
> - int ret = -1, fd = -1;
> + int ret = -1;
> + VIR_AUTOCLOSE fd = -1;
> struct stat st;
> + size_t i;
> +
> g_autofree char *contents = NULL;
> + g_autofree uint8_t *contents_encrypted = NULL;
> + g_autofree uint8_t *decryptedValue = NULL;
Mix of snake-case and camel case
> + g_autofree char *encryptionScheme = NULL;
This variable is unused.
> +
> + size_t decryptedValueLen = 0;
> + uint8_t iv[16] = { 0 };
> + uint8_t *ciphertext = NULL;
> + size_t ciphertextLen = 0;
>
> if ((fd = open(obj->secretValueFile, O_RDONLY)) == -1) {
> if (errno == ENOENT) {
> @@ -841,25 +909,52 @@ virSecretLoadValue(virSecretObj *obj)
> goto cleanup;
> }
>
> - contents = g_new0(char, st.st_size + 1);
> -
> - if (saferead(fd, contents, st.st_size) != st.st_size) {
> - virReportSystemError(errno, _("cannot read '%1$s'"),
> - obj->secretValueFile);
> - goto cleanup;
> + /* Iterate over the encryption schemes and decrypt the contents
> + * of the file on the disk, by matching the file extension with the
> encryption
> + * scheme.
> + * If there is no scheme matching the file extension, then that secret
> is not loaded. */
> +
> + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) {
> + if (virStringHasSuffix(obj->secretValueFile, schemeInfo[i].suffix)) {
> + contents = g_new0(char, st.st_size + 1);
> + if (saferead(fd, contents, st.st_size) != st.st_size) {
> + virReportSystemError(errno, _("cannot read '%1$s'"),
> + obj->secretValueFile);
> + goto cleanup;
> + }
> + contents[st.st_size] = '\0';
> + if (schemeInfo[i].cipher != -1) {
> + contents_encrypted = g_base64_decode(contents,
> &obj->value_size);
> + if (sizeof(iv) > obj->value_size) {
> + virReportError(VIR_ERR_INVALID_SECRET,
> + _("Encrypted secret size '%1$zu' is
> invalid"),
> + obj->value_size);
> + goto cleanup;
> + }
> + memcpy(iv, contents_encrypted, sizeof(iv));
> + ciphertext = contents_encrypted + sizeof(iv);
> + ciphertextLen = st.st_size - sizeof(iv);
> + if (virCryptoDecryptData(schemeInfo[i].cipher,
> + secretsEncryptionKey, secretsKeyLen,
> + iv, sizeof(iv),
> + ciphertext, ciphertextLen,
> + &decryptedValue,
> &decryptedValueLen) < 0) {
> + goto cleanup;
> + }
> + g_free(obj->value);
> + obj->value = g_steal_pointer(&decryptedValue);
> + obj->value_size = decryptedValueLen;
> + } else {
> + obj->value = g_base64_decode(contents, &obj->value_size);
> + }
> + break;
> + }
> }
> - contents[st.st_size] = '\0';
> -
> - VIR_FORCE_CLOSE(fd);
> -
> - obj->value = g_base64_decode(contents, &obj->value_size);
> -
> ret = 0;
> -
> cleanup:
> - if (contents != NULL)
> - memset(contents, 0, st.st_size);
> - VIR_FORCE_CLOSE(fd);
> + virSecureErase(contents_encrypted, obj->value_size);
> + virSecureErase(contents, st.st_size);
> + virSecureErase(iv, sizeof(iv));
> return ret;
> }
>
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 408a629ea0..8c0796c86a 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -30,6 +30,7 @@
> #include "virlog.h"
> #include "viralloc.h"
> #include "secret_conf.h"
> +#include "secret_config.h"
This likely fixes the build issue from 3/5.
> #include "virsecretobj.h"
> #include "secret_driver.h"
> #include "virthread.h"
