On Thu, Nov 20, 2025 at 22:23:46 +0530, Arun Menon via Devel wrote:
> Since we now have the functionality to provide the secrets driver
> with an encryption key, and the newly introduced attribute to store
> the encryption scheme across driver restarts, we can use the key to encrypt
> secrets. While loading the secrets, we check whether the secret is
> encrypted or not and accordingly get the value.
>
> While the stored encryption scheme ensures the driver can
> successfully load secrets after a restart,
> If the user changes the encryption key between driver restarts,
> any secrets encrypted with the previous key will become permanently
> inaccessible upon the next restart. Users must ensure key consistency
> to maintain access to existing encrypted secrets.
>
> Signed-off-by: Arun Menon <[email protected]>
> ---
> src/conf/virsecretobj.c | 165 ++++++++++++++++++++++++++++++-------
> src/conf/virsecretobj.h | 10 ++-
> src/secret/secret_driver.c | 23 ++++--
> 3 files changed, 157 insertions(+), 41 deletions(-)
All changes related to adding of the secret driver config, parsing etc
ought to be either part of the commit that adds the config parser or a
separate commit after it, but not intermixed with the implementation of
encrypting the individual secrets.
Based on the feedback to 4/5 we also don't want to store the secret type
in the XML so I'll not comment on anything related to that.,
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index 66270e2751..37b2db960f 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -328,6 +332,8 @@ virSecretObjListAdd(virSecretObjList *secrets,
> virSecretObj *obj;
> virSecretDef *objdef;
> virSecretObj *ret = NULL;
> + const char *encryptionScheme = NULL;
> + const char *encryptionSchemeExt = NULL;
You declare these as const, then assign string copies into them and then
use g_free on this. Use normal strings instead and auto-free them.
> char uuidstr[VIR_UUID_STRING_BUFLEN];
>
> virObjectRWLockWrite(secrets);
> @@ -379,10 +385,26 @@ virSecretObjListAdd(virSecretObjList *secrets,
> goto cleanup;
>
> /* Generate the possible configFile and base64File strings
> - * using the configDir, uuidstr, and appropriate suffix
> + * using the configDir, uuidstr, and appropriate encryption scheme
> */
> - if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
> ".xml")) ||
> - !(obj->base64File = virFileBuildPath(configDir, uuidstr,
> ".base64")))
> + if ((*newdef)->encryption_scheme != VIR_SECRET_ENCRYPTION_SCHEME_NONE
> + && (*newdef)->encryption_scheme != -1) {
> + encryptionScheme =
> virSecretEncryptionSchemeTypeToString((*newdef)->encryption_scheme);
> + if (!encryptionScheme) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown secret encryption scheme %1$d"),
> (*newdef)->encryption_scheme);
> + goto cleanup;
> + }
> + encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL);
> + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr,
> encryptionSchemeExt))) {
> + goto cleanup;
> + }
> + } else {
> + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr,
> ".base64"))) {
> + goto cleanup;
> + }
> + }
You'll need to probe instead which of the suffixes we support exist in
order from the most preffered one.
Also rename the variable holding the filename to something like
'secretValueFile'.
> + if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
> ".xml")))
> goto cleanup;
>
> if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
[...]
> @@ -680,17 +703,49 @@ virSecretObjSaveConfig(virSecretObj *obj)
> return 0;
> }
>
> -
> int
> -virSecretObjSaveData(virSecretObj *obj)
> +virSecretObjSaveData(virSecretObj *obj,
> + virSecretDaemonConfig *driverConfig)
> {
> g_autofree char *base64 = NULL;
> + g_autofree uint8_t *encryptedValue = NULL;
> + size_t encryptedValueLen = 0;
> + size_t base64Len = 0;
> + uint8_t iv[16] = { 0 };
>
> if (!obj->value)
> return 0;
>
> - base64 = g_base64_encode(obj->value, obj->value_size);
> -
> + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE
> + || obj->def->encryption_scheme == -1) {
coding style
> + base64 = g_base64_encode(obj->value, obj->value_size);
> + } else {
> + if (driverConfig == NULL || driverConfig->secrets_encryption_key ==
> NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot encrypt secret value without encryption
> key"));
> + return -1;
> + }
> + if (virRandomBytes(iv, sizeof(iv)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to
> generate random IV"));
virRandomBytes already reports an error; we usually don't overwrite them
> + return -1;
> + }
> + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC,
> + driverConfig->secrets_encryption_key,
> driverConfig->secretsKeyLen,
> + iv, sizeof(iv),
> + (uint8_t *)obj->value, obj->value_size,
> + &encryptedValue, &encryptedValueLen) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to
> encrypt secret value"));
Same here.
> + return -1;
> + }
> + base64Len = sizeof(iv) + encryptedValueLen;
> + base64 = g_new0(char, base64Len);
> + memcpy(base64, iv, sizeof(iv));
> + memcpy(base64 + sizeof(iv), encryptedValue, encryptedValueLen);
> + /* Now the secret is encrypted and stored on disk. However,
> + * we did not change anything in the obj->value. This is done on
> + * purpose, as SecretObjGetValue should be able to read it as is.
> + * This will indeed be a base64 encoded secret*/
I didn't really go check why we store these as base64 but I'd suggest
that the encrypted value is still stored as base64
> + }
> if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0)
> return -1;
>
> @@ -733,27 +788,25 @@ virSecretObjGetValue(virSecretObj *obj)
> return ret;
> }
>
> -
> int
Spurious whitespace change. Our coding style prefers 2 empty lines
between functions.
> virSecretObjSetValue(virSecretObj *obj,
> const unsigned char *value,
> - size_t value_size)
> + size_t value_size,
> + virSecretDaemonConfig *driverConfig)
> {
> virSecretDef *def = obj->def;
> g_autofree unsigned char *old_value = NULL;
> g_autofree unsigned char *new_value = NULL;
> size_t old_value_size;
> -
> new_value = g_new0(unsigned char, value_size);
>
> old_value = obj->value;
> old_value_size = obj->value_size;
> -
> memcpy(new_value, value, value_size);
> obj->value = g_steal_pointer(&new_value);
> obj->value_size = value_size;
All of the whitespace changes are spurious.
>
> - if (!def->isephemeral && virSecretObjSaveData(obj) < 0)
> + if (!def->isephemeral && virSecretObjSaveData(obj, driverConfig) < 0)
> goto error;
>
> /* Saved successfully - drop old value */
> @@ -786,7 +839,6 @@ virSecretObjSetValueSize(virSecretObj *obj,
> obj->value_size = value_size;
> }
>
> -
> static int
ditto
> virSecretLoadValidateUUID(virSecretDef *def,
> const char *file)
> @@ -807,11 +859,18 @@ virSecretLoadValidateUUID(virSecretDef *def,
>
>
> static int
> -virSecretLoadValue(virSecretObj *obj)
> +virSecretLoadValue(virSecretObj *obj,
> + virSecretDaemonConfig *driverConfig)
> {
> int ret = -1, fd = -1;
> struct stat st;
> g_autofree char *contents = NULL;
> + g_autofree uint8_t *contents_encrypted = NULL;
> + g_autofree uint8_t *decryptedValue = NULL;
> + size_t decryptedValueLen = 0;
> + uint8_t iv[16] = { 0 };
> + uint8_t *ciphertext = NULL;
> + size_t ciphertextLen = 0;
>
> if ((fd = open(obj->base64File, O_RDONLY)) == -1) {
> if (errno == ENOENT) {
> @@ -841,25 +900,65 @@ virSecretLoadValue(virSecretObj *obj)
> 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->base64File);
> - goto cleanup;
> + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE ||
> + obj->def->encryption_scheme == -1) {
> + 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->base64File);
> + goto cleanup;
> + }
> + contents[st.st_size] = '\0';
> + obj->value = g_base64_decode(contents, &obj->value_size);
> + if (obj->value == NULL) {
> + virReportError(VIR_ERR_INVALID_SECRET, "%s",
> + _("cannot decode base64 secret value"));
> + goto cleanup;
> + }
> + } else {
> + if (driverConfig->secrets_encryption_key == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot decrypt secret value without encryption
> key"));
> + goto cleanup;
> + }
> + contents_encrypted = g_new0(uint8_t, st.st_size);
> + if (saferead(fd, contents_encrypted, st.st_size) != st.st_size) {
> + virReportSystemError(errno, _("cannot read '%1$s'"),
> + obj->base64File);
> + goto cleanup;
> + }
> + if ((st.st_size) < sizeof(iv)) {
> + virReportError(VIR_ERR_INVALID_SECRET, "%s",
> + _("Encrypted secret size is invalid"));
> + goto cleanup;
> + }
> + memcpy(iv, contents_encrypted, sizeof(iv));
> + ciphertext = contents_encrypted + sizeof(iv);
> + ciphertextLen = st.st_size - sizeof(iv);
> + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC,
> + driverConfig->secrets_encryption_key,
> driverConfig->secretsKeyLen,
> + iv, sizeof(iv),
> + ciphertext, ciphertextLen,
> + &decryptedValue, &decryptedValueLen) < 0) {
> + virReportError(VIR_ERR_INVALID_SECRET, "%s",
> + _("Decryption of secret value failed"));
> + goto cleanup;
> + }
> + g_free(obj->value);
> + obj->value = g_steal_pointer(&decryptedValue);
> + obj->value_size = decryptedValueLen;
> }
> - contents[st.st_size] = '\0';
> -
> - VIR_FORCE_CLOSE(fd);
> -
> - obj->value = g_base64_decode(contents, &obj->value_size);
> -
> ret = 0;
>
> cleanup:
> - if (contents != NULL)
> - memset(contents, 0, st.st_size);
> + if (contents != NULL) {
> + memset(contents, 0, st.st_size+1);
> + }
> + if (contents_encrypted != NULL) {
> + memset(contents_encrypted, 0, st.st_size);
> + }
Use virSecureErase; memset can be optimized out. Also for any existing
code.
> VIR_FORCE_CLOSE(fd);
> + virSecureErase(iv, sizeof(iv));
> return ret;
> }
[...]
> @@ -70,6 +75,8 @@ struct _virSecretDriverState {
>
> /* Immutable pointer, self-locking APIs */
> virInhibitor *inhibitor;
> +
> + virSecretDaemonConfig *config;
Please annotate this one as the other pointers are.
