On Thu, Nov 27, 2025 at 12:52:32 +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 cipher, we can use the key to save and load secrets.
>
> Encrypt all secrets stored on disk by default.
> When loading secrets, identify the decryption method by matching the file
> extension against known encryptionSchemeType values, iterating from the most
> recent.
> 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 on the disk that were encrypted with the former key.
>
> Signed-off-by: Arun Menon <[email protected]>
> ---
> src/conf/virsecretobj.c | 159 +++++++++++++++++++++++++++++--------
> src/conf/virsecretobj.h | 13 ++-
> src/secret/secret_driver.c | 27 +++++--
> 3 files changed, 156 insertions(+), 43 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index a3dd7983bb..8b658a6f4c 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
>
> @@ -323,12 +327,16 @@ virSecretObj *
> virSecretObjListAdd(virSecretObjList *secrets,
> virSecretDef **newdef,
> const char *configDir,
> - virSecretDef **oldDef)
> + virSecretDef **oldDef,
> + virSecretDaemonConfig *driverConfig)
> {
> virSecretObj *obj;
> virSecretDef *objdef;
> virSecretObj *ret = NULL;
> + g_autofree char *encryptionScheme = NULL;
> + g_autofree char *encryptionSchemeExt = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virSecretEncryptionSchemeType latestEncryptionScheme;
Declare these in the appropriate scope rather than top level.
>
> virObjectRWLockWrite(secrets);
>
> @@ -379,10 +387,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 scheme will be used to encrypt
> secrets.
> */
> - if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
> ".xml")) ||
> - !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr,
> ".base64")))
> +
> + latestEncryptionScheme = VIR_SECRET_ENCRYPTION_SCHEME_LAST - 1;
> + encryptionScheme =
> g_strdup(virSecretEncryptionSchemeTypeToString(latestEncryptionScheme));
So you create a copy of the second-to-last scheme string ...
> + encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL);> +
... just to concatenate it with a dot and never use it again?
encryptionScheme = g_strdup_printf(".%s",
virSecretEncryptionSchemeTypeToString(VIR_SECRET_ENCRYPTION_SCHEME_LAST- 1));
You save two variables.
> + if (driverConfig->encrypt_data) {
> + if (!(obj->secretValueFile = virFileBuildPath(configDir,
> uuidstr, encryptionSchemeExt))) {
> + goto cleanup;
> + }
> + } else {
> + if (!(obj->secretValueFile = virFileBuildPath(configDir,
> uuidstr, ".base64"))) {
> + goto cleanup;
> + }
> + }
There's no need to have the above code twice:
const char *secretSuffix = ".base64";
g_autofree char *encryptionSchemeSuffix = NULL;
if (driverConfig->encrypt_data)
encryptionSchemeSuffix = g_strdup_printf(".%s",
virSecretEncryptionSchemeTypeToString(VIR_SECRET_ENCRYPTION_SCHEME_LAST- 1));
if (!(obj->secretValueFile = virFileBuildPath(configDir, uuidstr,
secretSuffix))) {
...
> + if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
> ".xml")))
> goto cleanup;
>
> if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> @@ -407,6 +429,7 @@ struct virSecretCountData {
> int count;
> };
>
> +
> static int
> virSecretObjListNumOfSecretsCallback(void *payload,
> const char *name G_GNUC_UNUSED,
> @@ -530,6 +553,7 @@ struct _virSecretObjListExportData {
> bool error;
> };
>
> +
> static int
> virSecretObjListExportCallback(void *payload,
> const char *name G_GNUC_UNUSED,
Two instances of spurious whitespace change.
> @@ -682,15 +706,38 @@ virSecretObjSaveConfig(virSecretObj *obj)
>
>
> int
> -virSecretObjSaveData(virSecretObj *obj)
> +virSecretObjSaveData(virSecretObj *obj,
> + virSecretDaemonConfig *driverConfig)
> {
> g_autofree char *base64 = NULL;
> + g_autofree uint8_t *secret = NULL;
> + g_autofree uint8_t *encryptedValue = NULL;
> + size_t encryptedValueLen = 0;
> + size_t secretLen = 0;
> + uint8_t iv[16] = { 0 };
>
> if (!obj->value)
> return 0;
>
> - base64 = g_base64_encode(obj->value, obj->value_size);
> -
> + if (driverConfig->encrypt_data && driverConfig->secrets_encryption_key) {
> + if (virRandomBytes(iv, sizeof(iv)) < 0) {
> + return -1;
> + }
> + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC,
Here you hardcode the scheme. Make them into a const static global
variable (or a preprocessor macro) along with the suffix and use that
instead.
Because now the filename generator is "extensible" but this is
hardcoded.
> + driverConfig->secrets_encryption_key,
> driverConfig->secretsKeyLen,
> + iv, sizeof(iv),
> + (uint8_t *)obj->value, obj->value_size,
> + &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);
> + } else {
> + base64 = g_base64_encode(obj->value, obj->value_size);
> + }
> if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) <
> 0)
> return -1;
[...]
> @@ -807,11 +853,23 @@ virSecretLoadValidateUUID(virSecretDef *def,
>
>
> static int
> -virSecretLoadValue(virSecretObj *obj)
> +virSecretLoadValue(virSecretObj *obj,
> + virSecretDaemonConfig *driverConfig)
> {
> - int ret = -1, fd = -1;
> + int ret = -1;
> + VIR_AUTOCLOSE fd = -1;
> struct stat st;
> +
> g_autofree char *contents = NULL;
> + g_autofree uint8_t *contents_encrypted = NULL;
> + g_autofree uint8_t *decryptedValue = NULL;
> + g_autofree char *encryptionScheme = NULL;
> +
> + size_t decryptedValueLen = 0;
> + uint8_t iv[16] = { 0 };
> + uint8_t *ciphertext = NULL;
> + size_t ciphertextLen = 0;
> + virSecretEncryptionSchemeType latestEncryptionScheme;
>
> if ((fd = open(obj->secretValueFile, O_RDONLY)) == -1) {
> if (errno == ENOENT) {
> @@ -841,25 +899,60 @@ virSecretLoadValue(virSecretObj *obj)
> goto cleanup;
> }
>
> - contents = g_new0(char, st.st_size + 1);
> + /* Iterate over the encryption schemes starting with the latest one and
> + * decrypt the contents of the file on the disk, by matching the file
> + * extention with the encryption scheme. If there is no scheme matching
> + * the file extention, then that secret is not loaded. */
I suggest you create an array of schemes that contains also base 64 so
that the code will share the loop.
Now that the file is kept base64 encoded you just skip the decryption
code (e.g. assign the base64 decoded value directly into output. rahtehr
than having two very separate branches.
>
> - if (saferead(fd, contents, st.st_size) != st.st_size) {
> - virReportSystemError(errno, _("cannot read '%1$s'"),
> - obj->secretValueFile);
> - goto cleanup;
> + if (virStringHasSuffix(obj->secretValueFile, ".base64")) {
> + 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->secretValueFile);
> + goto cleanup;
> + }
> + contents[st.st_size] = '\0';
> + obj->value = g_base64_decode(contents, &obj->value_size);
> + } else {
> + for (latestEncryptionScheme = VIR_SECRET_ENCRYPTION_SCHEME_LAST-1;
> latestEncryptionScheme > 0; latestEncryptionScheme--) {
> + encryptionScheme =
> g_strdup(virSecretEncryptionSchemeTypeToString(latestEncryptionScheme));
> + if (virStringHasSuffix(obj->secretValueFile, encryptionScheme)) {
> + 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->secretValueFile);
> + goto cleanup;
> + }
> + if ((st.st_size) < sizeof(iv)) {
> + virReportError(VIR_ERR_INVALID_SECRET, "%s",
> + _("Encrypted secret size is invalid"));
> + goto cleanup;
Since this is base64 encoded now; this check is invalid. This ought to
hapen after base64 decode.
> + }
> + contents[st.st_size] = '\0';
> + contents_encrypted = g_base64_decode(contents,
> &obj->value_size);
> +
> + memcpy(iv, contents_encrypted, sizeof(iv));
> + ciphertext = contents_encrypted + sizeof(iv);
> + ciphertextLen = st.st_size - sizeof(iv);
> + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC,
Umm so you have this declarative loop that takes schemes from the enum
of supported ones but then you hardcode to decrypt everything using
VIR_CRYPTO_CIPHER_AES256CBC?
Since it's unlikely that we'll add another one any time soon, you can
make this a problem of the future you (or future someone else) by just
removing the attempts to make it declarative and hardcode
aes256cbc->base64 instead of this half-done approach.
> +
> 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);
> - VIR_FORCE_CLOSE(fd);
> + virSecureErase(iv, sizeof(iv));
> return ret;
> }
>
[...]
> @@ -70,6 +75,9 @@ struct _virSecretDriverState {
>
> /* Immutable pointer, self-locking APIs */
> virInhibitor *inhibitor;
> +
> + /* Settings from secrets.conf file */
> + virSecretDaemonConfig *config;
see below.
> };
>
> @@ -307,7 +316,6 @@ secretGetXMLDesc(virSecretPtr secret,
> return ret;
> }
>
> -
Spurious whithespace change.
> static int
> secretSetValue(virSecretPtr secret,
> const unsigned char *value,
> @@ -327,8 +335,7 @@ secretSetValue(virSecretPtr secret,
> def = virSecretObjGetDef(obj);
> if (virSecretSetValueEnsureACL(secret->conn, def) < 0)
> goto cleanup;
> -
> - if (virSecretObjSetValue(obj, value, value_size) < 0)
> + if (virSecretObjSetValue(obj, value, value_size, driver->config) < 0)
> goto cleanup;
>
> event = virSecretEventValueChangedNew(def->uuid,
> @@ -454,6 +461,7 @@ secretStateCleanupLocked(void)
> VIR_FREE(driver->configDir);
>
> virObjectUnref(driver->secretEventState);
> + virObjectUnref(driver->config);
This belongs to the patch adding the config infra.
> virInhibitorFree(driver->inhibitor);
>
> if (driver->lockFD != -1)
> @@ -518,6 +526,8 @@ secretStateInitialize(bool privileged,
> driver->stateDir);
> goto error;
> }
> + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged)))
> + goto error;
This too.
>
> driver->inhibitor = virInhibitorNew(
> VIR_INHIBITOR_WHAT_NONE,
> @@ -534,7 +544,7 @@ secretStateInitialize(bool privileged,
> if (!(driver->secrets = virSecretObjListNew()))
> goto error;
>
> - if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0)
> + if (virSecretLoadAllConfigs(driver->secrets, driver->configDir,
> driver->config) < 0)
> goto error;
>
> return VIR_DRV_STATE_INIT_COMPLETE;
> @@ -553,7 +563,10 @@ secretStateReload(void)
> if (!driver)
> return -1;
>
> - ignore_value(virSecretLoadAllConfigs(driver->secrets,
> driver->configDir));
> + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged)))
This too. I'm fairly sure I've noted this in v2.
> + return -1;
> +
> + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir,
> driver->config));
>
> return 0;
> }
> --
> 2.51.1
>
