[RFC 0/4] 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. First we introduce a secrets.conf file that has 2 configuration settings. 1. encrypt_data - This can be set to 0 or 1. If it is set to 1 then we encrypt the secrets. We recommend to keep this as the default setting. 2. master_encryption_key - This allows the user to set the path of the encryption file. The secrets.conf file is parsed during secrets driver initialization and accordingly the secrets are stored on the disk. By default the secrets driver will look into the CREDENTIALS_DIRECTORY environment variable in systemd. The systemd unit file is configured with a pre configured key using the SetCredentialEncrypted directive. This encrypted secret key is provided to the virtsecretd on service activation. If this file is not available, then the virtsecretd driver will check the master_encryption_key configuration in secrets.conf file, provided it is made available by the user. A boolean flag called value_encrypted is added to the secrets object, to indicate whether it is encrypted or not. This is not stored on the disk yet. It is important to add this functionality so that the secrets service behaves properly across restarts. 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 Arun Menon (4): util: Add support for GnuTLS decryption secret: Set up default encrypted master key for the virtsecretd service secret: Add secrets.conf configuration file and parse it secret: Add functionality to load and save secrets in encrypted format libvirt.spec.in | 1 + src/conf/virsecretobj.c | 13 ++ src/conf/virsecretobj.h | 7 + src/libvirt_private.syms | 3 + src/secret/meson.build | 7 + src/secret/secret_driver.c | 168 +++++++++++++++++++++++- src/secret/secrets.conf.in | 14 ++ src/secret/virtsecretd.service.extra.in | 8 ++ src/util/vircrypto.c | 130 +++++++++++++++++- src/util/vircrypto.h | 8 ++ tests/vircryptotest.c | 65 +++++++++ 11 files changed, 420 insertions(+), 4 deletions(-) create mode 100644 src/secret/secrets.conf.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 | 130 ++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 8 +++ tests/vircryptotest.c | 65 ++++++++++++++++++++ 4 files changed, 202 insertions(+), 2 deletions(-) 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..e0d2b794a1 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 * @@ -200,7 +200,7 @@ virCryptoEncryptData(virCryptoCipher algorithm, { switch (algorithm) { case VIR_CRYPTO_CIPHER_AES256CBC: - if (enckeylen != 32) { + if (enckeylen < 32) { virReportError(VIR_ERR_INVALID_ARG, _("AES256CBC encryption invalid keylen=%1$zu"), enckeylen); @@ -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) { + virSecureErase(plaintext, plaintextlen); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to decrypt the data: '%1$s'"), + gnutls_strerror(rc)); + return -1; + } + if (plaintextlen == 0) { + virSecureErase(plaintext, plaintextlen); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has zero length")); + return -1; + } + i = plaintext[plaintextlen - 1]; + if (i > plaintextlen) { + virSecureErase(plaintext, plaintextlen); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has invalid padding")); + return -1; + } + *plaintextlenret = plaintextlen - i; + *plaintextret = g_steal_pointer(&plaintext); + return 0; +} + +/* 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 13, 2025 at 07:02:20PM +0530, Arun Menon 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 | 130 ++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 8 +++ tests/vircryptotest.c | 65 ++++++++++++++++++++ 4 files changed, 202 insertions(+), 2 deletions(-)
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..e0d2b794a1 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 * @@ -200,7 +200,7 @@ virCryptoEncryptData(virCryptoCipher algorithm, { switch (algorithm) { case VIR_CRYPTO_CIPHER_AES256CBC: - if (enckeylen != 32) { + if (enckeylen < 32) {
Why change this ? For AES-256 the key *must* be exactly 32 bytes, no more, no less.
virReportError(VIR_ERR_INVALID_ARG, _("AES256CBC encryption invalid keylen=%1$zu"), enckeylen); @@ -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) { + virSecureErase(plaintext, plaintextlen);
Instead of repeating virSecureErase many times over, create an 'error:' label and jump to that after the virReportError call.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to decrypt the data: '%1$s'"), + gnutls_strerror(rc)); + return -1; + } + if (plaintextlen == 0) { + virSecureErase(plaintext, plaintextlen); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has zero length")); + return -1; + } + i = plaintext[plaintextlen - 1]; + if (i > plaintextlen) {
Is that check right ? I thought that padding would be in the range 0 -> key length. IOW, 0 through 31 inclusive for AES256 ?
+ virSecureErase(plaintext, plaintextlen); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has invalid padding")); + return -1; + } + *plaintextlenret = plaintextlen - i; + *plaintextret = g_steal_pointer(&plaintext); + return 0; +}
+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) {
Again, why < instead of != ?
+ 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;
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 :|
This commit sets the foundation for encrypting the libvirt secrets by providing a secure way to pass a master encryption key to the virtsecretd service. Add a default, pre-generated, master encryption key to the credentials, that can be consumed by the virtsecretd service. By using the "SetCredentialEncrypted=" directive, we make sure that passing data to the service is secure. 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. Users can customize this setting, by replacing the default encrypted string with their own. A subsequent commit will introduce the logic for virtsecretd to access and use this key via the $CREDENTIALS_DIRECTORY environment variable. [2] In order to add the default encryption key, a random 32 byte key was generated and encrypted: dd if=/dev/urandom of=/tmp/master.key bs=1 count=32 systemd-creds encrypt --name=master-encryption-key -p /tmp/master.key - This generates a SetCredentialEncrypted= line suitable for inclusion in the unit file. [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> --- src/secret/virtsecretd.service.extra.in | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..0f65bc3bb1 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. +# +[Service] +Environment=MASTER_ENCRYPTION_KEY=%d/master-encryption-key +SetCredentialEncrypted=master-encryption-key: \ + Whxqht+dQJax1aZeCGLxmiAAAAABAAAADAAAABAAAAD9m5CsEfoZf8Lj/dQAAAAAFSvJ7 \ + eSEmqQthu+A4Eqn4vEKp6jx7ScbcM98bcW5Do0K9V0eTPWD+eNJJrB+xS/MAklo3rkf0S \ + 7n7rXk8SQZ0FQ5Uv8ZoOuidWPHHiLZGS9bxAJwTZvN/VX+pe+biC16 +LoadCredentialEncrypted=master-encryption-key -- 2.51.1
On Thu, Nov 13, 2025 at 07:02:21PM +0530, Arun Menon wrote:
This commit sets the foundation for encrypting the libvirt secrets by providing a secure way to pass a master encryption key to the virtsecretd service.
Add a default, pre-generated, master encryption key to the credentials, that can be consumed by the virtsecretd service. By using the "SetCredentialEncrypted=" directive, we make sure that passing data to the service is secure. 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. Users can customize this setting, by replacing the default encrypted string with their own. A subsequent commit will introduce the logic for virtsecretd to access and use this key via the $CREDENTIALS_DIRECTORY environment variable. [2]
In order to add the default encryption key, a random 32 byte key was generated and encrypted: dd if=/dev/urandom of=/tmp/master.key bs=1 count=32 systemd-creds encrypt --name=master-encryption-key -p /tmp/master.key -
This generates a SetCredentialEncrypted= line suitable for inclusion in the unit file.
[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> --- src/secret/virtsecretd.service.extra.in | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..0f65bc3bb1 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. +# +[Service] +Environment=MASTER_ENCRYPTION_KEY=%d/master-encryption-key +SetCredentialEncrypted=master-encryption-key: \ + Whxqht+dQJax1aZeCGLxmiAAAAABAAAADAAAABAAAAD9m5CsEfoZf8Lj/dQAAAAAFSvJ7 \ + eSEmqQthu+A4Eqn4vEKp6jx7ScbcM98bcW5Do0K9V0eTPWD+eNJJrB+xS/MAklo3rkf0S \ + 7n7rXk8SQZ0FQ5Uv8ZoOuidWPHHiLZGS9bxAJwTZvN/VX+pe+biC16 +LoadCredentialEncrypted=master-encryption-key
We cannot ship with a hardcoded default secret - that is an instant CVE. We need to have an 'ExecStartPre=' directive that auto-generates a secret on first use. ExecStartPre=test -f /var/lib/libvirt/secrets/encryption.key || (dd if=/dev/urandom status=none bs=1 count=32 | systemd-creds encrypt --name=master-encryption-key - /var/lib/libvirt/secrets/encryption.key) Also, bs=32 count=1 is preferrable to bs=1 count=32 as we don't want to be doing single byte reads & writes. and then use LoadCredentialEncrypted=master-encryption-key:/var/lib/libvirt/secrets/encryption.key Also, lets call this 'secrets-encryption-key', and likewise SECRETS_ENCRYPTION_KEY as the env variable. 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 :|
Hi Daniel, Thanks for the review. On Thu, Nov 13, 2025 at 03:01:21PM +0000, Daniel P. Berrangé wrote:
On Thu, Nov 13, 2025 at 07:02:21PM +0530, Arun Menon wrote:
This commit sets the foundation for encrypting the libvirt secrets by providing a secure way to pass a master encryption key to the virtsecretd service.
Add a default, pre-generated, master encryption key to the credentials, that can be consumed by the virtsecretd service. By using the "SetCredentialEncrypted=" directive, we make sure that passing data to the service is secure. 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. Users can customize this setting, by replacing the default encrypted string with their own. A subsequent commit will introduce the logic for virtsecretd to access and use this key via the $CREDENTIALS_DIRECTORY environment variable. [2]
In order to add the default encryption key, a random 32 byte key was generated and encrypted: dd if=/dev/urandom of=/tmp/master.key bs=1 count=32 systemd-creds encrypt --name=master-encryption-key -p /tmp/master.key -
This generates a SetCredentialEncrypted= line suitable for inclusion in the unit file.
[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> --- src/secret/virtsecretd.service.extra.in | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..0f65bc3bb1 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. +# +[Service] +Environment=MASTER_ENCRYPTION_KEY=%d/master-encryption-key +SetCredentialEncrypted=master-encryption-key: \ + Whxqht+dQJax1aZeCGLxmiAAAAABAAAADAAAABAAAAD9m5CsEfoZf8Lj/dQAAAAAFSvJ7 \ + eSEmqQthu+A4Eqn4vEKp6jx7ScbcM98bcW5Do0K9V0eTPWD+eNJJrB+xS/MAklo3rkf0S \ + 7n7rXk8SQZ0FQ5Uv8ZoOuidWPHHiLZGS9bxAJwTZvN/VX+pe+biC16 +LoadCredentialEncrypted=master-encryption-key
We cannot ship with a hardcoded default secret - that is an instant CVE.
We need to have an 'ExecStartPre=' directive that auto-generates a secret on first use.
ExecStartPre=test -f /var/lib/libvirt/secrets/encryption.key || (dd if=/dev/urandom status=none bs=1 count=32 | systemd-creds encrypt --name=master-encryption-key - /var/lib/libvirt/secrets/encryption.key)
Also, bs=32 count=1 is preferrable to bs=1 count=32 as we don't want to be doing single byte reads & writes.
and then use
LoadCredentialEncrypted=master-encryption-key:/var/lib/libvirt/secrets/encryption.key
Also, lets call this 'secrets-encryption-key', and likewise SECRETS_ENCRYPTION_KEY as the env variable.
Yes, will do.
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 :|
I wanted to do exactly this, but it turns out that there is a precedence in execution. Meaning "ExecStartPre" is executed at a later stage than "LoadCredentialEncrypted". As a result, LoadCredentialEncrypted does not get the value that was set in ExecStartPre. I did not know about this, We can remove the line and add it in the readme docs so that the reader knows how to set up encryption using the service file. The user can create a secret key however they want, and that way it will not be tied to the service unit file. Regards, Arun Menon
On Thu, Nov 13, 2025 at 10:47:52PM +0530, Arun Menon wrote:
Hi Daniel,
Thanks for the review.
On Thu, Nov 13, 2025 at 03:01:21PM +0000, Daniel P. Berrangé wrote:
On Thu, Nov 13, 2025 at 07:02:21PM +0530, Arun Menon wrote:
This commit sets the foundation for encrypting the libvirt secrets by providing a secure way to pass a master encryption key to the virtsecretd service.
Add a default, pre-generated, master encryption key to the credentials, that can be consumed by the virtsecretd service. By using the "SetCredentialEncrypted=" directive, we make sure that passing data to the service is secure. 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. Users can customize this setting, by replacing the default encrypted string with their own. A subsequent commit will introduce the logic for virtsecretd to access and use this key via the $CREDENTIALS_DIRECTORY environment variable. [2]
In order to add the default encryption key, a random 32 byte key was generated and encrypted: dd if=/dev/urandom of=/tmp/master.key bs=1 count=32 systemd-creds encrypt --name=master-encryption-key -p /tmp/master.key -
This generates a SetCredentialEncrypted= line suitable for inclusion in the unit file.
[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> --- src/secret/virtsecretd.service.extra.in | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index 1fc8c672f7..0f65bc3bb1 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. +# +[Service] +Environment=MASTER_ENCRYPTION_KEY=%d/master-encryption-key +SetCredentialEncrypted=master-encryption-key: \ + Whxqht+dQJax1aZeCGLxmiAAAAABAAAADAAAABAAAAD9m5CsEfoZf8Lj/dQAAAAAFSvJ7 \ + eSEmqQthu+A4Eqn4vEKp6jx7ScbcM98bcW5Do0K9V0eTPWD+eNJJrB+xS/MAklo3rkf0S \ + 7n7rXk8SQZ0FQ5Uv8ZoOuidWPHHiLZGS9bxAJwTZvN/VX+pe+biC16 +LoadCredentialEncrypted=master-encryption-key
We cannot ship with a hardcoded default secret - that is an instant CVE.
We need to have an 'ExecStartPre=' directive that auto-generates a secret on first use.
ExecStartPre=test -f /var/lib/libvirt/secrets/encryption.key || (dd if=/dev/urandom status=none bs=1 count=32 | systemd-creds encrypt --name=master-encryption-key - /var/lib/libvirt/secrets/encryption.key)
Also, bs=32 count=1 is preferrable to bs=1 count=32 as we don't want to be doing single byte reads & writes.
and then use
LoadCredentialEncrypted=master-encryption-key:/var/lib/libvirt/secrets/encryption.key
Also, lets call this 'secrets-encryption-key', and likewise SECRETS_ENCRYPTION_KEY as the env variable.
Yes, will do.
I wanted to do exactly this, but it turns out that there is a precedence in execution. Meaning "ExecStartPre" is executed at a later stage than "LoadCredentialEncrypted". As a result, LoadCredentialEncrypted does not get the value that was set in ExecStartPre. I did not know about this,
Urgh, that's annoying.
We can remove the line and add it in the readme docs so that the reader knows how to set up encryption using the service file. The user can create a secret key however they want, and that way it will not be tied to the service unit file.
No, better if we do it with a separate unit file Create a virtsecretd-init-encryption.service, and have that listed as a dependency of virtsecretd.service. In that we can use ConditionPathExists=!/var/run/libvirt/secrets/encryption.cred soo that it is only launched if the encrypted credential has not yet been created. That'll effectively make the new service a one-shot thing. 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 master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt. By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf Add logic to parse the encryption key file and store the key. When systemd will start the secrets driver, it will read the secret.conf file and check if encrypt_data flag is set to 1. In that case, the secrets will be stored in encrypted format on the disk. The encryption and decryption logic will be added in the subsequent patches. Signed-off-by: Arun Menon <armenon@redhat.com> --- libvirt.spec.in | 1 + src/secret/meson.build | 7 +++ src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++ src/secret/secrets.conf.in | 14 ++++++ 4 files changed, 118 insertions(+) create mode 100644 src/secret/secrets.conf.in diff --git a/libvirt.spec.in b/libvirt.spec.in index 79738bd7bb..f27247b7c1 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -2246,6 +2246,7 @@ exit 0 %config(noreplace) %{_sysconfdir}/libvirt/virtsecretd.conf %{_datadir}/augeas/lenses/virtsecretd.aug %{_datadir}/augeas/lenses/tests/test_virtsecretd.aug +%config(noreplace) %{_sysconfdir}/libvirt/secrets.conf %{_unitdir}/virtsecretd.service %{_unitdir}/virtsecretd.socket %{_unitdir}/virtsecretd-ro.socket diff --git a/src/secret/meson.build b/src/secret/meson.build index 3b859ea7b4..a211ffed83 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -27,6 +27,13 @@ if conf.has('WITH_SECRETS') ], } + secrets_conf = configure_file( + input: 'secrets.conf.in', + output: 'secrets.conf', + copy: true + ) + virt_conf_files += secrets_conf + virt_daemon_confs += { 'name': 'virtsecretd', } diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 04c3ca49f1..0b415e5ef3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -42,6 +42,7 @@ #include "secret_event.h" #include "virutil.h" #include "virinhibitor.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -70,6 +71,17 @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + /* master encryption key value from secret.conf file */ + char *masterKeyPath; + + /* Indicates if the secrets are encrypted or not. 0 if not encrypted + * and 1 if encrypted. + */ + int encrypt_data; + + unsigned char* masterKey; + size_t masterKeyLen; }; static virSecretDriverState *driver; @@ -307,6 +319,44 @@ secretGetXMLDesc(virSecretPtr secret, return ret; } +static bool secretGetMasterKey(uint8_t **masterKey, size_t *masterKeyLen) +{ + int fd = -1; + struct stat st; + + if ((fd = open(driver->masterKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open master key file '%1$s'"), + driver->masterKeyPath); + return false; + } + if (fstat(fd, &st) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat master key file '%1$s'"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *masterKeyLen = st.st_size; + if (*masterKeyLen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s is empty"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *masterKey = g_new0(uint8_t, *masterKeyLen); + if (saferead(fd, &masterKey, *masterKeyLen) != *masterKeyLen) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read master key file '%1$s'"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + VIR_FORCE_CLOSE(fd); + if (*masterKeyLen < 32) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s must be atleast 32 bytes"), + driver->masterKeyPath); + return false; + } + return true; +} static int secretSetValue(virSecretPtr secret, @@ -482,6 +532,10 @@ secretStateInitialize(bool privileged, void *opaque) { VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + g_autofree char *secretsconf = NULL; + g_autofree char *credentials_directory = NULL; + g_autofree char *master_encryption_key_path = NULL; + g_autoptr(virConf) conf = NULL; driver = g_new0(virSecretDriverState, 1); @@ -537,6 +591,48 @@ secretStateInitialize(bool privileged, if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) goto error; + secretsconf = g_strdup_printf("%s/libvirt/secrets.conf", SYSCONFDIR); + credentials_directory = getenv("CREDENTIALS_DIRECTORY"); + + if (credentials_directory) { + VIR_DEBUG("Using credentials directory from environment: %s", + credentials_directory); + master_encryption_key_path = g_strdup_printf("%s/master-encryption-key", credentials_directory); + if (access(master_encryption_key_path, R_OK) == 0) { + driver->masterKeyPath = g_strdup(master_encryption_key_path); + } + } else if (access(secretsconf, R_OK) == 0) { + if (!(conf = virConfReadFile(secretsconf, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to read secrets.conf from %1$s"), + secretsconf); + goto error; + } + + if (virConfGetValueString(conf, "master_encryption_key", &driver->masterKeyPath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get master_encryption_key from %1$s"), + secretsconf); + goto error; + } + } else { + VIR_DEBUG("No secrets configuration found %s, skipping", driver->configDir); + driver->masterKeyPath = NULL; + driver->masterKeyLen = 0; + } + if (driver->masterKeyPath) { + if (!secretGetMasterKey(&driver->masterKey, &driver->masterKeyLen)) { + goto error; + } + VIR_DEBUG("Master encryption key loaded from %s", driver->masterKeyPath); + VIR_DEBUG("Master encryption key length: %zu bytes", driver->masterKeyLen); + } + if (virConfGetValueInt(conf, "encrypt_data", &driver->encrypt_data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get encrypt_data from %1$s"), + secretsconf); + goto error; + } return VIR_DRV_STATE_INIT_COMPLETE; error: diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1 -- 2.51.1
On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
Is there any plan to be able to pass the secret do the secrets driver/daemon in an ephemeral way? Because both the systemd secrets and the config file seem to just store it on the same host. Thus for root-owned files it's just a slightly bigger hurdle rather than any real security.
When systemd will start the secrets driver, it will read the secret.conf file and check if encrypt_data flag is set to 1. In that case, the secrets will be stored in encrypted format on the disk. The encryption and decryption logic will be added in the subsequent patches.
Signed-off-by: Arun Menon <armenon@redhat.com> --- libvirt.spec.in | 1 + src/secret/meson.build | 7 +++ src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++ src/secret/secrets.conf.in | 14 ++++++ 4 files changed, 118 insertions(+) create mode 100644 src/secret/secrets.conf.in
[...]
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
As the default secret seems to be handed in via systemd, which will it make available to any upgraded installation, I don't think you can unconditionally enable this option as it would break existing un-encrypted secrets.
On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
Is there any plan to be able to pass the secret do the secrets driver/daemon in an ephemeral way?
Because both the systemd secrets and the config file seem to just store it on the same host. Thus for root-owned files it's just a slightly bigger hurdle rather than any real security.
IIUC in the systemd case, the credentials on disk are only visible in mount namespace given to the service. I've not checked, but I would imagine the dir is backed with tmpfs. IOW, the plain text secret should be inaccessible to anything else on the host, unless they have fully privileged root account access needed to join the mount namespace. The service can delete the creds file from this mount location when it has been loaded, so the window of availability is only the startup sequence. None the less any app can access /proc/$PID/mem and fetch the secrets from memory, if they have full root privileges no matter how we pass the secret across to virtsecretd. An alternative in the non-systemd case would be to pass the creds in via a FIFO, so it is read-once and not actually on even a temporary disk mount.
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
As the default secret seems to be handed in via systemd, which will it make available to any upgraded installation, I don't think you can unconditionally enable this option as it would break existing un-encrypted secrets.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
As the default secret seems to be handed in via systemd, which will it make available to any upgraded installation, I don't think you can unconditionally enable this option as it would break existing un-encrypted secrets.
When I discussed the design with Arun offlist, my suggestion was that our goal should be to have encryption enabled out of the box on all systemd based hosts, with zero config by the admin. This implies that we need the ability to read *both* plain text and encrypted secrets off disk, as we may have a mixture during the transitional times of an upgrade. Whether we proactively encrypt all plain text secrets at startup, or simply always read them on startup in whatever format they're stored is a choice we have. Not immediately encrypting all plain text secrets gives the ability to downgrade libvirt again without loosing access to previously created secrets on disk. So I think that's marginally better. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 13, 2025 at 14:48:12 +0000, Daniel P. Berrangé wrote:
On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
As the default secret seems to be handed in via systemd, which will it make available to any upgraded installation, I don't think you can unconditionally enable this option as it would break existing un-encrypted secrets.
When I discussed the design with Arun offlist, my suggestion was that our goal should be to have encryption enabled out of the box on all systemd based hosts, with zero config by the admin.
I agree with that. We also ought to auto-generate a properly random secret key on each host at installation time, otherwise it's unlikely that an admin will change the key if we provide a default.
This implies that we need the ability to read *both* plain text and encrypted secrets off disk, as we may have a mixture during the transitional times of an upgrade. Whether we proactively encrypt all plain text secrets at startup, or simply always read them on startup in whatever format they're stored is a choice we have.
Not immediately encrypting all plain text secrets gives the ability to downgrade libvirt again without loosing access to previously created secrets on disk. So I think that's marginally better.
Encrypting non-encrypted secrets will still require libvirt knowing whether they are encrypted or not, so it's simpler to just leave them in whatever state they were in. That information will need to be stored somewhere out-of-band (e.g. by the filename or location of the file) in order to properly detect the current state. The config setting flag will then influence the format only when storign the secret.
On Thu, Nov 13, 2025 at 03:57:32PM +0100, Peter Krempa wrote:
On Thu, Nov 13, 2025 at 14:48:12 +0000, Daniel P. Berrangé wrote:
On Thu, Nov 13, 2025 at 02:57:13PM +0100, Peter Krempa via Devel wrote:
On Thu, Nov 13, 2025 at 19:02:22 +0530, Arun Menon via Devel wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
As the default secret seems to be handed in via systemd, which will it make available to any upgraded installation, I don't think you can unconditionally enable this option as it would break existing un-encrypted secrets.
When I discussed the design with Arun offlist, my suggestion was that our goal should be to have encryption enabled out of the box on all systemd based hosts, with zero config by the admin.
I agree with that. We also ought to auto-generate a properly random secret key on each host at installation time, otherwise it's unlikely that an admin will change the key if we provide a default.
Yes, I've just replied to that effect on the previous patch.
This implies that we need the ability to read *both* plain text and encrypted secrets off disk, as we may have a mixture during the transitional times of an upgrade. Whether we proactively encrypt all plain text secrets at startup, or simply always read them on startup in whatever format they're stored is a choice we have.
Not immediately encrypting all plain text secrets gives the ability to downgrade libvirt again without loosing access to previously created secrets on disk. So I think that's marginally better.
Encrypting non-encrypted secrets will still require libvirt knowing whether they are encrypted or not, so it's simpler to just leave them in whatever state they were in. That information will need to be stored somewhere out-of-band (e.g. by the filename or location of the file) in order to properly detect the current state.
We should be using a different file extension for the plain text vs cipher text secrets to make this unambiguous at a glance.
The config setting flag will then influence the format only when storign the secret.
Yep. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Nov 13, 2025 at 07:02:22PM +0530, Arun Menon wrote:
A new configuration file called secrets.conf is introduced to let the user configure the path to the master encryption key. This key will be used to encrypt/decrypt the secrets in libvirt.
By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. The virtsecretd driver checks if the credentials are available in the CREDENTIALS_DIRECTORY. In case it is not present, then the user is expected to provide the encryption key path in secrets.conf
Add logic to parse the encryption key file and store the key. When systemd will start the secrets driver, it will read the secret.conf file and check if encrypt_data flag is set to 1. In that case, the secrets will be stored in encrypted format on the disk. The encryption and decryption logic will be added in the subsequent patches.
Signed-off-by: Arun Menon <armenon@redhat.com> --- libvirt.spec.in | 1 + src/secret/meson.build | 7 +++ src/secret/secret_driver.c | 96 ++++++++++++++++++++++++++++++++++++++ src/secret/secrets.conf.in | 14 ++++++
New config files need to be accompanied with augeas files for processing them. See the simple examples in src/network/libvirtd_network.aug src/network/test_libvirtd_network.aug.in and the src/network/meson.build file for how to wire them up. They also need to be in the RPM spec.
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 04c3ca49f1..0b415e5ef3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -42,6 +42,7 @@ #include "secret_event.h" #include "virutil.h" #include "virinhibitor.h" +#include "virfile.h"
#define VIR_FROM_THIS VIR_FROM_SECRET
@@ -70,6 +71,17 @@ struct _virSecretDriverState {
/* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + /* master encryption key value from secret.conf file */ + char *masterKeyPath;
Per the previous patch, lets call this secretsEncryptionKeyPath
+ + /* Indicates if the secrets are encrypted or not. 0 if not encrypted + * and 1 if encrypted. + */
Lets specify "newly written secrets" to clarify that setting this to '1' wouldn't make it encrypt any pre-existing plain text secrets
+ int encrypt_data; + + unsigned char* masterKey; + size_t masterKeyLen; };
None of these should be in this struct. Our normal practice is to define a virSecretDriverConfig struct to hold all config parameters, and an pointer to that config in the state struct. See virNetworkDriverState/Config for a simple example.
static virSecretDriverState *driver; @@ -307,6 +319,44 @@ secretGetXMLDesc(virSecretPtr secret, return ret; }
+static bool secretGetMasterKey(uint8_t **masterKey, size_t *masterKeyLen) +{ + int fd = -1; + struct stat st; + + if ((fd = open(driver->masterKeyPath, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot open master key file '%1$s'"), + driver->masterKeyPath); + return false; + } + if (fstat(fd, &st) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot stat master key file '%1$s'"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *masterKeyLen = st.st_size; + if (*masterKeyLen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s is empty"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + *masterKey = g_new0(uint8_t, *masterKeyLen); + if (saferead(fd, &masterKey, *masterKeyLen) != *masterKeyLen) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot read master key file '%1$s'"), + driver->masterKeyPath); + VIR_FORCE_CLOSE(fd); + return false; + } + VIR_FORCE_CLOSE(fd); + if (*masterKeyLen < 32) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Master encryption key file %1$s must be atleast 32 bytes"), + driver->masterKeyPath); + return false; + } + return true; +}
static int secretSetValue(virSecretPtr secret, @@ -482,6 +532,10 @@ secretStateInitialize(bool privileged, void *opaque) { VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + g_autofree char *secretsconf = NULL; + g_autofree char *credentials_directory = NULL; + g_autofree char *master_encryption_key_path = NULL; + g_autoptr(virConf) conf = NULL;
driver = g_new0(virSecretDriverState, 1);
@@ -537,6 +591,48 @@ secretStateInitialize(bool privileged, if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) goto error;
+ secretsconf = g_strdup_printf("%s/libvirt/secrets.conf", SYSCONFDIR); + credentials_directory = getenv("CREDENTIALS_DIRECTORY"); + + if (credentials_directory) { + VIR_DEBUG("Using credentials directory from environment: %s", + credentials_directory); + master_encryption_key_path = g_strdup_printf("%s/master-encryption-key", credentials_directory); + if (access(master_encryption_key_path, R_OK) == 0) { + driver->masterKeyPath = g_strdup(master_encryption_key_path); + } + } else if (access(secretsconf, R_OK) == 0) { + if (!(conf = virConfReadFile(secretsconf, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to read secrets.conf from %1$s"), + secretsconf); + goto error; + } + + if (virConfGetValueString(conf, "master_encryption_key", &driver->masterKeyPath) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get master_encryption_key from %1$s"), + secretsconf); + goto error; + } + } else { + VIR_DEBUG("No secrets configuration found %s, skipping", driver->configDir); + driver->masterKeyPath = NULL; + driver->masterKeyLen = 0; + } + if (driver->masterKeyPath) { + if (!secretGetMasterKey(&driver->masterKey, &driver->masterKeyLen)) { + goto error; + } + VIR_DEBUG("Master encryption key loaded from %s", driver->masterKeyPath); + VIR_DEBUG("Master encryption key length: %zu bytes", driver->masterKeyLen); + } + if (virConfGetValueInt(conf, "encrypt_data", &driver->encrypt_data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get encrypt_data from %1$s"), + secretsconf); + goto error; + } return VIR_DRV_STATE_INIT_COMPLETE;
Config file handling code should not be put directly in the state initialization method. All loading of the config should be isolated in separate methods, and live in secret_driver_conf.{h,c} files. Again take a look at bridge_driver_conf.{h,c} for a simple example of prior art. In terms of handling the 'encrypt_data" parameter we need this psuedo-logic: if encrypt_data is not set if key path exists encrypt_data = 1 else encrypt_data = 1 else if encrypt_data == 1 if key path does not exist ..report error...
error: diff --git a/src/secret/secrets.conf.in b/src/secret/secrets.conf.in new file mode 100644 index 0000000000..80bb9654ce --- /dev/null +++ b/src/secret/secrets.conf.in @@ -0,0 +1,14 @@ +# +# Master configuration file for the secrets driver. +# + +# The master encryption key is used to override default master encryption +# key path. The user can create an encryption key and set the master_encryption_key +# to the path on which it resides. +# The key must be atleast 32-bytes long. +# +# master_encryption_key = "/run/libvirt/secrets/master.key" +# +# The encrypt_data setting is used to indicate if the encryption is on or off. +# 0 indicates off and 1 indicates on. By default it is set to on. +encrypt_data = 1
This setting should be commented out - the default value should be set by the code, and should be 0 or 1, depending on whether or not any encryption key exists on disk. # This setting determines whether newly written secret values # are encrypted or not. Setting this to 1 will not trigger # encryption of existing plain text secrets on disk. # # Will default to 1 if the master_encryption_key path exists # on disk, otherwise will default to 0. Uncomment this to # force use of encryption and report an error if the key path # is missing. # encrypt_data = 1 With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Since we now have the functionality to provide the secrets driver with an encryption key, we can use it to encrypt the secrets. While loading the secrets, we check whether the secret is encrypted or not and accordingly get the value. The value_encrypted boolean flag is currently ephemeral (in memory). This flag must be persisted to the disk to ensure that the secrets service knows whether the secret is in the plaintext or ciphertext format across restarts. This is vital and will be addressed in subsequent commits. Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/virsecretobj.c | 13 +++++++ src/conf/virsecretobj.h | 7 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 72 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 66270e2751..8184c3e49e 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -43,6 +43,7 @@ struct _virSecretObj { virSecretDef *def; unsigned char *value; /* May be NULL */ size_t value_size; + bool value_encrypted; }; static virClass *virSecretObjClass; @@ -786,6 +787,18 @@ virSecretObjSetValueSize(virSecretObj *obj, obj->value_size = value_size; } +bool +virSecretObjGetEncryptionFlag(virSecretObj *obj) +{ + return obj->value_encrypted; +} + +void +virSecretObjSetEncryptionFlag(virSecretObj *obj, + bool encryption) +{ + obj->value_encrypted = encryption; +} static int virSecretLoadValidateUUID(virSecretDef *def, diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 17897c5513..4e3b285b82 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -113,3 +113,10 @@ virSecretObjSetValueSize(virSecretObj *obj, int virSecretLoadAllConfigs(virSecretObjList *secrets, const char *configDir); + +void +virSecretObjSetEncryptionFlag(virSecretObj *obj, + bool encryption); + +bool +virSecretObjGetEncryptionFlag(virSecretObj *obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc5fdb00f4..cf6a4ffe03 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1483,6 +1483,7 @@ virSecretObjDeleteConfig; virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjGetDef; +virSecretObjGetEncryptionFlag; virSecretObjGetValue; virSecretObjGetValueSize; virSecretObjListAdd; @@ -1496,6 +1497,7 @@ virSecretObjListRemove; virSecretObjSaveConfig; virSecretObjSaveData; virSecretObjSetDef; +virSecretObjSetEncryptionFlag; virSecretObjSetValue; virSecretObjSetValueSize; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 0b415e5ef3..44f0611fdc 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -43,6 +43,9 @@ #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 @@ -369,6 +372,12 @@ secretSetValue(virSecretPtr secret, virSecretDef *def; virObjectEvent *event = NULL; + g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + g_autofree uint8_t *valueToSave = NULL; + size_t valueToSaveLen = 0; + virCheckFlags(0, -1); if (!(obj = secretObjFromSecret(secret))) @@ -378,8 +387,32 @@ secretSetValue(virSecretPtr secret, if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup; - if (virSecretObjSetValue(obj, value, value_size) < 0) - goto cleanup; + if (driver->encrypt_data != 0 && driver->masterKeyLen >= 32) { + virSecretObjSetEncryptionFlag(obj, true); + if (virRandomBytes(iv, sizeof(iv)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate random IV")); + goto cleanup; + } + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driver->masterKey, driver->masterKeyLen, + iv, sizeof(iv), + (uint8_t *)value, value_size, + &encryptedValue, &encryptedValueLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to encrypt secret value")); + goto cleanup; + } + valueToSaveLen = sizeof(iv) + encryptedValueLen; + valueToSave = g_new0(uint8_t, valueToSaveLen); + memcpy(valueToSave, iv, sizeof(iv)); + memcpy(valueToSave + sizeof(iv), encryptedValue, encryptedValueLen); + + if (virSecretObjSetValue(obj, valueToSave, valueToSaveLen) < 0) + goto cleanup; + } else { + virSecretObjSetEncryptionFlag(obj, false); + if (virSecretObjSetValue(obj, value, value_size) < 0) + goto cleanup; + } event = virSecretEventValueChangedNew(def->uuid, def->usage_type, @@ -387,6 +420,9 @@ secretSetValue(virSecretPtr secret, ret = 0; cleanup: + virSecureErase(encryptedValue, encryptedValueLen); + virSecureErase(iv, sizeof(iv)); + virSecureErase(valueToSave, valueToSaveLen); virSecretObjEndAPI(&obj); virObjectEventStateQueue(driver->secretEventState, event); @@ -402,6 +438,11 @@ secretGetValue(virSecretPtr secret, unsigned char *ret = NULL; virSecretObj *obj; virSecretDef *def; + g_autofree uint8_t *decryptedValue = NULL; + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0; virCheckFlags(0, NULL); @@ -444,6 +485,32 @@ secretGetValue(virSecretPtr secret, *value_size = virSecretObjGetValueSize(obj); + if (virSecretObjGetEncryptionFlag(obj) && driver->masterKeyLen >= 32) { + if (*value_size < sizeof(iv)) { + /* The encrypted secret size should be greater than IV */ + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Encrypted secret size is invalid")); + goto cleanup; + } + memcpy(iv, ret, sizeof(iv)); + ciphertext = ret + sizeof(iv); + ciphertextLen = *value_size - sizeof(iv); + + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driver->masterKey, driver->masterKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Decryption of secret value failed")); + goto cleanup; + } + + g_free(ret); + ret = g_steal_pointer(&decryptedValue); + *value_size = decryptedValueLen; + } + cleanup: virSecretObjEndAPI(&obj); @@ -502,6 +569,7 @@ secretStateCleanupLocked(void) virObjectUnref(driver->secrets); VIR_FREE(driver->configDir); + VIR_FREE(driver->masterKeyPath); virObjectUnref(driver->secretEventState); virInhibitorFree(driver->inhibitor); -- 2.51.1
On Thu, Nov 13, 2025 at 07:02:23PM +0530, Arun Menon wrote:
Since we now have the functionality to provide the secrets driver with an encryption key, we can use it to encrypt the secrets. While loading the secrets, we check whether the secret is encrypted or not and accordingly get the value.
The value_encrypted boolean flag is currently ephemeral (in memory). This flag must be persisted to the disk to ensure that the secrets service knows whether the secret is in the plaintext or ciphertext format across restarts. This is vital and will be addressed in subsequent commits.
Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/virsecretobj.c | 13 +++++++ src/conf/virsecretobj.h | 7 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 72 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 66270e2751..8184c3e49e 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -43,6 +43,7 @@ struct _virSecretObj { virSecretDef *def; unsigned char *value; /* May be NULL */ size_t value_size; + bool value_encrypted; };
static virClass *virSecretObjClass; @@ -786,6 +787,18 @@ virSecretObjSetValueSize(virSecretObj *obj, obj->value_size = value_size; }
+bool +virSecretObjGetEncryptionFlag(virSecretObj *obj) +{ + return obj->value_encrypted; +} + +void +virSecretObjSetEncryptionFlag(virSecretObj *obj, + bool encryption) +{ + obj->value_encrypted = encryption; +}
We shouldn't need these IMHO. Whether or not a value is encrypted should only matter to the code that is loading the secrets, and should not persist thereafter. If the plain txt secret has its value updated, then the new file on disk should always reflect the currently configured encryption settings. This implies the code saving the secret needs to delete any old written secret files
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 17897c5513..4e3b285b82 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -113,3 +113,10 @@ virSecretObjSetValueSize(virSecretObj *obj, int virSecretLoadAllConfigs(virSecretObjList *secrets, const char *configDir); + +void +virSecretObjSetEncryptionFlag(virSecretObj *obj, + bool encryption); + +bool +virSecretObjGetEncryptionFlag(virSecretObj *obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc5fdb00f4..cf6a4ffe03 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1483,6 +1483,7 @@ virSecretObjDeleteConfig; virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjGetDef; +virSecretObjGetEncryptionFlag; virSecretObjGetValue; virSecretObjGetValueSize; virSecretObjListAdd; @@ -1496,6 +1497,7 @@ virSecretObjListRemove; virSecretObjSaveConfig; virSecretObjSaveData; virSecretObjSetDef; +virSecretObjSetEncryptionFlag; virSecretObjSetValue; virSecretObjSetValueSize;
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 0b415e5ef3..44f0611fdc 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -43,6 +43,9 @@ #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
@@ -369,6 +372,12 @@ secretSetValue(virSecretPtr secret, virSecretDef *def; virObjectEvent *event = NULL;
+ g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + g_autofree uint8_t *valueToSave = NULL; + size_t valueToSaveLen = 0; + virCheckFlags(0, -1);
if (!(obj = secretObjFromSecret(secret))) @@ -378,8 +387,32 @@ secretSetValue(virSecretPtr secret, if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup;
- if (virSecretObjSetValue(obj, value, value_size) < 0) - goto cleanup; + if (driver->encrypt_data != 0 && driver->masterKeyLen >= 32) { + virSecretObjSetEncryptionFlag(obj, true); + if (virRandomBytes(iv, sizeof(iv)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate random IV")); + goto cleanup; + } + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC,
We should anticipate that in future we'll need to use algorithms other than AES-256-CBC. This would suggest that on disk we use the encryption scheme as the file suffix eg <uuid>.base64 -> plain text <uuid>.aes256cbc -> AES-256-CBC this make it easy to switch algs later. This encryption logic likely needs to move into the virSecretObjLoadValue and virSecretObjSaaveData methods. This will require passing the encryption key and scheme down into this methods from the driver here.
+ driver->masterKey, driver->masterKeyLen, + iv, sizeof(iv), + (uint8_t *)value, value_size, + &encryptedValue, &encryptedValueLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to encrypt secret value")); + goto cleanup; + } + valueToSaveLen = sizeof(iv) + encryptedValueLen; + valueToSave = g_new0(uint8_t, valueToSaveLen); + memcpy(valueToSave, iv, sizeof(iv)); + memcpy(valueToSave + sizeof(iv), encryptedValue, encryptedValueLen); + + if (virSecretObjSetValue(obj, valueToSave, valueToSaveLen) < 0) + goto cleanup; + } else { + virSecretObjSetEncryptionFlag(obj, false); + if (virSecretObjSetValue(obj, value, value_size) < 0) + goto cleanup; + }
event = virSecretEventValueChangedNew(def->uuid, def->usage_type, @@ -387,6 +420,9 @@ secretSetValue(virSecretPtr secret, ret = 0;
cleanup: + virSecureErase(encryptedValue, encryptedValueLen); + virSecureErase(iv, sizeof(iv)); + virSecureErase(valueToSave, valueToSaveLen); virSecretObjEndAPI(&obj); virObjectEventStateQueue(driver->secretEventState, event);
@@ -402,6 +438,11 @@ secretGetValue(virSecretPtr secret, unsigned char *ret = NULL; virSecretObj *obj; virSecretDef *def; + g_autofree uint8_t *decryptedValue = NULL; + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0;
virCheckFlags(0, NULL);
@@ -444,6 +485,32 @@ secretGetValue(virSecretPtr secret,
*value_size = virSecretObjGetValueSize(obj);
+ if (virSecretObjGetEncryptionFlag(obj) && driver->masterKeyLen >= 32) { + if (*value_size < sizeof(iv)) { + /* The encrypted secret size should be greater than IV */ + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Encrypted secret size is invalid")); + goto cleanup; + } + memcpy(iv, ret, sizeof(iv)); + ciphertext = ret + sizeof(iv); + ciphertextLen = *value_size - sizeof(iv); + + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driver->masterKey, driver->masterKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Decryption of secret value failed")); + goto cleanup; + } + + g_free(ret); + ret = g_steal_pointer(&decryptedValue); + *value_size = decryptedValueLen; + } + cleanup: virSecretObjEndAPI(&obj);
@@ -502,6 +569,7 @@ secretStateCleanupLocked(void)
virObjectUnref(driver->secrets); VIR_FREE(driver->configDir); + VIR_FREE(driver->masterKeyPath);
This belongs in the previous patch which created the new struct field.
virObjectUnref(driver->secretEventState); virInhibitorFree(driver->inhibitor);
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 :|
participants (3)
-
Arun Menon -
Daniel P. Berrangé -
Peter Krempa