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 <[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)
> > ...
[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.
