On Tue, Feb 10, 2026 at 13:30:11 +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 array 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 array 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 | 249 ++++++++++++++++++++++++++++---------
> src/conf/virsecretobj.h | 16 ++-
> src/secret/secret_driver.c | 20 ++-
> 3 files changed, 222 insertions(+), 63 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index a3dd7983bb..49b69b4867 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[] = {
> + { ".aes256cbc", VIR_CRYPTO_CIPHER_AES256CBC },
> + { ".base64", -1 },
> +};
> +
> static virClass *virSecretObjClass;
> static virClass *virSecretObjListClass;
> static void virSecretObjDispose(void *obj);
> @@ -377,12 +391,14 @@ virSecretObjListAdd(virSecretObjList *secrets,
>
> if (!(obj = virSecretObjNew()))
> goto cleanup;
> -
> /* Generate the possible configFile and secretValueFile strings
> - * using the configDir, uuidstr, and appropriate suffix
> + * using the configDir, uuidstr, and appropriate suffix.
> + * Note that secretValueFile extension is not set here. It is
> determined
> + * based on a) existing availability of secret file
> (virSecretLoadValue) or
> + * b) target storage format (virSecretObjSaveData)
> */
> if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
> ".xml")) ||
> - !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr,
> ".base64")))
> + !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr,
> NULL)))
So here you create the leading part of the filename (without the suffix)
...
> goto cleanup;
>
> if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> @@ -682,18 +698,75 @@ virSecretObjSaveConfig(virSecretObj *obj)
>
>
> int
> -virSecretObjSaveData(virSecretObj *obj)
> +virSecretObjSaveData(virSecretObj *obj,
> + const char *configDir,
> + bool encryptData,
> + uint8_t *secretsEncryptionKey,
> + size_t secretsKeyLen)
> {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> g_autofree char *base64 = NULL;
> + g_autofree char *newSecretFile = NULL;
> + g_autofree uint8_t *secret = NULL;
> + g_autofree uint8_t *encryptedValue = NULL;
> +
> + const char *selectedSuffix = NULL;
> + size_t encryptedValueLen = 0;
> +
> + size_t i;
> + size_t secretLen = 0;
> + uint8_t iv[16] = { 0 };
>
> if (!obj->value)
> return 0;
>
> - base64 = g_base64_encode(obj->value, obj->value_size);
> + virUUIDFormat(obj->def->uuid, uuidstr);
> +
> + /* Based on whether encryption is on/off, save the secret in either
> + * the latest encryption scheme or in base64 formats.
> + * Subsequently, delete the other formats of the same uuid on the disk.
> + */
> + if (encryptData && secretsEncryptionKey) {
> + selectedSuffix = schemeInfo[0].suffix;
> + if (virRandomBytes(iv, sizeof(iv)) < 0) {
> + return -1;
> + }
> + if (virCryptoEncryptData(schemeInfo[0].cipher,
> + 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 {
> + int baseElement = G_N_ELEMENTS(schemeInfo) - 1;
> + selectedSuffix = schemeInfo[baseElement].suffix;
> + base64 = g_base64_encode(obj->value, obj->value_size);
> + }
> +
> + if (!(newSecretFile = virFileBuildPath(configDir, uuidstr,
> selectedSuffix))) {
> + return -1;
> + }
... but you don't use it here. Instead new filename is generated.
> + g_free(obj->secretValueFile);
> + obj->secretValueFile = g_steal_pointer(&newSecretFile);
>
> if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) <
> 0)
> return -1;
>
> + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) {
> + g_autofree char* deleteFile = virFileBuildPath(configDir,
> + uuidstr,
> + schemeInfo[i].suffix);
Aand here again.
> + if (STRNEQ_NULLABLE(schemeInfo[i].suffix, selectedSuffix)) {
> + unlink(deleteFile);
> + }
> + }
> return 0;
> }
>
> @@ -737,7 +810,11 @@ virSecretObjGetValue(virSecretObj *obj)
> int
> virSecretObjSetValue(virSecretObj *obj,
> const unsigned char *value,
> - size_t value_size)
> + size_t value_size,
> + const char *configDir,
> + bool encryptData,
> + uint8_t *secretsEncryptionKey,
> + size_t secretsKeyLen)
> {
> virSecretDef *def = obj->def;
> g_autofree unsigned char *old_value = NULL;
> @@ -753,7 +830,11 @@ virSecretObjSetValue(virSecretObj *obj,
> obj->value = g_steal_pointer(&new_value);
> obj->value_size = value_size;
>
> - if (!def->isephemeral && virSecretObjSaveData(obj) < 0)
> + if (!def->isephemeral && virSecretObjSaveData(obj,
> + configDir,
> + encryptData,
> + secretsEncryptionKey,
> + secretsKeyLen) < 0)
> goto error;
>
> /* Saved successfully - drop old value */
> @@ -807,59 +888,109 @@ virSecretLoadValidateUUID(virSecretDef *def,
>
>
> static int
> -virSecretLoadValue(virSecretObj *obj)
> +virSecretLoadValue(virSecretObj *obj,
> + const char *configDir,
> + uint8_t *secretsEncryptionKey,
> + size_t secretsKeyLen)
> {
> - int ret = -1, fd = -1;
> + int ret = -1;
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + VIR_AUTOCLOSE fd = -1;
> struct stat st;
> - g_autofree char *contents = NULL;
> + size_t i;
>
> - if ((fd = open(obj->secretValueFile, O_RDONLY)) == -1) {
> - if (errno == ENOENT) {
> - ret = 0;
> + g_autofree char *contents = NULL;
> + g_autofree uint8_t *contentsEncrypted = NULL;
> + g_autofree uint8_t *decryptedValue = NULL;
> +
> + size_t decryptedValueLen = 0;
> + uint8_t iv[16] = { 0 };
> + uint8_t *ciphertext = NULL;
> + size_t ciphertextLen = 0;
> +
> + virUUIDFormat(obj->def->uuid, uuidstr);
> +
> + /* Iterate over the list of suffixes, find the one that when appended to
> the
> + * uuid will result in a file that exists on the disk. This essentially
> is the
> + * secret file. Subsequently, load/decrypt the secret by using the
> appropriate
> + * encryption scheme.
> + */
> + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) {
> + g_autofree char *candidatePath = NULL;
> + if (!(candidatePath = virFileBuildPath(configDir,
> + uuidstr,
> + schemeInfo[i].suffix))) {
> goto cleanup;
> }
Same here. So actually obj->secretValueFile is unused. Instead IMO we
should take that vallue and concatenate it with the suffix rather than
regenerate it completely in multiple places.
> - virReportSystemError(errno, _("cannot open '%1$s'"),
> - obj->secretValueFile);
> - goto cleanup;
[...]
> - }
> -
> - if (fstat(fd, &st) < 0) {
> - virReportSystemError(errno, _("cannot stat '%1$s'"),
> - obj->secretValueFile);
> - goto cleanup;
> - }
> -
> - if ((size_t)st.st_size != st.st_size) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("'%1$s' file does not fit in memory"),
> - obj->secretValueFile);
> - goto cleanup;
> - }
> -
> - if (st.st_size < 1) {
> - ret = 0;
> - 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;
> + if (virFileExists(candidatePath)) {
> + if ((fd = open(candidatePath, O_RDONLY)) == -1) {
> + if (errno == ENOENT) {
> + ret = 0;
> + } else {
> + virReportSystemError(errno, _("cannot open '%1$s'"),
> + candidatePath);
> + }
> + goto cleanup;
> + }
> + if (fstat(fd, &st) < 0) {
> + virReportSystemError(errno, _("cannot stat '%1$s'"),
> + candidatePath);
> + goto cleanup;
> + }
> + if ((size_t)st.st_size != st.st_size) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("'%1$s' file does not fit in memory"),
> + candidatePath);
> + goto cleanup;
> + }
> + if (st.st_size < 1) {
> + ret = 0;
> + 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'"),
> + candidatePath);
> + goto cleanup;
> + }
> + contents[st.st_size] = '\0';
> + if (schemeInfo[i].cipher != -1) {
> + contentsEncrypted = 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, contentsEncrypted, sizeof(iv));
> + ciphertext = contentsEncrypted + sizeof(iv);
> + ciphertextLen = obj->value_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);
> + }
> +
> + g_free(obj->secretValueFile);
> + obj->secretValueFile = g_steal_pointer(&candidatePath);
> +
> + break;
> + }
It also seems we're missing handling of the case [1] when the file
didn't exist at all.
Both should be trivial to fix though. I'll be having a deeper look at
this later today. If you want to address my comments yourself, go ahead
in the next few hours. Otherwise I'll likely fix them myself before
pushing.
