Re: [PATCH v5 5/6] secret: Add functionality to load and save secrets in encrypted format

12 Feb 2026

On Tue, Feb 10, 2026 at 15:52:43 +0100, Peter Krempa wrote:
...
On Tue, Feb 10, 2026 at 17:14:45 +0530, Arun Menon wrote:
...
Hi Peter,
Thanks for response.
On Tue, Feb 10, 2026 at 11:26:25AM +0100, Peter Krempa wrote:
...
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(-)
[...]
...
...
...
It also seems we're missing handling of the case [1] when the file
didn't exist at all.
If the file did not exist at all, we come out of the loop and return 0.
That means the value will not be set, although the secret is defined.a
In previous code it would return an error I didn't check if that's a
problem but this changes what's happening.
Okay I see what was happening. It did in fact retur 0;

Anyways to avoid more backs-and forths there are 2 things:

1) you didn't configure your git-send-email tool properly, specifically

 git config --global format.forceInBodyFrom true

 https://www.libvirt.org/submitting-patches.html#git-configuration

Thus the patches lack the extra From line and the commit author is thus
mangled. Please re-send.

2) apply the following diff which does what I've requested:

  - changes to use obj->secretValueFile as base
  - does local suffix lookups
  - refactors code so that the base64 encoding is in common code paths
  - moves local variable declaration inside blocks using them
  - refactors loader to use virFileReadAll instead of open-coding
  - fixes virSecretObjDeleteData to not rely on the actual filename

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 49b69b4867..b448be493a 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -670,9 +670,15 @@ virSecretObjDeleteConfig(virSecretObj *obj)
 void
 virSecretObjDeleteData(virSecretObj *obj)
 {
+    size_t i;
+
     /* The configFile will already be removed, so secret won't be
      * loaded again if this fails */
-    unlink(obj->secretValueFile);
+    for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) {
+        g_autofree char *deleteFile = g_strconcat(obj->secretValueFile, schemeInfo[i].suffix, NULL);
+
+        ignore_value(unlink(deleteFile));
+    }
 }


@@ -699,74 +705,68 @@ virSecretObjSaveConfig(virSecretObj *obj)

 int
 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;
-
+    g_autofree char *filename = NULL;
     size_t i;
+    g_autofree unsigned char *secretbuf = NULL;
+    const unsigned char *secret = NULL;
     size_t secretLen = 0;
-    uint8_t iv[16] = { 0 };

     if (!obj->value)
         return 0;

-    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) {
+        g_autofree uint8_t *encryptedValue = NULL;
+        size_t encryptedValueLen = 0;
+        const size_t ivlen = 16;
+        g_autofree unsigned char *ivbuf = g_new0(unsigned char, ivlen);
+
+        filename = g_strconcat(obj->secretValueFile, schemeInfo[0].suffix, NULL);
+
+        if (virRandomBytes(ivbuf, ivlen) < 0)
             return -1;
-        }
+
         if (virCryptoEncryptData(schemeInfo[0].cipher,
                                  secretsEncryptionKey, secretsKeyLen,
-                                 iv, sizeof(iv),
+                                 ivbuf, ivlen,
                                  (uint8_t *)obj->value, obj->value_size,
-                                 &encryptedValue, &encryptedValueLen) < 0) {
+                                 &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);
+
+        ivbuf = g_realloc(ivbuf, ivlen + encryptedValueLen);
+        memcpy(ivbuf + ivlen, encryptedValue, encryptedValueLen);
+
+        secretbuf = g_steal_pointer(&ivbuf);
+        secret = secretbuf;
+        secretLen = ivlen + encryptedValueLen;
     } else {
-        int baseElement = G_N_ELEMENTS(schemeInfo) - 1;
-        selectedSuffix = schemeInfo[baseElement].suffix;
-        base64 = g_base64_encode(obj->value, obj->value_size);
+        filename = g_strconcat(obj->secretValueFile, schemeInfo[G_N_ELEMENTS(schemeInfo) - 1].suffix, NULL);
+        secret = (unsigned char *) obj->value;
+        secretLen = obj->value_size;
     }

-    if (!(newSecretFile = virFileBuildPath(configDir, uuidstr, selectedSuffix))) {
-        return -1;
-    }
-    g_free(obj->secretValueFile);
-    obj->secretValueFile = g_steal_pointer(&newSecretFile);
+    base64 = g_base64_encode(secret, secretLen);

-    if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) < 0)
+    if (virFileRewriteStr(filename, 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);
-        }
+        g_autofree char *deleteFile = g_strconcat(obj->secretValueFile, schemeInfo[i].suffix, NULL);
+
+        if (STREQ(filename, deleteFile))
+            continue;
+
+        ignore_value(unlink(deleteFile));
     }
+
     return 0;
 }

@@ -811,7 +811,6 @@ int
 virSecretObjSetValue(virSecretObj *obj,
                      const unsigned char *value,
                      size_t value_size,
-                     const char *configDir,
                      bool encryptData,
                      uint8_t *secretsEncryptionKey,
                      size_t secretsKeyLen)
@@ -831,7 +830,6 @@ virSecretObjSetValue(virSecretObj *obj,
     obj->value_size = value_size;

     if (!def->isephemeral && virSecretObjSaveData(obj,
-                                                  configDir,
                                                   encryptData,
                                                   secretsEncryptionKey,
                                                   secretsKeyLen) < 0)
@@ -889,109 +887,64 @@ virSecretLoadValidateUUID(virSecretDef *def,

 static int
 virSecretLoadValue(virSecretObj *obj,
-                   const char *configDir,
                    uint8_t *secretsEncryptionKey,
                    size_t secretsKeyLen)
 {
-    int ret = -1;
-    char uuidstr[VIR_UUID_STRING_BUFLEN];
-    VIR_AUTOCLOSE fd = -1;
-    struct stat st;
+    const size_t secretFileMaxLen = 10 * 1024 * 1024; /* more won't fit in the RPC buffer */
     size_t i;

-    g_autofree char *contents = NULL;
-    g_autofree uint8_t *contentsEncrypted = NULL;
-    g_autofree uint8_t *decryptedValue = NULL;
+    /* Find the file and match the storage scheme based on suffix. */
+    for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) {
+        g_autofree char *filename = g_strconcat(obj->secretValueFile,
+                                                schemeInfo[i].suffix, NULL);
+        g_autofree char *filecontent = NULL;
+        int filelen = 0;
+        g_autofree unsigned char *decoded = NULL;
+        size_t decodedlen = 0;
+
+        if (!virFileExists(filename))
+            continue;

-    size_t decryptedValueLen = 0;
-    uint8_t iv[16] = { 0 };
-    uint8_t *ciphertext = NULL;
-    size_t ciphertextLen = 0;
+        if ((filelen = virFileReadAll(filename, secretFileMaxLen, &filecontent)) < 0)
+            return -1;

-    virUUIDFormat(obj->def->uuid, uuidstr);
+        filecontent = g_realloc(filecontent, filelen + 1);
+        filecontent[filelen] = '\0';

-    /* 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;
-        }
-        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);
+        decoded = g_base64_decode(filecontent, &decodedlen);

-            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);
+        virSecureErase(filecontent, filelen);
+
+        if (schemeInfo[i].cipher == -1) {
+            obj->value = g_steal_pointer(&decoded);
+            obj->value_size = decodedlen;
+        } else {
+            size_t ivlen = 16;
+            int rc;
+
+            if (decodedlen < ivlen) {
+                virReportError(VIR_ERR_INVALID_SECRET,
+                               _("Encrypted secret size '%1$zu' is invalid"),
+                               obj->value_size);
+
+                virSecureErase(decoded, decodedlen);
+                return -1;
             }

-            g_free(obj->secretValueFile);
-            obj->secretValueFile = g_steal_pointer(&candidatePath);
+            rc = virCryptoDecryptData(schemeInfo[i].cipher,
+                                      secretsEncryptionKey, secretsKeyLen,
+                                      decoded, ivlen, /* initialization vector is stored at start of the buffer */
+                                      decoded + ivlen, decodedlen - ivlen,
+                                      &obj->value, &obj->value_size);

-            break;
+            virSecureErase(decoded, decodedlen);
+
+            if (rc < 0)
+                return -1;
         }
     }
-    ret = 0;
- cleanup:
-    virSecureErase(contentsEncrypted, obj->value_size);
-    virSecureErase(contents, st.st_size);
-    virSecureErase(iv, sizeof(iv));
-    return ret;
+
+    return 0;
 }


@@ -1015,7 +968,7 @@ virSecretLoad(virSecretObjList *secrets,
     if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL)))
         return NULL;

-    if (virSecretLoadValue(obj, configDir, secretsEncryptionKey, secretsKeyLen) < 0) {
+    if (virSecretLoadValue(obj, secretsEncryptionKey, secretsKeyLen) < 0) {
         virSecretObjListRemove(secrets, obj);
         g_clear_pointer(&obj, virObjectUnref);
         return NULL;
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index 74a36baf6d..4e872f7b29 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -87,7 +87,6 @@ virSecretObjSaveConfig(virSecretObj *obj);

 int
 virSecretObjSaveData(virSecretObj *obj,
-                     const char *configDir,
                      bool encryptData,
                      uint8_t *secretsEncryptionKey,
                      size_t secretsKeyLen);
@@ -106,7 +105,6 @@ int
 virSecretObjSetValue(virSecretObj *obj,
                      const unsigned char *value,
                      size_t value_size,
-                     const char *configDir,
                      bool encryptData,
                      uint8_t *secretsEncryptionKey,
                      size_t secretsKeyLen);
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index e1668730dd..2f4ac60f5a 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -231,7 +231,6 @@ secretDefineXML(virConnectPtr conn,
         if (backup && backup->isephemeral) {
             if (virSecretObjSaveData(obj,
                                      driver->configDir,
-                                     driver->config->encryptData,
                                      driver->config->secretsEncryptionKey,
                                      driver->config->secretsKeyLen) < 0)
                 goto restore_backup;
@@ -339,7 +338,6 @@ secretSetValue(virSecretPtr secret,

     if (virSecretObjSetValue(obj, value, value_size,
                              driver->configDir,
-                             driver->config->encryptData,
                              driver->config->secretsEncryptionKey,
                              driver->config->secretsKeyLen) < 0)
         goto cleanup;

    