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 <armenon@redhat.com> --- 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.