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 <armenon@redhat.com> --- 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.