On Thu, Nov 13, 2025 at 07:02:22PM +0530, Arun Menon wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
Add logic to parse the encryption key file and store the key. When systemd will start the secrets driver, it will read the secret.conf file and check if encrypt_data flag is set to 1. In that case, the secrets will be stored in encrypted format on the disk. The encryption and decryption logic will be added in the subsequent patches.
Signed-off-by: Arun Menon <armenon@redhat.com> --- libvirt.spec.in | 1 + src/secret/meson.build | 7 +++ src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++ src/secret/secrets.conf.in | 14 ++++++
New config files need to be accompanied with augeas files for processing them. See the simple examples in src/network/libvirtd_network.aug src/network/test_libvirtd_network.aug.in and the src/network/meson.build file for how to wire them up. They also need to be in the RPM spec.
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 04c3ca49f1..0b415e5ef3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -42,6 +42,7 @@ #include "secret_event.h" #include "virutil.h" #include "virinhibitor.h" +#include "virfile.h"
#define VIR_FROM_THIS VIR_FROM_SECRET
@@ -70,6 +71,17 @@ struct _virSecretDriverState {
/* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + /* master encryption key value from secret.conf file */ + char *masterKeyPath;
Per the previous patch, lets call this secretsEncryptionKeyPath
+ + /* Indicates if the secrets are encrypted or not. 0 if not encrypted + * and 1 if encrypted. + */
Lets specify "newly written secrets" to clarify that setting this to '1' wouldn't make it encrypt any pre-existing plain text secrets
+ int encrypt_data; + + unsigned char* masterKey; + size_t masterKeyLen; };
None of these should be in this struct. Our normal practice is to define a virSecretDriverConfig struct to hold all config parameters, and an pointer to that config in the state struct. See virNetworkDriverState/Config for a simple example.
static virSecretDriverState *driver; @@ -307,6 +319,44 @@ secretGetXMLDesc(virSecretPtr secret, return ret; }
+static bool secretGetMasterKey(uint8_t **masterKey, size_t *masterKeyLen) +{ + int fd = -1; + struct stat st; + + if ((fd = open(driver->masterKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open master key file '%1$s'"), + driver->masterKeyPath); + return false; + } + if (fstat(fd, &st) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat master key file '%1$s'"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *masterKeyLen = st.st_size; + if (*masterKeyLen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s is empty"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *masterKey = g_new0(uint8_t, *masterKeyLen); + if (saferead(fd, &masterKey, *masterKeyLen) != *masterKeyLen) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read master key file '%1$s'"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + VIR_FORCE_CLOSE(fd); + if (*masterKeyLen < 32) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s must be atleast 32 bytes"), + driver->masterKeyPath); + return false; + } + return true; +}
static int secretSetValue(virSecretPtr secret, @@ -482,6 +532,10 @@ secretStateInitialize(bool privileged, void *opaque) { VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + g_autofree char *secretsconf = NULL; + g_autofree char *credentials_directory = NULL; + g_autofree char *master_encryption_key_path = NULL; + g_autoptr(virConf) conf = NULL;
driver = g_new0(virSecretDriverState, 1);
@@ -537,6 +591,48 @@ secretStateInitialize(bool privileged, if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) goto error;
+ secretsconf = g_strdup_printf("%s/libvirt/secrets.conf", SYSCONFDIR); + credentials_directory = getenv("CREDENTIALS_DIRECTORY"); + + if (credentials_directory) { + VIR_DEBUG("Using credentials directory from environment: %s", + credentials_directory); + master_encryption_key_path = g_strdup_printf("%s/master-encryption-key", credentials_directory); + if (access(master_encryption_key_path, R_OK) == 0) { + driver->masterKeyPath = g_strdup(master_encryption_key_path); + } + } else if (access(secretsconf, R_OK) == 0) { + if (!(conf = virConfReadFile(secretsconf, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to read secrets.conf from %1$s"), + secretsconf); + goto error; + } + + if (virConfGetValueString(conf, "master_encryption_key", &driver->masterKeyPath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get master_encryption_key from %1$s"), + secretsconf); + goto error; + } + } else { + VIR_DEBUG("No secrets configuration found %s, skipping", driver->configDir); + driver->masterKeyPath = NULL; + driver->masterKeyLen = 0; + } + if (driver->masterKeyPath) { + if (!secretGetMasterKey(&driver->masterKey, &driver->masterKeyLen)) { + goto error; + } + VIR_DEBUG("Master encryption key loaded from %s", driver->masterKeyPath); + VIR_DEBUG("Master encryption key length: %zu bytes", driver->masterKeyLen); + } + if (virConfGetValueInt(conf, "encrypt_data", &driver->encrypt_data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get encrypt_data from %1$s"), + secretsconf); + goto error; + } return VIR_DRV_STATE_INIT_COMPLETE;
Config file handling code should not be put directly in the state initialization method. All loading of the config should be isolated in separate methods, and live in secret_driver_conf.{h,c} files. Again take a look at bridge_driver_conf.{h,c} for a simple example of prior art. In terms of handling the 'encrypt_data" parameter we need this psuedo-logic: if encrypt_data is not set if key path exists encrypt_data = 1 else encrypt_data = 1 else if encrypt_data == 1 if key path does not exist ..report error...
error: diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
This setting should be commented out - the default value should be set by the code, and should be 0 or 1, depending on whether or not any encryption key exists on disk. # This setting determines whether newly written secret values # are encrypted or not. Setting this to 1 will not trigger # encryption of existing plain text secrets on disk. # # Will default to 1 if the master_encryption_key path exists # on disk, otherwise will default to 0. Uncomment this to # force use of encryption and report an error if the key path # is missing. # encrypt_data = 1 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 :|