A new configuration file called secrets.conf is introduced to
let the user configure the path to the master 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.
The virtsecretd driver checks if the credentials are available
in the CREDENTIALS_DIRECTORY. In case it is not present, then the
user is expected to provide the encryption key path in secrets.conf

Add logic to parse the encryption key file and store the key.
When systemd will start the secrets driver, it will read the secret.conf
file and check if encrypt_data flag is set to 1. In that case, the secrets
will be stored in encrypted format on the disk. The encryption and decryption
logic will be added in the subsequent patches.

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 libvirt.spec.in            |  1 +
 src/secret/meson.build     |  7 +++
 src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++
 src/secret/secrets.conf.in | 14 ++++++
 4 files changed, 118 insertions(+)
 create mode 100644 src/secret/secrets.conf.in

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 79738bd7bb..f27247b7c1 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2246,6 +2246,7 @@ exit 0
 %config(noreplace) %{_sysconfdir}/libvirt/virtsecretd.conf
 %{_datadir}/augeas/lenses/virtsecretd.aug
 %{_datadir}/augeas/lenses/tests/test_virtsecretd.aug
+%config(noreplace) %{_sysconfdir}/libvirt/secrets.conf
 %{_unitdir}/virtsecretd.service
 %{_unitdir}/virtsecretd.socket
 %{_unitdir}/virtsecretd-ro.socket
diff --git a/src/secret/meson.build b/src/secret/meson.build
index 3b859ea7b4..a211ffed83 100644
--- a/src/secret/meson.build
+++ b/src/secret/meson.build
@@ -27,6 +27,13 @@ if conf.has('WITH_SECRETS')
     ],
   }
 
+  secrets_conf = configure_file(
+    input: 'secrets.conf.in',
+    output: 'secrets.conf',
+    copy: true
+  )
+  virt_conf_files += secrets_conf
+
   virt_daemon_confs += {
     'name': 'virtsecretd',
   }
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 04c3ca49f1..0b415e5ef3 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -42,6 +42,7 @@
 #include "secret_event.h"
 #include "virutil.h"
 #include "virinhibitor.h"
+#include "virfile.h"
 
 #define VIR_FROM_THIS VIR_FROM_SECRET
 
@@ -70,6 +71,17 @@ struct _virSecretDriverState {
 
     /* Immutable pointer, self-locking APIs */
     virInhibitor *inhibitor;
+
+    /* master encryption key value from secret.conf file */
+    char *masterKeyPath;
+
+    /* Indicates if the secrets are encrypted or not. 0 if not encrypted
+     * and 1 if encrypted.
+     */
+    int encrypt_data;
+
+    unsigned char* masterKey;
+    size_t masterKeyLen;
 };
 
 static virSecretDriverState *driver;
@@ -307,6 +319,44 @@ secretGetXMLDesc(virSecretPtr secret,
     return ret;
 }
 
+static bool secretGetMasterKey(uint8_t **masterKey, size_t *masterKeyLen)
+{
+    int fd = -1;
+    struct stat st;
+
+    if ((fd = open(driver->masterKeyPath, O_RDONLY)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open master key file '%1$s'"),
+                       driver->masterKeyPath);
+        return false;
+    }
+    if (fstat(fd, &st) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat master key file '%1$s'"),
+                       driver->masterKeyPath);
+        VIR_FORCE_CLOSE(fd);
+        return false;
+    }
+    *masterKeyLen = st.st_size;
+    if (*masterKeyLen == 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s is empty"),
+                       driver->masterKeyPath);
+        VIR_FORCE_CLOSE(fd);
+        return false;
+    }
+    *masterKey = g_new0(uint8_t, *masterKeyLen);
+    if (saferead(fd, &masterKey, *masterKeyLen) != *masterKeyLen) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read master key file '%1$s'"),
+                       driver->masterKeyPath);
+        VIR_FORCE_CLOSE(fd);
+        return false;
+    }
+    VIR_FORCE_CLOSE(fd);
+    if (*masterKeyLen < 32) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s must be atleast 32 bytes"),
+                       driver->masterKeyPath);
+        return false;
+    }
+    return true;
+}
 
 static int
 secretSetValue(virSecretPtr secret,
@@ -482,6 +532,10 @@ secretStateInitialize(bool privileged,
                       void *opaque)
 {
     VIR_LOCK_GUARD lock = virLockGuardLock(&mutex);
+    g_autofree char *secretsconf = NULL;
+    g_autofree char *credentials_directory = NULL;
+    g_autofree char *master_encryption_key_path = NULL;
+    g_autoptr(virConf) conf = NULL;
 
     driver = g_new0(virSecretDriverState, 1);
 
@@ -537,6 +591,48 @@ secretStateInitialize(bool privileged,
     if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0)
         goto error;
 
+    secretsconf = g_strdup_printf("%s/libvirt/secrets.conf", SYSCONFDIR);
+    credentials_directory = getenv("CREDENTIALS_DIRECTORY");
+
+    if (credentials_directory) {
+        VIR_DEBUG("Using credentials directory from environment: %s",
+                  credentials_directory);
+        master_encryption_key_path = g_strdup_printf("%s/master-encryption-key", credentials_directory);
+        if (access(master_encryption_key_path, R_OK) == 0) {
+            driver->masterKeyPath = g_strdup(master_encryption_key_path);
+        }
+    } else if (access(secretsconf, R_OK) == 0) {
+        if (!(conf = virConfReadFile(secretsconf, 0))) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Failed to read secrets.conf from %1$s"),
+                           secretsconf);
+            goto error;
+        }
+
+        if (virConfGetValueString(conf, "master_encryption_key", &driver->masterKeyPath) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Failed to get master_encryption_key from %1$s"),
+                           secretsconf);
+            goto error;
+        }
+    } else {
+        VIR_DEBUG("No secrets configuration found %s, skipping", driver->configDir);
+        driver->masterKeyPath = NULL;
+        driver->masterKeyLen = 0;
+    }
+    if (driver->masterKeyPath) {
+        if (!secretGetMasterKey(&driver->masterKey, &driver->masterKeyLen)) {
+            goto error;
+        }
+        VIR_DEBUG("Master encryption key loaded from %s", driver->masterKeyPath);
+        VIR_DEBUG("Master encryption key length: %zu bytes", driver->masterKeyLen);
+    }
+    if (virConfGetValueInt(conf, "encrypt_data", &driver->encrypt_data) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Failed to get encrypt_data from %1$s"),
+                       secretsconf);
+        goto error;
+    }
     return VIR_DRV_STATE_INIT_COMPLETE;
 
  error:
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
new file mode 100644
index 0000000000..80bb9654ce
--- /dev/null
+++ b/src/secret/secrets.conf.in
@@ -0,0 +1,14 @@
+#
+# Master configuration file for the secrets driver.
+#
+
+# The master encryption key is used to override default master encryption
+# key path. The user can create an encryption key and set the master_encryption_key
+# to the path on which it resides.
+# The key must be atleast 32-bytes long.
+#
+# master_encryption_key = "/run/libvirt/secrets/master.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
-- 
2.51.1

On Thu, Nov 13, 2025 at 07:02:22PM +0530, Arun Menon wrote:
> A new configuration file called secrets.conf is introduced to
> let the user configure the path to the master 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.
> The virtsecretd driver checks if the credentials are available
> in the CREDENTIALS_DIRECTORY. In case it is not present, then the
> user is expected to provide the encryption key path in secrets.conf
> 
> Add logic to parse the encryption key file and store the key.
> When systemd will start the secrets driver, it will read the secret.conf
> file and check if encrypt_data flag is set to 1. In that case, the secrets
> will be stored in encrypted format on the disk. The encryption and decryption
> logic will be added in the subsequent patches.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  libvirt.spec.in            |  1 +
>  src/secret/meson.build     |  7 +++
>  src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++
>  src/secret/secrets.conf.in | 14 ++++++

New config files need to be accompanied with augeas files for
processing them. See the simple examples in

  src/network/libvirtd_network.aug
  src/network/test_libvirtd_network.aug.in

and the src/network/meson.build file for how to wire them up.
They also need to be in the RPM spec.

> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 04c3ca49f1..0b415e5ef3 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -42,6 +42,7 @@
>  #include "secret_event.h"
>  #include "virutil.h"
>  #include "virinhibitor.h"
> +#include "virfile.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_SECRET
>  
> @@ -70,6 +71,17 @@ struct _virSecretDriverState {
>  
>      /* Immutable pointer, self-locking APIs */
>      virInhibitor *inhibitor;
> +
> +    /* master encryption key value from secret.conf file */
> +    char *masterKeyPath;

Per the previous patch, lets call this  secretsEncryptionKeyPath

> +
> +    /* Indicates if the secrets are encrypted or not. 0 if not encrypted
> +     * and 1 if encrypted.
> +     */

Lets specify  "newly written secrets"  to clarify that setting this
to '1' wouldn't make it encrypt any pre-existing plain text secrets

> +    int encrypt_data;
> +
> +    unsigned char* masterKey;
> +    size_t masterKeyLen;
>  };

None of these should be in this struct. Our normal practice
is to define a virSecretDriverConfig  struct to hold all
config parameters, and an pointer to that config in the
state struct. See virNetworkDriverState/Config  for a
simple example.

>  
>  static virSecretDriverState *driver;
> @@ -307,6 +319,44 @@ secretGetXMLDesc(virSecretPtr secret,
>      return ret;
>  }
>  
> +static bool secretGetMasterKey(uint8_t **masterKey, size_t *masterKeyLen)
> +{
> +    int fd = -1;
> +    struct stat st;
> +
> +    if ((fd = open(driver->masterKeyPath, O_RDONLY)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open master key file '%1$s'"),
> +                       driver->masterKeyPath);
> +        return false;
> +    }
> +    if (fstat(fd, &st) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat master key file '%1$s'"),
> +                       driver->masterKeyPath);
> +        VIR_FORCE_CLOSE(fd);
> +        return false;
> +    }
> +    *masterKeyLen = st.st_size;
> +    if (*masterKeyLen == 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s is empty"),
> +                       driver->masterKeyPath);
> +        VIR_FORCE_CLOSE(fd);
> +        return false;
> +    }
> +    *masterKey = g_new0(uint8_t, *masterKeyLen);
> +    if (saferead(fd, &masterKey, *masterKeyLen) != *masterKeyLen) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read master key file '%1$s'"),
> +                       driver->masterKeyPath);
> +        VIR_FORCE_CLOSE(fd);
> +        return false;
> +    }
> +    VIR_FORCE_CLOSE(fd);
> +    if (*masterKeyLen < 32) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s must be atleast 32 bytes"),
> +                       driver->masterKeyPath);
> +        return false;
> +    }
> +    return true;
> +}
>  
>  static int
>  secretSetValue(virSecretPtr secret,
> @@ -482,6 +532,10 @@ secretStateInitialize(bool privileged,
>                        void *opaque)
>  {
>      VIR_LOCK_GUARD lock = virLockGuardLock(&mutex);
> +    g_autofree char *secretsconf = NULL;
> +    g_autofree char *credentials_directory = NULL;
> +    g_autofree char *master_encryption_key_path = NULL;
> +    g_autoptr(virConf) conf = NULL;
>  
>      driver = g_new0(virSecretDriverState, 1);
>  
> @@ -537,6 +591,48 @@ secretStateInitialize(bool privileged,
>      if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0)
>          goto error;
>  
> +    secretsconf = g_strdup_printf("%s/libvirt/secrets.conf", SYSCONFDIR);
> +    credentials_directory = getenv("CREDENTIALS_DIRECTORY");
> +
> +    if (credentials_directory) {
> +        VIR_DEBUG("Using credentials directory from environment: %s",
> +                  credentials_directory);
> +        master_encryption_key_path = g_strdup_printf("%s/master-encryption-key", credentials_directory);
> +        if (access(master_encryption_key_path, R_OK) == 0) {
> +            driver->masterKeyPath = g_strdup(master_encryption_key_path);
> +        }
> +    } else if (access(secretsconf, R_OK) == 0) {
> +        if (!(conf = virConfReadFile(secretsconf, 0))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to read secrets.conf from %1$s"),
> +                           secretsconf);
> +            goto error;
> +        }
> +
> +        if (virConfGetValueString(conf, "master_encryption_key", &driver->masterKeyPath) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to get master_encryption_key from %1$s"),
> +                           secretsconf);
> +            goto error;
> +        }
> +    } else {
> +        VIR_DEBUG("No secrets configuration found %s, skipping", driver->configDir);
> +        driver->masterKeyPath = NULL;
> +        driver->masterKeyLen = 0;
> +    }
> +    if (driver->masterKeyPath) {
> +        if (!secretGetMasterKey(&driver->masterKey, &driver->masterKeyLen)) {
> +            goto error;
> +        }
> +        VIR_DEBUG("Master encryption key loaded from %s", driver->masterKeyPath);
> +        VIR_DEBUG("Master encryption key length: %zu bytes", driver->masterKeyLen);
> +    }
> +    if (virConfGetValueInt(conf, "encrypt_data", &driver->encrypt_data) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to get encrypt_data from %1$s"),
> +                       secretsconf);
> +        goto error;
> +    }
>      return VIR_DRV_STATE_INIT_COMPLETE;

Config file handling code should not be put directly in the state
initialization method. All loading of the config should be isolated
in separate methods, and live in secret_driver_conf.{h,c} files.
Again take a look at bridge_driver_conf.{h,c} for a simple example
of prior art.


In terms of handling the 'encrypt_data" parameter we need this
psuedo-logic:

   if encrypt_data is not set
        if key path exists
	    encrypt_data = 1
	else
	    encrypt_data = 1
   else if encrypt_data == 1
        if key path does not exist
	    ..report error...
	    

>  
>   error:
> diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
> new file mode 100644
> index 0000000000..80bb9654ce
> --- /dev/null
> +++ b/src/secret/secrets.conf.in
> @@ -0,0 +1,14 @@
> +#
> +# Master configuration file for the secrets driver.
> +#
> +
> +# The master encryption key is used to override default master encryption
> +# key path. The user can create an encryption key and set the master_encryption_key
> +# to the path on which it resides.
> +# The key must be atleast 32-bytes long.
> +#
> +# master_encryption_key = "/run/libvirt/secrets/master.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

This setting should be commented out - the default value should be
set by the code, and should be 0 or 1, depending on whether or not
any encryption key exists on disk.

 # This setting determines whether newly written secret values
 # are encrypted or not. Setting this to 1 will not trigger
 # encryption of existing plain text secrets on disk.
 #
 # Will default to 1 if the master_encryption_key path exists
 # on disk, otherwise will default to 0. Uncomment this to
 # force use of encryption and report an error if the key path
 # is missing.
 # encrypt_data = 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 Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
> A new configuration file called secrets.conf is introduced to
> let the user configure the path to the master 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.
> The virtsecretd driver checks if the credentials are available
> in the CREDENTIALS_DIRECTORY. In case it is not present, then the
> user is expected to provide the encryption key path in secrets.conf

Is there any plan to be able to pass the secret do the secrets
driver/daemon in an ephemeral way?

Because both the systemd secrets and the config file seem to just store
it on the same host. Thus for root-owned files it's just a slightly
bigger hurdle rather than any real security.


> When systemd will start the secrets driver, it will read the secret.conf
> file and check if encrypt_data flag is set to 1. In that case, the secrets
> will be stored in encrypted format on the disk. The encryption and decryption
> logic will be added in the subsequent patches.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  libvirt.spec.in            |  1 +
>  src/secret/meson.build     |  7 +++
>  src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++
>  src/secret/secrets.conf.in | 14 ++++++
>  4 files changed, 118 insertions(+)
>  create mode 100644 src/secret/secrets.conf.in

[...]

> diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
> new file mode 100644
> index 0000000000..80bb9654ce
> --- /dev/null
> +++ b/src/secret/secrets.conf.in
> @@ -0,0 +1,14 @@
> +#
> +# Master configuration file for the secrets driver.
> +#
> +
> +# The master encryption key is used to override default master encryption
> +# key path. The user can create an encryption key and set the master_encryption_key
> +# to the path on which it resides.
> +# The key must be atleast 32-bytes long.
> +#
> +# master_encryption_key = "/run/libvirt/secrets/master.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

As the default secret seems to be handed in via systemd, which will it
make available to any upgraded installation,  I don't think you can
unconditionally enable this option as it would break existing
un-encrypted secrets.

On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
> On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
> > A new configuration file called secrets.conf is introduced to
> > let the user configure the path to the master 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.
> > The virtsecretd driver checks if the credentials are available
> > in the CREDENTIALS_DIRECTORY. In case it is not present, then the
> > user is expected to provide the encryption key path in secrets.conf


> > diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
> > new file mode 100644
> > index 0000000000..80bb9654ce
> > --- /dev/null
> > +++ b/src/secret/secrets.conf.in
> > @@ -0,0 +1,14 @@
> > +#
> > +# Master configuration file for the secrets driver.
> > +#
> > +
> > +# The master encryption key is used to override default master encryption
> > +# key path. The user can create an encryption key and set the master_encryption_key
> > +# to the path on which it resides.
> > +# The key must be atleast 32-bytes long.
> > +#
> > +# master_encryption_key = "/run/libvirt/secrets/master.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
> 
> As the default secret seems to be handed in via systemd, which will it
> make available to any upgraded installation,  I don't think you can
> unconditionally enable this option as it would break existing
> un-encrypted secrets.

When I discussed the design with Arun offlist, my suggestion was that
our goal should be to have encryption enabled out of the box on all
systemd based hosts, with zero config by the admin.

This implies that we need the ability to read *both* plain text and
encrypted secrets off disk, as we may have a mixture during the
transitional times of an upgrade.  Whether we proactively encrypt
all plain text secrets at startup, or simply always read them on
startup in whatever format they're stored is a choice we have.

Not immediately encrypting all plain text secrets gives the ability
to downgrade libvirt again without loosing access to previously
created  secrets on disk. So I think that's marginally better.

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 13, 2025 at 14:48:12 +0000, Daniel P. Berrangé wrote:
> On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
> > On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
> > > A new configuration file called secrets.conf is introduced to
> > > let the user configure the path to the master 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.
> > > The virtsecretd driver checks if the credentials are available
> > > in the CREDENTIALS_DIRECTORY. In case it is not present, then the
> > > user is expected to provide the encryption key path in secrets.conf
> 
> 
> > > diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
> > > new file mode 100644
> > > index 0000000000..80bb9654ce
> > > --- /dev/null
> > > +++ b/src/secret/secrets.conf.in
> > > @@ -0,0 +1,14 @@
> > > +#
> > > +# Master configuration file for the secrets driver.
> > > +#
> > > +
> > > +# The master encryption key is used to override default master encryption
> > > +# key path. The user can create an encryption key and set the master_encryption_key
> > > +# to the path on which it resides.
> > > +# The key must be atleast 32-bytes long.
> > > +#
> > > +# master_encryption_key = "/run/libvirt/secrets/master.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
> > 
> > As the default secret seems to be handed in via systemd, which will it
> > make available to any upgraded installation,  I don't think you can
> > unconditionally enable this option as it would break existing
> > un-encrypted secrets.
> 
> When I discussed the design with Arun offlist, my suggestion was that
> our goal should be to have encryption enabled out of the box on all
> systemd based hosts, with zero config by the admin.

I agree with that. We also ought to auto-generate a properly random
secret key on each host at installation time, otherwise it's unlikely
that an admin will change the key if we provide a default.

> This implies that we need the ability to read *both* plain text and
> encrypted secrets off disk, as we may have a mixture during the
> transitional times of an upgrade.  Whether we proactively encrypt
> all plain text secrets at startup, or simply always read them on
> startup in whatever format they're stored is a choice we have.
> 
> Not immediately encrypting all plain text secrets gives the ability
> to downgrade libvirt again without loosing access to previously
> created  secrets on disk. So I think that's marginally better.

Encrypting non-encrypted secrets will still require libvirt knowing
whether they are encrypted or not, so it's simpler to just leave them in
whatever state they were in. That information will need to be stored
somewhere out-of-band (e.g. by the filename or location of the file) in
order to properly detect the current state.

The config setting flag will then influence the format only when storign
the secret.

On Thu, Nov 13, 2025 at 03:57:32PM +0100, Peter Krempa wrote:
> On Thu, Nov 13, 2025 at 14:48:12 +0000, Daniel P. Berrangé wrote:
> > On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
> > > On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
> > > > A new configuration file called secrets.conf is introduced to
> > > > let the user configure the path to the master 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.
> > > > The virtsecretd driver checks if the credentials are available
> > > > in the CREDENTIALS_DIRECTORY. In case it is not present, then the
> > > > user is expected to provide the encryption key path in secrets.conf
> > 
> > 
> > > > diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
> > > > new file mode 100644
> > > > index 0000000000..80bb9654ce
> > > > --- /dev/null
> > > > +++ b/src/secret/secrets.conf.in
> > > > @@ -0,0 +1,14 @@
> > > > +#
> > > > +# Master configuration file for the secrets driver.
> > > > +#
> > > > +
> > > > +# The master encryption key is used to override default master encryption
> > > > +# key path. The user can create an encryption key and set the master_encryption_key
> > > > +# to the path on which it resides.
> > > > +# The key must be atleast 32-bytes long.
> > > > +#
> > > > +# master_encryption_key = "/run/libvirt/secrets/master.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
> > > 
> > > As the default secret seems to be handed in via systemd, which will it
> > > make available to any upgraded installation,  I don't think you can
> > > unconditionally enable this option as it would break existing
> > > un-encrypted secrets.
> > 
> > When I discussed the design with Arun offlist, my suggestion was that
> > our goal should be to have encryption enabled out of the box on all
> > systemd based hosts, with zero config by the admin.
> 
> I agree with that. We also ought to auto-generate a properly random
> secret key on each host at installation time, otherwise it's unlikely
> that an admin will change the key if we provide a default.

Yes, I've just replied to that effect on the previous patch.

> > This implies that we need the ability to read *both* plain text and
> > encrypted secrets off disk, as we may have a mixture during the
> > transitional times of an upgrade.  Whether we proactively encrypt
> > all plain text secrets at startup, or simply always read them on
> > startup in whatever format they're stored is a choice we have.
> > 
> > Not immediately encrypting all plain text secrets gives the ability
> > to downgrade libvirt again without loosing access to previously
> > created  secrets on disk. So I think that's marginally better.
> 
> Encrypting non-encrypted secrets will still require libvirt knowing
> whether they are encrypted or not, so it's simpler to just leave them in
> whatever state they were in. That information will need to be stored
> somewhere out-of-band (e.g. by the filename or location of the file) in
> order to properly detect the current state.

We should be using a different file extension for the plain text vs
cipher text secrets to make this unambiguous at a glance.

> The config setting flag will then influence the format only when storign
> the secret.

Yep.

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 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
> On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
> > A new configuration file called secrets.conf is introduced to
> > let the user configure the path to the master 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.
> > The virtsecretd driver checks if the credentials are available
> > in the CREDENTIALS_DIRECTORY. In case it is not present, then the
> > user is expected to provide the encryption key path in secrets.conf
> 
> Is there any plan to be able to pass the secret do the secrets
> driver/daemon in an ephemeral way?
> 
> Because both the systemd secrets and the config file seem to just store
> it on the same host. Thus for root-owned files it's just a slightly
> bigger hurdle rather than any real security.

IIUC in the systemd case, the credentials on disk are only visible
in mount namespace given to the service. I've not checked, but I
would imagine the dir is backed with tmpfs.

IOW, the plain text secret should be inaccessible to anything
else on the host, unless they have fully privileged root account
access needed to join the mount namespace.

The service can delete the creds file from this mount location
when it has been loaded, so the window of availability is only
the startup  sequence.

None the less any app can access /proc/$PID/mem and fetch the
secrets from memory, if they have full root privileges no matter
how we pass the secret across to virtsecretd.

An alternative in the non-systemd case would be to pass the
creds in via a FIFO, so it is read-once and not actually on
even a temporary disk mount. 

> > diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in
> > new file mode 100644
> > index 0000000000..80bb9654ce
> > --- /dev/null
> > +++ b/src/secret/secrets.conf.in
> > @@ -0,0 +1,14 @@
> > +#
> > +# Master configuration file for the secrets driver.
> > +#
> > +
> > +# The master encryption key is used to override default master encryption
> > +# key path. The user can create an encryption key and set the master_encryption_key
> > +# to the path on which it resides.
> > +# The key must be atleast 32-bytes long.
> > +#
> > +# master_encryption_key = "/run/libvirt/secrets/master.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
> 
> As the default secret seems to be handed in via systemd, which will it
> make available to any upgraded installation,  I don't think you can
> unconditionally enable this option as it would break existing
> un-encrypted secrets.
> 

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