On Fri, Nov 21, 2025 at 08:41:53AM +0100, Peter Krempa via Devel wrote:
> On Thu, Nov 20, 2025 at 22:23:43 +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 <[email protected]>
> > ---
> > libvirt.spec.in | 7 +++++++
> > src/secret/meson.build | 8 ++++++++
> > src/secret/secret-init-encryption.in | 11 +++++++++++
> > src/secret/virtsecretd.service.extra.in | 8 ++++++++
> > 4 files changed, 34 insertions(+)
> > create mode 100644 src/secret/secret-init-encryption.in
> >
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 79738bd7bb..fa477db031 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -1889,13 +1889,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
> > @@ -2247,9 +2250,13 @@ 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
> > +%{_unitdir}/virt-secret-init-encryption.socket
> > +%{_unitdir}/virt-secret-init-encryption-ro.socket
> > +%{_unitdir}/virt-secret-init-encryption-admin.socket
>
> Looking at the change to src/secret/virtsecretd.service.extra.in
> these *.socket files are not actually referenced as a dependancy. Why
> are you creating those?
>
> Are they created by our build system and this is just a hack to make
> RPM builds work?
>
>
> > %attr(0755, root, root) %{_sbindir}/virtsecretd
> > %dir %attr(0700, root, root) %{_sysconfdir}/libvirt/secrets/
> > %ghost %dir %attr(0700, root, root) %{_rundir}/libvirt/secrets/
> > diff --git a/src/secret/meson.build b/src/secret/meson.build
> > index 3b859ea7b4..d8861fcbcd 100644
> > --- a/src/secret/meson.build
> > +++ b/src/secret/meson.build
> > @@ -42,6 +42,14 @@ if conf.has('WITH_SECRETS')
> > ],
> > }
> >
> > + virt_daemon_units += {
> > + 'service': 'virt-secret-init-encryption',
> > + 'name': 'secret-init-encryption',
> > + 'service_in': files('secret-init-encryption.in'),
> > + 'service_extra_in': [],
> > + 'socket_extra_in': [],
> > + }
>
> So this is what is creating those .socket files. If you don't want them
> and I don't see why they should exist you'll need to build this in a
> different way.
Yep, this virt_daemon_units was only designed for the main daemon
unit file processing.
In this case, we should just call the meson 'configure_file'
function directly to generate secret-init-encryption.service
> > +
> > openrc_init_files += {
> > 'name': 'virtsecretd',
> > 'in_file': files('virtsecretd.init.in'),
> > diff --git a/src/secret/secret-init-encryption.in
> > b/src/secret/secret-init-encryption.in
> > new file mode 100644
> > index 0000000000..4dc00e5bbb
> > --- /dev/null
> > +++ b/src/secret/secret-init-encryption.in
> > @@ -0,0 +1,11 @@
> > +[Unit]
> > +Before=virtsecretd.service
> > +ConditionPathExists=!/var/lib/libvirt/secrets/secrets-encryption-key
>
> So this shouldn't start if the file exists.
>
> You also use full paths here, which don't respect configure options such
> as 'sysconfdir' etc.
>
> > +
> > +[Service]
> > +Type=oneshot
> > +ExecStart=/usr/bin/sh -c 'test -f
> > /var/lib/libvirt/secrets/secrets-encryption-key || (dd if=/dev/urandom
> > status=none bs=32 count=1 | systemd-creds encrypt
> > --name=secrets-encryption-key -
> > /var/lib/libvirt/secrets/secrets-encryption-key)'
>
> So why is this checking the existance again?
>
> > +ExecStart=-/usr/bin/chmod 0400
> > /var/lib/libvirt/secrets/secrets-encryption-key
>
> 'chmod'-ing after the file is created leaves a theoretical chance to
> leak the secret. I think you'll need to pre-create an empty file, chmod
> it and jus then fill it with the secret.
Easier to use the 'umask' command first
"umask 0066 && (dd if=... | systemd-creds .....)"
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 :|
