On Thu, Nov 27, 2025 at 12:52:30 +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/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
src/conf/ is meant for common XML infra. This is the config of the
secret driver so it ought to be in src/secret/secret_conf.c
> @@ -0,0 +1,177 @@
> +/*
> + * secret_config.c: secrets.conf config file handling
> + *
> + * Copyright (C) 2025 Red Hat, Inc.
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
The SPDX line is enough, drop the rest.
> +
> +#include <config.h>
> +#include <fcntl.h>
> +#include "configmake.h"
> +#include "datatypes.h"
> +#include "virlog.h"
> +#include "virerror.h"
> +#include "virfile.h"
> +#include "virutil.h"
> +#include "secret_config.h"
> +
> +
> +#define VIR_FROM_THIS VIR_FROM_CONF
VIR_FROM_SECRET
> +
> +VIR_LOG_INIT("secret.secret_config");
> +
> +static virClass *virSecretDaemonConfigClass;
> +static void virSecretDaemonConfigDispose(void *obj);
> +
> +static int
> +virSecretConfigOnceInit(void)
> +{
> + if (!VIR_CLASS_NEW(virSecretDaemonConfig, virClassForObject()))
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +VIR_ONCE_GLOBAL_INIT(virSecretConfig);
> +
> +
> +int
> +virSecretDaemonConfigFilePath(bool privileged, char **configfile)
> +{
> + if (privileged) {
> + *configfile = g_strdup(SYSCONFDIR "/libvirt/secrets.conf");
Configs for drivers are named based on the uri. So this ought to be
'secret.conf'
> + } else {
> + g_autofree char *configdir = NULL;
> +
> + configdir = virGetUserConfigDirectory();
> +
> + *configfile = g_strdup_printf("%s/secrets.conf", configdir);
ditto
> + }
> +
> + return 0;
> +}
> +
> +
> +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;
This is a boolean now.
> +
> + if (virFileExists(filename)) {
> + conf = virConfReadFile(filename, 0);
> + if (!conf)
> + return -1;
> + if (virConfGetValueBool(conf, "encrypt_data", &cfg->encrypt_data) <
> 0) {
> + virReportError(VIR_ERR_CONF_SYNTAX,
> + _("Failed to get encrypt_data from %1$s"),
> + filename);
> + return -1;
> + }
> +
> + if (virConfGetValueString(conf, "secrets_encryption_key",
> + &cfg->secretsEncryptionKeyPath) < 0) {
> + virReportError(VIR_ERR_CONF_SYNTAX,
> + _("Failed to get secrets_encryption_key from
> %1$s"),
> + filename);
virConfGetValue* functions already set an error; don't overwrite it.
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> +
> +static int virGetSecretsEncryptionKey(virSecretDaemonConfig *cfg,
> + uint8_t **secrets_encryption_key, size_t
> *secrets_encryption_keylen)
In v2 I've noted that this doesn't follow the coding style (just look at
other functions. You changed the name but didn't fix the coding style
So to be explicit:
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);
None of the errors in this function are VIR_ERR_INTERNAL_ERROR.
> + 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;
> + }
> + if (encryption_key_length != VIR_SECRETS_ENCRYPTION_KEY_LEN) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key
> file %1$s must be 32 bytes"),
Use the VIR_SECRETS_ENCRYPTION_KEY_LEN constant instead of hardcoding 32
in the error message
> + 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);
> + }
> + VIR_DEBUG("Secrets encryption key path: %s",
> NULLSTR(cfg->secretsEncryptionKeyPath));
> +
> + if (cfg->secretsEncryptionKeyPath &&
> virFileExists(cfg->secretsEncryptionKeyPath)) {
> + if (virGetSecretsEncryptionKey(cfg, &cfg->secrets_encryption_key,
> &cfg->secretsKeyLen) < 0) {
> + return NULL;
> + }
> + }
> + if (cfg->encrypt_data == 1) {
Firstly; it's a bool now. Secondly you don't know if this was an
explicit config from the user ....
> + if (!cfg->secretsEncryptionKeyPath) {
... where refusing startup would be appropriate, or an old config
(without the config file) where you assumed that this is enabled but no
key is present.
Thus this error ought to be printed only when the user explicitly
requested encryption not when we assumed it.
Since it's in a different fuinction you'll either need to remember if
you've seen the user config, or convert the value to tristate.
> + 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);
This ought to be securely disposed.
> + g_free(cfg->secretsEncryptionKeyPath);
> +}
> 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
Just the SPDX line.
> + */
> +
> +#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.
It's now a bool so
> + */
> + 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/libvirt_secrets.aug b/src/secret/libvirt_secrets.aug
> new file mode 100644
> index 0000000000..092cdef41f
> --- /dev/null
> +++ b/src/secret/libvirt_secrets.aug
> @@ -0,0 +1,40 @@
> +(* /etc/libvirt/secrets.conf *)
> +
> +module Libvirt_secrets =
> + autoload xfm
> +
> + let eol = del /[ \t]*\n/ "\n"
> + let value_sep = del /[ \t]*=[ \t]*/ " = "
> + let indent = del /[ \t]*/ ""
> +
> + let array_sep = del /,[ \t\n]*/ ", "
> + let array_start = del /\[[ \t\n]*/ "[ "
> + let array_end = del /\]/ "]"
> +
> + let str_val = del /\"/ "\"" . store /[^\"]*/ . del /\"/ "\""
> + let bool_val = store /0|1/
> + let int_val = store /[0-9]+/
> + let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
> + let str_array_val = counter "el" . array_start . ( str_array_element . (
> array_sep . str_array_element ) * ) ? . array_end
> +
> + let str_entry (kw:string) = [ key kw . value_sep . str_val ]
> + let bool_entry (kw:string) = [ key kw . value_sep . bool_val ]
> + let int_entry (kw:string) = [ key kw . value_sep . int_val ]
> + let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ]
> +
> + let secrets_entry = str_entry "secrets_encryption_key"
> + | bool_entry "encrypt_data"
> +
> + (* Each entry in the config is one of the following three ... *)
> + let entry = secrets_entry
> + let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^
> \t\n][^\n]*)?/ . del /\n/ "\n" ]
> + let empty = [ label "#empty" . eol ]
> +
> + let record = indent . entry . eol
> +
> + let lns = ( record | comment | empty ) *
> +
> + let filter = incl "/etc/libvirt/secrets.conf"
> + . Util.stdexcl
> +
> + let xfm = transform lns filter
> diff --git a/src/secret/meson.build b/src/secret/meson.build
> index c02d1064a9..cff0f0678d 100644
> --- a/src/secret/meson.build
> +++ b/src/secret/meson.build
> @@ -27,6 +27,24 @@ if conf.has('WITH_SECRETS')
> ],
> }
>
> + secrets_conf = configure_file(
> + input: 'secrets.conf.in',
> + output: 'secrets.conf',
> + copy: true
> + )
> + virt_conf_files += secrets_conf
> +
> + virt_aug_files += files('libvirt_secrets.aug')
> +
> + virt_test_aug_files += {
> + 'name': 'test_libvirt_secrets.aug',
> + 'aug': files('test_libvirt_secrets.aug.in'),
> + 'conf': files('secrets.conf.in'),
> + 'test_name': 'libvirt_secrets',
> + 'test_srcdir': meson.current_source_dir(),
> + 'test_builddir': meson.current_build_dir(),
> + }
> +
> virt_daemon_confs += {
> 'name': 'virtsecretd',
> }
> 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
'secret.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.
> +#encrypt_data = 1
