On Tue, Dec 09, 2025 at 01:22:29 +0530, Arun Menon via Devel wrote:
> A new configuration file called secret.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.
>
> 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]>
> ---
> include/libvirt/virterror.h | 1 +
> libvirt.spec.in | 3 +
> po/POTFILES | 1 +
> src/secret/libvirt_secrets.aug | 40 ++++++
> src/secret/meson.build | 19 +++
> src/secret/secret.conf.in | 14 ++
> src/secret/secret_config.c | 171 +++++++++++++++++++++++++
> src/secret/secret_config.h | 40 ++++++
> src/secret/secret_driver.c | 9 ++
> src/secret/test_libvirt_secrets.aug.in | 6 +
> src/util/virerror.c | 3 +
> 11 files changed, 307 insertions(+)
> create mode 100644 src/secret/libvirt_secrets.aug
> create mode 100644 src/secret/secret.conf.in
> create mode 100644 src/secret/secret_config.c
> create mode 100644 src/secret/secret_config.h
> create mode 100644 src/secret/test_libvirt_secrets.aug.in
Build fails after this patch with:
' '-Dabs_top_srcdir="/home/pipo/libvirt"' -MD -MQ
src/libvirt_driver_secret.so.p/secret_secret_driver.c.o -MF
src/libvirt_driver_secret.so.p/secret_secret_driver.c.o.d -o
src/libvirt_driver_secret.so.p/secret_secret_driver.c.o -c
../../../libvirt/src/secret/secret_driver.c
../../../libvirt/src/secret/secret_driver.c:75:5: error: unknown type name
‘virSecretDaemonConfig’
75 | virSecretDaemonConfig *config;
| ^~~~~~~~~~~~~~~~~~~~~
../../../libvirt/src/secret/secret_driver.c: In function
‘secretStateInitialize’:
../../../libvirt/src/secret/secret_driver.c:525:28: error: implicit declaration
of function ‘virSecretDaemonConfigNew’ [-Wimplicit-function-declaration]
525 | if (!(driver->config =
virSecretDaemonConfigNew(driver->privileged)))
| ^~~~~~~~~~~~~~~~~~~~~~~~
../../../libvirt/src/secret/secret_driver.c:525:28: error: nested extern
declaration of ‘virSecretDaemonConfigNew’ [-Werror=nested-externs]
../../../libvirt/src/secret/secret_driver.c:525:26: error: assignment to ‘int
*’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
525 | if (!(driver->config =
virSecretDaemonConfigNew(driver->privileged)))
| ^
../../../libvirt/src/secret/secret_driver.c: In function ‘secretStateReload’:
../../../libvirt/src/secret/secret_driver.c:562:26: error: assignment to ‘int
*’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
562 | if (!(driver->config =
virSecretDaemonConfigNew(driver->privileged)))
| ^
Since it succeeds after the last patch you likely misplaced some header
inclusion into the wrong patch.
[...]
> diff --git a/src/secret/secret_config.c b/src/secret/secret_config.c
> new file mode 100644
> index 0000000000..b63567944e
> --- /dev/null
> +++ b/src/secret/secret_config.c
[...]
> +
> +
> +static int virGetSecretsEncryptionKey(virSecretDaemonConfig *cfg,
> + uint8_t **secretsEncryptionKey,
> + size_t *secretsKeyLen)
inconsistent function header formatting
> +{
> + VIR_AUTOCLOSE fd = -1;
> + int encryption_key_length;
> +
> + if ((encryption_key_length =
> virFileReadAll(cfg->secretsEncryptionKeyPath, VIR_SECRETS_ENCRYPTION_KEY_LEN,
> (char**)secretsEncryptionKey)) < 0) {
break the very long line on argument boundaries
> + return -1;
> + }
> + if (encryption_key_length != VIR_SECRETS_ENCRYPTION_KEY_LEN) {
> + virReportError(VIR_ERR_INVALID_ENCR_KEY_SECRET,
> + _("Encryption key length must be '%1$d' '%2$s'"),
> + VIR_SECRETS_ENCRYPTION_KEY_LEN,
> cfg->secretsEncryptionKeyPath);
> + }
> +
> + *secretsKeyLen = (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;
all of 'cfg' ought to be initialized to NULL at this point so this isn't
needed.
> +
> + 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);
> + if (!virFileExists(cfg->secretsEncryptionKeyPath)) {
> + g_clear_pointer(&cfg->secretsEncryptionKeyPath, g_free);
> + }
> + }
> +
> + if (!cfg->encryptDataWasSet) {
> + if (!cfg->secretsEncryptionKeyPath) {
> + /* No path specified by user or environment, disable encryption
> */
> + cfg->encryptData = false;
> + } else {
> + cfg->encryptData = true;
> + }
> + } else {
> + if (cfg->encryptData) {
> + if (!cfg->secretsEncryptionKeyPath) {
> + /* Built-in default path must be used */
> + cfg->secretsEncryptionKeyPath =
> g_strdup("/run/libvirt/secrets/encryption-key");
This ought to use some form of config-time variable to place it
correctly, otherwise it won't respect users' build-time options.
> + }
> + }
> + }
> + VIR_DEBUG("Secrets encryption key path: %s",
> NULLSTR(cfg->secretsEncryptionKeyPath));
> +
> + if (cfg->encryptData) {
> + if (virGetSecretsEncryptionKey(cfg, &cfg->secretsEncryptionKey,
> &cfg->secretsKeyLen) < 0) {
> + return NULL;
> + }
> + }
> + return g_steal_pointer(&cfg);
> +}
[...]
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 04c3ca49f1..408a629ea0 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -70,6 +70,9 @@ struct _virSecretDriverState {
>
> /* Immutable pointer, self-locking APIs */
> virInhibitor *inhibitor;
> +
> + /* Settings from secret.conf file */
> + virSecretDaemonConfig *config;
> };
Please use the annotation about how to access the data (as all other
values above have).
In this case I expect that the correct annotation is:
/* Require lock to get reference on 'config',
* then lockless thereafter */
same as the 'config' member of struct _virQEMUDriver.
