Hi Peter,
Thank you for the review.
On Thu, Nov 27, 2025 at 04:09:07PM +0100, Peter Krempa wrote:
> 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
Sure, I will move it.
>
>
> > @@ -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.
Yes.
>
> > +
> > +#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
Yes, I will change it
>
> > +
> > +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'
I will change it.
>
>
> > + } else {
> > + g_autofree char *configdir = NULL;
> > +
> > + configdir = virGetUserConfigDirectory();
> > +
> > + *configfile = g_strdup_printf("%s/secrets.conf", configdir);
>
> ditto
Yes.
>
>
> > + }
> > +
> > + 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.
Yes, I will not set it in the new version.
>
> > +
> > + 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)
>
>
Thank you, I was not sure about this one.
> > +{
> > + 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.
I think I need to create a new one called VIR_ERR_INVALID_ENCR_KEY_LENGTH
in include/libvirt/virterror.h if that is okay. I shall use it for the
key length check here.
>
>
> > + 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.
Got it. I realize now we shouldn't force encryption.
I'll update the code to enable encrypt_data only if the systemd credentials
exist or the path is in secret.conf.
I wasn't sure before because I thought the user was supposed to use that flag
to manually toggle encryption.
>
>
> > + 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.
Yes, will do. Thanks
>
> > + 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.
Sure.
>
>
> > + */
> > +
> > +#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
I think we should drop the boolean from the secret.conf config file,
because the user will not be able to toggle encryption on/off based on
this field. Encryption is on if the path is set or if systemd credentials
is set. This attribute can be used internally.
>
> > + */
> > + 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
>
Regards,
Arun Menon
