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.
