On Thu, Nov 13, 2025 at 07:02:23PM +0530, Arun Menon wrote:
> Since we now have the functionality to provide the secrets driver
> with an encryption key, we can use it to encrypt the secrets.
> While loading the secrets, we check whether the secret is
> encrypted or not and accordingly get the value.
>
> The value_encrypted boolean flag is currently ephemeral (in memory).
> This flag must be persisted to the disk to ensure that the secrets
> service knows whether the secret is in the plaintext or ciphertext
> format across restarts. This is vital and will be addressed in subsequent
> commits.
>
> Signed-off-by: Arun Menon <[email protected]>
> ---
> src/conf/virsecretobj.c | 13 +++++++
> src/conf/virsecretobj.h | 7 ++++
> src/libvirt_private.syms | 2 ++
> src/secret/secret_driver.c | 72 ++++++++++++++++++++++++++++++++++++--
> 4 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index 66270e2751..8184c3e49e 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -43,6 +43,7 @@ struct _virSecretObj {
> virSecretDef *def;
> unsigned char *value; /* May be NULL */
> size_t value_size;
> + bool value_encrypted;
> };
>
> static virClass *virSecretObjClass;
> @@ -786,6 +787,18 @@ virSecretObjSetValueSize(virSecretObj *obj,
> obj->value_size = value_size;
> }
>
> +bool
> +virSecretObjGetEncryptionFlag(virSecretObj *obj)
> +{
> + return obj->value_encrypted;
> +}
> +
> +void
> +virSecretObjSetEncryptionFlag(virSecretObj *obj,
> + bool encryption)
> +{
> + obj->value_encrypted = encryption;
> +}
We shouldn't need these IMHO. Whether or not a value is
encrypted should only matter to the code that is loading
the secrets, and should not persist thereafter.
If the plain txt secret has its value updated, then the
new file on disk should always reflect the currently
configured encryption settings. This implies the code
saving the secret needs to delete any old written
secret files
> diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
> index 17897c5513..4e3b285b82 100644
> --- a/src/conf/virsecretobj.h
> +++ b/src/conf/virsecretobj.h
> @@ -113,3 +113,10 @@ virSecretObjSetValueSize(virSecretObj *obj,
> int
> virSecretLoadAllConfigs(virSecretObjList *secrets,
> const char *configDir);
> +
> +void
> +virSecretObjSetEncryptionFlag(virSecretObj *obj,
> + bool encryption);
> +
> +bool
> +virSecretObjGetEncryptionFlag(virSecretObj *obj);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fc5fdb00f4..cf6a4ffe03 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1483,6 +1483,7 @@ virSecretObjDeleteConfig;
> virSecretObjDeleteData;
> virSecretObjEndAPI;
> virSecretObjGetDef;
> +virSecretObjGetEncryptionFlag;
> virSecretObjGetValue;
> virSecretObjGetValueSize;
> virSecretObjListAdd;
> @@ -1496,6 +1497,7 @@ virSecretObjListRemove;
> virSecretObjSaveConfig;
> virSecretObjSaveData;
> virSecretObjSetDef;
> +virSecretObjSetEncryptionFlag;
> virSecretObjSetValue;
> virSecretObjSetValueSize;
>
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 0b415e5ef3..44f0611fdc 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -43,6 +43,9 @@
> #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
>
> @@ -369,6 +372,12 @@ secretSetValue(virSecretPtr secret,
> virSecretDef *def;
> virObjectEvent *event = NULL;
>
> + g_autofree uint8_t *encryptedValue = NULL;
> + size_t encryptedValueLen = 0;
> + uint8_t iv[16] = { 0 };
> + g_autofree uint8_t *valueToSave = NULL;
> + size_t valueToSaveLen = 0;
> +
> virCheckFlags(0, -1);
>
> if (!(obj = secretObjFromSecret(secret)))
> @@ -378,8 +387,32 @@ secretSetValue(virSecretPtr secret,
> if (virSecretSetValueEnsureACL(secret->conn, def) < 0)
> goto cleanup;
>
> - if (virSecretObjSetValue(obj, value, value_size) < 0)
> - goto cleanup;
> + if (driver->encrypt_data != 0 && driver->masterKeyLen >= 32) {
> + virSecretObjSetEncryptionFlag(obj, true);
> + if (virRandomBytes(iv, sizeof(iv)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to
> generate random IV"));
> + goto cleanup;
> + }
> + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC,
We should anticipate that in future we'll need to use algorithms
other than AES-256-CBC.
This would suggest that on disk we use the encryption scheme as
the file suffix eg
<uuid>.base64 -> plain text
<uuid>.aes256cbc -> AES-256-CBC
this make it easy to switch algs later.
This encryption logic likely needs to move into the
virSecretObjLoadValue and virSecretObjSaaveData methods.
This will require passing the encryption key and scheme
down into this methods from the driver here.
> + driver->masterKey, driver->masterKeyLen,
> + iv, sizeof(iv),
> + (uint8_t *)value, value_size,
> + &encryptedValue, &encryptedValueLen) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to
> encrypt secret value"));
> + goto cleanup;
> + }
> + valueToSaveLen = sizeof(iv) + encryptedValueLen;
> + valueToSave = g_new0(uint8_t, valueToSaveLen);
> + memcpy(valueToSave, iv, sizeof(iv));
> + memcpy(valueToSave + sizeof(iv), encryptedValue, encryptedValueLen);
> +
> + if (virSecretObjSetValue(obj, valueToSave, valueToSaveLen) < 0)
> + goto cleanup;
> + } else {
> + virSecretObjSetEncryptionFlag(obj, false);
> + if (virSecretObjSetValue(obj, value, value_size) < 0)
> + goto cleanup;
> + }
>
> event = virSecretEventValueChangedNew(def->uuid,
> def->usage_type,
> @@ -387,6 +420,9 @@ secretSetValue(virSecretPtr secret,
> ret = 0;
>
> cleanup:
> + virSecureErase(encryptedValue, encryptedValueLen);
> + virSecureErase(iv, sizeof(iv));
> + virSecureErase(valueToSave, valueToSaveLen);
> virSecretObjEndAPI(&obj);
> virObjectEventStateQueue(driver->secretEventState, event);
>
> @@ -402,6 +438,11 @@ secretGetValue(virSecretPtr secret,
> unsigned char *ret = NULL;
> virSecretObj *obj;
> virSecretDef *def;
> + g_autofree uint8_t *decryptedValue = NULL;
> + size_t decryptedValueLen = 0;
> + uint8_t iv[16] = { 0 };
> + uint8_t *ciphertext = NULL;
> + size_t ciphertextLen = 0;
>
> virCheckFlags(0, NULL);
>
> @@ -444,6 +485,32 @@ secretGetValue(virSecretPtr secret,
>
> *value_size = virSecretObjGetValueSize(obj);
>
> + if (virSecretObjGetEncryptionFlag(obj) && driver->masterKeyLen >= 32) {
> + if (*value_size < sizeof(iv)) {
> + /* The encrypted secret size should be greater than IV */
> + virReportError(VIR_ERR_INVALID_SECRET, "%s",
> + _("Encrypted secret size is invalid"));
> + goto cleanup;
> + }
> + memcpy(iv, ret, sizeof(iv));
> + ciphertext = ret + sizeof(iv);
> + ciphertextLen = *value_size - sizeof(iv);
> +
> + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC,
> + driver->masterKey, driver->masterKeyLen,
> + iv, sizeof(iv),
> + ciphertext, ciphertextLen,
> + &decryptedValue, &decryptedValueLen) < 0) {
> + virReportError(VIR_ERR_INVALID_SECRET, "%s",
> + _("Decryption of secret value failed"));
> + goto cleanup;
> + }
> +
> + g_free(ret);
> + ret = g_steal_pointer(&decryptedValue);
> + *value_size = decryptedValueLen;
> + }
> +
> cleanup:
> virSecretObjEndAPI(&obj);
>
> @@ -502,6 +569,7 @@ secretStateCleanupLocked(void)
>
> virObjectUnref(driver->secrets);
> VIR_FREE(driver->configDir);
> + VIR_FREE(driver->masterKeyPath);
This belongs in the previous patch which created the new struct field.
>
> virObjectUnref(driver->secretEventState);
> virInhibitorFree(driver->inhibitor);
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
