Hi Daniel,
Thank you for the review.
On Thu, Nov 27, 2025 at 03:13:18PM +0000, Daniel P. Berrangé wrote:
> 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
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
true if we find the encryption key file in systemd credentials or if the path is
configured in the secret.conf file.
>
> > +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.
Yes, thanks. I will use virFileReadAll.
>
> > + 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.
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.
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()
> ...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.
Yes, will do.
>
> > +}
> > 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 :|
>
Regards,
Arun Menon
