On Sat, Nov 29, 2025 at 12:24:37PM +0530, Arun Menon wrote:
Hi Daniel, Thank you for the review.
On Thu, Nov 27, 2025 at 12:52:30PM +0530, Arun Menon via Devel wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the secrets 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. After parsing the file, the virtsecretd driver checks if an encryption key is present in the path and is valid.
By default, if no encryption key is present in the path, then the service will by default use the encryption key stored in the CREDENTIALS_DIRECTORY.
Add logic to parse the encryption key file and store the key. It also checks for the encrypt_data attribute in the config file. The encryption and decryption logic will be added in the subsequent patches.
Signed-off-by: Arun Menon <armenon@redhat.com> --- libvirt.spec.in | 3 + po/POTFILES | 1 + src/conf/meson.build | 1 + src/conf/secret_config.c | 177 +++++++++++++++++++++++++ src/conf/secret_config.h | 38 ++++++ src/libvirt_private.syms | 2 + src/secret/libvirt_secrets.aug | 40 ++++++ src/secret/meson.build | 18 +++ src/secret/secrets.conf.in | 12 ++ src/secret/test_libvirt_secrets.aug.in | 6 + 10 files changed, 298 insertions(+) create mode 100644 src/conf/secret_config.c create mode 100644 src/conf/secret_config.h create mode 100644 src/secret/libvirt_secrets.aug create mode 100644 src/secret/secrets.conf.in create mode 100644 src/secret/test_libvirt_secrets.aug.in
diff --git a/libvirt.spec.in b/libvirt.spec.in index dba8a71311..01ecf7e7ca 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -2240,6 +2240,9 @@ exit 0 %config(noreplace) %{_sysconfdir}/libvirt/virtsecretd.conf %{_datadir}/augeas/lenses/virtsecretd.aug %{_datadir}/augeas/lenses/tests/test_virtsecretd.aug +%{_datadir}/augeas/lenses/libvirt_secrets.aug +%{_datadir}/augeas/lenses/tests/test_libvirt_secrets.aug +%config(noreplace) %{_sysconfdir}/libvirt/secrets.conf %{_unitdir}/virtsecretd.service %{_unitdir}/virt-secret-init-encryption.service %{_unitdir}/virtsecretd.socket diff --git a/po/POTFILES b/po/POTFILES index f0aad35c8c..a64e4b2d87 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -53,6 +53,7 @@ src/conf/nwfilter_conf.c src/conf/nwfilter_params.c src/conf/object_event.c src/conf/secret_conf.c +src/conf/secret_config.c src/conf/snapshot_conf.c src/conf/storage_adapter_conf.c src/conf/storage_conf.c diff --git a/src/conf/meson.build b/src/conf/meson.build index 5116c23fe3..9c51e99107 100644 --- a/src/conf/meson.build +++ b/src/conf/meson.build @@ -68,6 +68,7 @@ interface_conf_sources = [
secret_conf_sources = [ 'secret_conf.c', + 'secret_config.c', 'virsecretobj.c', ]
diff --git a/src/conf/secret_config.c b/src/conf/secret_config.c new file mode 100644 index 0000000000..5bc0b24380 --- /dev/null +++ b/src/conf/secret_config.c
+static int +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg, + const char *filename) +{ + g_autoptr(virConf) conf = NULL; + /* Encrypt secrets by default unless the configuration sets it otherwise */ + cfg->encrypt_data = 1;
We can't do that, as it'll break back compat for unprivileged daemons and non-systemd hosts. It must only be set to 1 if an explicit path was configured in the config (regardless of whether it exists on disk), or the implict systemd credential exists on disk I see. That means encrypt_data is totally dependent on the existence of the key path. Even if the user configures it to 0 in the secret.conf file, then we overwrite it to
On Thu, Nov 27, 2025 at 03:13:18PM +0000, Daniel P. Berrangé wrote: true if we find the encryption key file in systemd credentials or if the path is configured in the secret.conf file.
No, we have three distinct values for 'encrypt_data' which control the behaviour * Not set - User specified path must be used if set - Else, systemd credential must be used if present on disk - Else, do not attempt encryption * Set to 1 - User specified path must be used if set - Else, systemd credential must be used if present on disk - Else, built-in default path must be used * Set to 0 - must honour that - Do not attempt encryption
+virSecretDaemonConfig * +virSecretDaemonConfigNew(bool privileged) +{ + g_autoptr(virSecretDaemonConfig) cfg = NULL; + g_autofree char *configdir = NULL; + g_autofree char *configfile = NULL; + const char *credentials_directory; + + if (virSecretConfigInitialize() < 0) + return NULL; + + if (!(cfg = virObjectNew(virSecretDaemonConfigClass))) + return NULL; + + cfg->secretsEncryptionKeyPath = NULL; + + if (virSecretDaemonConfigFilePath(privileged, &configfile) < 0) + return NULL; + + if (virSecretLoadDaemonConfig(cfg, configfile) < 0) + return NULL; + + credentials_directory = getenv("CREDENTIALS_DIRECTORY"); + + if (!cfg->secretsEncryptionKeyPath && credentials_directory) { + cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", + credentials_directory);
This must have
if (!virFileExists(cfg->secretsEncryptionKeyPath)) g_clear_pointer(cfg->secretsEncryptionKeyPath, g_free);
so that we don't assume the systemd credential exists, while still always honouring an explicitly cnofigured path. Correct me if I am wrong, are we skipping virFileExists check for the user configured secretsEncryptionKeyPath because open() will anyway fail and return an error in case the file is not found? I am sorry, I did not understand why we do not want to check if the file exists, for both the cases, systemd credential and the user configured keypath.
These are different scenarios, see my outline of expected behaviour earlier.
We can have the following outside the block of systemd credentials if (!virFileExists(cfg->secretsEncryptionKeyPath)) g_clear_pointer(cfg->secretsEncryptionKeyPath, g_free);
followed by setting cfg->encrypt_data = cfg->secretsEncryptionKeyPath != NULL
+ }
There here have
cfg->encrypt_data = cfg->secretsEncryptionKeyPath != NULL;
After we set the encrypt_data attribute here, by checking cfg->secretsEncryptionKeyPath ...
+ VIR_DEBUG("Secrets encryption key path: %s", NULLSTR(cfg->secretsEncryptionKeyPath)); + + if (cfg->secretsEncryptionKeyPath && virFileExists(cfg->secretsEncryptionKeyPath)) {
This virFileExists check is undesirable here. We do a virFileExists check for a systemd credential earlier, but for a explicitly configured file we must always honour it. What we should do is
if (cfg->encrypt_data) { if (!cfg->secretsEncryptionKeyPath) {
IMO, this condition will not be required. Because now we are checking in reverse. encrypt_data is already derived from whether secretsEncryptionKeyPath is set or not. If encrypt_data is set, then that implies secretsEncryptionKeyPath is set. So I think we can directly call virGetSecretsEncryptionKey()
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 :|