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(-)
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) ...
[2]
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.
Yes, I do agree and that was my initial thought based on the suggestion. But there was this specific scenario where it failed for me. IIRC, the scenario was when we upgrade libvirt.
For older version of libvirt, the secret will be stored as /etc/libvirt/secrets/<uuid>.base64
obj->secretValueFile stores '/etc/libvirt/secrets/<uuid>' per above initialization.
after libvirt is upgraded, the virSecretLoadValue() will be called to load an existing value from the disk, and that will set obj->secretValueFile to /etc/libvirt/secrets/<uuid>.base64.
So you take it and add the '.base64' suffix.
During an update to the same secret, using virsh secret-set-value <uuid> obj->secretValueFile still has /etc/libvirt/secret/<uuid>.base64 and it will concatenate and make it
no, this is still '/etc/libvirt/secrets/<uuid>' per the code that initializes it see [2] above.
/etc/libvirt/secret/<uuid>.base64.aes256cbc. I got this file during testing, and thats when I changed the logic.
you thus concatenate it with .aes256cbc and everything is fine.
To avoid this double extension, I would have to implement some logic to strip the old extension in virSecretObjSaveData() before appending a new one. I thought that regenerating the whole path was cleane
- 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.
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.
If the user tries to set a value, then it will actually populate obj->secretValueFile cleanly. Whereas if the user tries to undefine the secret, then unlink(obj->secretValueFile) where obj->secretValueFile is /etc/libvirt/secrets/<uuid> will fail. That should be safe.
Sure, I need to check the callers.