On Thu, Nov 27, 2025 at 12:52:29 +0530, Arun Menon via Devel wrote:

This commit sets the foundation for encrypting the libvirt secrets by providing a secure way to pass a secret encryption key to the virtsecretd service.

A random secret key is generated using the new virt-secret-init-encryption service. This key can be consumed by the virtsecretd service.

By using the "Before=" directive in the new secret-init-encryption service and using "Requires=" directive in the virtsecretd service, we make sure that the daemon is run only after we have an encrypted secret key file generated and placed in /var/lib/libvirt/secrets. The virtsecretd service can then read the key from CREDENTIALS_DIRECTORY. [1]

This setup therefore provides a default key out-of-the-box for initial use. A subsequent commit will introduce the logic for virtsecretd to access and use this key via the $CREDENTIALS_DIRECTORY environment variable. [2]

[1] https://www.freedesktop.org/software/systemd/man/latest/systemd-creds.html [2] https://systemd.io/CREDENTIALS/

Signed-off-by: Arun Menon <armenon@redhat.com> --- libvirt.spec.in | 4 ++++ src/meson.build | 1 + src/secret/meson.build | 24 ++++++++++++++++++++++++ src/secret/secret-init-encryption.in | 10 ++++++++++ src/secret/virtsecretd.service.extra.in | 8 ++++++++ 5 files changed, 47 insertions(+) create mode 100644 src/secret/secret-init-encryption.in

diff --git a/libvirt.spec.in b/libvirt.spec.in index 62af7fb517..dba8a71311 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1880,13 +1880,16 @@ exit 0 %pre daemon-driver-secret %libvirt_sysconfig_pre virtsecretd %libvirt_systemd_unix_pre virtsecretd +%libvirt_systemd_oneshot_pre virt-secret-init-encryption

%posttrans daemon-driver-secret %libvirt_sysconfig_posttrans virtsecretd %libvirt_systemd_unix_posttrans virtsecretd +%libvirt_systemd_unix_posttrans virt-secret-init-encryption

%preun daemon-driver-secret %libvirt_systemd_unix_preun virtsecretd +%libvirt_systemd_unix_preun virt-secret-init-encryption

%pre daemon-driver-storage-core %libvirt_sysconfig_pre virtstoraged @@ -2238,6 +2241,7 @@ exit 0 %{_datadir}/augeas/lenses/virtsecretd.aug %{_datadir}/augeas/lenses/tests/test_virtsecretd.aug %{_unitdir}/virtsecretd.service +%{_unitdir}/virt-secret-init-encryption.service %{_unitdir}/virtsecretd.socket %{_unitdir}/virtsecretd-ro.socket %{_unitdir}/virtsecretd-admin.socket

diff --git a/src/secret/meson.build b/src/secret/meson.build index 3b859ea7b4..c02d1064a9 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -31,6 +31,30 @@ if conf.has('WITH_SECRETS') 'name': 'virtsecretd', }

+ secret_init_encryption_unit = { + 'service': 'virt-secret-init-encryption', + 'name': 'Libvirt Secret Encryption Init', + 'input': 'secret-init-encryption.in', + }

You can put these values directly inline rather than have this extra object.

+ unit_conf = configuration_data()

Preferrably use a more descriptive name for the variable. e.g. virt_secret_init_encryption_conf

+ unit_conf.set('runstatedir', runstatedir) + unit_conf.set('sbindir', sbindir) + unit_conf.set('localstatedir', localstatedir) + unit_conf.set('sysconfdir', sysconfdir) + unit_conf.set('initconfdir', initconfdir) + unit_conf.set('name', secret_init_encryption_unit['name']) + unit_conf.set('service', secret_init_encryption_unit['service']) + unit_conf.set('SERVICE', secret_init_encryption_unit['service'].to_upper())

The secret-init-encryption.in uses only 'localstatedir' replacement, anything else isn't used here.

+ configure_file( + input: secret_init_encryption_unit['input'], + output: '@0@.service'.format(secret_init_encryption_unit['service']), + configuration: unit_conf, + install: true, + install_dir: unitdir, + ) + virt_daemon_units += { 'service': 'virtsecretd', 'name': 'secret',

diff --git a/src/secret/secret-init-encryption.in b/src/secret/secret-init-encryption.in

Please rename this to 'virt-secret-init-encryption.service.in'

new file mode 100644 index 0000000000..29da6dbcce --- /dev/null +++ b/src/secret/secret-init-encryption.in @@ -0,0 +1,10 @@ +[Unit] +Before=virtsecretd.service +ConditionPathExists=!@localstatedir@/lib/libvirt/secrets/secrets-encryption-key + +[Service] +Type=oneshot +ExecStart=/usr/bin/sh -c 'umask 0066 && (dd if=/dev/urandom status=none bs=32 count=1 | systemd-creds encrypt --name=secrets-encryption-key - @localstatedir@/lib/libvirt/secrets/secrets-encryption-key)' + +[Install] +WantedBy=multi-user.target

What does this actually do?

diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..116458b22a 100644 --- a/src/secret/virtsecretd.service.extra.in +++ b/src/secret/virtsecretd.service.extra.in @@ -1,2 +1,10 @@ # The contents of this unit will be merged into a base template. # Additional units might be merged as well. See meson.build for details. +# +[Unit] +Requires=virt-secret-init-encryption.service +After=virt-secret-init-encryption.service + +[Service] +LoadCredentialEncrypted=secrets-encryption-key:@localstatedir@/lib/libvirt/secrets/secrets-encryption-key +Environment=SECRETS_ENCRYPTION_KEY=%d/secrets-encryption-key

This will likely be needed also for the monolithic daemon's unit (libvirtd.service) as that can also start the secret driver in those setups.

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 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 :|

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 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

On Thu, Nov 27, 2025 at 03:13:18PM +0000, Daniel P. Berrangé wrote: 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:31 +0530, Arun Menon via Devel wrote:

The new attribute will store the available ciphers with which secrets can be encrypted. At the moment only aes256cbc encryption method is used. This can be extended in future.

Rename the file name structure attribute from base64File to secretValueFile.

Signed-off-by: Arun Menon <armenon@redhat.com> --- include/libvirt/libvirt-secret.h | 20 ++++++++++++++++++++ src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 22 +++++++++++----------- src/util/virsecret.c | 4 ++++ src/util/virsecret.h | 1 + 5 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/include/libvirt/libvirt-secret.h b/include/libvirt/libvirt-secret.h index 761437d4ad..768c92c10c 100644 --- a/include/libvirt/libvirt-secret.h +++ b/include/libvirt/libvirt-secret.h @@ -70,6 +70,26 @@ typedef enum { # endif } virSecretUsageType;

+/** + * virSecretEncryptionSchemeType: + * + * Since: 11.10.0 + */ +typedef enum { + VIR_SECRET_ENCRYPTION_SCHEME_NONE = 0, /* (Since: 11.10.0) */ + VIR_SECRET_ENCRYPTION_SCHEME_AES256CBC = 1, /* (Since: 11.10.0) */ +# ifdef VIR_ENUM_SENTINELS + VIR_SECRET_ENCRYPTION_SCHEME_LAST + /* + * NB: this enum value will increase over time as new encryption schemes are + * added to the libvirt API. It reflects the last enncryption scheme supported + * by this version of the libvirt API. + * + * Since: 11.10.0 + */ +# endif +} virSecretEncryptionSchemeType; +

AFAIU we decided not to expose these constants to the user in any way, but this puts them into the public header file. Also note, that libvirt is in freeze for 11.10.0. So while this part will be deleted and not exposed (thus not requiring version numbers) any other version numbers ought to be updated to 12.0.0.

On Thu, Nov 27, 2025 at 12:52:32 +0530, Arun Menon via Devel wrote:

Since we now have the functionality to provide the secrets driver with an encryption key, and the newly introduced attribute to store the cipher, we can use the key to save and load secrets.

Encrypt all secrets stored on disk by default. When loading secrets, identify the decryption method by matching the file extension against known encryptionSchemeType values, iterating from the most recent. If no matching scheme is found, the secret is skipped. If the encryption key is changed across restarts, then also the secret driver will fail to load the secrets on the disk that were encrypted with the former key.

Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/virsecretobj.c | 159 +++++++++++++++++++++++++++++-------- src/conf/virsecretobj.h | 13 ++- src/secret/secret_driver.c | 27 +++++-- 3 files changed, 156 insertions(+), 43 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index a3dd7983bb..8b658a6f4c 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -31,6 +31,10 @@ #include "virhash.h" #include "virlog.h" #include "virstring.h" +#include "virsecret.h" +#include "virrandom.h" +#include "vircrypto.h" +#include "virsecureerase.h"

#define VIR_FROM_THIS VIR_FROM_SECRET

@@ -323,12 +327,16 @@ virSecretObj * virSecretObjListAdd(virSecretObjList *secrets, virSecretDef **newdef, const char *configDir, - virSecretDef **oldDef) + virSecretDef **oldDef, + virSecretDaemonConfig *driverConfig) { virSecretObj *obj; virSecretDef *objdef; virSecretObj *ret = NULL; + g_autofree char *encryptionScheme = NULL; + g_autofree char *encryptionSchemeExt = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virSecretEncryptionSchemeType latestEncryptionScheme;

Declare these in the appropriate scope rather than top level.

virObjectRWLockWrite(secrets);

@@ -379,10 +387,24 @@ virSecretObjListAdd(virSecretObjList *secrets, goto cleanup;

/* Generate the possible configFile and secretValueFile strings - * using the configDir, uuidstr, and appropriate suffix + * using the configDir, uuidstr, and appropriate suffix. + * By default, the latest encryption scheme will be used to encrypt secrets. */ - if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, ".base64"))) + + latestEncryptionScheme = VIR_SECRET_ENCRYPTION_SCHEME_LAST - 1; + encryptionScheme = g_strdup(virSecretEncryptionSchemeTypeToString(latestEncryptionScheme));

So you create a copy of the second-to-last scheme string ...

+ encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL);> +

... just to concatenate it with a dot and never use it again? encryptionScheme = g_strdup_printf(".%s", virSecretEncryptionSchemeTypeToString(VIR_SECRET_ENCRYPTION_SCHEME_LAST- 1)); You save two variables.

+ if (driverConfig->encrypt_data) { + if (!(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, encryptionSchemeExt))) { + goto cleanup; + } + } else { + if (!(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, ".base64"))) { + goto cleanup; + } + }

There's no need to have the above code twice: const char *secretSuffix = ".base64"; g_autofree char *encryptionSchemeSuffix = NULL; if (driverConfig->encrypt_data) encryptionSchemeSuffix = g_strdup_printf(".%s", virSecretEncryptionSchemeTypeToString(VIR_SECRET_ENCRYPTION_SCHEME_LAST- 1)); if (!(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, secretSuffix))) { ...

+ if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))) goto cleanup;

if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) @@ -407,6 +429,7 @@ struct virSecretCountData { int count; };

+ static int virSecretObjListNumOfSecretsCallback(void *payload, const char *name G_GNUC_UNUSED, @@ -530,6 +553,7 @@ struct _virSecretObjListExportData { bool error; };

+ static int virSecretObjListExportCallback(void *payload, const char *name G_GNUC_UNUSED,

Two instances of spurious whitespace change.

@@ -682,15 +706,38 @@ virSecretObjSaveConfig(virSecretObj *obj)

int -virSecretObjSaveData(virSecretObj *obj) +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { g_autofree char *base64 = NULL; + g_autofree uint8_t *secret = NULL; + g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + size_t secretLen = 0; + uint8_t iv[16] = { 0 };

if (!obj->value) return 0;

- base64 = g_base64_encode(obj->value, obj->value_size); - + if (driverConfig->encrypt_data && driverConfig->secrets_encryption_key) { + if (virRandomBytes(iv, sizeof(iv)) < 0) { + return -1; + } + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC,

Here you hardcode the scheme. Make them into a const static global variable (or a preprocessor macro) along with the suffix and use that instead. Because now the filename generator is "extensible" but this is hardcoded.

+ driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + (uint8_t *)obj->value, obj->value_size, + &encryptedValue, &encryptedValueLen) < 0) { + return -1; + } + secretLen = sizeof(iv) + encryptedValueLen; + secret = g_new0(uint8_t, secretLen); + memcpy(secret, iv, sizeof(iv)); + memcpy(secret + sizeof(iv), encryptedValue, encryptedValueLen); + base64 = g_base64_encode(secret, secretLen); + } else { + base64 = g_base64_encode(obj->value, obj->value_size); + } if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) < 0) return -1;

[...]

@@ -807,11 +853,23 @@ virSecretLoadValidateUUID(virSecretDef *def,

static int -virSecretLoadValue(virSecretObj *obj) +virSecretLoadValue(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { - int ret = -1, fd = -1; + int ret = -1; + VIR_AUTOCLOSE fd = -1; struct stat st; + g_autofree char *contents = NULL; + g_autofree uint8_t *contents_encrypted = NULL; + g_autofree uint8_t *decryptedValue = NULL; + g_autofree char *encryptionScheme = NULL; + + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0; + virSecretEncryptionSchemeType latestEncryptionScheme;

if ((fd = open(obj->secretValueFile, O_RDONLY)) == -1) { if (errno == ENOENT) { @@ -841,25 +899,60 @@ virSecretLoadValue(virSecretObj *obj) goto cleanup; }

- contents = g_new0(char, st.st_size + 1); + /* Iterate over the encryption schemes starting with the latest one and + * decrypt the contents of the file on the disk, by matching the file + * extention with the encryption scheme. If there is no scheme matching + * the file extention, then that secret is not loaded. */

I suggest you create an array of schemes that contains also base 64 so that the code will share the loop. Now that the file is kept base64 encoded you just skip the decryption code (e.g. assign the base64 decoded value directly into output. rahtehr than having two very separate branches.

- if (saferead(fd, contents, st.st_size) != st.st_size) { - virReportSystemError(errno, _("cannot read '%1$s'"), - obj->secretValueFile); - goto cleanup; + if (virStringHasSuffix(obj->secretValueFile, ".base64")) { + contents = g_new0(char, st.st_size + 1); + if (saferead(fd, contents, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->secretValueFile); + goto cleanup; + } + contents[st.st_size] = '\0'; + obj->value = g_base64_decode(contents, &obj->value_size); + } else { + for (latestEncryptionScheme = VIR_SECRET_ENCRYPTION_SCHEME_LAST-1; latestEncryptionScheme > 0; latestEncryptionScheme--) { + encryptionScheme = g_strdup(virSecretEncryptionSchemeTypeToString(latestEncryptionScheme)); + if (virStringHasSuffix(obj->secretValueFile, encryptionScheme)) { + contents = g_new0(char, st.st_size + 1); + if (saferead(fd, contents, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->secretValueFile); + goto cleanup; + } + if ((st.st_size) < sizeof(iv)) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Encrypted secret size is invalid")); + goto cleanup;

Since this is base64 encoded now; this check is invalid. This ought to hapen after base64 decode.

+ } + contents[st.st_size] = '\0'; + contents_encrypted = g_base64_decode(contents, &obj->value_size); + + memcpy(iv, contents_encrypted, sizeof(iv)); + ciphertext = contents_encrypted + sizeof(iv); + ciphertextLen = st.st_size - sizeof(iv); + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC,

Umm so you have this declarative loop that takes schemes from the enum of supported ones but then you hardcode to decrypt everything using VIR_CRYPTO_CIPHER_AES256CBC? Since it's unlikely that we'll add another one any time soon, you can make this a problem of the future you (or future someone else) by just removing the attempts to make it declarative and hardcode aes256cbc->base64 instead of this half-done approach.

+ driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Decryption of secret value failed")); + goto cleanup; + } + g_free(obj->value); + obj->value = g_steal_pointer(&decryptedValue); + obj->value_size = decryptedValueLen; + } + } } - contents[st.st_size] = '\0'; - - VIR_FORCE_CLOSE(fd); - - obj->value = g_base64_decode(contents, &obj->value_size); - ret = 0;

cleanup: - if (contents != NULL) - memset(contents, 0, st.st_size); - VIR_FORCE_CLOSE(fd); + virSecureErase(iv, sizeof(iv)); return ret; }

[...]

@@ -70,6 +75,9 @@ struct _virSecretDriverState {

/* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + /* Settings from secrets.conf file */ + virSecretDaemonConfig *config;

see below.

};

@@ -307,7 +316,6 @@ secretGetXMLDesc(virSecretPtr secret, return ret; }

-

Spurious whithespace change.

static int secretSetValue(virSecretPtr secret, const unsigned char *value, @@ -327,8 +335,7 @@ secretSetValue(virSecretPtr secret, def = virSecretObjGetDef(obj); if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup; - - if (virSecretObjSetValue(obj, value, value_size) < 0) + if (virSecretObjSetValue(obj, value, value_size, driver->config) < 0) goto cleanup;

event = virSecretEventValueChangedNew(def->uuid, @@ -454,6 +461,7 @@ secretStateCleanupLocked(void) VIR_FREE(driver->configDir);

virObjectUnref(driver->secretEventState); + virObjectUnref(driver->config);

This belongs to the patch adding the config infra.

virInhibitorFree(driver->inhibitor);

if (driver->lockFD != -1) @@ -518,6 +526,8 @@ secretStateInitialize(bool privileged, driver->stateDir); goto error; } + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + goto error;

This too.

driver->inhibitor = virInhibitorNew( VIR_INHIBITOR_WHAT_NONE, @@ -534,7 +544,7 @@ secretStateInitialize(bool privileged, if (!(driver->secrets = virSecretObjListNew())) goto error;

- if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) + if (virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config) < 0) goto error;

return VIR_DRV_STATE_INIT_COMPLETE; @@ -553,7 +563,10 @@ secretStateReload(void) if (!driver) return -1;

- ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged)))

This too. I'm fairly sure I've noted this in v2.

+ return -1; + + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config));

return 0; } -- 2.51.1