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 <armenon@redhat.com>
---
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/libvirt.spec.in b/libvirt.spec.in
index fa477db031..8462d08c61 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2249,6 +2249,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 23da794f84..1a76e0505a 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..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/>.
+ */
+
+#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) {
+ 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);
+ 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);
+ return -1;
+ }
+ }
+ return 0;
+}
+
+static bool getSecretsEncryptionKey(virSecretDaemonConfig *cfg,
+ uint8_t **secrets_encryption_key, size_t *secrets_encryption_keylen)
+{
+ int fd = -1;
+ 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;
+ }
+ if (fstat(fd, &st) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat secrets key file '%1$s'"),
+ cfg->secretsEncryptionKeyPath);
+ VIR_FORCE_CLOSE(fd);
+ 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);
+ 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;
+ }
+ 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);
+
+ if (virSecretLoadDaemonConfig(cfg, configfile) < 0)
+ goto error;
+
+ if (!(credentials_directory = getenv("CREDENTIALS_DIRECTORY"))) {
+ credentials_directory = NULL;
+ }
+
+ 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;
+ }
+ 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;
+ }
+ }
+
+ if (cfg->encrypt_data != 1) {
+ cfg->encrypt_data = (cfg->secretsKeyLen == 32) ? 1 : 0;
+ } 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);
+ goto error;
+ }
+ }
+ return g_steal_pointer(&cfg);
+ error:
+ virSecretDaemonConfigDispose(cfg);
+ return NULL;
+}
+
+static void
+virSecretDaemonConfigDispose(void *obj)
+{
+ virSecretDaemonConfig *cfg = obj;
+
+ g_free(cfg->secrets_encryption_key);
+ 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;
+ size_t secretsKeyLen;
+
+ /* Indicates if the newly written secrets are encrypted or not.
+ * 0 if not encrypted and 1 if encrypted.
+ */
+ int 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 fc5fdb00f4..7ecb573851 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1064,6 +1064,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 d8861fcbcd..e453e71464 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
@@ -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
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.1On 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 <armenon@redhat.com>
> ---
> 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.
> +
> +#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.
> + 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
Also encrypt_data really ought to be a bool based on usage. Here you'll
have to convert it properly on parsing.
> + 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.
> +{
> + int fd = -1;
Declare this using VIR_AUTOCLOSE so that you don't have to deal with it
in this whole file.
> + 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
> + 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.
> + 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.
> + 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.
> + }
> + 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.
> + 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.
> + }
> +
> + 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));
> + 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)
> + }
> + }
> +
> + 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.
> + } 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.
> + 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.
> + 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...
> + 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.
> +};
> +
> +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
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 <armenon@redhat.com>
> > ---
> > 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
© 2016 - 2026 Red Hat, Inc.