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
+static int virGetSecretsEncryptionKey(virSecretDaemonConfig *cfg, + uint8_t **secrets_encryption_key, size_t *secrets_encryption_keylen) +{ + VIR_AUTOCLOSE fd = -1; + ssize_t encryption_key_length; + + if (!virFileExists(cfg->secretsEncryptionKeyPath)) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets key file '%1$s' does not exist"), + cfg->secretsEncryptionKeyPath); + return -1; + } + + if ((fd = open(cfg->secretsEncryptionKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + return -1; + } + + *secrets_encryption_key = g_new0(uint8_t, VIR_SECRETS_ENCRYPTION_KEY_LEN); + + if ((encryption_key_length = saferead(fd, *secrets_encryption_key, VIR_SECRETS_ENCRYPTION_KEY_LEN)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + return -1; + }
I'm not sure we need a virFileExists check separate from open. And indeed open+saferead could be replaced with virFileReadAll, following by a data length check.
+ if (encryption_key_length != VIR_SECRETS_ENCRYPTION_KEY_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s must be 32 bytes"), + cfg->secretsEncryptionKeyPath); + return -1; + } + + *secrets_encryption_keylen = (size_t)encryption_key_length; + return 0; +} + + +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.
+ }
There here have cfg->encrypt_data = cfg->secretsEncryptionKeyPath != NULL;
+ 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) { ...error... } if (virGetSecretsEncryptionKey(...)) { .... } }
+ if (virGetSecretsEncryptionKey(cfg, &cfg->secrets_encryption_key, &cfg->secretsKeyLen) < 0) { + return NULL; + } + } + if (cfg->encrypt_data == 1) { + if (!cfg->secretsEncryptionKeyPath) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("secretsEncryptionKeyPath must be set if encrypt_data is 1 in %1$s"), + configfile); + return NULL; + } + } + return g_steal_pointer(&cfg); +} + + +static void +virSecretDaemonConfigDispose(void *obj) +{ + virSecretDaemonConfig *cfg = obj; + + g_free(cfg->secrets_encryption_key); + g_free(cfg->secretsEncryptionKeyPath);
Pick one naming style only for the struct fields, snakecase or not.
+} diff --git a/src/conf/secret_config.h b/src/conf/secret_config.h new file mode 100644 index 0000000000..4cc6589814 --- /dev/null +++ b/src/conf/secret_config.h @@ -0,0 +1,38 @@ +/* + * secret_config.h: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#pragma once + +#include "internal.h" +#include "virinhibitor.h" +#include "secret_event.h" +#define VIR_SECRETS_ENCRYPTION_KEY_LEN 32 + +typedef struct _virSecretDaemonConfig virSecretDaemonConfig; +struct _virSecretDaemonConfig { + virObject parent; + /* secrets encryption key path from secrets.conf file */ + char *secretsEncryptionKeyPath; + + /* Store the key to encrypt secrets on the disk */ + unsigned char *secrets_encryption_key; + + size_t secretsKeyLen; + + /* Indicates if the newly written secrets are encrypted or not. + * 0 if not encrypted and 1 if encrypted. + */ + bool encrypt_data; +}; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecretDaemonConfig, virObjectUnref); + +int virSecretDaemonConfigFilePath(bool privileged, char **configfile); +virSecretDaemonConfig *virSecretDaemonConfigNew(bool privileged); +int virSecretDaemonConfigLoadFile(virSecretDaemonConfig *data, + const char *filename, + bool allow_missing); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 63a1ae4c70..cdf5426af6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1066,6 +1066,8 @@ virSecretDefParse; virSecretUsageTypeFromString; virSecretUsageTypeToString;
+# conf/secret_config.h +virSecretDaemonConfigNew;
# conf/secret_event.h virSecretEventLifecycleNew;
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..d998940140 --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,12 @@ +# +# Configuration file for the secrets driver. +# +# The secret encryption key is used to override default encryption +# key path. The user can create an encryption key and set the secret_encryption_key +# to the path on which it resides. +# The key must be 32-bytes long. +#secrets_encryption_key = "/run/libvirt/secrets/secret-encryption-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.
"By default it is on if secrets_encryption_key is set to a non-NULL path, or if a systemd credential named "secrets-encryption-key" exists."
+#encrypt_data = 1 diff --git a/src/secret/test_libvirt_secrets.aug.in b/src/secret/test_libvirt_secrets.aug.in new file mode 100644 index 0000000000..1bb205e0f2 --- /dev/null +++ b/src/secret/test_libvirt_secrets.aug.in @@ -0,0 +1,6 @@ +module Test_libvirt_secrets = + @CONFIG@ + + test Libvirt_secrets.lns get conf = +{ "secrets_encryption_key" = "/run/libvirt/secrets/secret-encryption-key" } +{ "encrypt_data" = "1" } -- 2.51.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 :|