Hi Peter,
Thank you for the review.
On Fri, Nov 21, 2025 at 12:13:09PM +0100, Peter Krempa wrote:
> On Thu, Nov 20, 2025 at 22:23:44 +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 | 207 +++++++++++++++++++++++++
> > src/conf/secret_config.h | 48 ++++++
> > 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, 338 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..a1c9b6bc2f
> > --- /dev/null
> > +++ b/src/conf/secret_config.c
> > @@ -0,0 +1,207 @@
> > +/*
> > + * secret_config.c: secrets.conf config file handling
> > + *
> > + * Copyright (C) 2025 Red Hat, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + */
>
> For new files we usually use the SPDX header:
>
> /*
> * SPDX-License-Identifier: LGPL-2.1-or-later
> */
>
> instead of the full blob above.
Yes, will do. Thanks
>
> > +
> > +#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_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");
> > + } else {
> > + g_autofree char *configdir = NULL;
> > +
> > + configdir = virGetUserConfigDirectory();
> > +
> > + *configfile = g_strdup_printf("%s/secrets.conf", configdir);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg,
> > + const char *filename)
> > +{
> > + g_autoptr(virConf) conf = NULL;
> > +
> > + if (access(filename, R_OK) == 0) {
>
> Use virFileExists instead of open-coding.
Yes, will do.
>
> > + conf = virConfReadFile(filename, 0);
> > + if (!conf)
> > + return -1;
> > + if (virConfGetValueInt(conf, "encrypt_data", &cfg->encrypt_data) <
> > 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to get encrypt_data from %1$s"),
> > + filename);
>
> Not an internal error. VIR_ERR_CONF_SYNTAX
yes, will change it.
>
> Also encrypt_data really ought to be a bool based on usage. Here you'll
> have to convert it properly on parsing.
I will change it to boolean.
>
> > + return -1;
> > + }
> > +
> > + if (virConfGetValueString(conf, "secrets_encryption_key",
> > + &cfg->secretsEncryptionKeyPath) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to get secrets_encryption_key from
> > %1$s"),
> > + filename);
>
> ditto
>
> > + return -1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static bool getSecretsEncryptionKey(virSecretDaemonConfig *cfg,
> > + uint8_t **secrets_encryption_key,
> > size_t *secrets_encryption_keylen)
>
> This function doesn't follow the naming convention and coding style.
I think virSecretGetEncryptionKey should be okay?
>
> > +{
> > + int fd = -1;
>
> Declare this using VIR_AUTOCLOSE so that you don't have to deal with it
> in this whole file.
Sure, thanks.
>
> > + struct stat st;
> > +
> > + if ((fd = open(cfg->secretsEncryptionKeyPath, O_RDONLY)) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open secrets key
> > file '%1$s'"),
> > + cfg->secretsEncryptionKeyPath);
> > + return false;
> > + }
>
> I'd suggest you factor out the useful bits from
> 'qemuDomainMasterKeyReadFile' and then reuse it rather than reimplement
> this
Yes, will do.
>
>
> > + if (fstat(fd, &st) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat secrets key
> > file '%1$s'"),
> > + cfg->secretsEncryptionKeyPath);
> > + VIR_FORCE_CLOSE(fd);
>
> With the autoclose helper this won't be needed.
Yes.
>
>
> > + return false;
> > + }
> > + *secrets_encryption_keylen = st.st_size;
> > + if (*secrets_encryption_keylen == 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key
> > file %1$s is empty"),
> > + cfg->secretsEncryptionKeyPath);
> > + VIR_FORCE_CLOSE(fd);
> > + return false;
> > + }
> > + *secrets_encryption_key = g_new0(uint8_t, *secrets_encryption_keylen);
>
> This allocates buffer based on the actual size, IMO yoiu should limit it
> to something more sensible.
I will #define the key length and limit the secrets_encryption_key to that size.
>
> > + if (saferead(fd, &secrets_encryption_key, *secrets_encryption_keylen)
> > != *secrets_encryption_keylen) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read secrets key
> > file '%1$s'"),
> > + cfg->secretsEncryptionKeyPath);
> > + VIR_FORCE_CLOSE(fd);
> > + return false;
>
> For functions using libvirt errors please stick to the '0' '-1' return
> value convntion.
Yes. Will change it.
>
> > + }
> > + VIR_FORCE_CLOSE(fd);
> > + if (*secrets_encryption_keylen != 32) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key
> > file %1$s must be 32 bytes"),
> > + cfg->secretsEncryptionKeyPath);
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +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)
> > + goto error;
> > +
> > + if (!(cfg = virObjectNew(virSecretDaemonConfigClass)))
> > + goto error;
> > +
> > + cfg->secretsEncryptionKeyPath = NULL;
> > +
> > + if (privileged) {
> > + configdir = g_strdup(SYSCONFDIR "/libvirt");
> > + } else {
> > + g_autofree char *rundir = virGetUserRuntimeDirectory();
> > + configdir = virGetUserConfigDirectory();
> > + }
> > + configfile = g_strconcat(configdir, "/secrets.conf", NULL);
>
> This open-codes what the unused 'virSecretDaemonConfigFilePath' function
> does. Either use the function or delete it.
Will use the function. Also I noticed rundir is not really used here.
>
> > + if (virSecretLoadDaemonConfig(cfg, configfile) < 0)
> > + goto error;
> > +
> > + if (!(credentials_directory = getenv("CREDENTIALS_DIRECTORY"))) {
> > + credentials_directory = NULL;
>
> This is a pointless condition. If getenv("CREDENTIALS_DIRECTORY")
> returns NULL you set it to NULL.
Yes.
>
>
> > + }
> > +
> > + if (cfg->secretsEncryptionKeyPath) {
> > + VIR_DEBUG("Secrets encryption key path: %s",
> > cfg->secretsEncryptionKeyPath);
> > + } else if (credentials_directory) {
> > + VIR_DEBUG("Using credentials directory from environment: %s",
> > + credentials_directory);
> > + cfg->secretsEncryptionKeyPath =
> > g_strdup_printf("%s/secrets-encryption-key",
> > +
> > credentials_directory);
> > + } else {
> > + VIR_DEBUG("No secrets encryption key found in credentials
> > directory");
> > + cfg->secretsEncryptionKeyPath = NULL;
> > + }
>
> This logic is a bit convoluted. I suggest you do an unconditional log of
> the actual path after you determine whic one to use:
>
> if (!cfg->secretsEncryptionKeyPath && credentials_directory) {
> VIR_DEBUG("Using credentials directory from environment: %s",
> credentials_directory);
>
> cfg->secretsEncryptionKeyPath =
> g_strdup_printf("%s/secrets-encryption-key",
>
> credentials_directory);
> }
>
> VIR_DEBUG("Secrets encryption key path: %s",
> NULLSTR(cfg->secretsEncryptionKeyPath));
>
>
Sure
> > + if (cfg->secretsEncryptionKeyPath &&
> > access(cfg->secretsEncryptionKeyPath, R_OK) == 0) {
> > + if (!getSecretsEncryptionKey(cfg, &cfg->secrets_encryption_key,
> > &cfg->secretsKeyLen)) {
> > + VIR_DEBUG("Failed to get secrets encryption key from path: %s",
> > + cfg->secretsEncryptionKeyPath);
> > + goto error;
>
> You must properly report the error if you are to bail from daemon startup.
> Here !getSecretsEncryptionKey sets the value but it doesn't seem so
> because you do a pointless DEBUG (the error was already reported)
>
I will remove the debug statement.
> > + }
> > + }
> > +
> > + if (cfg->encrypt_data != 1) {
> > + cfg->encrypt_data = (cfg->secretsKeyLen == 32) ? 1 : 0;
>
> So you set this to 1 even when the user set it to 0? Also 'encrypt_data'
> really ought to be a bool in the config struct. If users set this to 0
> explicitly you shouldn't try to re-infer it.
>
> Here you also duplicate a magic constant used to limit the size so if
> anyone ever changes it in the other place only this will stop to work.
>
> If you want to automatically set this you'll also need to remember
> (possibly in another bool) if the explicit config option was seen or
> not.
can we set this to 1 before even reading the config file?
Because we want to encrypt the secrets by default
That way, it can either be changed to 0 or remain 1.
>
> > + } else if (cfg->encrypt_data == 1) {
> > + if (!cfg->secretsEncryptionKeyPath) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("secretsEncryptionKeyPath must be set if
> > encrypt_data is 1 in %1$s"),
> > + configfile);
>
> This isn't an internal error, it's user's wrong config.
>
> Also make it a standalone check rather than an else branch.
yes will do.
>
> > + goto error;
> > + }
> > + }
> > + return g_steal_pointer(&cfg);
> > + error:
> > + virSecretDaemonConfigDispose(cfg);
>
> cfg is declared as autoptr so this is not needed as it ought to be
> called automatically. This also means that the whole 'error' label can
> be removed and 'NULL returned directly.
>
> > + return NULL;
> > +}
> > +
> > +static void
> > +virSecretDaemonConfigDispose(void *obj)
> > +{
> > + virSecretDaemonConfig *cfg = obj;
> > +
> > + g_free(cfg->secrets_encryption_key);
>
> Use the function to securely clear it first, before freeing it here.
yes, will do.
>
> > + g_free(cfg->secretsEncryptionKeyPath);
> > +}
> > diff --git a/src/conf/secret_config.h b/src/conf/secret_config.h
> > new file mode 100644
> > index 0000000000..638b7c49a4
> > --- /dev/null
> > +++ b/src/conf/secret_config.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * secret_config.h: secrets.conf config file handling
> > + *
> > + * Copyright (C) 2025 Red Hat, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#pragma once
> > +
> > +#include "internal.h"
> > +#include "virinhibitor.h"
> > +#include "secret_event.h"
> > +
> > +typedef struct _virSecretDaemonConfig virSecretDaemonConfig;
> > +struct _virSecretDaemonConfig {
> > + virObject parent;
> > + /* secrets encryption key path from secrets.conf file */
> > + char *secretsEncryptionKeyPath;
> > +
> > + unsigned char* secrets_encryption_key;
>
> unsigned char *sec...
Yes.
>
> > + size_t secretsKeyLen;
> > +
> > + /* Indicates if the newly written secrets are encrypted or not.
> > + * 0 if not encrypted and 1 if encrypted.
> > + */
> > + int encrypt_data;
>
> This ought to be either a 'bool' or one of the tristates. Alternatively
> two bools.
Can we keep it a boolean, and by default set it to 1? After reading config, it
will stay 1 or change to 0.
>
> > +};
> > +
> > +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
>
Regards,
Arun Menon
