Hi Peter, On Fri, Feb 06, 2026 at 03:49:08PM +0100, Peter Krempa wrote: > On Fri, Jan 09, 2026 at 23:39:35 +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 | 175 ++++++++++++++++++++++++++++++------- > > src/conf/virsecretobj.h | 18 +++- > > src/secret/secret_driver.c | 23 +++-- > > 3 files changed, 176 insertions(+), 40 deletions(-) > > > > diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c > > index a3dd7983bb..4dcb32f69a 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); > > @@ -323,7 +337,8 @@ virSecretObj * > > virSecretObjListAdd(virSecretObjList *secrets, > > virSecretDef **newdef, > > const char *configDir, > > - virSecretDef **oldDef) > > + virSecretDef **oldDef, > > + bool encryptData) > > { > > virSecretObj *obj; > > virSecretDef *objdef; > > @@ -363,6 +378,11 @@ virSecretObjListAdd(virSecretObjList *secrets, > > } else { > > /* No existing secret with same UUID, > > * try look for matching usage instead */ > > + const char *secretSuffix = ".base64"; > > + g_autofree char *encryptionSchemeSuffix = NULL; > > + g_autofree char *oldSecretValueFile = virFileBuildPath(configDir, > > + uuidstr, > > + > > secretSuffix); > > if ((obj = virSecretObjListFindByUsageLocked(secrets, > > (*newdef)->usage_type, > > > > (*newdef)->usage_id))) { > > @@ -379,10 +399,24 @@ virSecretObjListAdd(virSecretObjList *secrets, > > goto cleanup; > > > > /* Generate the possible configFile and secretValueFile strings > > - * using the configDir, uuidstr, and appropriate suffix > > + * using the configDir, uuidstr, and appropriate suffix. > > + * By default, the latest encryption cipher will be used to > > encrypt secrets. > > */ > > + if (encryptData) { > > + /* The virSecretObjListAdd() function is called during both > > + * loading a secret and creating a new one. Check if there is > > an unencrypted > > + * .base64 secret present on the disk. > > + */ > > + if (virFileExists(oldSecretValueFile)) { > > + encryptionSchemeSuffix = g_strdup(secretSuffix); > > + } else { > > + encryptionSchemeSuffix = g_strdup(schemeInfo[0].suffix); > > + } > > + } else { > > + encryptionSchemeSuffix = g_strdup(secretSuffix); > > + } > > if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, > > ".xml")) || > > - !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, > > ".base64"))) > > + !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, > > encryptionSchemeSuffix))) > > goto cleanup; > > > So as-written this doesn't work properly. First I'll demonstrate the bug > the bug. > > 1) Assume you have an existing secret prior to running this version: > > ~ # ls -1 /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872* > /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.base64 > /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.xml > ~ # virsh -q secret-get-value --plain c021dc3e-8227-4bfa-8f1c-ebd0356fb872 > ble > ~ # base64 -d > /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.base64 > ble > ~ # cat /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.base64; > echo > YmxlCg== > > 2) update secret (set the same thing) > > ~ # virsh secret-set-value --secret c021dc3e-8227-4bfa-8f1c-ebd0356fb872 > --base64 YmxlCg== > warning: Passing secret value as command-line argument is insecure! > Secret value set > > ~ # ls -1 /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872* > /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.base64 > /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.xml > ~ # cat /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.base64; > echo > 3gInXNNABHGXFK64pFkH4AfxTc5HzSLiZnyXm+7h+ms= > ~ # base64 -d > /etc/libvirt/secrets/c021dc3e-8227-4bfa-8f1c-ebd0356fb872.base64 > Þ'\Ó@q®¸¤YàñMÎGÍ"âf|îáúki > > 3) after daemon restart this obviously corrupts further: > > ~ # virsh secret-get-value c021dc3e-8227-4bfa-8f1c-ebd0356fb872 --plain > Þ'\Ó@q®¸¤YàñMÎGÍ"âf|îáúk > > As you can see the old filename is used but the value is saved using the > new encryption scheme. > > For this to work properly we must either > A) Store the format based on the file where we loaded it from and on > subsequent updates on the secret use the saved format > > or > > B) On any update use the newer format but nuke the old file too. > > In addition I don't like that virSecretObjListAdd() is looking at files > in the host in the first place. In addition it also duplicates the > format checking which is then done in virSecretLoadValue > > Thus I think that: > 1) virSecretObjListAdd ought to only generate the file name prefix, not > the actual full filename > 2) virSecretLoadValue will concatenate the prefix with the suffix based > on the list of supported schemes/formats, check if given file exists > and load it if yes, otherwise continue down the list > 3) virSecretObjSetValue will (if we go with option B) save using the > most recent format, then go down the list and unlink any other > files with the same prefix using the older formats/schemes > > (or in case of option A first check if any existing variant exists, > if yes use that, otherwise pick the one from top of the list.) > > The rest of the patch looks good to me. > > Since this will require another version don't forget to apply the other > things I've pointed out. >
Thank you so much for the feedback and testing the feature. I totally missed the scenario of upgrade followed by updating the old secrets. I liked your second suggestion of updating the secret with the newer format. I have incorporated that, along with other changes you suggested in my new version v5. Regards, Arun Menon
