On Tue, Feb 10, 2026 at 15:52:43 +0100, Peter Krempa wrote: > On Tue, Feb 10, 2026 at 17:14:45 +0530, Arun Menon wrote: > > Hi Peter, > > Thanks for response. > > > > On Tue, Feb 10, 2026 at 11:26:25AM +0100, Peter Krempa wrote: > > > On Tue, Feb 10, 2026 at 13:30:11 +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 | 249 ++++++++++++++++++++++++++++--------- > > > > src/conf/virsecretobj.h | 16 ++- > > > > src/secret/secret_driver.c | 20 ++- > > > > 3 files changed, 222 insertions(+), 63 deletions(-)
[...] > > > It also seems we're missing handling of the case [1] when the file > > > didn't exist at all. > > > > If the file did not exist at all, we come out of the loop and return 0. > > That means the value will not be set, although the secret is defined.a > > In previous code it would return an error I didn't check if that's a > problem but this changes what's happening. Okay I see what was happening. It did in fact retur 0; Anyways to avoid more backs-and forths there are 2 things: 1) you didn't configure your git-send-email tool properly, specifically git config --global format.forceInBodyFrom true https://www.libvirt.org/submitting-patches.html#git-configuration Thus the patches lack the extra From line and the commit author is thus mangled. Please re-send. 2) apply the following diff which does what I've requested: - changes to use obj->secretValueFile as base - does local suffix lookups - refactors code so that the base64 encoding is in common code paths - moves local variable declaration inside blocks using them - refactors loader to use virFileReadAll instead of open-coding - fixes virSecretObjDeleteData to not rely on the actual filename diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 49b69b4867..b448be493a 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -670,9 +670,15 @@ virSecretObjDeleteConfig(virSecretObj *obj) void virSecretObjDeleteData(virSecretObj *obj) { + size_t i; + /* The configFile will already be removed, so secret won't be * loaded again if this fails */ - unlink(obj->secretValueFile); + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) { + g_autofree char *deleteFile = g_strconcat(obj->secretValueFile, schemeInfo[i].suffix, NULL); + + ignore_value(unlink(deleteFile)); + } } @@ -699,74 +705,68 @@ virSecretObjSaveConfig(virSecretObj *obj) int virSecretObjSaveData(virSecretObj *obj, - const char *configDir, bool encryptData, uint8_t *secretsEncryptionKey, size_t secretsKeyLen) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - g_autofree char *base64 = NULL; - g_autofree char *newSecretFile = NULL; - g_autofree uint8_t *secret = NULL; - g_autofree uint8_t *encryptedValue = NULL; - - const char *selectedSuffix = NULL; - size_t encryptedValueLen = 0; - + g_autofree char *filename = NULL; size_t i; + g_autofree unsigned char *secretbuf = NULL; + const unsigned char *secret = NULL; size_t secretLen = 0; - uint8_t iv[16] = { 0 }; if (!obj->value) return 0; - virUUIDFormat(obj->def->uuid, uuidstr); - /* Based on whether encryption is on/off, save the secret in either * the latest encryption scheme or in base64 formats. * Subsequently, delete the other formats of the same uuid on the disk. */ if (encryptData && secretsEncryptionKey) { - selectedSuffix = schemeInfo[0].suffix; - if (virRandomBytes(iv, sizeof(iv)) < 0) { + g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + const size_t ivlen = 16; + g_autofree unsigned char *ivbuf = g_new0(unsigned char, ivlen); + + filename = g_strconcat(obj->secretValueFile, schemeInfo[0].suffix, NULL); + + if (virRandomBytes(ivbuf, ivlen) < 0) return -1; - } + if (virCryptoEncryptData(schemeInfo[0].cipher, secretsEncryptionKey, secretsKeyLen, - iv, sizeof(iv), + ivbuf, ivlen, (uint8_t *)obj->value, obj->value_size, - &encryptedValue, &encryptedValueLen) < 0) { + &encryptedValue, &encryptedValueLen) < 0) return -1; - } - secretLen = sizeof(iv) + encryptedValueLen; - secret = g_new0(uint8_t, secretLen); - memcpy(secret, iv, sizeof(iv)); - memcpy(secret + sizeof(iv), encryptedValue, encryptedValueLen); - base64 = g_base64_encode(secret, secretLen); + + ivbuf = g_realloc(ivbuf, ivlen + encryptedValueLen); + memcpy(ivbuf + ivlen, encryptedValue, encryptedValueLen); + + secretbuf = g_steal_pointer(&ivbuf); + secret = secretbuf; + secretLen = ivlen + encryptedValueLen; } else { - int baseElement = G_N_ELEMENTS(schemeInfo) - 1; - selectedSuffix = schemeInfo[baseElement].suffix; - base64 = g_base64_encode(obj->value, obj->value_size); + filename = g_strconcat(obj->secretValueFile, schemeInfo[G_N_ELEMENTS(schemeInfo) - 1].suffix, NULL); + secret = (unsigned char *) obj->value; + secretLen = obj->value_size; } - if (!(newSecretFile = virFileBuildPath(configDir, uuidstr, selectedSuffix))) { - return -1; - } - g_free(obj->secretValueFile); - obj->secretValueFile = g_steal_pointer(&newSecretFile); + base64 = g_base64_encode(secret, secretLen); - if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) < 0) + if (virFileRewriteStr(filename, S_IRUSR | S_IWUSR, base64) < 0) return -1; for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) { - g_autofree char* deleteFile = virFileBuildPath(configDir, - uuidstr, - schemeInfo[i].suffix); - if (STRNEQ_NULLABLE(schemeInfo[i].suffix, selectedSuffix)) { - unlink(deleteFile); - } + g_autofree char *deleteFile = g_strconcat(obj->secretValueFile, schemeInfo[i].suffix, NULL); + + if (STREQ(filename, deleteFile)) + continue; + + ignore_value(unlink(deleteFile)); } + return 0; } @@ -811,7 +811,6 @@ int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, size_t value_size, - const char *configDir, bool encryptData, uint8_t *secretsEncryptionKey, size_t secretsKeyLen) @@ -831,7 +830,6 @@ virSecretObjSetValue(virSecretObj *obj, obj->value_size = value_size; if (!def->isephemeral && virSecretObjSaveData(obj, - configDir, encryptData, secretsEncryptionKey, secretsKeyLen) < 0) @@ -889,109 +887,64 @@ virSecretLoadValidateUUID(virSecretDef *def, static int virSecretLoadValue(virSecretObj *obj, - const char *configDir, uint8_t *secretsEncryptionKey, size_t secretsKeyLen) { - int ret = -1; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - VIR_AUTOCLOSE fd = -1; - struct stat st; + const size_t secretFileMaxLen = 10 * 1024 * 1024; /* more won't fit in the RPC buffer */ size_t i; - g_autofree char *contents = NULL; - g_autofree uint8_t *contentsEncrypted = NULL; - g_autofree uint8_t *decryptedValue = NULL; + /* Find the file and match the storage scheme based on suffix. */ + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) { + g_autofree char *filename = g_strconcat(obj->secretValueFile, + schemeInfo[i].suffix, NULL); + g_autofree char *filecontent = NULL; + int filelen = 0; + g_autofree unsigned char *decoded = NULL; + size_t decodedlen = 0; + + if (!virFileExists(filename)) + continue; - size_t decryptedValueLen = 0; - uint8_t iv[16] = { 0 }; - uint8_t *ciphertext = NULL; - size_t ciphertextLen = 0; + if ((filelen = virFileReadAll(filename, secretFileMaxLen, &filecontent)) < 0) + return -1; - virUUIDFormat(obj->def->uuid, uuidstr); + filecontent = g_realloc(filecontent, filelen + 1); + filecontent[filelen] = '\0'; - /* Iterate over the list of suffixes, find the one that when appended to the - * uuid will result in a file that exists on the disk. This essentially is the - * secret file. Subsequently, load/decrypt the secret by using the appropriate - * encryption scheme. - */ - for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) { - g_autofree char *candidatePath = NULL; - if (!(candidatePath = virFileBuildPath(configDir, - uuidstr, - schemeInfo[i].suffix))) { - goto cleanup; - } - if (virFileExists(candidatePath)) { - if ((fd = open(candidatePath, O_RDONLY)) == -1) { - if (errno == ENOENT) { - ret = 0; - } else { - virReportSystemError(errno, _("cannot open '%1$s'"), - candidatePath); - } - goto cleanup; - } - if (fstat(fd, &st) < 0) { - virReportSystemError(errno, _("cannot stat '%1$s'"), - candidatePath); - goto cleanup; - } - if ((size_t)st.st_size != st.st_size) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%1$s' file does not fit in memory"), - candidatePath); - goto cleanup; - } - if (st.st_size < 1) { - ret = 0; - goto cleanup; - } - contents = g_new0(char, st.st_size + 1); + decoded = g_base64_decode(filecontent, &decodedlen); - if (saferead(fd, contents, st.st_size) != st.st_size) { - virReportSystemError(errno, _("cannot read '%1$s'"), - candidatePath); - goto cleanup; - } - contents[st.st_size] = '\0'; - if (schemeInfo[i].cipher != -1) { - contentsEncrypted = g_base64_decode(contents, &obj->value_size); - if (sizeof(iv) > obj->value_size) { - virReportError(VIR_ERR_INVALID_SECRET, - _("Encrypted secret size '%1$zu' is invalid"), - obj->value_size); - goto cleanup; - } - memcpy(iv, contentsEncrypted, sizeof(iv)); - ciphertext = contentsEncrypted + sizeof(iv); - ciphertextLen = obj->value_size - sizeof(iv); - if (virCryptoDecryptData(schemeInfo[i].cipher, - secretsEncryptionKey, secretsKeyLen, - iv, sizeof(iv), - ciphertext, ciphertextLen, - &decryptedValue, &decryptedValueLen) < 0) { - goto cleanup; - } - g_free(obj->value); - obj->value = g_steal_pointer(&decryptedValue); - obj->value_size = decryptedValueLen; - } else { - obj->value = g_base64_decode(contents, &obj->value_size); + virSecureErase(filecontent, filelen); + + if (schemeInfo[i].cipher == -1) { + obj->value = g_steal_pointer(&decoded); + obj->value_size = decodedlen; + } else { + size_t ivlen = 16; + int rc; + + if (decodedlen < ivlen) { + virReportError(VIR_ERR_INVALID_SECRET, + _("Encrypted secret size '%1$zu' is invalid"), + obj->value_size); + + virSecureErase(decoded, decodedlen); + return -1; } - g_free(obj->secretValueFile); - obj->secretValueFile = g_steal_pointer(&candidatePath); + rc = virCryptoDecryptData(schemeInfo[i].cipher, + secretsEncryptionKey, secretsKeyLen, + decoded, ivlen, /* initialization vector is stored at start of the buffer */ + decoded + ivlen, decodedlen - ivlen, + &obj->value, &obj->value_size); - break; + virSecureErase(decoded, decodedlen); + + if (rc < 0) + return -1; } } - ret = 0; - cleanup: - virSecureErase(contentsEncrypted, obj->value_size); - virSecureErase(contents, st.st_size); - virSecureErase(iv, sizeof(iv)); - return ret; + + return 0; } @@ -1015,7 +968,7 @@ virSecretLoad(virSecretObjList *secrets, if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL))) return NULL; - if (virSecretLoadValue(obj, configDir, secretsEncryptionKey, secretsKeyLen) < 0) { + if (virSecretLoadValue(obj, secretsEncryptionKey, secretsKeyLen) < 0) { virSecretObjListRemove(secrets, obj); g_clear_pointer(&obj, virObjectUnref); return NULL; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 74a36baf6d..4e872f7b29 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -87,7 +87,6 @@ virSecretObjSaveConfig(virSecretObj *obj); int virSecretObjSaveData(virSecretObj *obj, - const char *configDir, bool encryptData, uint8_t *secretsEncryptionKey, size_t secretsKeyLen); @@ -106,7 +105,6 @@ int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, size_t value_size, - const char *configDir, bool encryptData, uint8_t *secretsEncryptionKey, size_t secretsKeyLen); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index e1668730dd..2f4ac60f5a 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -231,7 +231,6 @@ secretDefineXML(virConnectPtr conn, if (backup && backup->isephemeral) { if (virSecretObjSaveData(obj, driver->configDir, - driver->config->encryptData, driver->config->secretsEncryptionKey, driver->config->secretsKeyLen) < 0) goto restore_backup; @@ -339,7 +338,6 @@ secretSetValue(virSecretPtr secret, if (virSecretObjSetValue(obj, value, value_size, driver->configDir, - driver->config->encryptData, driver->config->secretsEncryptionKey, driver->config->secretsKeyLen) < 0) goto cleanup;
