[RFC v2 0/5] cover: RFE libvirt secret encryption on disk
Libvirt secrets are stored unencrypted on the disk. With this series we want to start encrypting the secrets. 1. Introduce the GnuTLS decryption wrapper functions that work exact opposite to the encryption wrappers. 2. Add a new service called virt-secrets-init-encryption, that is linked to the virtsecretd service. virtsecretd service only starts after the new service generates a random encryption key. 3. Add a new secrets.conf configuration file that helps user to set a. secrets_encryption_key - allows the user to specify the encryption key file path, in case the default key is not to be used. b. encrypt_data - set to 0 or 1. If set to 1, then the newly added secrets will be encrypted. 4. Add functionality to store the encryption scheme (none, aes256cbc, etc.) to the disk. This will be helpful during service restarts or migrating from an older version. Depending on the scheme, the secrets will be reloaded to the daemon. If no scheme is present in the xml configuration, then the secrets will be stored/loaded in the default base64 encoded format. 5. Once we have the encryption key, and a reliable way to tell the daemon what encryption scheme the secret object is using, we can encrypt the secrets on disk and store them in <uuid>.<encryption_scheme> format. It is important to note that if the encryption key is changed between restarts, then the respective secret will not be loaded by the driver. This is a sincere attempt to improve upon the already submitted patch https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/KE6GV... Resolves: https://issues.redhat.com/browse/RHEL-7125 --- Changes in v2: - Corrected the encryption key length check. It should be 32. - Added a new patch that introduces the encryption scheme attribute. This will help us identify which secrets are encrypted. - A new systemd unit service file added that starts before virtsecretd, helping us to construct a random encryption key and pass it to the virtsecretd service. - Parsing logic of secrets.conf moved to a separate file. - Spec file changes, augeas. Arun Menon (5): util: Add support for GnuTLS decryption secret: Set up default encrypted secret key for the virtsecretd service secret: Add secrets.conf configuration file and parse it secret: Add encryptionScheme attribute to the secrets xml configuration secret: Add functionality to load and save secrets in encrypted format include/libvirt/libvirt-secret.h | 20 ++ libvirt.spec.in | 10 + po/POTFILES | 1 + src/conf/meson.build | 1 + src/conf/schemas/secret.rng | 5 + src/conf/secret_conf.c | 21 +++ src/conf/secret_conf.h | 1 + src/conf/secret_config.c | 207 +++++++++++++++++++++ src/conf/secret_config.h | 48 +++++ src/conf/virsecretobj.c | 165 ++++++++++++---- src/conf/virsecretobj.h | 10 +- src/libvirt_private.syms | 3 + src/secret/libvirt_secrets.aug | 40 ++++ src/secret/meson.build | 26 +++ src/secret/secret-init-encryption.in | 11 ++ src/secret/secret_driver.c | 23 ++- src/secret/secrets.conf.in | 12 ++ src/secret/test_libvirt_secrets.aug.in | 6 + src/secret/virtsecretd.service.extra.in | 8 + src/util/vircrypto.c | 128 ++++++++++++- src/util/vircrypto.h | 8 + src/util/virsecret.c | 4 + src/util/virsecret.h | 1 + tests/secretxml2xmlin/usage-ceph-space.xml | 1 + tests/secretxml2xmlin/usage-ceph.xml | 1 + tests/secretxml2xmlin/usage-iscsi.xml | 1 + tests/secretxml2xmlin/usage-tls.xml | 1 + tests/secretxml2xmlin/usage-volume.xml | 1 + tests/secretxml2xmlin/usage-vtpm.xml | 1 + tests/vircryptotest.c | 65 +++++++ 30 files changed, 788 insertions(+), 42 deletions(-) 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/secret-init-encryption.in create mode 100644 src/secret/secrets.conf.in create mode 100644 src/secret/test_libvirt_secrets.aug.in -- 2.51.1
Adds `virCryptoDecryptDataAESgnutls` and `virCryptoDecryptData` as wrapper functions for GnuTLS decryption. These functions are the inverse of the existing GnuTLS encryption wrappers. This commit also includes a corresponding test case to validate data decryption. Signed-off-by: Arun Menon <armenon@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircrypto.c | 128 ++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 8 +++ tests/vircryptotest.c | 65 ++++++++++++++++++++ 4 files changed, 201 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb482fff40..fc5fdb00f4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2252,6 +2252,7 @@ virConfWriteMem; # util/vircrypto.h +virCryptoDecryptData; virCryptoEncryptData; virCryptoHashBuf; virCryptoHashString; diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 3ce23264ca..fedb39b167 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -98,7 +98,7 @@ virCryptoHashString(virCryptoHash hash, } -/* virCryptoEncryptDataAESgntuls: +/* virCryptoEncryptDataAESgnutls: * * Performs the AES gnutls encryption * @@ -233,3 +233,129 @@ virCryptoEncryptData(virCryptoCipher algorithm, _("algorithm=%1$d is not supported"), algorithm); return -1; } + +/* virCryptoDecryptDataAESgnutls: + * + * Performs the AES gnutls decryption + * + * Same input as virCryptoDecryptData, except the algorithm is replaced + * by the specific gnutls algorithm. + * + * Decrypts the @data buffer using the @deckey and if available the @iv + * + * Returns 0 on success with the plaintext being filled. It is the + * caller's responsibility to clear and free it. Returns -1 on failure + * w/ error set. + */ +static int +virCryptoDecryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_dec_alg, + uint8_t *deckey, + size_t deckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **plaintextret, + size_t *plaintextlenret) +{ + int rc; + size_t i; + gnutls_cipher_hd_t handle = NULL; + gnutls_datum_t dec_key = { .data = deckey, .size = deckeylen }; + gnutls_datum_t iv_buf = { .data = iv, .size = ivlen }; + g_autofree uint8_t *plaintext = NULL; + size_t plaintextlen; + + if ((rc = gnutls_cipher_init(&handle, gnutls_dec_alg, + &dec_key, &iv_buf)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%1$s'"), + gnutls_strerror(rc)); + return -1; + } + + plaintext = g_memdup2(data, datalen); + plaintextlen = datalen; + + rc = gnutls_cipher_decrypt(handle, plaintext, plaintextlen); + gnutls_cipher_deinit(handle); + if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to decrypt the data: '%1$s'"), + gnutls_strerror(rc)); + goto error; + } + if (plaintextlen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has zero length")); + goto error; + } + i = plaintext[plaintextlen - 1]; + if (i > plaintextlen) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has invalid padding")); + goto error; + } + *plaintextlenret = plaintextlen - i; + *plaintextret = g_steal_pointer(&plaintext); + return 0; + error: + virSecureErase(plaintext, plaintextlen); + return -1; +} + +/* virCryptoDecryptData: + * @algorithm: algorithm desired for decryption + * @deckey: decryption key + * @deckeylen: decryption key length + * @iv: initialization vector + * @ivlen: length of initialization vector + * @data: data to decrypt + * @datalen: length of data + * @plaintext: stream of bytes allocated to store plaintext + * @plaintextlen: size of the stream of bytes + * Returns 0 on success, -1 on failure with error set + */ +int +virCryptoDecryptData(virCryptoCipher algorithm, + uint8_t *deckey, + size_t deckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **plaintext, + size_t *plaintextlen) +{ + switch (algorithm) { + case VIR_CRYPTO_CIPHER_AES256CBC: + if (deckeylen != 32) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC decryption invalid keylen=%1$zu"), + deckeylen); + return -1; + } + if (ivlen != 16) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC initialization vector invalid len=%1$zu"), + ivlen); + return -1; + } + /* + * Decrypt the data buffer using a decryption key and + * initialization vector via the gnutls_cipher_decrypt API + * for GNUTLS_CIPHER_AES_256_CBC. + */ + return virCryptoDecryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC, + deckey, deckeylen, iv, ivlen, + data, datalen, + plaintext, plaintextlen); + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + } + + virReportError(VIR_ERR_INVALID_ARG, + _("algorithm=%1$d is not supported"), algorithm); + return -1; +} diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h index 5f079ac335..2e8557839d 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -61,3 +61,11 @@ int virCryptoEncryptData(virCryptoCipher algorithm, uint8_t **ciphertext, size_t *ciphertextlen) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) G_GNUC_WARN_UNUSED_RESULT; + +int virCryptoDecryptData(virCryptoCipher algorithm, + uint8_t *deckey, size_t deckeylen, + uint8_t *iv, size_t ivlen, + uint8_t *data, size_t datalen, + uint8_t **plaintext, size_t *plaintextlen) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) G_GNUC_WARN_UNUSED_RESULT; diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index 9ffe70756e..864fa8838d 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -62,6 +62,14 @@ struct testCryptoEncryptData { size_t ciphertextlen; }; +struct testCryptoDecryptData { + virCryptoCipher algorithm; + uint8_t *input; + size_t inputlen; + uint8_t *plaintext; + size_t plaintextlen; +}; + static int testCryptoEncrypt(const void *opaque) { @@ -101,6 +109,44 @@ testCryptoEncrypt(const void *opaque) return 0; } +static int +testCryptoDecrypt(const void *opaque) +{ + const struct testCryptoDecryptData *data = opaque; + g_autofree uint8_t *deckey = NULL; + size_t deckeylen = 32; + g_autofree uint8_t *iv = NULL; + size_t ivlen = 16; + g_autofree uint8_t *plaintext = NULL; + size_t plaintextlen = 0; + + deckey = g_new0(uint8_t, deckeylen); + iv = g_new0(uint8_t, ivlen); + + if (virRandomBytes(deckey, deckeylen) < 0 || + virRandomBytes(iv, ivlen) < 0) { + fprintf(stderr, "Failed to generate random bytes\n"); + return -1; + } + + if (virCryptoDecryptData(data->algorithm, deckey, deckeylen, iv, ivlen, + data->input, data->inputlen, + &plaintext, &plaintextlen) < 0) + return -1; + + if (data->plaintextlen != plaintextlen) { + fprintf(stderr, "Expected plaintexlen(%zu) doesn't match (%zu)\n", + data->plaintextlen, plaintextlen); + return -1; + } + + if (memcmp(data->plaintext, plaintext, plaintextlen)) { + fprintf(stderr, "Expected plaintext doesn't match\n"); + return -1; + } + + return 0; +} static int mymain(void) @@ -155,7 +201,26 @@ mymain(void) #undef VIR_CRYPTO_ENCRYPT +#define VIR_CRYPTO_DECRYPT(a, n, i, il, c, cl) \ + do { \ + struct testCryptoDecryptData data = { \ + .algorithm = a, \ + .input = i, \ + .inputlen = il, \ + .plaintext = c, \ + .plaintextlen = cl, \ + }; \ + if (virTestRun("Decrypt " n, testCryptoDecrypt, &data) < 0) \ + ret = -1; \ + } while (0) + + VIR_CRYPTO_DECRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes256cbc", + expected_ciphertext, 16, secretdata, 7); + +#undef VIR_CRYPTO_DECRYPT + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; + } /* Forces usage of not so random virRandomBytes */ -- 2.51.1
On Thu, Nov 20, 2025 at 22:23:42 +0530, Arun Menon via Devel wrote:
Adds `virCryptoDecryptDataAESgnutls` and `virCryptoDecryptData` as wrapper functions for GnuTLS decryption.
These functions are the inverse of the existing GnuTLS encryption wrappers. This commit also includes a corresponding test case to validate data decryption.
Signed-off-by: Arun Menon <armenon@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircrypto.c | 128 ++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 8 +++ tests/vircryptotest.c | 65 ++++++++++++++++++++ 4 files changed, 201 insertions(+), 1 deletion(-)
@@ -233,3 +233,129 @@ virCryptoEncryptData(virCryptoCipher algorithm, _("algorithm=%1$d is not supported"), algorithm); return -1; } + +/* virCryptoDecryptDataAESgnutls: + * + * Performs the AES gnutls decryption + * + * Same input as virCryptoDecryptData, except the algorithm is replaced + * by the specific gnutls algorithm. + * + * Decrypts the @data buffer using the @deckey and if available the @iv + * + * Returns 0 on success with the plaintext being filled. It is the + * caller's responsibility to clear and free it. Returns -1 on failure + * w/ error set. + */ +static int +virCryptoDecryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_dec_alg, + uint8_t *deckey, + size_t deckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **plaintextret, + size_t *plaintextlenret) +{ + int rc; + size_t i;
The 'i' variable isn't used as an iterator in a loop. Please give it a proper name.
+ gnutls_cipher_hd_t handle = NULL; + gnutls_datum_t dec_key = { .data = deckey, .size = deckeylen }; + gnutls_datum_t iv_buf = { .data = iv, .size = ivlen }; + g_autofree uint8_t *plaintext = NULL; + size_t plaintextlen; + + if ((rc = gnutls_cipher_init(&handle, gnutls_dec_alg, + &dec_key, &iv_buf)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%1$s'"), + gnutls_strerror(rc)); + return -1; + } + + plaintext = g_memdup2(data, datalen); + plaintextlen = datalen;
So 'plaintextlen' is assigned from 'datalen'. 'datalen' wasn't modified so is the same as the argument.
+ rc = gnutls_cipher_decrypt(handle, plaintext, plaintextlen);
'plaintextlen' is passed by value so it can't be modified by 'gnutls_cipher_decrypt'.
+ gnutls_cipher_deinit(handle); + if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to decrypt the data: '%1$s'"), + gnutls_strerror(rc)); + goto error; + } + if (plaintextlen == 0) {
'plaintextlen' still wasn't modified and the value you are checking against was known from the beginning, so do this check right at the beginning.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has zero length")); + goto error; + }
+ i = plaintext[plaintextlen - 1];
So 'i' should be called 'paddinglen' and also be of 'uint8_t' type. Also here it'd be worth commenting why we're looking at the last byte to determine the padding lenght.
+ if (i > plaintextlen) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
Failed padding isn't IMO an internal error any more since it's data that can originate from somewhere else.
+ _("decrypted data has invalid padding")); + goto error; + } + *plaintextlenret = plaintextlen - i;
This at the first glance looked like plaintextlen - 1. So 'i' is not only wrong semantically but also can look confusing.
+ *plaintextret = g_steal_pointer(&plaintext); + return 0; + error: + virSecureErase(plaintext, plaintextlen); + return -1; +} + +/* virCryptoDecryptData: + * @algorithm: algorithm desired for decryption + * @deckey: decryption key + * @deckeylen: decryption key length + * @iv: initialization vector + * @ivlen: length of initialization vector + * @data: data to decrypt + * @datalen: length of data + * @plaintext: stream of bytes allocated to store plaintext + * @plaintextlen: size of the stream of bytes + * Returns 0 on success, -1 on failure with error set + */ +int +virCryptoDecryptData(virCryptoCipher algorithm, + uint8_t *deckey, + size_t deckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **plaintext, + size_t *plaintextlen) +{ + switch (algorithm) { + case VIR_CRYPTO_CIPHER_AES256CBC: + if (deckeylen != 32) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC decryption invalid keylen=%1$zu"), + deckeylen); + return -1; + } + if (ivlen != 16) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC initialization vector invalid len=%1$zu"), + ivlen); + return -1; + } + /* + * Decrypt the data buffer using a decryption key and + * initialization vector via the gnutls_cipher_decrypt API + * for GNUTLS_CIPHER_AES_256_CBC. + */
This comment doesn't really explain anything which wouldn't be obvious from what you're calling here. In contrast e.g. the padding lenght determination may not be obvious to the reader which would make a much better use of a comment.
+ return virCryptoDecryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC, + deckey, deckeylen, iv, ivlen, + data, datalen, + plaintext, plaintextlen); + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + } + + virReportError(VIR_ERR_INVALID_ARG, + _("algorithm=%1$d is not supported"), algorithm); + return -1; +}
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 <armenon@redhat.com> --- 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 %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': [], + } + 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 + +[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)' +ExecStart=-/usr/bin/chmod 0400 /var/lib/libvirt/secrets/secrets-encryption-key + +[Install] +WantedBy=multi-user.target diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..1df9163814 100644 --- a/src/secret/virtsecretd.service.extra.in +++ b/src/secret/virtsecretd.service.extra.in @@ -1,2 +1,10 @@ # The contents of this unit will be merged into a base template. # Additional units might be merged as well. See meson.build for details. +# +[Unit] +Requires=virt-secret-init-encryption.service +After=virt-secret-init-encryption.service + +[Service] +LoadCredentialEncrypted=secrets-encryption-key:/var/lib/libvirt/secrets/secrets-encryption-key +Environment=SECRETS_ENCRYPTION_KEY=%d/secrets-encryption-key -- 2.51.1
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 <armenon@redhat.com> --- 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.
+ 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.
+ +[Install] +WantedBy=multi-user.target diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..1df9163814 100644 --- a/src/secret/virtsecretd.service.extra.in +++ b/src/secret/virtsecretd.service.extra.in @@ -1,2 +1,10 @@ # The contents of this unit will be merged into a base template. # Additional units might be merged as well. See meson.build for details. +# +[Unit] +Requires=virt-secret-init-encryption.service +After=virt-secret-init-encryption.service + +[Service] +LoadCredentialEncrypted=secrets-encryption-key:/var/lib/libvirt/secrets/secrets-encryption-key +Environment=SECRETS_ENCRYPTION_KEY=%d/secrets-encryption-key -- 2.51.1
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 <armenon@redhat.com> --- 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 :|
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 <armenon@redhat.com> --- libvirt.spec.in | 3 + po/POTFILES | 1 + src/conf/meson.build | 1 + src/conf/secret_config.c | 207 +++++++++++++++++++++++++ src/conf/secret_config.h | 48 ++++++ 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, 338 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 fa477db031..8462d08c61 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -2249,6 +2249,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 23da794f84..1a76e0505a 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..a1c9b6bc2f --- /dev/null +++ b/src/conf/secret_config.c @@ -0,0 +1,207 @@ +/* + * secret_config.c: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> +#include <fcntl.h> +#include "configmake.h" +#include "datatypes.h" +#include "virlog.h" +#include "virerror.h" +#include "virfile.h" +#include "virutil.h" +#include "secret_config.h" + + +#define VIR_FROM_THIS VIR_FROM_CONF + +VIR_LOG_INIT("secret.secret_config"); + +static virClass *virSecretDaemonConfigClass; +static void virSecretDaemonConfigDispose(void *obj); + +static int +virSecretConfigOnceInit(void) +{ + if (!VIR_CLASS_NEW(virSecretDaemonConfig, virClassForObject())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSecretConfig); + +int +virSecretDaemonConfigFilePath(bool privileged, char **configfile) +{ + if (privileged) { + *configfile = g_strdup(SYSCONFDIR "/libvirt/secrets.conf"); + } else { + g_autofree char *configdir = NULL; + + configdir = virGetUserConfigDirectory(); + + *configfile = g_strdup_printf("%s/secrets.conf", configdir); + } + + return 0; +} + +static int +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg, + const char *filename) +{ + g_autoptr(virConf) conf = NULL; + + if (access(filename, R_OK) == 0) { + conf = virConfReadFile(filename, 0); + if (!conf) + return -1; + if (virConfGetValueInt(conf, "encrypt_data", &cfg->encrypt_data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get encrypt_data from %1$s"), + filename); + return -1; + } + + if (virConfGetValueString(conf, "secrets_encryption_key", + &cfg->secretsEncryptionKeyPath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get secrets_encryption_key from %1$s"), + filename); + return -1; + } + } + return 0; +} + +static bool getSecretsEncryptionKey(virSecretDaemonConfig *cfg, + uint8_t **secrets_encryption_key, size_t *secrets_encryption_keylen) +{ + int fd = -1; + struct stat st; + + if ((fd = open(cfg->secretsEncryptionKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + return false; + } + if (fstat(fd, &st) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *secrets_encryption_keylen = st.st_size; + if (*secrets_encryption_keylen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s is empty"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *secrets_encryption_key = g_new0(uint8_t, *secrets_encryption_keylen); + if (saferead(fd, &secrets_encryption_key, *secrets_encryption_keylen) != *secrets_encryption_keylen) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + VIR_FORCE_CLOSE(fd); + if (*secrets_encryption_keylen != 32) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s must be 32 bytes"), + cfg->secretsEncryptionKeyPath); + return false; + } + return true; +} + +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) + goto error; + + if (!(cfg = virObjectNew(virSecretDaemonConfigClass))) + goto error; + + cfg->secretsEncryptionKeyPath = NULL; + + if (privileged) { + configdir = g_strdup(SYSCONFDIR "/libvirt"); + } else { + g_autofree char *rundir = virGetUserRuntimeDirectory(); + configdir = virGetUserConfigDirectory(); + } + configfile = g_strconcat(configdir, "/secrets.conf", NULL); + + if (virSecretLoadDaemonConfig(cfg, configfile) < 0) + goto error; + + if (!(credentials_directory = getenv("CREDENTIALS_DIRECTORY"))) { + credentials_directory = NULL; + } + + if (cfg->secretsEncryptionKeyPath) { + VIR_DEBUG("Secrets encryption key path: %s", cfg->secretsEncryptionKeyPath); + } else if (credentials_directory) { + VIR_DEBUG("Using credentials directory from environment: %s", + credentials_directory); + cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", + credentials_directory); + } else { + VIR_DEBUG("No secrets encryption key found in credentials directory"); + cfg->secretsEncryptionKeyPath = NULL; + } + if (cfg->secretsEncryptionKeyPath && access(cfg->secretsEncryptionKeyPath, R_OK) == 0) { + if (!getSecretsEncryptionKey(cfg, &cfg->secrets_encryption_key, &cfg->secretsKeyLen)) { + VIR_DEBUG("Failed to get secrets encryption key from path: %s", + cfg->secretsEncryptionKeyPath); + goto error; + } + } + + if (cfg->encrypt_data != 1) { + cfg->encrypt_data = (cfg->secretsKeyLen == 32) ? 1 : 0; + } else if (cfg->encrypt_data == 1) { + if (!cfg->secretsEncryptionKeyPath) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("secretsEncryptionKeyPath must be set if encrypt_data is 1 in %1$s"), + configfile); + goto error; + } + } + return g_steal_pointer(&cfg); + error: + virSecretDaemonConfigDispose(cfg); + return NULL; +} + +static void +virSecretDaemonConfigDispose(void *obj) +{ + virSecretDaemonConfig *cfg = obj; + + g_free(cfg->secrets_encryption_key); + g_free(cfg->secretsEncryptionKeyPath); +} diff --git a/src/conf/secret_config.h b/src/conf/secret_config.h new file mode 100644 index 0000000000..638b7c49a4 --- /dev/null +++ b/src/conf/secret_config.h @@ -0,0 +1,48 @@ +/* + * secret_config.h: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" +#include "virinhibitor.h" +#include "secret_event.h" + +typedef struct _virSecretDaemonConfig virSecretDaemonConfig; +struct _virSecretDaemonConfig { + virObject parent; + /* secrets encryption key path from secrets.conf file */ + char *secretsEncryptionKeyPath; + + unsigned char* secrets_encryption_key; + size_t secretsKeyLen; + + /* Indicates if the newly written secrets are encrypted or not. + * 0 if not encrypted and 1 if encrypted. + */ + int encrypt_data; +}; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecretDaemonConfig, virObjectUnref); + +int virSecretDaemonConfigFilePath(bool privileged, char **configfile); +virSecretDaemonConfig *virSecretDaemonConfigNew(bool privileged); +int virSecretDaemonConfigLoadFile(virSecretDaemonConfig *data, + const char *filename, + bool allow_missing); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc5fdb00f4..7ecb573851 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1064,6 +1064,8 @@ virSecretDefParse; virSecretUsageTypeFromString; virSecretUsageTypeToString; +# conf/secret_config.h +virSecretDaemonConfigNew; # conf/secret_event.h virSecretEventLifecycleNew; diff --git a/src/secret/libvirt_secrets.aug b/src/secret/libvirt_secrets.aug new file mode 100644 index 0000000000..092cdef41f --- /dev/null +++ b/src/secret/libvirt_secrets.aug @@ -0,0 +1,40 @@ +(* /etc/libvirt/secrets.conf *) + +module Libvirt_secrets = + autoload xfm + + let eol = del /[ \t]*\n/ "\n" + let value_sep = del /[ \t]*=[ \t]*/ " = " + let indent = del /[ \t]*/ "" + + let array_sep = del /,[ \t\n]*/ ", " + let array_start = del /\[[ \t\n]*/ "[ " + let array_end = del /\]/ "]" + + let str_val = del /\"/ "\"" . store /[^\"]*/ . del /\"/ "\"" + let bool_val = store /0|1/ + let int_val = store /[0-9]+/ + let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ "" + let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end + + let str_entry (kw:string) = [ key kw . value_sep . str_val ] + let bool_entry (kw:string) = [ key kw . value_sep . bool_val ] + let int_entry (kw:string) = [ key kw . value_sep . int_val ] + let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ] + + let secrets_entry = str_entry "secrets_encryption_key" + | bool_entry "encrypt_data" + + (* Each entry in the config is one of the following three ... *) + let entry = secrets_entry + let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] + let empty = [ label "#empty" . eol ] + + let record = indent . entry . eol + + let lns = ( record | comment | empty ) * + + let filter = incl "/etc/libvirt/secrets.conf" + . Util.stdexcl + + let xfm = transform lns filter diff --git a/src/secret/meson.build b/src/secret/meson.build index d8861fcbcd..e453e71464 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -27,6 +27,24 @@ if conf.has('WITH_SECRETS') ], } + secrets_conf = configure_file( + input: 'secrets.conf.in', + output: 'secrets.conf', + copy: true + ) + virt_conf_files += secrets_conf + + virt_aug_files += files('libvirt_secrets.aug') + + virt_test_aug_files += { + 'name': 'test_libvirt_secrets.aug', + 'aug': files('test_libvirt_secrets.aug.in'), + 'conf': files('secrets.conf.in'), + 'test_name': 'libvirt_secrets', + 'test_srcdir': meson.current_source_dir(), + 'test_builddir': meson.current_build_dir(), + } + virt_daemon_confs += { 'name': 'virtsecretd', } diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..d998940140 --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,12 @@ +# +# Configuration file for the secrets driver. +# +# The secret encryption key is used to override default encryption +# key path. The user can create an encryption key and set the secret_encryption_key +# to the path on which it resides. +# The key must be 32-bytes long. +#secrets_encryption_key = "/run/libvirt/secrets/secret-encryption-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 diff --git a/src/secret/test_libvirt_secrets.aug.in b/src/secret/test_libvirt_secrets.aug.in new file mode 100644 index 0000000000..1bb205e0f2 --- /dev/null +++ b/src/secret/test_libvirt_secrets.aug.in @@ -0,0 +1,6 @@ +module Test_libvirt_secrets = + @CONFIG@ + + test Libvirt_secrets.lns get conf = +{ "secrets_encryption_key" = "/run/libvirt/secrets/secret-encryption-key" } +{ "encrypt_data" = "1" } -- 2.51.1
On Thu, Nov 20, 2025 at 22:23:44 +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 <armenon@redhat.com> --- libvirt.spec.in | 3 + po/POTFILES | 1 + src/conf/meson.build | 1 + src/conf/secret_config.c | 207 +++++++++++++++++++++++++ src/conf/secret_config.h | 48 ++++++ 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, 338 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/src/conf/secret_config.c b/src/conf/secret_config.c new file mode 100644 index 0000000000..a1c9b6bc2f --- /dev/null +++ b/src/conf/secret_config.c @@ -0,0 +1,207 @@ +/* + * secret_config.c: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */
For new files we usually use the SPDX header: /* * SPDX-License-Identifier: LGPL-2.1-or-later */ instead of the full blob above.
+ +#include <config.h> +#include <fcntl.h> +#include "configmake.h" +#include "datatypes.h" +#include "virlog.h" +#include "virerror.h" +#include "virfile.h" +#include "virutil.h" +#include "secret_config.h" + + +#define VIR_FROM_THIS VIR_FROM_CONF + +VIR_LOG_INIT("secret.secret_config"); + +static virClass *virSecretDaemonConfigClass; +static void virSecretDaemonConfigDispose(void *obj); + +static int +virSecretConfigOnceInit(void) +{ + if (!VIR_CLASS_NEW(virSecretDaemonConfig, virClassForObject())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSecretConfig); + +int +virSecretDaemonConfigFilePath(bool privileged, char **configfile) +{ + if (privileged) { + *configfile = g_strdup(SYSCONFDIR "/libvirt/secrets.conf"); + } else { + g_autofree char *configdir = NULL; + + configdir = virGetUserConfigDirectory(); + + *configfile = g_strdup_printf("%s/secrets.conf", configdir); + } + + return 0; +} + +static int +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg, + const char *filename) +{ + g_autoptr(virConf) conf = NULL; + + if (access(filename, R_OK) == 0) {
Use virFileExists instead of open-coding.
+ conf = virConfReadFile(filename, 0); + if (!conf) + return -1; + if (virConfGetValueInt(conf, "encrypt_data", &cfg->encrypt_data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get encrypt_data from %1$s"), + filename);
Not an internal error. VIR_ERR_CONF_SYNTAX Also encrypt_data really ought to be a bool based on usage. Here you'll have to convert it properly on parsing.
+ return -1; + } + + if (virConfGetValueString(conf, "secrets_encryption_key", + &cfg->secretsEncryptionKeyPath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get secrets_encryption_key from %1$s"), + filename);
ditto
+ return -1; + } + } + return 0; +} + +static bool getSecretsEncryptionKey(virSecretDaemonConfig *cfg, + uint8_t **secrets_encryption_key, size_t *secrets_encryption_keylen)
This function doesn't follow the naming convention and coding style.
+{ + int fd = -1;
Declare this using VIR_AUTOCLOSE so that you don't have to deal with it in this whole file.
+ struct stat st; + + if ((fd = open(cfg->secretsEncryptionKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + return false; + }
I'd suggest you factor out the useful bits from 'qemuDomainMasterKeyReadFile' and then reuse it rather than reimplement this
+ if (fstat(fd, &st) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd);
With the autoclose helper this won't be needed.
+ return false; + } + *secrets_encryption_keylen = st.st_size; + if (*secrets_encryption_keylen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s is empty"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *secrets_encryption_key = g_new0(uint8_t, *secrets_encryption_keylen);
This allocates buffer based on the actual size, IMO yoiu should limit it to something more sensible.
+ if (saferead(fd, &secrets_encryption_key, *secrets_encryption_keylen) != *secrets_encryption_keylen) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false;
For functions using libvirt errors please stick to the '0' '-1' return value convntion.
+ } + VIR_FORCE_CLOSE(fd); + if (*secrets_encryption_keylen != 32) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s must be 32 bytes"), + cfg->secretsEncryptionKeyPath); + return false; + } + return true; +} + +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) + goto error; + + if (!(cfg = virObjectNew(virSecretDaemonConfigClass))) + goto error; + + cfg->secretsEncryptionKeyPath = NULL; + + if (privileged) { + configdir = g_strdup(SYSCONFDIR "/libvirt"); + } else { + g_autofree char *rundir = virGetUserRuntimeDirectory(); + configdir = virGetUserConfigDirectory(); + } + configfile = g_strconcat(configdir, "/secrets.conf", NULL);
This open-codes what the unused 'virSecretDaemonConfigFilePath' function does. Either use the function or delete it.
+ if (virSecretLoadDaemonConfig(cfg, configfile) < 0) + goto error; + + if (!(credentials_directory = getenv("CREDENTIALS_DIRECTORY"))) { + credentials_directory = NULL;
This is a pointless condition. If getenv("CREDENTIALS_DIRECTORY") returns NULL you set it to NULL.
+ } + + if (cfg->secretsEncryptionKeyPath) { + VIR_DEBUG("Secrets encryption key path: %s", cfg->secretsEncryptionKeyPath); + } else if (credentials_directory) { + VIR_DEBUG("Using credentials directory from environment: %s", + credentials_directory); + cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", + credentials_directory); + } else { + VIR_DEBUG("No secrets encryption key found in credentials directory"); + cfg->secretsEncryptionKeyPath = NULL; + }
This logic is a bit convoluted. I suggest you do an unconditional log of the actual path after you determine whic one to use: if (!cfg->secretsEncryptionKeyPath && credentials_directory) { VIR_DEBUG("Using credentials directory from environment: %s", credentials_directory); cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", credentials_directory); } VIR_DEBUG("Secrets encryption key path: %s", NULLSTR(cfg->secretsEncryptionKeyPath));
+ if (cfg->secretsEncryptionKeyPath && access(cfg->secretsEncryptionKeyPath, R_OK) == 0) { + if (!getSecretsEncryptionKey(cfg, &cfg->secrets_encryption_key, &cfg->secretsKeyLen)) { + VIR_DEBUG("Failed to get secrets encryption key from path: %s", + cfg->secretsEncryptionKeyPath); + goto error;
You must properly report the error if you are to bail from daemon startup. Here !getSecretsEncryptionKey sets the value but it doesn't seem so because you do a pointless DEBUG (the error was already reported)
+ } + } + + if (cfg->encrypt_data != 1) { + cfg->encrypt_data = (cfg->secretsKeyLen == 32) ? 1 : 0;
So you set this to 1 even when the user set it to 0? Also 'encrypt_data' really ought to be a bool in the config struct. If users set this to 0 explicitly you shouldn't try to re-infer it. Here you also duplicate a magic constant used to limit the size so if anyone ever changes it in the other place only this will stop to work. If you want to automatically set this you'll also need to remember (possibly in another bool) if the explicit config option was seen or not.
+ } else if (cfg->encrypt_data == 1) { + if (!cfg->secretsEncryptionKeyPath) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("secretsEncryptionKeyPath must be set if encrypt_data is 1 in %1$s"), + configfile);
This isn't an internal error, it's user's wrong config. Also make it a standalone check rather than an else branch.
+ goto error; + } + } + return g_steal_pointer(&cfg); + error: + virSecretDaemonConfigDispose(cfg);
cfg is declared as autoptr so this is not needed as it ought to be called automatically. This also means that the whole 'error' label can be removed and 'NULL returned directly.
+ return NULL; +} + +static void +virSecretDaemonConfigDispose(void *obj) +{ + virSecretDaemonConfig *cfg = obj; + + g_free(cfg->secrets_encryption_key);
Use the function to securely clear it first, before freeing it here.
+ g_free(cfg->secretsEncryptionKeyPath); +} diff --git a/src/conf/secret_config.h b/src/conf/secret_config.h new file mode 100644 index 0000000000..638b7c49a4 --- /dev/null +++ b/src/conf/secret_config.h @@ -0,0 +1,48 @@ +/* + * secret_config.h: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" +#include "virinhibitor.h" +#include "secret_event.h" + +typedef struct _virSecretDaemonConfig virSecretDaemonConfig; +struct _virSecretDaemonConfig { + virObject parent; + /* secrets encryption key path from secrets.conf file */ + char *secretsEncryptionKeyPath; + + unsigned char* secrets_encryption_key;
unsigned char *sec...
+ size_t secretsKeyLen; + + /* Indicates if the newly written secrets are encrypted or not. + * 0 if not encrypted and 1 if encrypted. + */ + int encrypt_data;
This ought to be either a 'bool' or one of the tristates. Alternatively two bools.
+}; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecretDaemonConfig, virObjectUnref); + +int virSecretDaemonConfigFilePath(bool privileged, char **configfile); +virSecretDaemonConfig *virSecretDaemonConfigNew(bool privileged); +int virSecretDaemonConfigLoadFile(virSecretDaemonConfig *data, + const char *filename, + bool allow_missing); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
Hi Peter, Thank you for the review. On Fri, Nov 21, 2025 at 12:13:09PM +0100, Peter Krempa wrote:
On Thu, Nov 20, 2025 at 22:23:44 +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 <armenon@redhat.com> --- libvirt.spec.in | 3 + po/POTFILES | 1 + src/conf/meson.build | 1 + src/conf/secret_config.c | 207 +++++++++++++++++++++++++ src/conf/secret_config.h | 48 ++++++ 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, 338 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/src/conf/secret_config.c b/src/conf/secret_config.c new file mode 100644 index 0000000000..a1c9b6bc2f --- /dev/null +++ b/src/conf/secret_config.c @@ -0,0 +1,207 @@ +/* + * secret_config.c: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */
For new files we usually use the SPDX header:
/* * SPDX-License-Identifier: LGPL-2.1-or-later */
instead of the full blob above. Yes, will do. Thanks
+ +#include <config.h> +#include <fcntl.h> +#include "configmake.h" +#include "datatypes.h" +#include "virlog.h" +#include "virerror.h" +#include "virfile.h" +#include "virutil.h" +#include "secret_config.h" + + +#define VIR_FROM_THIS VIR_FROM_CONF + +VIR_LOG_INIT("secret.secret_config"); + +static virClass *virSecretDaemonConfigClass; +static void virSecretDaemonConfigDispose(void *obj); + +static int +virSecretConfigOnceInit(void) +{ + if (!VIR_CLASS_NEW(virSecretDaemonConfig, virClassForObject())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSecretConfig); + +int +virSecretDaemonConfigFilePath(bool privileged, char **configfile) +{ + if (privileged) { + *configfile = g_strdup(SYSCONFDIR "/libvirt/secrets.conf"); + } else { + g_autofree char *configdir = NULL; + + configdir = virGetUserConfigDirectory(); + + *configfile = g_strdup_printf("%s/secrets.conf", configdir); + } + + return 0; +} + +static int +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg, + const char *filename) +{ + g_autoptr(virConf) conf = NULL; + + if (access(filename, R_OK) == 0) {
Use virFileExists instead of open-coding. Yes, will do.
+ conf = virConfReadFile(filename, 0); + if (!conf) + return -1; + if (virConfGetValueInt(conf, "encrypt_data", &cfg->encrypt_data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get encrypt_data from %1$s"), + filename);
Not an internal error. VIR_ERR_CONF_SYNTAX yes, will change it.
Also encrypt_data really ought to be a bool based on usage. Here you'll have to convert it properly on parsing. I will change it to boolean.
+ return -1; + } + + if (virConfGetValueString(conf, "secrets_encryption_key", + &cfg->secretsEncryptionKeyPath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get secrets_encryption_key from %1$s"), + filename);
ditto
+ return -1; + } + } + return 0; +} + +static bool getSecretsEncryptionKey(virSecretDaemonConfig *cfg, + uint8_t **secrets_encryption_key, size_t *secrets_encryption_keylen)
This function doesn't follow the naming convention and coding style. I think virSecretGetEncryptionKey should be okay?
+{ + int fd = -1;
Declare this using VIR_AUTOCLOSE so that you don't have to deal with it in this whole file. Sure, thanks.
+ struct stat st; + + if ((fd = open(cfg->secretsEncryptionKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + return false; + }
I'd suggest you factor out the useful bits from 'qemuDomainMasterKeyReadFile' and then reuse it rather than reimplement this Yes, will do.
+ if (fstat(fd, &st) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd);
With the autoclose helper this won't be needed. Yes.
+ return false; + } + *secrets_encryption_keylen = st.st_size; + if (*secrets_encryption_keylen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s is empty"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *secrets_encryption_key = g_new0(uint8_t, *secrets_encryption_keylen);
This allocates buffer based on the actual size, IMO yoiu should limit it to something more sensible. I will #define the key length and limit the secrets_encryption_key to that size.
+ if (saferead(fd, &secrets_encryption_key, *secrets_encryption_keylen) != *secrets_encryption_keylen) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read secrets key file '%1$s'"), + cfg->secretsEncryptionKeyPath); + VIR_FORCE_CLOSE(fd); + return false;
For functions using libvirt errors please stick to the '0' '-1' return value convntion. Yes. Will change it.
+ } + VIR_FORCE_CLOSE(fd); + if (*secrets_encryption_keylen != 32) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Secrets encryption key file %1$s must be 32 bytes"), + cfg->secretsEncryptionKeyPath); + return false; + } + return true; +} + +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) + goto error; + + if (!(cfg = virObjectNew(virSecretDaemonConfigClass))) + goto error; + + cfg->secretsEncryptionKeyPath = NULL; + + if (privileged) { + configdir = g_strdup(SYSCONFDIR "/libvirt"); + } else { + g_autofree char *rundir = virGetUserRuntimeDirectory(); + configdir = virGetUserConfigDirectory(); + } + configfile = g_strconcat(configdir, "/secrets.conf", NULL);
This open-codes what the unused 'virSecretDaemonConfigFilePath' function does. Either use the function or delete it. Will use the function. Also I noticed rundir is not really used here.
+ if (virSecretLoadDaemonConfig(cfg, configfile) < 0) + goto error; + + if (!(credentials_directory = getenv("CREDENTIALS_DIRECTORY"))) { + credentials_directory = NULL;
This is a pointless condition. If getenv("CREDENTIALS_DIRECTORY") returns NULL you set it to NULL. Yes.
+ } + + if (cfg->secretsEncryptionKeyPath) { + VIR_DEBUG("Secrets encryption key path: %s", cfg->secretsEncryptionKeyPath); + } else if (credentials_directory) { + VIR_DEBUG("Using credentials directory from environment: %s", + credentials_directory); + cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", + credentials_directory); + } else { + VIR_DEBUG("No secrets encryption key found in credentials directory"); + cfg->secretsEncryptionKeyPath = NULL; + }
This logic is a bit convoluted. I suggest you do an unconditional log of the actual path after you determine whic one to use:
if (!cfg->secretsEncryptionKeyPath && credentials_directory) { VIR_DEBUG("Using credentials directory from environment: %s", credentials_directory);
cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", credentials_directory); }
VIR_DEBUG("Secrets encryption key path: %s", NULLSTR(cfg->secretsEncryptionKeyPath));
Sure
+ if (cfg->secretsEncryptionKeyPath && access(cfg->secretsEncryptionKeyPath, R_OK) == 0) { + if (!getSecretsEncryptionKey(cfg, &cfg->secrets_encryption_key, &cfg->secretsKeyLen)) { + VIR_DEBUG("Failed to get secrets encryption key from path: %s", + cfg->secretsEncryptionKeyPath); + goto error;
You must properly report the error if you are to bail from daemon startup. Here !getSecretsEncryptionKey sets the value but it doesn't seem so because you do a pointless DEBUG (the error was already reported)
I will remove the debug statement.
+ } + } + + if (cfg->encrypt_data != 1) { + cfg->encrypt_data = (cfg->secretsKeyLen == 32) ? 1 : 0;
So you set this to 1 even when the user set it to 0? Also 'encrypt_data' really ought to be a bool in the config struct. If users set this to 0 explicitly you shouldn't try to re-infer it.
Here you also duplicate a magic constant used to limit the size so if anyone ever changes it in the other place only this will stop to work.
If you want to automatically set this you'll also need to remember (possibly in another bool) if the explicit config option was seen or not.
can we set this to 1 before even reading the config file? Because we want to encrypt the secrets by default That way, it can either be changed to 0 or remain 1.
+ } else if (cfg->encrypt_data == 1) { + if (!cfg->secretsEncryptionKeyPath) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("secretsEncryptionKeyPath must be set if encrypt_data is 1 in %1$s"), + configfile);
This isn't an internal error, it's user's wrong config.
Also make it a standalone check rather than an else branch.
yes will do.
+ goto error; + } + } + return g_steal_pointer(&cfg); + error: + virSecretDaemonConfigDispose(cfg);
cfg is declared as autoptr so this is not needed as it ought to be called automatically. This also means that the whole 'error' label can be removed and 'NULL returned directly.
+ return NULL; +} + +static void +virSecretDaemonConfigDispose(void *obj) +{ + virSecretDaemonConfig *cfg = obj; + + g_free(cfg->secrets_encryption_key);
Use the function to securely clear it first, before freeing it here.
yes, will do.
+ g_free(cfg->secretsEncryptionKeyPath); +} diff --git a/src/conf/secret_config.h b/src/conf/secret_config.h new file mode 100644 index 0000000000..638b7c49a4 --- /dev/null +++ b/src/conf/secret_config.h @@ -0,0 +1,48 @@ +/* + * secret_config.h: secrets.conf config file handling + * + * Copyright (C) 2025 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" +#include "virinhibitor.h" +#include "secret_event.h" + +typedef struct _virSecretDaemonConfig virSecretDaemonConfig; +struct _virSecretDaemonConfig { + virObject parent; + /* secrets encryption key path from secrets.conf file */ + char *secretsEncryptionKeyPath; + + unsigned char* secrets_encryption_key;
unsigned char *sec...
Yes.
+ size_t secretsKeyLen; + + /* Indicates if the newly written secrets are encrypted or not. + * 0 if not encrypted and 1 if encrypted. + */ + int encrypt_data;
This ought to be either a 'bool' or one of the tristates. Alternatively two bools.
Can we keep it a boolean, and by default set it to 1? After reading config, it will stay 1 or change to 0.
+}; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecretDaemonConfig, virObjectUnref); + +int virSecretDaemonConfigFilePath(bool privileged, char **configfile); +virSecretDaemonConfig *virSecretDaemonConfigNew(bool privileged); +int virSecretDaemonConfigLoadFile(virSecretDaemonConfig *data, + const char *filename, + bool allow_missing); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
Regards, Arun Menon
A new attribute is required, to store the encryption scheme used while encrypting the secret. This value will be "none" if the secret is stored in base64 format. For backwards compatibility, the secret will not be encrypted when the attribute itself is absent in the configuration file. In other words, the secret will be stored on the disk in base64 encoded format. This new attribute is essential to be stored on the disk in the xml file, so that we can effectively decrypt the secrets while loading them. It also allows us to add more encryption schemes in the future. Signed-off-by: Arun Menon <armenon@redhat.com> --- include/libvirt/libvirt-secret.h | 20 ++++++++++++++++++++ src/conf/schemas/secret.rng | 5 +++++ src/conf/secret_conf.c | 21 +++++++++++++++++++++ src/conf/secret_conf.h | 1 + src/util/virsecret.c | 4 ++++ src/util/virsecret.h | 1 + tests/secretxml2xmlin/usage-ceph-space.xml | 1 + tests/secretxml2xmlin/usage-ceph.xml | 1 + tests/secretxml2xmlin/usage-iscsi.xml | 1 + tests/secretxml2xmlin/usage-tls.xml | 1 + tests/secretxml2xmlin/usage-volume.xml | 1 + tests/secretxml2xmlin/usage-vtpm.xml | 1 + 12 files changed, 58 insertions(+) diff --git a/include/libvirt/libvirt-secret.h b/include/libvirt/libvirt-secret.h index 761437d4ad..96a4359107 100644 --- a/include/libvirt/libvirt-secret.h +++ b/include/libvirt/libvirt-secret.h @@ -70,6 +70,26 @@ typedef enum { # endif } virSecretUsageType; +/** + * virSecretEncryptionSchemeType: + * + * Since: 11.10.0 + */ +typedef enum { + VIR_SECRET_ENCRYPTION_SCHEME_NONE = 0, /* (Since: 11.10.0) */ + VIR_SECRET_ENCRYPTION_SCHEME_AES256CBS = 1, /* (Since: 11.10.0) */ +# ifdef VIR_ENUM_SENTINELS + VIR_SECRET_ENCRYPTION_SCHEME_LAST + /* + * NB: this enum value will increase over time as new encryption schemes are + * added to the libvirt API. It reflects the last enncryption scheme supported + * by this version of the libvirt API. + * + * Since: 11.10.0 + */ +# endif +} virSecretEncryptionSchemeType; + virConnectPtr virSecretGetConnect (virSecretPtr secret); int virConnectNumOfSecrets (virConnectPtr conn); int virConnectListSecrets (virConnectPtr conn, diff --git a/src/conf/schemas/secret.rng b/src/conf/schemas/secret.rng index c90e2eb81f..ae6e62b438 100644 --- a/src/conf/schemas/secret.rng +++ b/src/conf/schemas/secret.rng @@ -42,6 +42,11 @@ </choice> </element> </optional> + <optional> + <element name="encryptionScheme"> + <text/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 966536599e..2fdf3f7f2c 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -131,6 +131,12 @@ virSecretParseXML(xmlXPathContext *ctxt) g_autofree char *ephemeralstr = NULL; g_autofree char *privatestr = NULL; g_autofree char *uuidstr = NULL; + g_autofree char *encryptionScheme = NULL; + + /* Encryption scheme is set to -1 to support existing xml secret configuration + * files. This indicates that no encryption scheme is specified in the XML + */ + int type = -1; def = g_new0(virSecretDef, 1); @@ -170,6 +176,15 @@ virSecretParseXML(xmlXPathContext *ctxt) if (virSecretDefParseUsage(ctxt, def) < 0) return NULL; + encryptionScheme = virXPathString("string(./encryptionScheme)", ctxt); + if (encryptionScheme) { + if ((type = virSecretEncryptionSchemeTypeFromString(encryptionScheme)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown secret encryption scheme %1$d"), def->encryption_scheme); + return NULL; + } + } + def->encryption_scheme = type; return g_steal_pointer(&def); } @@ -242,6 +257,7 @@ virSecretDefFormat(const virSecretDef *def) g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(&buf); + const char *type = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virBufferAsprintf(&attrBuf, " ephemeral='%s' private='%s'", @@ -257,6 +273,11 @@ virSecretDefFormat(const virSecretDef *def) virSecretDefFormatUsage(&childBuf, def) < 0) return NULL; + type = virSecretEncryptionSchemeTypeToString(def->encryption_scheme); + if (type != NULL) { + virBufferEscapeString(&childBuf, "<encryptionScheme>%s</encryptionScheme>\n", + type); + } virXMLFormatElement(&buf, "secret", &attrBuf, &childBuf); return virBufferContentAndReset(&buf); } diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 8f8f47933a..a12bc8e095 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -30,6 +30,7 @@ struct _virSecretDef { char *description; /* May be NULL */ virSecretUsageType usage_type; char *usage_id; /* May be NULL */ + virSecretEncryptionSchemeType encryption_scheme; /* virSecretEncryptionSchemeType */ }; void virSecretDefFree(virSecretDef *def); diff --git a/src/util/virsecret.c b/src/util/virsecret.c index 8e74df3b93..c9d9cf2c8a 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -36,6 +36,10 @@ VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, "none", "volume", "ceph", "iscsi", "tls", "vtpm", ); +VIR_ENUM_IMPL(virSecretEncryptionScheme, + VIR_SECRET_ENCRYPTION_SCHEME_LAST, + "none", "aes256cbc", +); void virSecretLookupDefClear(virSecretLookupTypeDef *def) diff --git a/src/util/virsecret.h b/src/util/virsecret.h index c803f0fe33..01998e307d 100644 --- a/src/util/virsecret.h +++ b/src/util/virsecret.h @@ -27,6 +27,7 @@ #include "virenum.h" VIR_ENUM_DECL(virSecretUsage); +VIR_ENUM_DECL(virSecretEncryptionScheme); typedef enum { VIR_SECRET_LOOKUP_TYPE_NONE, diff --git a/tests/secretxml2xmlin/usage-ceph-space.xml b/tests/secretxml2xmlin/usage-ceph-space.xml index 557b12474d..2a7a177931 100644 --- a/tests/secretxml2xmlin/usage-ceph-space.xml +++ b/tests/secretxml2xmlin/usage-ceph-space.xml @@ -4,4 +4,5 @@ <usage type='ceph'> <name>client.admin secret</name> </usage> + <encryptionScheme>none</encryptionScheme> </secret> diff --git a/tests/secretxml2xmlin/usage-ceph.xml b/tests/secretxml2xmlin/usage-ceph.xml index e880293a63..8a2501c21f 100644 --- a/tests/secretxml2xmlin/usage-ceph.xml +++ b/tests/secretxml2xmlin/usage-ceph.xml @@ -4,4 +4,5 @@ <usage type='ceph'> <name>CephCephCephCeph</name> </usage> + <encryptionScheme>none</encryptionScheme> </secret> diff --git a/tests/secretxml2xmlin/usage-iscsi.xml b/tests/secretxml2xmlin/usage-iscsi.xml index bfc94722e0..c36a0f8661 100644 --- a/tests/secretxml2xmlin/usage-iscsi.xml +++ b/tests/secretxml2xmlin/usage-iscsi.xml @@ -4,4 +4,5 @@ <usage type='iscsi'> <target>iscsitarget</target> </usage> + <encryptionScheme>none</encryptionScheme> </secret> diff --git a/tests/secretxml2xmlin/usage-tls.xml b/tests/secretxml2xmlin/usage-tls.xml index 88068b56e0..a021e96279 100644 --- a/tests/secretxml2xmlin/usage-tls.xml +++ b/tests/secretxml2xmlin/usage-tls.xml @@ -4,4 +4,5 @@ <usage type='tls'> <name>mumblyfratz</name> </usage> + <encryptionScheme>none</encryptionScheme> </secret> diff --git a/tests/secretxml2xmlin/usage-volume.xml b/tests/secretxml2xmlin/usage-volume.xml index e273c57686..7f9a4e13b8 100644 --- a/tests/secretxml2xmlin/usage-volume.xml +++ b/tests/secretxml2xmlin/usage-volume.xml @@ -4,4 +4,5 @@ <usage type='volume'> <volume>/var/lib/libvirt/images/image.img</volume> </usage> + <encryptionScheme>none</encryptionScheme> </secret> diff --git a/tests/secretxml2xmlin/usage-vtpm.xml b/tests/secretxml2xmlin/usage-vtpm.xml index 5baff3034d..f9b801f765 100644 --- a/tests/secretxml2xmlin/usage-vtpm.xml +++ b/tests/secretxml2xmlin/usage-vtpm.xml @@ -4,4 +4,5 @@ <usage type='vtpm'> <name>vTPMvTPMvTPM</name> </usage> + <encryptionScheme>aes256cbc</encryptionScheme> </secret> -- 2.51.1
On Thu, Nov 20, 2025 at 22:23:45 +0530, Arun Menon via Devel wrote:
A new attribute is required, to store the encryption scheme used while encrypting the secret. This value will be "none" if the secret is stored in base64 format.
For backwards compatibility, the secret will not be encrypted when the attribute itself is absent in the configuration file. In other words, the secret will be stored on the disk in base64 encoded format.
This new attribute is essential to be stored on the disk in the xml file, so that we can effectively decrypt the secrets while loading them. It also allows us to add more encryption schemes in the future.
Storing it in the XML also gives the user the possibility to control the encryption. Do we want that? The only reason I see for a user to configure this field is to ensure that secrets are "downgraded" to an older encryption scheme, but I'm not sure if that is actually useful. Shouldn't this remain an implementation detail and thus not exposed to the user? In such case you could tell them apart e.g. using a filename suffix.
Signed-off-by: Arun Menon <armenon@redhat.com> --- include/libvirt/libvirt-secret.h | 20 ++++++++++++++++++++ src/conf/schemas/secret.rng | 5 +++++ src/conf/secret_conf.c | 21 +++++++++++++++++++++ src/conf/secret_conf.h | 1 + src/util/virsecret.c | 4 ++++ src/util/virsecret.h | 1 + tests/secretxml2xmlin/usage-ceph-space.xml | 1 + tests/secretxml2xmlin/usage-ceph.xml | 1 + tests/secretxml2xmlin/usage-iscsi.xml | 1 + tests/secretxml2xmlin/usage-tls.xml | 1 + tests/secretxml2xmlin/usage-volume.xml | 1 + tests/secretxml2xmlin/usage-vtpm.xml | 1 + 12 files changed, 58 insertions(+)
diff --git a/include/libvirt/libvirt-secret.h b/include/libvirt/libvirt-secret.h index 761437d4ad..96a4359107 100644 --- a/include/libvirt/libvirt-secret.h +++ b/include/libvirt/libvirt-secret.h @@ -70,6 +70,26 @@ typedef enum { # endif } virSecretUsageType;
+/** + * virSecretEncryptionSchemeType: + * + * Since: 11.10.0
Note that libvirt will enter freeze on the 25th of November. You'll need to use 12.0.0 after that.
+ */ +typedef enum { + VIR_SECRET_ENCRYPTION_SCHEME_NONE = 0, /* (Since: 11.10.0) */ + VIR_SECRET_ENCRYPTION_SCHEME_AES256CBS = 1, /* (Since: 11.10.0) */
AES256CBC I presume
+# ifdef VIR_ENUM_SENTINELS + VIR_SECRET_ENCRYPTION_SCHEME_LAST + /* + * NB: this enum value will increase over time as new encryption schemes are + * added to the libvirt API. It reflects the last enncryption scheme supported + * by this version of the libvirt API. + * + * Since: 11.10.0 + */ +# endif +} virSecretEncryptionSchemeType;
Looking at the code this seems to be only the type for the variable holding the parsed XML. Since no API uses this users don't actually have possibility to see what the string values represented by the enum.
+ virConnectPtr virSecretGetConnect (virSecretPtr secret); int virConnectNumOfSecrets (virConnectPtr conn); int virConnectListSecrets (virConnectPtr conn, diff --git a/src/conf/schemas/secret.rng b/src/conf/schemas/secret.rng index c90e2eb81f..ae6e62b438 100644 --- a/src/conf/schemas/secret.rng +++ b/src/conf/schemas/secret.rng @@ -42,6 +42,11 @@ </choice> </element> </optional> + <optional> + <element name="encryptionScheme"> + <text/>
Since this is an enumeration field please enumerate all approved values as <choice>
+ </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 966536599e..2fdf3f7f2c 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -131,6 +131,12 @@ virSecretParseXML(xmlXPathContext *ctxt) g_autofree char *ephemeralstr = NULL; g_autofree char *privatestr = NULL; g_autofree char *uuidstr = NULL; + g_autofree char *encryptionScheme = NULL; + + /* Encryption scheme is set to -1 to support existing xml secret configuration + * files. This indicates that no encryption scheme is specified in the XML + */ + int type = -1;
def = g_new0(virSecretDef, 1);
@@ -170,6 +176,15 @@ virSecretParseXML(xmlXPathContext *ctxt) if (virSecretDefParseUsage(ctxt, def) < 0) return NULL;
+ encryptionScheme = virXPathString("string(./encryptionScheme)", ctxt); + if (encryptionScheme) { + if ((type = virSecretEncryptionSchemeTypeFromString(encryptionScheme)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR,
Use of VIR_ERR_INTERNAL_ERROR is not appropriate for errors coming from parsing user-originated XML. Please use something more appropriate such as VIR_ERR_XML_ERROR
+ _("Unknown secret encryption scheme %1$d"), def->encryption_scheme); + return NULL; + } + } + def->encryption_scheme = type;
The 'encryption_shceme' member is declared as ...
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 8f8f47933a..a12bc8e095 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -30,6 +30,7 @@ struct _virSecretDef { char *description; /* May be NULL */ virSecretUsageType usage_type; char *usage_id; /* May be NULL */ + virSecretEncryptionSchemeType encryption_scheme; /* virSecretEncryptionSchemeType */ };
which is an enum with natural numbers and thus the compiler considers it to be unsighed, yet tyou have the possibility to assign -1. Also no need to state the type in the comment if you declare it as such.
return g_steal_pointer(&def); }
@@ -242,6 +257,7 @@ virSecretDefFormat(const virSecretDef *def) g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(&buf); + const char *type = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN];
virBufferAsprintf(&attrBuf, " ephemeral='%s' private='%s'", @@ -257,6 +273,11 @@ virSecretDefFormat(const virSecretDef *def) virSecretDefFormatUsage(&childBuf, def) < 0) return NULL;
+ type = virSecretEncryptionSchemeTypeToString(def->encryption_scheme); + if (type != NULL) { + virBufferEscapeString(&childBuf, "<encryptionScheme>%s</encryptionScheme>\n", + type);
'virBufferEscapeString' is NULL, tolerant on the 3rd parameter and actually skips the whole string to format if the arg is NULL. Thus you don't need the extra block and variable here.
+ } virXMLFormatElement(&buf, "secret", &attrBuf, &childBuf); return virBufferContentAndReset(&buf); }
[...]
On Fri, Nov 21, 2025 at 08:08:20AM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 20, 2025 at 22:23:45 +0530, Arun Menon via Devel wrote:
A new attribute is required, to store the encryption scheme used while encrypting the secret. This value will be "none" if the secret is stored in base64 format.
For backwards compatibility, the secret will not be encrypted when the attribute itself is absent in the configuration file. In other words, the secret will be stored on the disk in base64 encoded format.
This new attribute is essential to be stored on the disk in the xml file, so that we can effectively decrypt the secrets while loading them. It also allows us to add more encryption schemes in the future.
Storing it in the XML also gives the user the possibility to control the encryption. Do we want that? The only reason I see for a user to configure this field is to ensure that secrets are "downgraded" to an older encryption scheme, but I'm not sure if that is actually useful.
Shouldn't this remain an implementation detail and thus not exposed to the user? In such case you could tell them apart e.g. using a filename suffix.
Agreed, the file suffix is all we want - the backend impl choice should not be exposed to the end user XML. 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 Fri, 21 Nov, 2025, 3:22 pm Daniel P. Berrangé, <berrange@redhat.com> wrote:
On Fri, Nov 21, 2025 at 08:08:20AM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 20, 2025 at 22:23:45 +0530, Arun Menon via Devel wrote:
A new attribute is required, to store the encryption scheme used while encrypting the secret. This value will be "none" if the secret is stored in base64 format.
For backwards compatibility, the secret will not be encrypted when the attribute itself is absent in the configuration file. In other words, the secret will be stored on the disk in base64 encoded format.
This new attribute is essential to be stored on the disk in the xml file, so that we can effectively decrypt the secrets while loading them. It also allows us to add more encryption schemes in the future.
Storing it in the XML also gives the user the possibility to control the encryption. Do we want that? The only reason I see for a user to configure this field is to ensure that secrets are "downgraded" to an older encryption scheme, but I'm not sure if that is actually useful.
Shouldn't this remain an implementation detail and thus not exposed to the user? In such case you could tell them apart e.g. using a filename suffix.
Agreed, the file suffix is all we want - the backend impl choice should not be exposed to the end user XML.
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 :|
Do you mean that we should not be using encryption scheme at all. While loading the secrets, all we need to do is check the file name suffix and if it is not ".base64", then enter the decryption logic? Regards, Arun Menon he/him Software Engineer, Virtualization Red Hat APAC
On Fri, Nov 21, 2025 at 20:20:56 +0530, Arun Menon wrote:
On Fri, 21 Nov, 2025, 3:22 pm Daniel P. Berrangé, <berrange@redhat.com> wrote:
On Fri, Nov 21, 2025 at 08:08:20AM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 20, 2025 at 22:23:45 +0530, Arun Menon via Devel wrote:
A new attribute is required, to store the encryption scheme used while encrypting the secret. This value will be "none" if the secret is stored in base64 format.
For backwards compatibility, the secret will not be encrypted when the attribute itself is absent in the configuration file. In other words, the secret will be stored on the disk in base64 encoded format.
This new attribute is essential to be stored on the disk in the xml file, so that we can effectively decrypt the secrets while loading them. It also allows us to add more encryption schemes in the future.
Storing it in the XML also gives the user the possibility to control the encryption. Do we want that? The only reason I see for a user to configure this field is to ensure that secrets are "downgraded" to an older encryption scheme, but I'm not sure if that is actually useful.
Shouldn't this remain an implementation detail and thus not exposed to the user? In such case you could tell them apart e.g. using a filename suffix.
Agreed, the file suffix is all we want - the backend impl choice should not be exposed to the end user XML.
[...]
Do you mean that we should not be using encryption scheme at all. While loading the secrets, all we need to do is check the file name suffix and if it is not ".base64", then enter the decryption logic?
You can encode which cipher was used in the suffix of the filename e.g: SECRETNAME.aes256, SECRETNAME.base64 If we ever switch to another cipher we'll use a different suffix. And when loading them look at the most recently used cipher/format first, and go back the list. You then can either remember which ciplher one was used internally, so that we use the same on output, or we'll unconditionally write out in the newest format possible. In the latter case you must ensure that the older format file was wiped once the value is written out.
On Fri, Nov 21, 2025 at 8:38 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Fri, 21 Nov, 2025, 3:22 pm Daniel P. Berrangé, <berrange@redhat.com> wrote:
On Fri, Nov 21, 2025 at 08:08:20AM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 20, 2025 at 22:23:45 +0530, Arun Menon via Devel wrote:
A new attribute is required, to store the encryption scheme used while encrypting the secret. This value will be "none" if the secret is stored in base64 format.
For backwards compatibility, the secret will not be encrypted when
attribute itself is absent in the configuration file. In other words, the secret will be stored on the disk in base64 encoded format.
This new attribute is essential to be stored on the disk in the xml file, so that we can effectively decrypt the secrets while loading them. It also allows us to add more encryption schemes in the future.
Storing it in the XML also gives the user the possibility to control
On Fri, Nov 21, 2025 at 20:20:56 +0530, Arun Menon wrote: the the
encryption. Do we want that? The only reason I see for a user to configure this field is to ensure that secrets are "downgraded" to an older encryption scheme, but I'm not sure if that is actually useful.
Shouldn't this remain an implementation detail and thus not exposed to the user? In such case you could tell them apart e.g. using a filename suffix.
Agreed, the file suffix is all we want - the backend impl choice should not be exposed to the end user XML.
[...]
Do you mean that we should not be using encryption scheme at all. While loading the secrets, all we need to do is check the file name suffix and if it is not ".base64", then enter the decryption logic?
You can encode which cipher was used in the suffix of the filename e.g:
SECRETNAME.aes256, SECRETNAME.base64
If we ever switch to another cipher we'll use a different suffix.
And when loading them look at the most recently used cipher/format first, and go back the list.
You then can either remember which ciplher one was used internally, so that we use the same on output, or we'll unconditionally write out in the newest format possible. In the latter case you must ensure that the older format file was wiped once the value is written out.
I see. I was only afraid of the scenario where we migrate from an older version of libvirt to the new one and the encryption scheme changes in between. A secret on the disk which was encrypted with scheme A will not be loaded automatically, because the new scheme will not be able to decrypt it.
On Fri, Nov 21, 2025 at 21:05:23 +0530, Arun Menon wrote:
On Fri, Nov 21, 2025 at 8:38 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Fri, Nov 21, 2025 at 20:20:56 +0530, Arun Menon wrote:
On Fri, 21 Nov, 2025, 3:22 pm Daniel P. Berrangé, <berrange@redhat.com>
[...]
Do you mean that we should not be using encryption scheme at all. While loading the secrets, all we need to do is check the file name suffix and if it is not ".base64", then enter the decryption logic?
You can encode which cipher was used in the suffix of the filename e.g:
SECRETNAME.aes256, SECRETNAME.base64
If we ever switch to another cipher we'll use a different suffix.
And when loading them look at the most recently used cipher/format first, and go back the list.
You then can either remember which ciplher one was used internally, so that we use the same on output, or we'll unconditionally write out in the newest format possible. In the latter case you must ensure that the older format file was wiped once the value is written out.
I see. I was only afraid of the scenario where we migrate from an older version of libvirt to the new one and the encryption scheme changes in between. A secret on the disk which was encrypted with scheme A will not be loaded automatically, because the new scheme will not be able to decrypt it.
Yes you definitely need to be able to tell which cipher/method was used on the actual on-disk file. But since we don't want to expose it the filename seems to be the most straightforward solution.
Since we now have the functionality to provide the secrets driver with an encryption key, and the newly introduced attribute to store the encryption scheme across driver restarts, we can use the key to encrypt secrets. While loading the secrets, we check whether the secret is encrypted or not and accordingly get the value. While the stored encryption scheme ensures the driver can successfully load secrets after a restart, If the user changes the encryption key between driver restarts, any secrets encrypted with the previous key will become permanently inaccessible upon the next restart. Users must ensure key consistency to maintain access to existing encrypted secrets. Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/virsecretobj.c | 165 ++++++++++++++++++++++++++++++------- src/conf/virsecretobj.h | 10 ++- src/secret/secret_driver.c | 23 ++++-- 3 files changed, 157 insertions(+), 41 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 66270e2751..37b2db960f 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -31,6 +31,10 @@ #include "virhash.h" #include "virlog.h" #include "virstring.h" +#include "virsecret.h" +#include "virrandom.h" +#include "vircrypto.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -328,6 +332,8 @@ virSecretObjListAdd(virSecretObjList *secrets, virSecretObj *obj; virSecretDef *objdef; virSecretObj *ret = NULL; + const char *encryptionScheme = NULL; + const char *encryptionSchemeExt = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virObjectRWLockWrite(secrets); @@ -379,10 +385,26 @@ virSecretObjListAdd(virSecretObjList *secrets, goto cleanup; /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix + * using the configDir, uuidstr, and appropriate encryption scheme */ - if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + if ((*newdef)->encryption_scheme != VIR_SECRET_ENCRYPTION_SCHEME_NONE + && (*newdef)->encryption_scheme != -1) { + encryptionScheme = virSecretEncryptionSchemeTypeToString((*newdef)->encryption_scheme); + if (!encryptionScheme) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown secret encryption scheme %1$d"), (*newdef)->encryption_scheme); + goto cleanup; + } + encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL); + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, encryptionSchemeExt))) { + goto cleanup; + } + } else { + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) { + goto cleanup; + } + } + if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))) goto cleanup; if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) @@ -397,6 +419,7 @@ virSecretObjListAdd(virSecretObjList *secrets, cleanup: virSecretObjEndAPI(&obj); virObjectRWUnlock(secrets); + g_clear_pointer((gpointer *)&encryptionSchemeExt, g_free); return ret; } @@ -680,17 +703,49 @@ virSecretObjSaveConfig(virSecretObj *obj) return 0; } - int -virSecretObjSaveData(virSecretObj *obj) +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { g_autofree char *base64 = NULL; + g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + size_t base64Len = 0; + uint8_t iv[16] = { 0 }; if (!obj->value) return 0; - base64 = g_base64_encode(obj->value, obj->value_size); - + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE + || obj->def->encryption_scheme == -1) { + base64 = g_base64_encode(obj->value, obj->value_size); + } else { + if (driverConfig == NULL || driverConfig->secrets_encryption_key == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot encrypt secret value without encryption key")); + return -1; + } + if (virRandomBytes(iv, sizeof(iv)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate random IV")); + return -1; + } + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + (uint8_t *)obj->value, obj->value_size, + &encryptedValue, &encryptedValueLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to encrypt secret value")); + return -1; + } + base64Len = sizeof(iv) + encryptedValueLen; + base64 = g_new0(char, base64Len); + memcpy(base64, iv, sizeof(iv)); + memcpy(base64 + sizeof(iv), encryptedValue, encryptedValueLen); + /* Now the secret is encrypted and stored on disk. However, + * we did not change anything in the obj->value. This is done on + * purpose, as SecretObjGetValue should be able to read it as is. + * This will indeed be a base64 encoded secret*/ + } if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) return -1; @@ -733,27 +788,25 @@ virSecretObjGetValue(virSecretObj *obj) return ret; } - int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size) + size_t value_size, + virSecretDaemonConfig *driverConfig) { virSecretDef *def = obj->def; g_autofree unsigned char *old_value = NULL; g_autofree unsigned char *new_value = NULL; size_t old_value_size; - new_value = g_new0(unsigned char, value_size); old_value = obj->value; old_value_size = obj->value_size; - memcpy(new_value, value, value_size); obj->value = g_steal_pointer(&new_value); obj->value_size = value_size; - if (!def->isephemeral && virSecretObjSaveData(obj) < 0) + if (!def->isephemeral && virSecretObjSaveData(obj, driverConfig) < 0) goto error; /* Saved successfully - drop old value */ @@ -786,7 +839,6 @@ virSecretObjSetValueSize(virSecretObj *obj, obj->value_size = value_size; } - static int virSecretLoadValidateUUID(virSecretDef *def, const char *file) @@ -807,11 +859,18 @@ virSecretLoadValidateUUID(virSecretDef *def, static int -virSecretLoadValue(virSecretObj *obj) +virSecretLoadValue(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { int ret = -1, fd = -1; struct stat st; g_autofree char *contents = NULL; + g_autofree uint8_t *contents_encrypted = NULL; + g_autofree uint8_t *decryptedValue = NULL; + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0; if ((fd = open(obj->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { @@ -841,25 +900,65 @@ virSecretLoadValue(virSecretObj *obj) goto cleanup; } - contents = g_new0(char, st.st_size + 1); - - if (saferead(fd, contents, st.st_size) != st.st_size) { - virReportSystemError(errno, _("cannot read '%1$s'"), - obj->base64File); - goto cleanup; + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE || + obj->def->encryption_scheme == -1) { + contents = g_new0(char, st.st_size + 1); + if (saferead(fd, contents, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->base64File); + goto cleanup; + } + contents[st.st_size] = '\0'; + obj->value = g_base64_decode(contents, &obj->value_size); + if (obj->value == NULL) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("cannot decode base64 secret value")); + goto cleanup; + } + } else { + if (driverConfig->secrets_encryption_key == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot decrypt secret value without encryption key")); + goto cleanup; + } + contents_encrypted = g_new0(uint8_t, st.st_size); + if (saferead(fd, contents_encrypted, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->base64File); + goto cleanup; + } + if ((st.st_size) < sizeof(iv)) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Encrypted secret size is invalid")); + goto cleanup; + } + memcpy(iv, contents_encrypted, sizeof(iv)); + ciphertext = contents_encrypted + sizeof(iv); + ciphertextLen = st.st_size - sizeof(iv); + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Decryption of secret value failed")); + goto cleanup; + } + g_free(obj->value); + obj->value = g_steal_pointer(&decryptedValue); + obj->value_size = decryptedValueLen; } - contents[st.st_size] = '\0'; - - VIR_FORCE_CLOSE(fd); - - obj->value = g_base64_decode(contents, &obj->value_size); - ret = 0; cleanup: - if (contents != NULL) - memset(contents, 0, st.st_size); + if (contents != NULL) { + memset(contents, 0, st.st_size+1); + } + if (contents_encrypted != NULL) { + memset(contents_encrypted, 0, st.st_size); + } VIR_FORCE_CLOSE(fd); + virSecureErase(iv, sizeof(iv)); return ret; } @@ -868,7 +967,8 @@ static virSecretObj * virSecretLoad(virSecretObjList *secrets, const char *file, const char *path, - const char *configDir) + const char *configDir, + virSecretDaemonConfig *driverConfig) { g_autoptr(virSecretDef) def = NULL; virSecretObj *obj = NULL; @@ -882,7 +982,7 @@ virSecretLoad(virSecretObjList *secrets, if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL))) return NULL; - if (virSecretLoadValue(obj) < 0) { + if (virSecretLoadValue(obj, driverConfig) < 0) { virSecretObjListRemove(secrets, obj); g_clear_pointer(&obj, virObjectUnref); return NULL; @@ -894,7 +994,8 @@ virSecretLoad(virSecretObjList *secrets, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir) + const char *configDir, + virSecretDaemonConfig *driverConfig) { g_autoptr(DIR) dir = NULL; struct dirent *de; @@ -915,7 +1016,7 @@ virSecretLoadAllConfigs(virSecretObjList *secrets, if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) continue; - if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) { + if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir, driverConfig))) { VIR_ERROR(_("Error reading secret: %1$s"), virGetLastErrorMessage()); continue; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 17897c5513..f49600a75c 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -23,6 +23,7 @@ #include "internal.h" #include "secret_conf.h" +#include "secret_config.h" typedef struct _virSecretObj virSecretObj; @@ -86,7 +87,8 @@ int virSecretObjSaveConfig(virSecretObj *obj); int -virSecretObjSaveData(virSecretObj *obj); +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig); virSecretDef * virSecretObjGetDef(virSecretObj *obj); @@ -101,7 +103,8 @@ virSecretObjGetValue(virSecretObj *obj); int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size); + size_t value_size, + virSecretDaemonConfig *driverConfig); size_t virSecretObjGetValueSize(virSecretObj *obj); @@ -112,4 +115,5 @@ virSecretObjSetValueSize(virSecretObj *obj, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir); + const char *configDir, + virSecretDaemonConfig *cfg); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 04c3ca49f1..c0cac39a28 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -30,6 +30,7 @@ #include "virlog.h" #include "viralloc.h" #include "secret_conf.h" +#include "secret_config.h" #include "virsecretobj.h" #include "secret_driver.h" #include "virthread.h" @@ -42,6 +43,10 @@ #include "secret_event.h" #include "virutil.h" #include "virinhibitor.h" +#include "virfile.h" +#include "virrandom.h" +#include "vircrypto.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -70,6 +75,8 @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + virSecretDaemonConfig *config; }; static virSecretDriverState *driver; @@ -224,7 +231,7 @@ secretDefineXML(virConnectPtr conn, if (!objDef->isephemeral) { if (backup && backup->isephemeral) { - if (virSecretObjSaveData(obj) < 0) + if (virSecretObjSaveData(obj, driver->config) < 0) goto restore_backup; } @@ -307,7 +314,6 @@ secretGetXMLDesc(virSecretPtr secret, return ret; } - static int secretSetValue(virSecretPtr secret, const unsigned char *value, @@ -327,8 +333,7 @@ secretSetValue(virSecretPtr secret, def = virSecretObjGetDef(obj); if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup; - - if (virSecretObjSetValue(obj, value, value_size) < 0) + if (virSecretObjSetValue(obj, value, value_size, driver->config) < 0) goto cleanup; event = virSecretEventValueChangedNew(def->uuid, @@ -454,6 +459,7 @@ secretStateCleanupLocked(void) VIR_FREE(driver->configDir); virObjectUnref(driver->secretEventState); + virObjectUnref(driver->config); virInhibitorFree(driver->inhibitor); if (driver->lockFD != -1) @@ -518,6 +524,8 @@ secretStateInitialize(bool privileged, driver->stateDir); goto error; } + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + goto error; driver->inhibitor = virInhibitorNew( VIR_INHIBITOR_WHAT_NONE, @@ -534,7 +542,7 @@ secretStateInitialize(bool privileged, if (!(driver->secrets = virSecretObjListNew())) goto error; - if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) + if (virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config) < 0) goto error; return VIR_DRV_STATE_INIT_COMPLETE; @@ -553,7 +561,10 @@ secretStateReload(void) if (!driver) return -1; - ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + return -1; + + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config)); return 0; } -- 2.51.1
On Thu, Nov 20, 2025 at 22:23:46 +0530, Arun Menon via Devel wrote:
Since we now have the functionality to provide the secrets driver with an encryption key, and the newly introduced attribute to store the encryption scheme across driver restarts, we can use the key to encrypt secrets. While loading the secrets, we check whether the secret is encrypted or not and accordingly get the value.
While the stored encryption scheme ensures the driver can successfully load secrets after a restart, If the user changes the encryption key between driver restarts, any secrets encrypted with the previous key will become permanently inaccessible upon the next restart. Users must ensure key consistency to maintain access to existing encrypted secrets.
Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/virsecretobj.c | 165 ++++++++++++++++++++++++++++++------- src/conf/virsecretobj.h | 10 ++- src/secret/secret_driver.c | 23 ++++-- 3 files changed, 157 insertions(+), 41 deletions(-)
All changes related to adding of the secret driver config, parsing etc ought to be either part of the commit that adds the config parser or a separate commit after it, but not intermixed with the implementation of encrypting the individual secrets. Based on the feedback to 4/5 we also don't want to store the secret type in the XML so I'll not comment on anything related to that.,
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 66270e2751..37b2db960f 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c
@@ -328,6 +332,8 @@ virSecretObjListAdd(virSecretObjList *secrets, virSecretObj *obj; virSecretDef *objdef; virSecretObj *ret = NULL; + const char *encryptionScheme = NULL; + const char *encryptionSchemeExt = NULL;
You declare these as const, then assign string copies into them and then use g_free on this. Use normal strings instead and auto-free them.
char uuidstr[VIR_UUID_STRING_BUFLEN];
virObjectRWLockWrite(secrets); @@ -379,10 +385,26 @@ virSecretObjListAdd(virSecretObjList *secrets, goto cleanup;
/* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix + * using the configDir, uuidstr, and appropriate encryption scheme */ - if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + if ((*newdef)->encryption_scheme != VIR_SECRET_ENCRYPTION_SCHEME_NONE + && (*newdef)->encryption_scheme != -1) { + encryptionScheme = virSecretEncryptionSchemeTypeToString((*newdef)->encryption_scheme); + if (!encryptionScheme) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown secret encryption scheme %1$d"), (*newdef)->encryption_scheme); + goto cleanup; + } + encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL); + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, encryptionSchemeExt))) { + goto cleanup; + } + } else { + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) { + goto cleanup; + } + }
You'll need to probe instead which of the suffixes we support exist in order from the most preffered one. Also rename the variable holding the filename to something like 'secretValueFile'.
+ if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))) goto cleanup;
if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
[...]
@@ -680,17 +703,49 @@ virSecretObjSaveConfig(virSecretObj *obj) return 0; }
- int -virSecretObjSaveData(virSecretObj *obj) +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { g_autofree char *base64 = NULL; + g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + size_t base64Len = 0; + uint8_t iv[16] = { 0 };
if (!obj->value) return 0;
- base64 = g_base64_encode(obj->value, obj->value_size); - + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE + || obj->def->encryption_scheme == -1) {
coding style
+ base64 = g_base64_encode(obj->value, obj->value_size); + } else { + if (driverConfig == NULL || driverConfig->secrets_encryption_key == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot encrypt secret value without encryption key")); + return -1; + } + if (virRandomBytes(iv, sizeof(iv)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate random IV"));
virRandomBytes already reports an error; we usually don't overwrite them
+ return -1; + } + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + (uint8_t *)obj->value, obj->value_size, + &encryptedValue, &encryptedValueLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to encrypt secret value"));
Same here.
+ return -1; + } + base64Len = sizeof(iv) + encryptedValueLen; + base64 = g_new0(char, base64Len); + memcpy(base64, iv, sizeof(iv)); + memcpy(base64 + sizeof(iv), encryptedValue, encryptedValueLen); + /* Now the secret is encrypted and stored on disk. However, + * we did not change anything in the obj->value. This is done on + * purpose, as SecretObjGetValue should be able to read it as is. + * This will indeed be a base64 encoded secret*/
I didn't really go check why we store these as base64 but I'd suggest that the encrypted value is still stored as base64
+ } if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) return -1;
@@ -733,27 +788,25 @@ virSecretObjGetValue(virSecretObj *obj) return ret; }
- int
Spurious whitespace change. Our coding style prefers 2 empty lines between functions.
virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size) + size_t value_size, + virSecretDaemonConfig *driverConfig) { virSecretDef *def = obj->def; g_autofree unsigned char *old_value = NULL; g_autofree unsigned char *new_value = NULL; size_t old_value_size; - new_value = g_new0(unsigned char, value_size);
old_value = obj->value; old_value_size = obj->value_size; - memcpy(new_value, value, value_size); obj->value = g_steal_pointer(&new_value); obj->value_size = value_size;
All of the whitespace changes are spurious.
- if (!def->isephemeral && virSecretObjSaveData(obj) < 0) + if (!def->isephemeral && virSecretObjSaveData(obj, driverConfig) < 0) goto error;
/* Saved successfully - drop old value */ @@ -786,7 +839,6 @@ virSecretObjSetValueSize(virSecretObj *obj, obj->value_size = value_size; }
- static int
ditto
virSecretLoadValidateUUID(virSecretDef *def, const char *file) @@ -807,11 +859,18 @@ virSecretLoadValidateUUID(virSecretDef *def,
static int -virSecretLoadValue(virSecretObj *obj) +virSecretLoadValue(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { int ret = -1, fd = -1; struct stat st; g_autofree char *contents = NULL; + g_autofree uint8_t *contents_encrypted = NULL; + g_autofree uint8_t *decryptedValue = NULL; + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0;
if ((fd = open(obj->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { @@ -841,25 +900,65 @@ virSecretLoadValue(virSecretObj *obj) goto cleanup; }
- contents = g_new0(char, st.st_size + 1); - - if (saferead(fd, contents, st.st_size) != st.st_size) { - virReportSystemError(errno, _("cannot read '%1$s'"), - obj->base64File); - goto cleanup; + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE || + obj->def->encryption_scheme == -1) { + contents = g_new0(char, st.st_size + 1); + if (saferead(fd, contents, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->base64File); + goto cleanup; + } + contents[st.st_size] = '\0'; + obj->value = g_base64_decode(contents, &obj->value_size); + if (obj->value == NULL) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("cannot decode base64 secret value")); + goto cleanup; + } + } else { + if (driverConfig->secrets_encryption_key == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot decrypt secret value without encryption key")); + goto cleanup; + } + contents_encrypted = g_new0(uint8_t, st.st_size); + if (saferead(fd, contents_encrypted, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->base64File); + goto cleanup; + } + if ((st.st_size) < sizeof(iv)) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Encrypted secret size is invalid")); + goto cleanup; + } + memcpy(iv, contents_encrypted, sizeof(iv)); + ciphertext = contents_encrypted + sizeof(iv); + ciphertextLen = st.st_size - sizeof(iv); + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Decryption of secret value failed")); + goto cleanup; + } + g_free(obj->value); + obj->value = g_steal_pointer(&decryptedValue); + obj->value_size = decryptedValueLen; } - contents[st.st_size] = '\0'; - - VIR_FORCE_CLOSE(fd); - - obj->value = g_base64_decode(contents, &obj->value_size); - ret = 0;
cleanup: - if (contents != NULL) - memset(contents, 0, st.st_size); + if (contents != NULL) { + memset(contents, 0, st.st_size+1); + } + if (contents_encrypted != NULL) { + memset(contents_encrypted, 0, st.st_size); + }
Use virSecureErase; memset can be optimized out. Also for any existing code.
VIR_FORCE_CLOSE(fd); + virSecureErase(iv, sizeof(iv)); return ret; }
[...]
@@ -70,6 +75,8 @@ struct _virSecretDriverState {
/* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + virSecretDaemonConfig *config;
Please annotate this one as the other pointers are.
participants (3)
-
Arun Menon -
Daniel P. Berrangé -
Peter Krempa