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 <[email protected]>
> ---
> 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 :|
