[RFC v2 5/5] secret: Add functionality to load and save secrets in encrypted format

21 Nov 2025

Since we now have the functionality to provide the secrets driver
with an encryption key, and the newly introduced attribute to store
the encryption scheme across driver restarts, we can use the key to encrypt
secrets. While loading the secrets, we check whether the secret is
encrypted or not and accordingly get the value.

While the stored encryption scheme ensures the driver can
successfully load secrets after a restart,
If the user changes the encryption key between driver restarts,
any secrets encrypted with the previous key will become permanently
inaccessible upon the next restart. Users must ensure key consistency
to maintain access to existing encrypted secrets.

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 src/conf/virsecretobj.c    | 165 ++++++++++++++++++++++++++++++-------
 src/conf/virsecretobj.h    |  10 ++-
 src/secret/secret_driver.c |  23 ++++--
 3 files changed, 157 insertions(+), 41 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 66270e2751..37b2db960f 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
 
@@ -328,6 +332,8 @@ virSecretObjListAdd(virSecretObjList *secrets,
     virSecretObj *obj;
     virSecretDef *objdef;
     virSecretObj *ret = NULL;
+    const char *encryptionScheme = NULL;
+    const char *encryptionSchemeExt = NULL;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
 
     virObjectRWLockWrite(secrets);
@@ -379,10 +385,26 @@ virSecretObjListAdd(virSecretObjList *secrets,
             goto cleanup;
 
         /* Generate the possible configFile and base64File strings
-         * using the configDir, uuidstr, and appropriate suffix
+         * using the configDir, uuidstr, and appropriate encryption scheme
          */
-        if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
-            !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
+        if ((*newdef)->encryption_scheme != VIR_SECRET_ENCRYPTION_SCHEME_NONE
+            && (*newdef)->encryption_scheme != -1) {
+            encryptionScheme = virSecretEncryptionSchemeTypeToString((*newdef)->encryption_scheme);
+            if (!encryptionScheme) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("unknown secret encryption scheme %1$d"), (*newdef)->encryption_scheme);
+                goto cleanup;
+            }
+            encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL);
+            if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, encryptionSchemeExt))) {
+                goto cleanup;
+            }
+        } else {
+            if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) {
+                goto cleanup;
+            }
+        }
+        if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")))
             goto cleanup;
 
         if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
@@ -397,6 +419,7 @@ virSecretObjListAdd(virSecretObjList *secrets,
  cleanup:
     virSecretObjEndAPI(&obj);
     virObjectRWUnlock(secrets);
+    g_clear_pointer((gpointer *)&encryptionSchemeExt, g_free);
     return ret;
 }
 
@@ -680,17 +703,49 @@ virSecretObjSaveConfig(virSecretObj *obj)
     return 0;
 }
 
-
 int
-virSecretObjSaveData(virSecretObj *obj)
+virSecretObjSaveData(virSecretObj *obj,
+                     virSecretDaemonConfig *driverConfig)
 {
     g_autofree char *base64 = NULL;
+    g_autofree uint8_t *encryptedValue = NULL;
+    size_t encryptedValueLen = 0;
+    size_t base64Len = 0;
+    uint8_t iv[16] = { 0 };
 
     if (!obj->value)
         return 0;
 
-    base64 = g_base64_encode(obj->value, obj->value_size);
-
+    if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE
+        || obj->def->encryption_scheme == -1) {
+        base64 = g_base64_encode(obj->value, obj->value_size);
+    } else {
+        if (driverConfig == NULL || driverConfig->secrets_encryption_key == NULL) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("cannot encrypt secret value without encryption key"));
+            return -1;
+        }
+        if (virRandomBytes(iv, sizeof(iv)) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate random IV"));
+            return -1;
+        }
+        if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC,
+                                 driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen,
+                                 iv, sizeof(iv),
+                                 (uint8_t *)obj->value, obj->value_size,
+                                 &encryptedValue, &encryptedValueLen) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to encrypt secret value"));
+            return -1;
+        }
+        base64Len = sizeof(iv) + encryptedValueLen;
+        base64 = g_new0(char, base64Len);
+        memcpy(base64, iv, sizeof(iv));
+        memcpy(base64 + sizeof(iv), encryptedValue, encryptedValueLen);
+        /* Now the secret is encrypted and stored on disk. However,
+         * we did not change anything in the obj->value. This is done on
+         * purpose, as SecretObjGetValue should be able to read it as is.
+         * This will indeed be a base64 encoded secret*/
+    }
     if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0)
         return -1;
 
@@ -733,27 +788,25 @@ virSecretObjGetValue(virSecretObj *obj)
     return ret;
 }
 
-
 int
 virSecretObjSetValue(virSecretObj *obj,
                      const unsigned char *value,
-                     size_t value_size)
+                     size_t value_size,
+                     virSecretDaemonConfig *driverConfig)
 {
     virSecretDef *def = obj->def;
     g_autofree unsigned char *old_value = NULL;
     g_autofree unsigned char *new_value = NULL;
     size_t old_value_size;
-
     new_value = g_new0(unsigned char, value_size);
 
     old_value = obj->value;
     old_value_size = obj->value_size;
-
     memcpy(new_value, value, value_size);
     obj->value = g_steal_pointer(&new_value);
     obj->value_size = value_size;
 
-    if (!def->isephemeral && virSecretObjSaveData(obj) < 0)
+    if (!def->isephemeral && virSecretObjSaveData(obj, driverConfig) < 0)
         goto error;
 
     /* Saved successfully - drop old value */
@@ -786,7 +839,6 @@ virSecretObjSetValueSize(virSecretObj *obj,
     obj->value_size = value_size;
 }
 
-
 static int
 virSecretLoadValidateUUID(virSecretDef *def,
                           const char *file)
@@ -807,11 +859,18 @@ virSecretLoadValidateUUID(virSecretDef *def,
 
 
 static int
-virSecretLoadValue(virSecretObj *obj)
+virSecretLoadValue(virSecretObj *obj,
+                   virSecretDaemonConfig *driverConfig)
 {
     int ret = -1, fd = -1;
     struct stat st;
     g_autofree char *contents = NULL;
+    g_autofree uint8_t *contents_encrypted = NULL;
+    g_autofree uint8_t *decryptedValue = NULL;
+    size_t decryptedValueLen = 0;
+    uint8_t iv[16] = { 0 };
+    uint8_t *ciphertext = NULL;
+    size_t ciphertextLen = 0;
 
     if ((fd = open(obj->base64File, O_RDONLY)) == -1) {
         if (errno == ENOENT) {
@@ -841,25 +900,65 @@ 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->base64File);
-        goto cleanup;
+    if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE ||
+        obj->def->encryption_scheme == -1) {
+            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->base64File);
+                goto cleanup;
+            }
+            contents[st.st_size] = '\0';
+            obj->value = g_base64_decode(contents, &obj->value_size);
+            if (obj->value == NULL) {
+                virReportError(VIR_ERR_INVALID_SECRET, "%s",
+                               _("cannot decode base64 secret value"));
+                goto cleanup;
+            }
+    } else {
+        if (driverConfig->secrets_encryption_key == NULL) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("cannot decrypt secret value without encryption key"));
+            goto cleanup;
+        }
+        contents_encrypted = g_new0(uint8_t, st.st_size);
+        if (saferead(fd, contents_encrypted, st.st_size) != st.st_size) {
+            virReportSystemError(errno, _("cannot read '%1$s'"),
+                                 obj->base64File);
+            goto cleanup;
+        }
+        if ((st.st_size) < sizeof(iv)) {
+            virReportError(VIR_ERR_INVALID_SECRET, "%s",
+                           _("Encrypted secret size is invalid"));
+            goto cleanup;
+        }
+        memcpy(iv, contents_encrypted, sizeof(iv));
+        ciphertext = contents_encrypted + sizeof(iv);
+        ciphertextLen = st.st_size - sizeof(iv);
+        if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC,
+                                 driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen,
+                                 iv, sizeof(iv),
+                                 ciphertext, ciphertextLen,
+                                 &decryptedValue, &decryptedValueLen) < 0) {
+            virReportError(VIR_ERR_INVALID_SECRET, "%s",
+                           _("Decryption of secret value failed"));
+            goto cleanup;
+        }
+        g_free(obj->value);
+        obj->value = g_steal_pointer(&decryptedValue);
+        obj->value_size = decryptedValueLen;
     }
-    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);
+    if (contents != NULL) {
+        memset(contents, 0, st.st_size+1);
+    }
+    if (contents_encrypted != NULL) {
+        memset(contents_encrypted, 0, st.st_size);
+    }
     VIR_FORCE_CLOSE(fd);
+    virSecureErase(iv, sizeof(iv));
     return ret;
 }
 
@@ -868,7 +967,8 @@ static virSecretObj *
 virSecretLoad(virSecretObjList *secrets,
               const char *file,
               const char *path,
-              const char *configDir)
+              const char *configDir,
+              virSecretDaemonConfig *driverConfig)
 {
     g_autoptr(virSecretDef) def = NULL;
     virSecretObj *obj = NULL;
@@ -882,7 +982,7 @@ virSecretLoad(virSecretObjList *secrets,
     if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL)))
         return NULL;
 
-    if (virSecretLoadValue(obj) < 0) {
+    if (virSecretLoadValue(obj, driverConfig) < 0) {
         virSecretObjListRemove(secrets, obj);
         g_clear_pointer(&obj, virObjectUnref);
         return NULL;
@@ -894,7 +994,8 @@ virSecretLoad(virSecretObjList *secrets,
 
 int
 virSecretLoadAllConfigs(virSecretObjList *secrets,
-                        const char *configDir)
+                        const char *configDir,
+                        virSecretDaemonConfig *driverConfig)
 {
     g_autoptr(DIR) dir = NULL;
     struct dirent *de;
@@ -915,7 +1016,7 @@ 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, driverConfig))) {
             VIR_ERROR(_("Error reading secret: %1$s"),
                       virGetLastErrorMessage());
             continue;
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index 17897c5513..f49600a75c 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -23,6 +23,7 @@
 #include "internal.h"
 
 #include "secret_conf.h"
+#include "secret_config.h"
 
 typedef struct _virSecretObj virSecretObj;
 
@@ -86,7 +87,8 @@ int
 virSecretObjSaveConfig(virSecretObj *obj);
 
 int
-virSecretObjSaveData(virSecretObj *obj);
+virSecretObjSaveData(virSecretObj *obj,
+                     virSecretDaemonConfig *driverConfig);
 
 virSecretDef *
 virSecretObjGetDef(virSecretObj *obj);
@@ -101,7 +103,8 @@ virSecretObjGetValue(virSecretObj *obj);
 int
 virSecretObjSetValue(virSecretObj *obj,
                      const unsigned char *value,
-                     size_t value_size);
+                     size_t value_size,
+                     virSecretDaemonConfig *driverConfig);
 
 size_t
 virSecretObjGetValueSize(virSecretObj *obj);
@@ -112,4 +115,5 @@ virSecretObjSetValueSize(virSecretObj *obj,
 
 int
 virSecretLoadAllConfigs(virSecretObjList *secrets,
-                        const char *configDir);
+                        const char *configDir,
+                        virSecretDaemonConfig *cfg);
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 04c3ca49f1..c0cac39a28 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"
 #include "virsecretobj.h"
 #include "secret_driver.h"
 #include "virthread.h"
@@ -42,6 +43,10 @@
 #include "secret_event.h"
 #include "virutil.h"
 #include "virinhibitor.h"
+#include "virfile.h"
+#include "virrandom.h"
+#include "vircrypto.h"
+#include "virsecureerase.h"
 
 #define VIR_FROM_THIS VIR_FROM_SECRET
 
@@ -70,6 +75,8 @@ struct _virSecretDriverState {
 
     /* Immutable pointer, self-locking APIs */
     virInhibitor *inhibitor;
+
+    virSecretDaemonConfig *config;
 };
 
 static virSecretDriverState *driver;
@@ -224,7 +231,7 @@ secretDefineXML(virConnectPtr conn,
 
     if (!objDef->isephemeral) {
         if (backup && backup->isephemeral) {
-            if (virSecretObjSaveData(obj) < 0)
+            if (virSecretObjSaveData(obj, driver->config) < 0)
                 goto restore_backup;
         }
 
@@ -307,7 +314,6 @@ secretGetXMLDesc(virSecretPtr secret,
     return ret;
 }
 
-
 static int
 secretSetValue(virSecretPtr secret,
                const unsigned char *value,
@@ -327,8 +333,7 @@ secretSetValue(virSecretPtr secret,
     def = virSecretObjGetDef(obj);
     if (virSecretSetValueEnsureACL(secret->conn, def) < 0)
         goto cleanup;
-
-    if (virSecretObjSetValue(obj, value, value_size) < 0)
+    if (virSecretObjSetValue(obj, value, value_size, driver->config) < 0)
         goto cleanup;
 
     event = virSecretEventValueChangedNew(def->uuid,
@@ -454,6 +459,7 @@ secretStateCleanupLocked(void)
     VIR_FREE(driver->configDir);
 
     virObjectUnref(driver->secretEventState);
+    virObjectUnref(driver->config);
     virInhibitorFree(driver->inhibitor);
 
     if (driver->lockFD != -1)
@@ -518,6 +524,8 @@ secretStateInitialize(bool privileged,
                              driver->stateDir);
         goto error;
     }
+    if (!(driver->config = virSecretDaemonConfigNew(driver->privileged)))
+        goto error;
 
     driver->inhibitor = virInhibitorNew(
         VIR_INHIBITOR_WHAT_NONE,
@@ -534,7 +542,7 @@ secretStateInitialize(bool privileged,
     if (!(driver->secrets = virSecretObjListNew()))
         goto error;
 
-    if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0)
+    if (virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config) < 0)
         goto error;
 
     return VIR_DRV_STATE_INIT_COMPLETE;
@@ -553,7 +561,10 @@ secretStateReload(void)
     if (!driver)
         return -1;
 
-    ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir));
+    if (!(driver->config = virSecretDaemonConfigNew(driver->privileged)))
+        return -1;
+
+    ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config));
 
     return 0;
 }
-- 
2.51.1

    