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 | 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
@@ -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
+ */
+
+#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;
+ /* Encrypt secrets by default unless the configuration sets it otherwise */
+ cfg->encrypt_data = 1;
+
+ 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);
+ return -1;
+ }
+ }
+ return 0;
+}
+
+
+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;
+ }
+ 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);
+ }
+ 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) {
+ 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);
+}
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/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
@@ -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 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 <armenon@redhat.com>
> ---
> 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
> +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.
> + 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.
> + }
There here have
cfg->encrypt_data = cfg->secretsEncryptionKeyPath != NULL;
> + 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) {
...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.
> +}
> 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 :|
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 <armenon@redhat.com>
> > ---
> > 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
On Sat, Nov 29, 2025 at 12:24:37PM +0530, Arun Menon wrote:
> 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 <armenon@redhat.com>
> > > ---
> > > 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.
No, we have three distinct values for 'encrypt_data' which control
the behaviour
* Not set
- User specified path must be used if set
- Else, systemd credential must be used if present on disk
- Else, do not attempt encryption
* Set to 1
- User specified path must be used if set
- Else, systemd credential must be used if present on disk
- Else, built-in default path must be used
* Set to 0 - must honour that
- Do not attempt encryption
> > > +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.
These are different scenarios, see my outline of expected behaviour
earlier.
>
> 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()
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 :|
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 <armenon@redhat.com>
> ---
> 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
Hi Peter,
Thanks 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 <armenon@redhat.com>
> > ---
> > 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
The functions in secret_config.c are used by the src/conf/virsecretobj.c code.
If I add it in src/secret/ dir, I will not be able to include secret/secret_conf.c
inside src/conf/ because that would be like calling higher level code from lower level
code. Please guide.
>
>
> > @@ -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
Will change this.
>
> > +
> > +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
>
Regards,
Arun Menon
On Tue, Dec 02, 2025 at 19:16:31 +0530, Arun Menon wrote: > Hi Peter, > Thanks 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 <armenon@redhat.com> > > > --- > > > 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 > > The functions in secret_config.c are used by the src/conf/virsecretobj.c code. Which ones? This patch puts the follwing functions into secret_config.h: virSecretDaemonConfigFilePath virSecretDaemonConfigNew virSecretDaemonConfigLoadFile None of those have anything to do with the XML or config of the object itself. > If I add it in src/secret/ dir, I will not be able to include secret/secret_conf.c > inside src/conf/ because that would be like calling higher level code from lower level > code. Please guide. Per the list above there's nothing which should actually be needed for the secret object to access, everything is the configuration of the secrets driver. If you need to access data inside _virSecretDaemonConfig you can extract it and pass it separately.
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 <armenon@redhat.com>
> > ---
> > 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
© 2016 - 2025 Red Hat, Inc.