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/KE6GVZQ45JTYFTE54CT7DMONSO2W3ZPV/ 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 XXXXXXX..XXXXXXX 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -XXX,XX +XXX,XX @@ virConfWriteMem; # util/vircrypto.h +virCryptoDecryptData; virCryptoEncryptData; virCryptoHashBuf; virCryptoHashString; diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index XXXXXXX..XXXXXXX 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -XXX,XX +XXX,XX @@ virCryptoHashString(virCryptoHash hash, } -/* virCryptoEncryptDataAESgntuls: +/* virCryptoEncryptDataAESgnutls: * * Performs the AES gnutls encryption * @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -XXX,XX +XXX,XX @@ 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) { @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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 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 XXXXXXX..XXXXXXX 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/secret-init-encryption.in @@ -XXX,XX +XXX,XX @@ +[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 XXXXXXX..XXXXXXX 100644 --- a/src/secret/virtsecretd.service.extra.in +++ b/src/secret/virtsecretd.service.extra.in @@ -XXX,XX +XXX,XX @@ # 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 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 XXXXXXX..XXXXXXX 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/conf/meson.build +++ b/src/conf/meson.build @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/src/conf/secret_config.c @@ -XXX,XX +XXX,XX @@ +/* + * 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/src/conf/secret_config.h @@ -XXX,XX +XXX,XX @@ +/* + * 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 XXXXXXX..XXXXXXX 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/libvirt_secrets.aug @@ -XXX,XX +XXX,XX @@ +(* /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 XXXXXXX..XXXXXXX 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/secrets.conf.in @@ -XXX,XX +XXX,XX @@ +# +# 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/test_libvirt_secrets.aug.in @@ -XXX,XX +XXX,XX @@ +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 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 XXXXXXX..XXXXXXX 100644 --- a/include/libvirt/libvirt-secret.h +++ b/include/libvirt/libvirt-secret.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/conf/schemas/secret.rng +++ b/src/conf/schemas/secret.rng @@ -XXX,XX +XXX,XX @@ </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 XXXXXXX..XXXXXXX 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -XXX,XX +XXX,XX @@ 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); @@ -XXX,XX +XXX,XX @@ 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); } @@ -XXX,XX +XXX,XX @@ 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'", @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/util/virsecret.h +++ b/src/util/virsecret.h @@ -XXX,XX +XXX,XX @@ #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 XXXXXXX..XXXXXXX 100644 --- a/tests/secretxml2xmlin/usage-ceph-space.xml +++ b/tests/secretxml2xmlin/usage-ceph-space.xml @@ -XXX,XX +XXX,XX @@ <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 XXXXXXX..XXXXXXX 100644 --- a/tests/secretxml2xmlin/usage-ceph.xml +++ b/tests/secretxml2xmlin/usage-ceph.xml @@ -XXX,XX +XXX,XX @@ <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 XXXXXXX..XXXXXXX 100644 --- a/tests/secretxml2xmlin/usage-iscsi.xml +++ b/tests/secretxml2xmlin/usage-iscsi.xml @@ -XXX,XX +XXX,XX @@ <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 XXXXXXX..XXXXXXX 100644 --- a/tests/secretxml2xmlin/usage-tls.xml +++ b/tests/secretxml2xmlin/usage-tls.xml @@ -XXX,XX +XXX,XX @@ <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 XXXXXXX..XXXXXXX 100644 --- a/tests/secretxml2xmlin/usage-volume.xml +++ b/tests/secretxml2xmlin/usage-volume.xml @@ -XXX,XX +XXX,XX @@ <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 XXXXXXX..XXXXXXX 100644 --- a/tests/secretxml2xmlin/usage-vtpm.xml +++ b/tests/secretxml2xmlin/usage-vtpm.xml @@ -XXX,XX +XXX,XX @@ <usage type='vtpm'> <name>vTPMvTPMvTPM</name> </usage> + <encryptionScheme>aes256cbc</encryptionScheme> </secret> -- 2.51.1 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 XXXXXXX..XXXXXXX 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -XXX,XX +XXX,XX @@ #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 @@ -XXX,XX +XXX,XX @@ 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); @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ virSecretObjListAdd(virSecretObjList *secrets, cleanup: virSecretObjEndAPI(&obj); virObjectRWUnlock(secrets); + g_clear_pointer((gpointer *)&encryptionSchemeExt, g_free); return ret; } @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 */ @@ -XXX,XX +XXX,XX @@ virSecretObjSetValueSize(virSecretObj *obj, obj->value_size = value_size; } - static int virSecretLoadValidateUUID(virSecretDef *def, const char *file) @@ -XXX,XX +XXX,XX @@ 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) { @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ virSecretLoad(virSecretObjList *secrets, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir) + const char *configDir, + virSecretDaemonConfig *driverConfig) { g_autoptr(DIR) dir = NULL; struct dirent *de; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -XXX,XX +XXX,XX @@ #include "internal.h" #include "secret_conf.h" +#include "secret_config.h" typedef struct _virSecretObj virSecretObj; @@ -XXX,XX +XXX,XX @@ int virSecretObjSaveConfig(virSecretObj *obj); int -virSecretObjSaveData(virSecretObj *obj); +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig); virSecretDef * virSecretObjGetDef(virSecretObj *obj); @@ -XXX,XX +XXX,XX @@ 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); @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -XXX,XX +XXX,XX @@ #include "virlog.h" #include "viralloc.h" #include "secret_conf.h" +#include "secret_config.h" #include "virsecretobj.h" #include "secret_driver.h" #include "virthread.h" @@ -XXX,XX +XXX,XX @@ #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 @@ -XXX,XX +XXX,XX @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + virSecretDaemonConfig *config; }; static virSecretDriverState *driver; @@ -XXX,XX +XXX,XX @@ secretDefineXML(virConnectPtr conn, if (!objDef->isephemeral) { if (backup && backup->isephemeral) { - if (virSecretObjSaveData(obj) < 0) + if (virSecretObjSaveData(obj, driver->config) < 0) goto restore_backup; } @@ -XXX,XX +XXX,XX @@ secretGetXMLDesc(virSecretPtr secret, return ret; } - static int secretSetValue(virSecretPtr secret, const unsigned char *value, @@ -XXX,XX +XXX,XX @@ 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, @@ -XXX,XX +XXX,XX @@ secretStateCleanupLocked(void) VIR_FREE(driver->configDir); virObjectUnref(driver->secretEventState); + virObjectUnref(driver->config); virInhibitorFree(driver->inhibitor); if (driver->lockFD != -1) @@ -XXX,XX +XXX,XX @@ secretStateInitialize(bool privileged, driver->stateDir); goto error; } + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + goto error; driver->inhibitor = virInhibitorNew( VIR_INHIBITOR_WHAT_NONE, @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 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 encryption scheme or cipher attribute that will allow us to choose the last used cipher. 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. 6. Add documentation. This is a sincere attempt to improve upon the already submitted patch https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/KE6GVZQ45JTYFTE54CT7DMONSO2W3ZPV/ Resolves: https://issues.redhat.com/browse/RHEL-7125 --- Changes in v5: - Fix the bug of updating old secrets, prior to 12.1.0. Old secrets, when updated, now are stored with the latest encryption scheme if encryption is on. - Remove any logic in setting the secretValueFile in virSecretObjListAdd. Move it to virSecretLoadValue and virSecretObjSetValue. - Change urandom to random. Change default permission bits to 0077. - Update the documentation. Changes in v4: - Fix the regression of loading unencrypted secrets after an upgrade. Previously the .base64 unencrypted secrets were not being loaded. - Add documentation on encrypted secrets. Changes in v3: - Secrets xml configuration no longer stores the encryption scheme, therefore not allowing the user to toggle between ciphers. - Removed unnecessary socket files of the new service. It now has a general configuration with which it starts. - Addressed review comments from Peter on coding style and design. - Loading of secrets is dependent on the file extension. Most recent cipher is used while saving the secrets. 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 (6): util: Add support for GnuTLS decryption secret: Set up default encryption secret key for the virtsecretd service secret: Add secret.conf configuration file and parse it secret: Rename virSecretObj structure attribute from base64File to secretValueFile secret: Add functionality to load and save secrets in encrypted format docs: secret: Add documentation of secret encryption feature docs/drvsecret.rst | 4 + docs/meson.build | 1 + docs/secretencryption.rst | 105 +++++++ include/libvirt/virterror.h | 1 + libvirt.spec.in | 8 + po/POTFILES | 1 + src/conf/virsecretobj.c | 259 +++++++++++++----- src/conf/virsecretobj.h | 16 +- src/libvirt_private.syms | 1 + src/meson.build | 1 + src/remote/libvirtd.service.in | 4 + src/secret/libvirt_secrets.aug | 40 +++ src/secret/meson.build | 32 +++ src/secret/secret.conf.in | 14 + src/secret/secret_config.c | 179 ++++++++++++ src/secret/secret_config.h | 40 +++ src/secret/secret_driver.c | 31 ++- src/secret/test_libvirt_secrets.aug.in | 6 + .../virt-secret-init-encryption.service.in | 8 + src/secret/virtsecretd.service.extra.in | 8 + src/util/vircrypto.c | 126 ++++++++- src/util/vircrypto.h | 8 + src/util/virerror.c | 3 + tests/vircryptotest.c | 65 +++++ 24 files changed, 892 insertions(+), 69 deletions(-) create mode 100644 docs/secretencryption.rst create mode 100644 src/secret/libvirt_secrets.aug create mode 100644 src/secret/secret.conf.in create mode 100644 src/secret/secret_config.c create mode 100644 src/secret/secret_config.h create mode 100644 src/secret/test_libvirt_secrets.aug.in create mode 100644 src/secret/virt-secret-init-encryption.service.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> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircrypto.c | 126 ++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 8 +++ tests/vircryptotest.c | 65 ++++++++++++++++++++ 4 files changed, 199 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index XXXXXXX..XXXXXXX 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -XXX,XX +XXX,XX @@ virConfWriteMem; # util/vircrypto.h +virCryptoDecryptData; virCryptoEncryptData; virCryptoHashBuf; virCryptoHashString; diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index XXXXXXX..XXXXXXX 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -XXX,XX +XXX,XX @@ virCryptoHashString(virCryptoHash hash, } -/* virCryptoEncryptDataAESgntuls: +/* virCryptoEncryptDataAESgnutls: * * Performs the AES gnutls encryption * @@ -XXX,XX +XXX,XX @@ 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; + uint8_t padding_length; + 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; + if (plaintextlen == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("decrypted data has zero length")); + goto error; + } + 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; + } + /* Before encryption, padding is added to the data. + * The last byte indicates the padding length, because in PKCS#7, all + * padding bytes are set to the padding length value. + */ + padding_length = plaintext[plaintextlen - 1]; + if (padding_length > plaintextlen) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("decrypted data has invalid padding")); + goto error; + } + *plaintextlenret = plaintextlen - padding_length; + *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; + } + 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 XXXXXXX..XXXXXXX 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -XXX,XX +XXX,XX @@ 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) { @@ -XXX,XX +XXX,XX @@ 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) @@ -XXX,XX +XXX,XX @@ 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 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 virt-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> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- libvirt.spec.in | 5 +++++ src/meson.build | 1 + src/remote/libvirtd.service.in | 4 ++++ src/secret/meson.build | 13 +++++++++++++ src/secret/virt-secret-init-encryption.service.in | 8 ++++++++ src/secret/virtsecretd.service.extra.in | 8 ++++++++ 6 files changed, 39 insertions(+) create mode 100644 src/secret/virt-secret-init-encryption.service.in diff --git a/libvirt.spec.in b/libvirt.spec.in index XXXXXXX..XXXXXXX 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 %attr(0755, root, root) %{_sbindir}/virtsecretd %dir %attr(0700, root, root) %{_sysconfdir}/libvirt/secrets/ +%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/secrets/ %ghost %dir %attr(0700, root, root) %{_rundir}/libvirt/secrets/ %{_libdir}/libvirt/connection-driver/libvirt_driver_secret.so %{_mandir}/man8/virtsecretd.8* diff --git a/src/meson.build b/src/meson.build index XXXXXXX..XXXXXXX 100644 --- a/src/meson.build +++ b/src/meson.build @@ -XXX,XX +XXX,XX @@ if conf.has('WITH_LIBVIRTD') 'sbindir': sbindir, 'sysconfdir': sysconfdir, 'initconfdir': initconfdir, + 'localstatedir': localstatedir, 'name': unit['name'], 'service': unit['service'], 'SERVICE': unit['service'].to_upper(), diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index XXXXXXX..XXXXXXX 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -XXX,XX +XXX,XX @@ After=libvirtd.socket After=libvirtd-ro.socket After=libvirtd-admin.socket Requires=virtlogd.socket +Requires=virt-secret-init-encryption.service +After=virt-secret-init-encryption.service Wants=virtlockd.socket After=virtlogd.socket After=virtlockd.socket @@ -XXX,XX +XXX,XX @@ Conflicts=xendomains.service Type=notify-reload Environment=LIBVIRTD_ARGS="--timeout 120" EnvironmentFile=-@initconfdir@/libvirtd +Environment=SECRETS_ENCRYPTION_KEY=%d/secrets-encryption-key +LoadCredentialEncrypted=secrets-encryption-key:@localstatedir@/lib/libvirt/secrets/secrets-encryption-key ExecStart=@sbindir@/libvirtd $LIBVIRTD_ARGS ExecReload=/bin/kill -HUP $MAINPID KillMode=process diff --git a/src/secret/meson.build b/src/secret/meson.build index XXXXXXX..XXXXXXX 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -XXX,XX +XXX,XX @@ if conf.has('WITH_SECRETS') 'name': 'virtsecretd', } + virt_secret_init_encryption_conf = configuration_data() + + virt_secret_init_encryption_conf.set('localstatedir', localstatedir) + + configure_file( + input: 'virt-secret-init-encryption.service.in', + output: '@0@.service'.format('virt-secret-init-encryption'), + configuration: virt_secret_init_encryption_conf, + install: true, + install_dir: unitdir, + ) + virt_daemon_units += { 'service': 'virtsecretd', 'name': 'secret', @@ -XXX,XX +XXX,XX @@ if conf.has('WITH_SECRETS') virt_install_dirs += [ confdir / 'secrets', runstatedir / 'libvirt' / 'secrets', + localstatedir / 'lib' / 'libvirt' / 'secrets', ] endif diff --git a/src/secret/virt-secret-init-encryption.service.in b/src/secret/virt-secret-init-encryption.service.in new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/virt-secret-init-encryption.service.in @@ -XXX,XX +XXX,XX @@ +[Unit] +Before=virtsecretd.service +Before=libvirtd.service +ConditionPathExists=!@localstatedir@/lib/libvirt/secrets/secrets-encryption-key + +[Service] +Type=oneshot +ExecStart=/usr/bin/sh -c 'umask 0077 && (dd if=/dev/random status=none bs=32 count=1 | systemd-creds encrypt --name=secrets-encryption-key - @localstatedir@/lib/libvirt/secrets/secrets-encryption-key)' diff --git a/src/secret/virtsecretd.service.extra.in b/src/secret/virtsecretd.service.extra.in index XXXXXXX..XXXXXXX 100644 --- a/src/secret/virtsecretd.service.extra.in +++ b/src/secret/virtsecretd.service.extra.in @@ -XXX,XX +XXX,XX @@ # 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:@localstatedir@/lib/libvirt/secrets/secrets-encryption-key +Environment=SECRETS_ENCRYPTION_KEY=%d/secrets-encryption-key -- 2.51.1 A new configuration file called secret.conf is introduced to let the user configure the path to the secrets encryption key. This key will be used to encrypt/decrypt the secrets in libvirt. By default the path is set to the runtime directory /run/libvirt/secrets, and it is commented in the config file. After parsing the file, the virtsecretd driver checks if an encryption key is present in the path and is valid. If no encryption key is present in the path, then the service will by default use the encryption key stored in the CREDENTIALS_DIRECTORY. Add logic to parse the encryption key file and store the key. It also checks for the encrypt_data attribute in the config file. The encryption and decryption logic will be added in the subsequent patches. Signed-off-by: Arun Menon <armenon@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/virterror.h | 1 + libvirt.spec.in | 3 + po/POTFILES | 1 + src/secret/libvirt_secrets.aug | 40 ++++++ src/secret/meson.build | 19 +++ src/secret/secret.conf.in | 14 ++ src/secret/secret_config.c | 179 +++++++++++++++++++++++++ src/secret/secret_config.h | 40 ++++++ src/secret/secret_driver.c | 11 ++ src/secret/test_libvirt_secrets.aug.in | 6 + src/util/virerror.c | 3 + 11 files changed, 317 insertions(+) create mode 100644 src/secret/libvirt_secrets.aug create mode 100644 src/secret/secret.conf.in create mode 100644 src/secret/secret_config.c create mode 100644 src/secret/secret_config.h create mode 100644 src/secret/test_libvirt_secrets.aug.in diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index XXXXXXX..XXXXXXX 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -XXX,XX +XXX,XX @@ typedef enum { command within timeout (Since: 11.2.0) */ VIR_ERR_AGENT_COMMAND_FAILED = 113, /* guest agent responded with failure to a command (Since: 11.2.0) */ + VIR_ERR_INVALID_ENCR_KEY_SECRET = 114, /* encryption key is invalid (Since: 12.1.0) */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_NUMBER_LAST /* (Since: 5.0.0) */ diff --git a/libvirt.spec.in b/libvirt.spec.in index XXXXXXX..XXXXXXX 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -XXX,XX +XXX,XX @@ 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/secret.conf %{_unitdir}/virtsecretd.service %{_unitdir}/virt-secret-init-encryption.service %{_unitdir}/virtsecretd.socket diff --git a/po/POTFILES b/po/POTFILES index XXXXXXX..XXXXXXX 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -XXX,XX +XXX,XX @@ src/rpc/virnetsshsession.c src/rpc/virnettlscert.c src/rpc/virnettlsconfig.c src/rpc/virnettlscontext.c +src/secret/secret_config.c src/secret/secret_driver.c src/security/security_apparmor.c src/security/security_dac.c diff --git a/src/secret/libvirt_secrets.aug b/src/secret/libvirt_secrets.aug new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/libvirt_secrets.aug @@ -XXX,XX +XXX,XX @@ +(* /etc/libvirt/secret.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/secret.conf" + . Util.stdexcl + + let xfm = transform lns filter diff --git a/src/secret/meson.build b/src/secret/meson.build index XXXXXXX..XXXXXXX 100644 --- a/src/secret/meson.build +++ b/src/secret/meson.build @@ -XXX,XX +XXX,XX @@ secret_driver_sources = [ 'secret_driver.c', + 'secret_config.c', ] driver_source_files += files(secret_driver_sources) @@ -XXX,XX +XXX,XX @@ if conf.has('WITH_SECRETS') ], } + secret_conf = configure_file( + input: 'secret.conf.in', + output: 'secret.conf', + copy: true + ) + virt_conf_files += secret_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('secret.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/secret.conf.in b/src/secret/secret.conf.in new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/secret.conf.in @@ -XXX,XX +XXX,XX @@ +# +# 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 on +# if secrets_encryption_key is set to a non-NULL +# path, or if a systemd credential named "secrets-encryption-key" exists. +#encrypt_data = 1 diff --git a/src/secret/secret_config.c b/src/secret/secret_config.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/secret_config.c @@ -XXX,XX +XXX,XX @@ +/* + * secret_config.c: secret.conf config file handling + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#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 "virsecureerase.h" +#include "secret_config.h" + + +#define VIR_FROM_THIS VIR_FROM_SECRET + +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/secret.conf"); + } else { + g_autofree char *configdir = NULL; + + configdir = virGetUserConfigDirectory(); + + *configfile = g_strdup_printf("%s/secret.conf", configdir); + } + + return 0; +} + + +static int +virSecretLoadDaemonConfig(virSecretDaemonConfig *cfg, + const char *filename) +{ + g_autoptr(virConf) conf = NULL; + int res; + + if (virFileExists(filename)) { + conf = virConfReadFile(filename, 0); + if (!conf) + return -1; + res = virConfGetValueBool(conf, "encrypt_data", &cfg->encryptData); + if (res < 0) { + return -1; + } else if (res == 1) { + cfg->encryptDataWasSet = true; + } else { + cfg->encryptDataWasSet = false; + } + + if (virConfGetValueString(conf, "secrets_encryption_key", + &cfg->secretsEncryptionKeyPath) < 0) { + return -1; + } + } + return 0; +} + + +static int +virGetSecretsEncryptionKey(virSecretDaemonConfig *cfg, + uint8_t **secretsEncryptionKey, + size_t *secretsKeyLen) +{ + VIR_AUTOCLOSE fd = -1; + int encryptionKeyLength; + + if ((encryptionKeyLength = virFileReadAll(cfg->secretsEncryptionKeyPath, + VIR_SECRETS_ENCRYPTION_KEY_LEN, + (char**)secretsEncryptionKey)) < 0) { + return -1; + } + if (encryptionKeyLength != VIR_SECRETS_ENCRYPTION_KEY_LEN) { + virReportError(VIR_ERR_INVALID_ENCR_KEY_SECRET, + _("Encryption key length must be '%1$d' '%2$s'"), + VIR_SECRETS_ENCRYPTION_KEY_LEN, + cfg->secretsEncryptionKeyPath); + return -1; + } + + *secretsKeyLen = (size_t)encryptionKeyLength; + return 0; +} + + +virSecretDaemonConfig * +virSecretDaemonConfigNew(bool privileged) +{ + g_autoptr(virSecretDaemonConfig) cfg = NULL; + g_autofree char *configdir = NULL; + g_autofree char *configfile = NULL; + g_autofree char *rundir = NULL; + const char *credentialsDirectory; + + if (virSecretConfigInitialize() < 0) + return NULL; + + if (!(cfg = virObjectNew(virSecretDaemonConfigClass))) + return NULL; + + if (virSecretDaemonConfigFilePath(privileged, &configfile) < 0) + return NULL; + + if (virSecretLoadDaemonConfig(cfg, configfile) < 0) + return NULL; + + credentialsDirectory = getenv("CREDENTIALS_DIRECTORY"); + + if (!cfg->secretsEncryptionKeyPath && credentialsDirectory) { + cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets-encryption-key", + credentialsDirectory); + if (!virFileExists(cfg->secretsEncryptionKeyPath)) { + g_clear_pointer(&cfg->secretsEncryptionKeyPath, g_free); + } + } + + if (!cfg->encryptDataWasSet) { + if (!cfg->secretsEncryptionKeyPath) { + /* No path specified by user or environment, disable encryption */ + cfg->encryptData = false; + } else { + cfg->encryptData = true; + } + } else { + if (cfg->encryptData) { + if (!cfg->secretsEncryptionKeyPath) { + /* Built-in default path must be used */ + rundir = virGetUserRuntimeDirectory(); + cfg->secretsEncryptionKeyPath = g_strdup_printf("%s/secrets/encryption-key", + rundir); + } + } + } + VIR_DEBUG("Secrets encryption key path: %s", NULLSTR(cfg->secretsEncryptionKeyPath)); + + if (cfg->encryptData) { + if (virGetSecretsEncryptionKey(cfg, + &cfg->secretsEncryptionKey, + &cfg->secretsKeyLen) < 0) { + return NULL; + } + } + return g_steal_pointer(&cfg); +} + + +static void +virSecretDaemonConfigDispose(void *obj) +{ + virSecretDaemonConfig *cfg = obj; + + virSecureErase(cfg->secretsEncryptionKey, cfg->secretsKeyLen); + g_free(cfg->secretsEncryptionKeyPath); +} diff --git a/src/secret/secret_config.h b/src/secret/secret_config.h new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/secret_config.h @@ -XXX,XX +XXX,XX @@ +/* + * secret_config.h: secret.conf config file handling + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#pragma once + +#include "internal.h" +#include "virinhibitor.h" +#include "secret_event.h" +#define VIR_SECRETS_ENCRYPTION_KEY_LEN 32 + +typedef struct _virSecretDaemonConfig virSecretDaemonConfig; +struct _virSecretDaemonConfig { + virObject parent; + /* secrets encryption key path from secret.conf file */ + char *secretsEncryptionKeyPath; + + /* Store the key to encrypt secrets on the disk */ + unsigned char *secretsEncryptionKey; + + size_t secretsKeyLen; + + /* Indicates if the newly written secrets are encrypted or not. + */ + bool encryptData; + + /* Indicates if the config file has encrypt_data set or not. + */ + bool encryptDataWasSet; +}; + +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/secret/secret_driver.c b/src/secret/secret_driver.c index XXXXXXX..XXXXXXX 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -XXX,XX +XXX,XX @@ #include "secret_event.h" #include "virutil.h" #include "virinhibitor.h" +#include "secret_config.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -XXX,XX +XXX,XX @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + /* Require lock to get reference on 'config', + * then lockless thereafter */ + virSecretDaemonConfig *config; }; static virSecretDriverState *driver; @@ -XXX,XX +XXX,XX @@ secretStateCleanupLocked(void) VIR_FREE(driver->configDir); virObjectUnref(driver->secretEventState); + virObjectUnref(driver->config); virInhibitorFree(driver->inhibitor); if (driver->lockFD != -1) @@ -XXX,XX +XXX,XX @@ secretStateInitialize(bool privileged, driver->stateDir); goto error; } + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + goto error; driver->inhibitor = virInhibitorNew( VIR_INHIBITOR_WHAT_NONE, @@ -XXX,XX +XXX,XX @@ secretStateReload(void) if (!driver) return -1; + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + return -1; + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); return 0; diff --git a/src/secret/test_libvirt_secrets.aug.in b/src/secret/test_libvirt_secrets.aug.in new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/src/secret/test_libvirt_secrets.aug.in @@ -XXX,XX +XXX,XX @@ +module Test_libvirt_secrets = + @CONFIG@ + + test Libvirt_secrets.lns get conf = +{ "secrets_encryption_key" = "/run/libvirt/secrets/secret-encryption-key" } +{ "encrypt_data" = "1" } diff --git a/src/util/virerror.c b/src/util/virerror.c index XXXXXXX..XXXXXXX 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -XXX,XX +XXX,XX @@ static const virErrorMsgTuple virErrorMsgStrings[] = { [VIR_ERR_AGENT_COMMAND_FAILED] = { N_("guest agent command failed"), N_("guest agent command failed: %1$s") }, + [VIR_ERR_INVALID_ENCR_KEY_SECRET] = { + N_("Invalid encryption key for the secret"), + N_("Invalid encryption key for the secret: %1$s") }, }; G_STATIC_ASSERT(G_N_ELEMENTS(virErrorMsgStrings) == VIR_ERR_NUMBER_LAST); -- 2.51.1 Change the attribute name of _virSecretObj because we want it to have a generic name to indicate that secret values can be stored in it in both base64 and encrypted formats. Signed-off-by: Arun Menon <armenon@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virsecretobj.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index XXXXXXX..XXXXXXX 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -XXX,XX +XXX,XX @@ VIR_LOG_INIT("conf.virsecretobj"); struct _virSecretObj { virObjectLockable parent; char *configFile; - char *base64File; + char *secretValueFile; virSecretDef *def; unsigned char *value; /* May be NULL */ size_t value_size; @@ -XXX,XX +XXX,XX @@ virSecretObjDispose(void *opaque) g_free(obj->value); } g_free(obj->configFile); - g_free(obj->base64File); + g_free(obj->secretValueFile); } @@ -XXX,XX +XXX,XX @@ virSecretObjListAdd(virSecretObjList *secrets, if (!(obj = virSecretObjNew())) goto cleanup; - /* Generate the possible configFile and base64File strings + /* Generate the possible configFile and secretValueFile strings * using the configDir, uuidstr, and appropriate suffix */ if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup; if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) @@ -XXX,XX +XXX,XX @@ virSecretObjDeleteData(virSecretObj *obj) { /* The configFile will already be removed, so secret won't be * loaded again if this fails */ - unlink(obj->base64File); + unlink(obj->secretValueFile); } @@ -XXX,XX +XXX,XX @@ virSecretObjSaveData(virSecretObj *obj) base64 = g_base64_encode(obj->value, obj->value_size); - if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) + if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) < 0) return -1; return 0; @@ -XXX,XX +XXX,XX @@ virSecretLoadValue(virSecretObj *obj) struct stat st; g_autofree char *contents = NULL; - if ((fd = open(obj->base64File, O_RDONLY)) == -1) { + if ((fd = open(obj->secretValueFile, O_RDONLY)) == -1) { if (errno == ENOENT) { ret = 0; goto cleanup; } virReportSystemError(errno, _("cannot open '%1$s'"), - obj->base64File); + obj->secretValueFile); goto cleanup; } if (fstat(fd, &st) < 0) { virReportSystemError(errno, _("cannot stat '%1$s'"), - obj->base64File); + obj->secretValueFile); goto cleanup; } if ((size_t)st.st_size != st.st_size) { virReportError(VIR_ERR_INTERNAL_ERROR, _("'%1$s' file does not fit in memory"), - obj->base64File); + obj->secretValueFile); goto cleanup; } @@ -XXX,XX +XXX,XX @@ virSecretLoadValue(virSecretObj *obj) if (saferead(fd, contents, st.st_size) != st.st_size) { virReportSystemError(errno, _("cannot read '%1$s'"), - obj->base64File); + obj->secretValueFile); goto cleanup; } contents[st.st_size] = '\0'; -- 2.51.1 Now that we have the functionality to provide the secrets driver with an encryption key through a configuration file or using system credentials, and the newly introduced array to iterate over the encryption schemes, we can use the key to save and load secrets. Encrypt all secrets that are going to be saved on the disk if the 'secrets_encryption_key' path is set in the secret.conf file OR if a valid systemd generated credential exists. While loading secrets, identify the decryption method by matching the file extension of the stored secret against the known array values. If no matching scheme is found, the secret is skipped. If the encryption key is changed across restarts, then also the secret driver will fail to load the secrets from the disk that were encrypted with the former key. Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/virsecretobj.c | 249 ++++++++++++++++++++++++++++--------- src/conf/virsecretobj.h | 16 ++- src/secret/secret_driver.c | 20 ++- 3 files changed, 222 insertions(+), 63 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index XXXXXXX..XXXXXXX 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -XXX,XX +XXX,XX @@ #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 @@ -XXX,XX +XXX,XX @@ struct _virSecretObj { size_t value_size; }; +typedef struct _virSecretSchemeInfo { + const char *suffix; + virCryptoCipher cipher; +} virSecretSchemeInfo; + +virSecretSchemeInfo schemeInfo[] = { + { ".aes256cbc", VIR_CRYPTO_CIPHER_AES256CBC }, + { ".base64", -1 }, +}; + static virClass *virSecretObjClass; static virClass *virSecretObjListClass; static void virSecretObjDispose(void *obj); @@ -XXX,XX +XXX,XX @@ virSecretObjListAdd(virSecretObjList *secrets, if (!(obj = virSecretObjNew())) goto cleanup; - /* Generate the possible configFile and secretValueFile strings - * using the configDir, uuidstr, and appropriate suffix + * using the configDir, uuidstr, and appropriate suffix. + * Note that secretValueFile extension is not set here. It is determined + * based on a) existing availability of secret file (virSecretLoadValue) or + * b) target storage format (virSecretObjSaveData) */ if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, ".base64"))) + !(obj->secretValueFile = virFileBuildPath(configDir, uuidstr, NULL))) goto cleanup; if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) @@ -XXX,XX +XXX,XX @@ virSecretObjSaveConfig(virSecretObj *obj) int -virSecretObjSaveData(virSecretObj *obj) +virSecretObjSaveData(virSecretObj *obj, + const char *configDir, + bool encryptData, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + g_autofree char *base64 = NULL; + g_autofree char *newSecretFile = NULL; + g_autofree uint8_t *secret = NULL; + g_autofree uint8_t *encryptedValue = NULL; + + const char *selectedSuffix = NULL; + size_t encryptedValueLen = 0; + + size_t i; + size_t secretLen = 0; + uint8_t iv[16] = { 0 }; if (!obj->value) return 0; - base64 = g_base64_encode(obj->value, obj->value_size); + virUUIDFormat(obj->def->uuid, uuidstr); + + /* Based on whether encryption is on/off, save the secret in either + * the latest encryption scheme or in base64 formats. + * Subsequently, delete the other formats of the same uuid on the disk. + */ + if (encryptData && secretsEncryptionKey) { + selectedSuffix = schemeInfo[0].suffix; + if (virRandomBytes(iv, sizeof(iv)) < 0) { + return -1; + } + if (virCryptoEncryptData(schemeInfo[0].cipher, + secretsEncryptionKey, secretsKeyLen, + iv, sizeof(iv), + (uint8_t *)obj->value, obj->value_size, + &encryptedValue, &encryptedValueLen) < 0) { + return -1; + } + secretLen = sizeof(iv) + encryptedValueLen; + secret = g_new0(uint8_t, secretLen); + memcpy(secret, iv, sizeof(iv)); + memcpy(secret + sizeof(iv), encryptedValue, encryptedValueLen); + base64 = g_base64_encode(secret, secretLen); + } else { + int baseElement = G_N_ELEMENTS(schemeInfo) - 1; + selectedSuffix = schemeInfo[baseElement].suffix; + base64 = g_base64_encode(obj->value, obj->value_size); + } + + if (!(newSecretFile = virFileBuildPath(configDir, uuidstr, selectedSuffix))) { + return -1; + } + g_free(obj->secretValueFile); + obj->secretValueFile = g_steal_pointer(&newSecretFile); if (virFileRewriteStr(obj->secretValueFile, S_IRUSR | S_IWUSR, base64) < 0) return -1; + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) { + g_autofree char* deleteFile = virFileBuildPath(configDir, + uuidstr, + schemeInfo[i].suffix); + if (STRNEQ_NULLABLE(schemeInfo[i].suffix, selectedSuffix)) { + unlink(deleteFile); + } + } return 0; } @@ -XXX,XX +XXX,XX @@ virSecretObjGetValue(virSecretObj *obj) int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size) + size_t value_size, + const char *configDir, + bool encryptData, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen) { virSecretDef *def = obj->def; g_autofree unsigned char *old_value = NULL; @@ -XXX,XX +XXX,XX @@ virSecretObjSetValue(virSecretObj *obj, obj->value = g_steal_pointer(&new_value); obj->value_size = value_size; - if (!def->isephemeral && virSecretObjSaveData(obj) < 0) + if (!def->isephemeral && virSecretObjSaveData(obj, + configDir, + encryptData, + secretsEncryptionKey, + secretsKeyLen) < 0) goto error; /* Saved successfully - drop old value */ @@ -XXX,XX +XXX,XX @@ virSecretLoadValidateUUID(virSecretDef *def, static int -virSecretLoadValue(virSecretObj *obj) +virSecretLoadValue(virSecretObj *obj, + const char *configDir, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen) { - int ret = -1, fd = -1; + int ret = -1; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + VIR_AUTOCLOSE fd = -1; struct stat st; - g_autofree char *contents = NULL; + size_t i; - if ((fd = open(obj->secretValueFile, O_RDONLY)) == -1) { - if (errno == ENOENT) { - ret = 0; + g_autofree char *contents = NULL; + g_autofree uint8_t *contentsEncrypted = NULL; + g_autofree uint8_t *decryptedValue = NULL; + + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0; + + virUUIDFormat(obj->def->uuid, uuidstr); + + /* Iterate over the list of suffixes, find the one that when appended to the + * uuid will result in a file that exists on the disk. This essentially is the + * secret file. Subsequently, load/decrypt the secret by using the appropriate + * encryption scheme. + */ + for (i = 0; i < G_N_ELEMENTS(schemeInfo); i++) { + g_autofree char *candidatePath = NULL; + if (!(candidatePath = virFileBuildPath(configDir, + uuidstr, + schemeInfo[i].suffix))) { goto cleanup; } - virReportSystemError(errno, _("cannot open '%1$s'"), - obj->secretValueFile); - goto cleanup; - } - - if (fstat(fd, &st) < 0) { - virReportSystemError(errno, _("cannot stat '%1$s'"), - obj->secretValueFile); - goto cleanup; - } - - if ((size_t)st.st_size != st.st_size) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%1$s' file does not fit in memory"), - obj->secretValueFile); - goto cleanup; - } - - if (st.st_size < 1) { - ret = 0; - 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->secretValueFile); - goto cleanup; + if (virFileExists(candidatePath)) { + if ((fd = open(candidatePath, O_RDONLY)) == -1) { + if (errno == ENOENT) { + ret = 0; + } else { + virReportSystemError(errno, _("cannot open '%1$s'"), + candidatePath); + } + goto cleanup; + } + if (fstat(fd, &st) < 0) { + virReportSystemError(errno, _("cannot stat '%1$s'"), + candidatePath); + goto cleanup; + } + if ((size_t)st.st_size != st.st_size) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%1$s' file does not fit in memory"), + candidatePath); + goto cleanup; + } + if (st.st_size < 1) { + ret = 0; + 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'"), + candidatePath); + goto cleanup; + } + contents[st.st_size] = '\0'; + if (schemeInfo[i].cipher != -1) { + contentsEncrypted = g_base64_decode(contents, &obj->value_size); + if (sizeof(iv) > obj->value_size) { + virReportError(VIR_ERR_INVALID_SECRET, + _("Encrypted secret size '%1$zu' is invalid"), + obj->value_size); + goto cleanup; + } + memcpy(iv, contentsEncrypted, sizeof(iv)); + ciphertext = contentsEncrypted + sizeof(iv); + ciphertextLen = obj->value_size - sizeof(iv); + if (virCryptoDecryptData(schemeInfo[i].cipher, + secretsEncryptionKey, secretsKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + goto cleanup; + } + g_free(obj->value); + obj->value = g_steal_pointer(&decryptedValue); + obj->value_size = decryptedValueLen; + } else { + obj->value = g_base64_decode(contents, &obj->value_size); + } + + g_free(obj->secretValueFile); + obj->secretValueFile = g_steal_pointer(&candidatePath); + + break; + } } - 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); - VIR_FORCE_CLOSE(fd); + virSecureErase(contentsEncrypted, obj->value_size); + virSecureErase(contents, st.st_size); + virSecureErase(iv, sizeof(iv)); return ret; } @@ -XXX,XX +XXX,XX @@ static virSecretObj * virSecretLoad(virSecretObjList *secrets, const char *file, const char *path, - const char *configDir) + const char *configDir, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen) { g_autoptr(virSecretDef) def = NULL; virSecretObj *obj = NULL; @@ -XXX,XX +XXX,XX @@ virSecretLoad(virSecretObjList *secrets, if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL))) return NULL; - if (virSecretLoadValue(obj) < 0) { + if (virSecretLoadValue(obj, configDir, secretsEncryptionKey, secretsKeyLen) < 0) { virSecretObjListRemove(secrets, obj); g_clear_pointer(&obj, virObjectUnref); return NULL; @@ -XXX,XX +XXX,XX @@ virSecretLoad(virSecretObjList *secrets, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir) + const char *configDir, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen) { g_autoptr(DIR) dir = NULL; struct dirent *de; @@ -XXX,XX +XXX,XX @@ 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, + secretsEncryptionKey, + secretsKeyLen))) { VIR_ERROR(_("Error reading secret: %1$s"), virGetLastErrorMessage()); continue; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index XXXXXXX..XXXXXXX 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -XXX,XX +XXX,XX @@ int virSecretObjSaveConfig(virSecretObj *obj); int -virSecretObjSaveData(virSecretObj *obj); +virSecretObjSaveData(virSecretObj *obj, + const char *configDir, + bool encryptData, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen); virSecretDef * virSecretObjGetDef(virSecretObj *obj); @@ -XXX,XX +XXX,XX @@ virSecretObjGetValue(virSecretObj *obj); int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size); + size_t value_size, + const char *configDir, + bool encryptData, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen); size_t virSecretObjGetValueSize(virSecretObj *obj); @@ -XXX,XX +XXX,XX @@ virSecretObjSetValueSize(virSecretObj *obj, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir); + const char *configDir, + uint8_t *secretsEncryptionKey, + size_t secretsKeyLen); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index XXXXXXX..XXXXXXX 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -XXX,XX +XXX,XX @@ secretDefineXML(virConnectPtr conn, if (!objDef->isephemeral) { if (backup && backup->isephemeral) { - if (virSecretObjSaveData(obj) < 0) + if (virSecretObjSaveData(obj, + driver->configDir, + driver->config->encryptData, + driver->config->secretsEncryptionKey, + driver->config->secretsKeyLen) < 0) goto restore_backup; } @@ -XXX,XX +XXX,XX @@ secretSetValue(virSecretPtr secret, if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup; - if (virSecretObjSetValue(obj, value, value_size) < 0) + if (virSecretObjSetValue(obj, value, value_size, + driver->configDir, + driver->config->encryptData, + driver->config->secretsEncryptionKey, + driver->config->secretsKeyLen) < 0) goto cleanup; event = virSecretEventValueChangedNew(def->uuid, @@ -XXX,XX +XXX,XX @@ secretStateInitialize(bool privileged, if (!(driver->secrets = virSecretObjListNew())) goto error; - if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) + if (virSecretLoadAllConfigs(driver->secrets, driver->configDir, + driver->config->secretsEncryptionKey, + driver->config->secretsKeyLen) < 0) goto error; return VIR_DRV_STATE_INIT_COMPLETE; @@ -XXX,XX +XXX,XX @@ secretStateReload(void) if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) return -1; - ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir, + driver->config->secretsEncryptionKey, + driver->config->secretsKeyLen)); return 0; } -- 2.51.1 Document the new encryption of secrets feature in secretencryption.rst. Signed-off-by: Arun Menon <armenon@redhat.com> --- docs/drvsecret.rst | 4 ++ docs/meson.build | 1 + docs/secretencryption.rst | 105 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+) create mode 100644 docs/secretencryption.rst diff --git a/docs/drvsecret.rst b/docs/drvsecret.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/drvsecret.rst +++ b/docs/drvsecret.rst @@ -XXX,XX +XXX,XX @@ the traditional system or session libvirt connections to QEMU. Normal practice would be to open the secret driver in embedded mode any time one of the other drivers is opened in embedded mode so that the two drivers can interact in-process. + +Further reading +---------------------------- +- `Secret Encryption <secretencryption.html>`__ diff --git a/docs/meson.build b/docs/meson.build index XXXXXXX..XXXXXXX 100644 --- a/docs/meson.build +++ b/docs/meson.build @@ -XXX,XX +XXX,XX @@ docs_rst_files = [ 'programming-languages', 'python', 'remote', + 'secretencryption', 'securityprocess', 'ssh-proxy', 'storage', diff --git a/docs/secretencryption.rst b/docs/secretencryption.rst new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/docs/secretencryption.rst @@ -XXX,XX +XXX,XX @@ +.. role:: since + +============================= +Secret storage and encryption +============================= + +.. contents:: + +The secret objects can either be ephemeral or persistent. +Ephemeral secrets are only kept in memory, never stored persistently on the disk. +See `Secrets <formatsecret.html>`__ + +:since:`Since 12.1.0` if a secret is defined as persistent, then it is stored **encrypted** on the disk. + + +Systemd Credentials Sealing +--------------------------- + +Out of the box, secrets are sealed using systemd credentials. This ties the +encrypted secret files to the specific host. + +The `virt-secret-init-encryption` service automatically generates a random +32-byte key and encrypts it using `systemd-creds`, storing the result in +`/var/lib/libvirt/secrets/secrets-encryption-key`. The `virtsecretd` service +then automatically loads this key securely via the systemd `LoadCredentialEncrypted` +mechanism. + +Disabling Systemd Credentials +----------------------------- + +You can control encryption behavior by editing the `secret.conf` configuration +file located in ``@SYSCONFDIR@/libvirt/secret.conf`` or ``$XDG_CONFIG_HOME/libvirt/secret.conf`` +depending on how the daemon was started (system mode or session mode respectively). + +To **disable encryption entirely** (which effectively disables the use of any +systemd credentials for this purpose): + +:: + + encrypt_data = 0 + +Setting ``encrypt_data = 0`` takes precedence over any available systemd +credentials. If you have existing encrypted secrets, this setting will prevent +the secret driver from loading the encryption key, making those secrets +inaccessible. New or updated secrets will be stored in plain base64 format. + +To **use a custom encryption key** instead of the systemd credential. +Defining a custom key path takes precedence over the systemd credential + +:: + + secrets_encryption_key = "/path/to/custom/key" + +Configuring Encryption on Non-Systemd Hosts +------------------------------------------- + +On hosts without systemd, or if you prefer to manage the key manually, you can +create a raw encryption key and configure libvirt to use it. + +Generate a random 32-byte key: + +:: + + dd if=/dev/random of=/path/to/key/file bs=32 count=1 + +Update `secret.conf` to point to this key: + +:: + + secrets_encryption_key = "/path/to/key/file" + +Manual Systemd Credential Creation +---------------------------------- + +If you want to use systemd credentials but need to customize the encryption parameters +(for example, to specify which TPM PCRs to bind to), you can generate the +credential file manually. + +To create the default `/var/lib/libvirt/secrets/secrets-encryption-key` manually +using `systemd-creds` (adjusting arguments to `systemd-creds encrypt` as needed): + +:: + + dd if=/dev/random bs=32 count=1 | \ + systemd-creds encrypt --name=secrets-encryption-key - \ + /var/lib/libvirt/secrets/secrets-encryption-key + +You can pass extra arguments to `systemd-creds encrypt <https://www.freedesktop.org/software/systemd/man/latest/systemd-creds.html?#encrypt%20input%7C-%20output%7C->`__, +such as ``--tpm2-device=...`` or ``--tpm2-pcrs=...``, to customize the sealing policy. + +Upgrading Libvirt for secret encryption +--------------------------------------- +Starting 12.1.0, secrets can be stored on the disk in an encrypted format, rather than +the default base64 encoding. + +Any secret created before upgrading libvirt, remain stored in their original base64 +format on the disk. +A pre-existing secret will only be encrypted if you explicitly update its value using +**virsh secret-set-value** after the upgrade, provided that encryption is enabled in +secret.conf configuration file. + +It is important to note that encrypted secrets are not backwards compatible. In case of +a downgrade to an older version of libvirt, the encrypted secrets will not be loaded from +the disk. Therefore, before reverting to an older version libvirt, make sure that all the +secrets have been reverted to the standard base64 format, to avoid service disruptions. -- 2.51.1