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 <armenon@redhat.com>
---
 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)))
             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;
+    }
+    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);
+        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;
         }
-        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;
+        }
     }
-    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(contentsEncrypted, obj->value_size);
+    virSecureErase(contents, st.st_size);
+    virSecureErase(iv, sizeof(iv));
     return ret;
 }
 
@@ -868,7 +999,9 @@ static virSecretObj *
 virSecretLoad(virSecretObjList *secrets,
               const char *file,
               const char *path,
-              const char *configDir)
+              const char *configDir,
+              uint8_t *secretsEncryptionKey,
+              size_t secretsKeyLen)
 {
     g_autoptr(virSecretDef) def = NULL;
     virSecretObj *obj = NULL;
@@ -882,7 +1015,7 @@ virSecretLoad(virSecretObjList *secrets,
     if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL)))
         return NULL;
 
-    if (virSecretLoadValue(obj) < 0) {
+    if (virSecretLoadValue(obj, configDir, secretsEncryptionKey, secretsKeyLen) < 0) {
         virSecretObjListRemove(secrets, obj);
         g_clear_pointer(&obj, virObjectUnref);
         return NULL;
@@ -894,7 +1027,9 @@ virSecretLoad(virSecretObjList *secrets,
 
 int
 virSecretLoadAllConfigs(virSecretObjList *secrets,
-                        const char *configDir)
+                        const char *configDir,
+                        uint8_t *secretsEncryptionKey,
+                        size_t secretsKeyLen)
 {
     g_autoptr(DIR) dir = NULL;
     struct dirent *de;
@@ -915,7 +1050,9 @@ virSecretLoadAllConfigs(virSecretObjList *secrets,
         if (!(path = virFileBuildPath(configDir, de->d_name, NULL)))
             continue;
 
-        if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) {
+        if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir,
+                                  secretsEncryptionKey,
+                                  secretsKeyLen))) {
             VIR_ERROR(_("Error reading secret: %1$s"),
                       virGetLastErrorMessage());
             continue;
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index 17897c5513..74a36baf6d 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -86,7 +86,11 @@ int
 virSecretObjSaveConfig(virSecretObj *obj);
 
 int
-virSecretObjSaveData(virSecretObj *obj);
+virSecretObjSaveData(virSecretObj *obj,
+                     const char *configDir,
+                     bool encryptData,
+                     uint8_t *secretsEncryptionKey,
+                     size_t secretsKeyLen);
 
 virSecretDef *
 virSecretObjGetDef(virSecretObj *obj);
@@ -101,7 +105,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);
 
 size_t
 virSecretObjGetValueSize(virSecretObj *obj);
@@ -112,4 +120,6 @@ virSecretObjSetValueSize(virSecretObj *obj,
 
 int
 virSecretLoadAllConfigs(virSecretObjList *secrets,
-                        const char *configDir);
+                        const char *configDir,
+                        uint8_t *secretsEncryptionKey,
+                        size_t secretsKeyLen);
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 9b13772ad3..e1668730dd 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -229,7 +229,11 @@ secretDefineXML(virConnectPtr conn,
 
     if (!objDef->isephemeral) {
         if (backup && backup->isephemeral) {
-            if (virSecretObjSaveData(obj) < 0)
+            if (virSecretObjSaveData(obj,
+                                     driver->configDir,
+                                     driver->config->encryptData,
+                                     driver->config->secretsEncryptionKey,
+                                     driver->config->secretsKeyLen) < 0)
                 goto restore_backup;
         }
 
@@ -333,7 +337,11 @@ secretSetValue(virSecretPtr secret,
     if (virSecretSetValueEnsureACL(secret->conn, def) < 0)
         goto cleanup;
 
-    if (virSecretObjSetValue(obj, value, value_size) < 0)
+    if (virSecretObjSetValue(obj, value, value_size,
+                             driver->configDir,
+                             driver->config->encryptData,
+                             driver->config->secretsEncryptionKey,
+                             driver->config->secretsKeyLen) < 0)
         goto cleanup;
 
     event = virSecretEventValueChangedNew(def->uuid,
@@ -542,7 +550,9 @@ secretStateInitialize(bool privileged,
     if (!(driver->secrets = virSecretObjListNew()))
         goto error;
 
-    if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0)
+    if (virSecretLoadAllConfigs(driver->secrets, driver->configDir,
+                                driver->config->secretsEncryptionKey,
+                                driver->config->secretsKeyLen) < 0)
         goto error;
 
     return VIR_DRV_STATE_INIT_COMPLETE;
@@ -564,7 +574,9 @@ secretStateReload(void)
     if (!(driver->config = virSecretDaemonConfigNew(driver->privileged)))
         return -1;
 
-    ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir));
+    ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir,
+                                         driver->config->secretsEncryptionKey,
+                                         driver->config->secretsKeyLen));
 
     return 0;
 }
-- 
2.51.1

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 <armenon@redhat.com>
> ---
>  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.