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 <[email protected]>
> > > ---
> > > libvirt.spec.in | 3 +
> > > po/POTFILES | 1 +
> > > src/conf/meson.build | 1 +
> > > src/conf/secret_config.c | 177 +++++++++++++++++++++++++
> > > src/conf/secret_config.h | 38 ++++++
> > > src/libvirt_private.syms | 2 +
> > > src/secret/libvirt_secrets.aug | 40 ++++++
> > > src/secret/meson.build | 18 +++
> > > src/secret/secrets.conf.in | 12 ++
> > > src/secret/test_libvirt_secrets.aug.in | 6 +
> > > 10 files changed, 298 insertions(+)
> > > create mode 100644 src/conf/secret_config.c
> > > create mode 100644 src/conf/secret_config.h
> > > create mode 100644 src/secret/libvirt_secrets.aug
> > > create mode 100644 src/secret/secrets.conf.in
> > > create mode 100644 src/secret/test_libvirt_secrets.aug.in
> > >
> > > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > > index dba8a71311..01ecf7e7ca 100644
> > > --- a/libvirt.spec.in
> > > +++ b/libvirt.spec.in
> > > @@ -2240,6 +2240,9 @@ exit 0
> > > %config(noreplace) %{_sysconfdir}/libvirt/virtsecretd.conf
> > > %{_datadir}/augeas/lenses/virtsecretd.aug
> > > %{_datadir}/augeas/lenses/tests/test_virtsecretd.aug
> > > +%{_datadir}/augeas/lenses/libvirt_secrets.aug
> > > +%{_datadir}/augeas/lenses/tests/test_libvirt_secrets.aug
> > > +%config(noreplace) %{_sysconfdir}/libvirt/secrets.conf
> > > %{_unitdir}/virtsecretd.service
> > > %{_unitdir}/virt-secret-init-encryption.service
> > > %{_unitdir}/virtsecretd.socket
> > > diff --git a/po/POTFILES b/po/POTFILES
> > > index f0aad35c8c..a64e4b2d87 100644
> > > --- a/po/POTFILES
> > > +++ b/po/POTFILES
> > > @@ -53,6 +53,7 @@ src/conf/nwfilter_conf.c
> > > src/conf/nwfilter_params.c
> > > src/conf/object_event.c
> > > src/conf/secret_conf.c
> > > +src/conf/secret_config.c
> > > src/conf/snapshot_conf.c
> > > src/conf/storage_adapter_conf.c
> > > src/conf/storage_conf.c
> > > diff --git a/src/conf/meson.build b/src/conf/meson.build
> > > index 5116c23fe3..9c51e99107 100644
> > > --- a/src/conf/meson.build
> > > +++ b/src/conf/meson.build
> > > @@ -68,6 +68,7 @@ interface_conf_sources = [
> > >
> > > secret_conf_sources = [
> > > 'secret_conf.c',
> > > + 'secret_config.c',
> > > 'virsecretobj.c',
> > > ]
> > >
> > > diff --git a/src/conf/secret_config.c b/src/conf/secret_config.c
> > > new file mode 100644
> > > index 0000000000..5bc0b24380
> > > --- /dev/null
> > > +++ b/src/conf/secret_config.c
> >
> >
> > > +static int
> > > +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg,
> > > + const char *filename)
> > > +{
> > > + g_autoptr(virConf) conf = NULL;
> > > + /* Encrypt secrets by default unless the configuration sets it
> > > otherwise */
> > > + cfg->encrypt_data = 1;
> >
> > We can't do that, as it'll break back compat for unprivileged
> > daemons and non-systemd hosts. It must only be set to 1 if
> > an explicit path was configured in the config (regardless of
> > whether it exists on disk), or the implict systemd credential
> > exists on disk
> I see. That means encrypt_data is totally dependent on the existence of the
> key path.
> Even if the user configures it to 0 in the secret.conf file, then we
> overwrite it to
> true if we find the encryption key file in systemd credentials or if the path
> is
> configured in the secret.conf file.
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 :|
