[libvirt] [PATCH v5 0/6] Add AES Secret Object support (for RBD only)
This is a combination of two series... The first 2 patches are a followon to (v1 from yesterday): http://www.redhat.com/archives/libvir-list/2016-May/msg01396.html But there really were a offshoot of the original AES/IV Secret changes (v4): http://www.redhat.com/archives/libvir-list/2016-May/msg01292.html Hopefully I haven't forgotten anything along the way. There's been numerous adjustments and changes along the way. Patch 1 is a combination with adjustments of patches 1&2 from v1. This should make virRandomBytes available in the virrandommock library which then is used in later patches. Patch 2 mostly adjust names, comments, adds #ifdef's for unavailable code This patch will make use of the virrandommock library instead of self-populating the enc_alg and iv_buf. Mainly because it's possible, but also since it's the basis for later patches to utilize the same virrandommock library. Patch 3 splits out the existing qemuDomainGenerateRandomKey into a vircrypto.c API. The vircrypto.c was chosen over virrandom.c because virrandom.c ends up being included in setuid_rpc_client and it wasn't overly clear that it was desired to drag in all of gnutls for just this one mock function. Patch 4 splits out the qemuDomainSecretSetup as was suggested in one review Patch 5 is new to handle the ability to have more than one mock library to preload from a VIRT_TEST_MAIN_PRELOAD macro. As it turns out the qemuxml2argvtest will need not only the qemuxml2argvmock, but also the virrandommock libraries. I went with comma separated, but a "space" separated list is fine with me too. Patch 6 is the remainder of the v4 of the original series. Splitting it up with ATTRIBUTE_UNUSED markers just no longer made sense. Lots of changes here to keep up with the previous patches, but also to adjust error messages, variable/API names, etc. Also changed were the secret alias (leading to adjustments in each of the .args file for the secret alias. Along the way I also had to adjust the expected encoded ciphertext and iv since the mock algorithm changed from all 0xff to an increasing sequence starting at 0x00 through the length of the buffer. John Ferlan (6): tests: Add mock for virRandomBytes util: Introduce encryption APIs util: Introduce virCryptoGenerateRandom qemu: Introduce qemuDomainSecretSetup tests: Allow comma separate list of libs to preload qemu: Utilize qemu secret objects for RBD auth/secret configure.ac | 1 + src/libvirt_private.syms | 3 + src/qemu/qemu_alias.c | 23 ++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 117 ++++++++++- src/qemu/qemu_domain.c | 200 +++++++++++++----- src/util/vircrypto.c | 233 ++++++++++++++++++++- src/util/vircrypto.h | 22 +- tests/Makefile.am | 12 ++ ...muxml2argv-disk-drive-network-rbd-auth-AES.args | 31 +++ ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml | 42 ++++ tests/qemuxml2argvmock.c | 16 ++ tests/qemuxml2argvtest.c | 5 +- tests/testutils.c | 12 +- tests/vircryptotest.c | 100 ++++++++- tests/virrandommock.c | 39 ++++ tests/virrandomtest.c | 86 ++++++++ 17 files changed, 879 insertions(+), 65 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml create mode 100644 tests/virrandommock.c create mode 100644 tests/virrandomtest.c -- 2.5.5
Create a mock for virRandomBytes to generate a not so random value. This should be usable by other tests that need a not so random number to be generated by including the virrandommock at preload. The "random number" generated is based upon the size of the expected stream of bytes being returned where each byte in the result gets the index of the array - hence a 4 byte array returns 0x00010203. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/Makefile.am | 12 +++++++ tests/virrandommock.c | 39 +++++++++++++++++++++++ tests/virrandomtest.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 tests/virrandommock.c create mode 100644 tests/virrandomtest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index c7c9a03..98b8bb9 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -180,6 +180,7 @@ test_programs = virshtest sockettest \ virbitmaptest \ vircgrouptest \ vircryptotest \ + virrandomtest \ virpcitest \ virendiantest \ virfiletest \ @@ -423,6 +424,7 @@ test_libraries = libshunload.la \ vircgroupmock.la \ virpcimock.la \ virnetdevmock.la \ + virrandommock.la \ nodeinfomock.la \ nssmock.la \ $(NULL) @@ -1080,6 +1082,10 @@ vircryptotest_SOURCES = \ vircryptotest.c testutils.h testutils.c vircryptotest_LDADD = $(LDADDS) +virrandomtest_SOURCES = \ + virrandomtest.c testutils.h testutils.c +virrandomtest_LDADD = $(LDADDS) + virhostdevtest_SOURCES = \ virhostdevtest.c testutils.h testutils.c virhostdevtest_LDADD = $(LDADDS) @@ -1094,6 +1100,12 @@ virpcimock_la_CFLAGS = $(AM_CFLAGS) virpcimock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) virpcimock_la_LIBADD = $(MOCKLIBS_LIBS) +virrandommock_la_SOURCES = \ + virrandommock.c +virrandommock_la_CFLAGS = $(AM_CFLAGS) +virrandommock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +virrandommock_la_LIBADD = $(MOCKLIBS_LIBS) + nodeinfomock_la_SOURCES = \ nodeinfomock.c nodeinfomock_la_CFLAGS = $(AM_CFLAGS) diff --git a/tests/virrandommock.c b/tests/virrandommock.c new file mode 100644 index 0000000..6df5e20 --- /dev/null +++ b/tests/virrandommock.c @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2016 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/>. + * + * Author: John Ferlan <jferlan@redhat.com> + */ + +#include <config.h> + +#include "internal.h" +#include "virrandom.h" +#include "virmock.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +int +virRandomBytes(unsigned char *buf, + size_t buflen) +{ + size_t i; + + for (i = 0; i < buflen; i++) + buf[i] = i; + + return 0; +} diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c new file mode 100644 index 0000000..f76911f --- /dev/null +++ b/tests/virrandomtest.c @@ -0,0 +1,86 @@ +/* + * Copyright (C) 2016 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/>. + * + * Author: John Ferlan <jferlan@redhat.com> + */ + +#include <config.h> + +#include "internal.h" +#include "viralloc.h" +#include "virrandom.h" +#include "testutils.h" + +#ifndef WIN32 + +# define VIR_FROM_THIS VIR_FROM_NONE + +static int +testRandomBytes(const void *unused ATTRIBUTE_UNUSED) +{ + int ret = -1; + size_t i; + uint8_t *data; + size_t datalen = 32; + + if (VIR_ALLOC_N(data, datalen) < 0) + return -1; + + if (virRandomBytes(data, datalen) < 0) { + fprintf(stderr, "Failed to generate random bytes"); + goto cleanup; + } + + for (i = 0; i < datalen; i++) { + if (data[i] != i) { + fprintf(stderr, + "virRandomBytes data[%zu]='%x' not in seqence\n", + i, data[i]); + goto cleanup; + } + } + + ret = 0; + + cleanup: + VIR_FREE(data); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("RandomBytes", testRandomBytes, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virrandommock.so") + +#else + +int +main(void) +{ + return EXIT_AM_SKIP; +} + +#endif -- 2.5.5
On Thu, May 19, 2016 at 16:29:00 -0400, John Ferlan wrote:
Create a mock for virRandomBytes to generate a not so random value. This should be usable by other tests that need a not so random number to be generated by including the virrandommock at preload.
The "random number" generated is based upon the size of the expected stream of bytes being returned where each byte in the result gets the index of the array - hence a 4 byte array returns 0x00010203.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/Makefile.am | 12 +++++++ tests/virrandommock.c | 39 +++++++++++++++++++++++ tests/virrandomtest.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 tests/virrandommock.c create mode 100644 tests/virrandomtest.c
[...]
diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c new file mode 100644 index 0000000..f76911f --- /dev/null +++ b/tests/virrandomtest.c @@ -0,0 +1,86 @@
[...]
+ + for (i = 0; i < datalen; i++) { + if (data[i] != i) { + fprintf(stderr, + "virRandomBytes data[%zu]='%x' not in seqence\n"
This is a very amusing test :). Also this is basically meta-testing since it's testing that the tests test correctly.
+ i, data[i]); + goto cleanup;
ACK
Introduce virCryptoHaveCipher and virCryptoEncryptData to handle performing encryption. virCryptoHaveCipher: Boolean function to determine whether the requested cipher algorithm is available. It's expected this API will be called prior to virCryptoEncryptdata. It will return true/false. virCryptoEncryptData: Based on the requested cipher type, call the specific encryption API to encrypt the data. Currently the only algorithm support is the AES 256 CBC encryption. Adjust tests for the API's Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 1 + src/libvirt_private.syms | 2 + src/util/vircrypto.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 20 ++++- tests/vircryptotest.c | 100 +++++++++++++++++++++++- 5 files changed, 312 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 378069d..10fbd20 100644 --- a/configure.ac +++ b/configure.ac @@ -1261,6 +1261,7 @@ if test "x$with_gnutls" != "xno"; then ]]) AC_CHECK_FUNCS([gnutls_rnd]) + AC_CHECK_FUNCS([gnutls_cipher_encrypt]) CFLAGS="$old_CFLAGS" LIBS="$old_LIBS" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 868bcfa..6c02b10 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1394,7 +1394,9 @@ virConfWriteMem; # util/vircrypto.h +virCryptoEncryptData; virCryptoHashString; +virCryptoHaveCipher; # util/virdbus.h diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 39a479a..b8f5554 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -1,7 +1,7 @@ /* * vircrypto.c: cryptographic helper APIs * - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014, 2016 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 @@ -21,11 +21,20 @@ #include <config.h> #include "vircrypto.h" +#include "virlog.h" #include "virerror.h" #include "viralloc.h" #include "md5.h" #include "sha256.h" +#ifdef WITH_GNUTLS +# include <gnutls/gnutls.h> +# if HAVE_GNUTLS_CRYPTO_H +# include <gnutls/crypto.h> +# endif +#endif + +VIR_LOG_INIT("util.crypto"); #define VIR_FROM_THIS VIR_FROM_CRYPTO @@ -76,3 +85,184 @@ virCryptoHashString(virCryptoHash hash, return 0; } + + +/* virCryptoHaveCipher: + * @algorithm: Specific cipher algorithm desired + * + * Expected to be called prior to virCryptoEncryptData in order + * to determine whether the requested encryption option is available, + * so that "other" alternatives can be taken if the algorithm is + * not available. + * + * Returns true if we can support the encryption. + */ +bool +virCryptoHaveCipher(virCryptoCipher algorithm) +{ + switch (algorithm) { + + case VIR_CRYPTO_CIPHER_AES256CBC: +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + return true; +#else + return false; +#endif + + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + }; + + return false; +} + + +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT +/* virCryptoEncryptDataAESgntuls: + * + * Performs the AES gnutls encryption + * + * Same input as virCryptoEncryptData, except the algoritm is replaced + * by the specific gnutls algorithm. + * + * Encrypts the @data buffer using the @enckey and if available the @iv + * + * Returns 0 on success with the ciphertext being filled. It is the + * caller's responsibility to clear and free it. Returns -1 on failure + * w/ error set. + */ +static int +virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, + uint8_t *enckey, + size_t enckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **ciphertextret, + size_t *ciphertextlenret) +{ + int rc; + size_t i; + gnutls_cipher_hd_t handle = NULL; + gnutls_datum_t enc_key; + gnutls_datum_t iv_buf; + uint8_t *ciphertext; + size_t ciphertextlen; + + /* Allocate a padded buffer, copy in the data */ + ciphertextlen = VIR_ROUND_UP(datalen, 16); + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0) + return -1; + memcpy(ciphertext, data, datalen); + + /* Fill in the padding of the buffer with the size of the padding + * which is required for decryption. */ + for (i = datalen; i < ciphertextlen; i++) + ciphertext[i] = ciphertextlen - datalen; + + /* Initialize the gnutls cipher */ + enc_key.size = enckeylen; + enc_key.data = enckey; + if (iv) { + iv_buf.size = ivlen; + iv_buf.data = iv; + } + if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg, + &enc_key, &iv_buf)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + /* Encrypt the data and free the memory for cipher operations */ + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); + gnutls_cipher_deinit(handle); + if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to encrypt the data: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + *ciphertextret = ciphertext; + *ciphertextlenret = ciphertextlen; + return 0; + + error: + VIR_DISPOSE_N(ciphertext, ciphertextlen); + return -1; +} +#endif + + +/* virCryptoEncryptData: + * @algorithm: algoritm desired for encryption + * @enckey: encryption key + * @enckeylen: encription key length + * @iv: initialization vector + * @ivlen: length of initialization vector + * @data: data to encrypt + * @datalen: length of data + * @ciphertext: stream of bytes allocated to store ciphertext + * @ciphertextlen: size of the stream of bytes + * + * If available, attempt and return the requested encryption type + * using the parameters passed. + * + * Returns 0 on success, -1 on failure with error set + */ +int +virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey, + size_t enckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **ciphertext, + size_t *ciphertextlen) +{ + switch (algorithm) { + case VIR_CRYPTO_CIPHER_AES256CBC: + if (enckeylen != 32) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC encryption invalid keylen=%zu"), + enckeylen); + return -1; + } + + if (ivlen != 16) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC initialization vector invalid len=%zu"), + ivlen); + return -1; + } + +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + /* + * Encrypt the data buffer using an encryption key and + * initialization vector via the gnutls_cipher_encrypt API + * for GNUTLS_CIPHER_AES_256_CBC. + */ + return virCryptoEncryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC, + enckey, enckeylen, iv, ivlen, + data, datalen, + ciphertext, ciphertextlen); +#else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("no encryption algorithm exists")); + return -1; +#endif + + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + } + + virReportError(VIR_ERR_INVALID_ARG, + _("algorithm=%d is not supported"), algorithm); + return -1; +} diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h index f67d49d..f0ec07b 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -1,7 +1,7 @@ /* * vircrypto.h: cryptographic helper APIs * - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014, 2016 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 @@ -30,6 +30,14 @@ typedef enum { VIR_CRYPTO_HASH_LAST } virCryptoHash; + +typedef enum { + VIR_CRYPTO_CIPHER_NONE = 0, + VIR_CRYPTO_CIPHER_AES256CBC, + + VIR_CRYPTO_CIPHER_LAST +} virCryptoCipher; + int virCryptoHashString(virCryptoHash hash, const char *input, @@ -37,4 +45,14 @@ virCryptoHashString(virCryptoHash hash, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +bool virCryptoHaveCipher(virCryptoCipher algorithm); + +int virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey, size_t enckeylen, + uint8_t *iv, size_t ivlen, + uint8_t *data, size_t datalen, + uint8_t **ciphertext, size_t *ciphertextlen) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(8) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_CRYPTO_H__ */ diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index bfc47db..1d719d9 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -21,9 +21,11 @@ #include <config.h> #include "vircrypto.h" +#include "virrandom.h" #include "testutils.h" +#define VIR_FROM_THIS VIR_FROM_NONE struct testCryptoHashData { virCryptoHash hash; @@ -56,10 +58,82 @@ testCryptoHash(const void *opaque) } +struct testCryptoEncryptData { + virCryptoCipher algorithm; + uint8_t *input; + size_t inputlen; + uint8_t *ciphertext; + size_t ciphertextlen; +}; + +static int +testCryptoEncrypt(const void *opaque) +{ + const struct testCryptoEncryptData *data = opaque; + size_t i; + uint8_t *enckey = NULL; + size_t enckeylen = 32; + uint8_t *iv = NULL; + size_t ivlen = 16; + uint8_t *ciphertext = NULL; + size_t ciphertextlen = 0; + int ret = -1; + + if (!virCryptoHaveCipher(data->algorithm)) { + fprintf(stderr, "Invalid cipher algorithm=%d\n", data->algorithm); + return -1; + } + + if (VIR_ALLOC_N(enckey, enckeylen) < 0 || + VIR_ALLOC_N(iv, ivlen) < 0) + goto cleanup; + + if (virRandomBytes(enckey, enckeylen) < 0 || + virRandomBytes(iv, ivlen) < 0) + goto cleanup; + + if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen, + data->input, data->inputlen, + &ciphertext, &ciphertextlen) < 0) + goto cleanup; + + if (data->ciphertextlen != ciphertextlen) { + fprintf(stderr, "Expected ciphertextlen(%zu) doesn't match (%zu)\n", + data->ciphertextlen, ciphertextlen); + goto cleanup; + } + + if (memcmp(data->ciphertext, ciphertext, ciphertextlen)) { + fprintf(stderr, "Expected ciphertext doesn't match\n"); + for (i = 0; i < ciphertextlen; i++) { + if (data->ciphertext[i] != ciphertext[i]) { + fprintf(stderr, "expected[%zu]=%x ciphertext[%zu]=%x\n", + i, data->ciphertext[i], + i, ciphertext[i]); + } + } + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(enckey); + VIR_FREE(iv); + VIR_FREE(ciphertext); + + return ret; +} + + static int mymain(void) { int ret = 0; + uint8_t secretdata[8]; + uint8_t expected_ciphertext[16] = {0x48, 0x8e, 0x9, 0xb9, + 0x6a, 0xa6, 0x24, 0x5f, + 0x1b, 0x8c, 0x3f, 0x48, + 0x27, 0xae, 0xb6, 0x7a}; #define VIR_CRYPTO_HASH(h, i, o) \ do { \ @@ -84,7 +158,31 @@ mymain(void) VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "The quick brown fox", "a2004f37730b9445670a738fa0fc9ee5"); VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "The quick brown fox", "5cac4f980fedc3d3f1f99b4be3472c9b30d56523e632d151237ec9309048bda9"); +#undef VIR_CRYPTO_HASH + +#define VIR_CRYPTO_ENCRYPT(a, n, i, il, c, cl) \ + do { \ + struct testCryptoEncryptData data = { \ + .algorithm = a, \ + .input = i, \ + .inputlen = il, \ + .ciphertext = c, \ + .ciphertextlen = cl, \ + }; \ + if (virtTestRun("Encrypt " n, testCryptoEncrypt, &data) < 0) \ + ret = -1; \ + } while (0) + + memset(&secretdata, 0, 8); + memcpy(&secretdata, "letmein", 7); + + VIR_CRYPTO_ENCRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes265cbc", + secretdata, 7, expected_ciphertext, 16); + +#undef VIR_CRYPTO_ENCRYPT + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIRT_TEST_MAIN(mymain) +/* Forces usage of not so random virRandomBytes */ +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virrandommock.so") -- 2.5.5
On Thu, May 19, 2016 at 16:29:01 -0400, John Ferlan wrote:
Introduce virCryptoHaveCipher and virCryptoEncryptData to handle performing encryption.
virCryptoHaveCipher: Boolean function to determine whether the requested cipher algorithm is available. It's expected this API will be called prior to virCryptoEncryptdata. It will return true/false.
virCryptoEncryptData: Based on the requested cipher type, call the specific encryption API to encrypt the data.
Currently the only algorithm support is the AES 256 CBC encryption.
Adjust tests for the API's
Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 1 + src/libvirt_private.syms | 2 + src/util/vircrypto.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 20 ++++- tests/vircryptotest.c | 100 +++++++++++++++++++++++- 5 files changed, 312 insertions(+), 3 deletions(-)
[...]
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 39a479a..b8f5554 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c
[...]
@@ -76,3 +85,184 @@ virCryptoHashString(virCryptoHash hash,
return 0; } + + +/* virCryptoHaveCipher: + * @algorithm: Specific cipher algorithm desired + * + * Expected to be called prior to virCryptoEncryptData in order + * to determine whether the requested encryption option is available, + * so that "other" alternatives can be taken if the algorithm is + * not available. + * + * Returns true if we can support the encryption. + */ +bool +virCryptoHaveCipher(virCryptoCipher algorithm) +{ + switch (algorithm) { + + case VIR_CRYPTO_CIPHER_AES256CBC: +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + return true; +#else + return false; +#endif + + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + }; + + return false; +} + + +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT +/* virCryptoEncryptDataAESgntuls: + * + * Performs the AES gnutls encryption + * + * Same input as virCryptoEncryptData, except the algoritm is replaced + * by the specific gnutls algorithm. + * + * Encrypts the @data buffer using the @enckey and if available the @iv + * + * Returns 0 on success with the ciphertext being filled. It is the + * caller's responsibility to clear and free it. Returns -1 on failure + * w/ error set. + */ +static int +virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, + uint8_t *enckey, + size_t enckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **ciphertextret, + size_t *ciphertextlenret) +{ + int rc; + size_t i; + gnutls_cipher_hd_t handle = NULL; + gnutls_datum_t enc_key; + gnutls_datum_t iv_buf; + uint8_t *ciphertext; + size_t ciphertextlen; + + /* Allocate a padded buffer, copy in the data */ + ciphertextlen = VIR_ROUND_UP(datalen, 16); + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0) + return -1; + memcpy(ciphertext, data, datalen); + + /* Fill in the padding of the buffer with the size of the padding + * which is required for decryption. */ + for (i = datalen; i < ciphertextlen; i++) + ciphertext[i] = ciphertextlen - datalen; + + /* Initialize the gnutls cipher */ + enc_key.size = enckeylen; + enc_key.data = enckey; + if (iv) { + iv_buf.size = ivlen; + iv_buf.data = iv; + } + if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg, + &enc_key, &iv_buf)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + /* Encrypt the data and free the memory for cipher operations */ + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); + gnutls_cipher_deinit(handle); + if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to encrypt the data: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + *ciphertextret = ciphertext; + *ciphertextlenret = ciphertextlen; + return 0; + + error: + VIR_DISPOSE_N(ciphertext, ciphertextlen); + return -1;
In both cases you should clear the stack'd copy of the encryption key.
+} +#endif + + +/* virCryptoEncryptData: + * @algorithm: algoritm desired for encryption + * @enckey: encryption key + * @enckeylen: encription key length + * @iv: initialization vector + * @ivlen: length of initialization vector + * @data: data to encrypt + * @datalen: length of data + * @ciphertext: stream of bytes allocated to store ciphertext + * @ciphertextlen: size of the stream of bytes + * + * If available, attempt and return the requested encryption type + * using the parameters passed. + * + * Returns 0 on success, -1 on failure with error set + */ +int +virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey, + size_t enckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **ciphertext, + size_t *ciphertextlen) +{ + switch (algorithm) { + case VIR_CRYPTO_CIPHER_AES256CBC: + if (enckeylen != 32) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC encryption invalid keylen=%zu"), + enckeylen); + return -1; + } + + if (ivlen != 16) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC initialization vector invalid len=%zu"), + ivlen); + return -1; + } + +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + /* + * Encrypt the data buffer using an encryption key and + * initialization vector via the gnutls_cipher_encrypt API + * for GNUTLS_CIPHER_AES_256_CBC. + */ + return virCryptoEncryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC, + enckey, enckeylen, iv, ivlen, + data, datalen, + ciphertext, ciphertextlen); +#else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("no encryption algorithm exists"));
That is quite a strong message. I'd say that the chosen algorithm is not supported.
+ return -1; +#endif + + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + } + + virReportError(VIR_ERR_INVALID_ARG, + _("algorithm=%d is not supported"), algorithm);
e.g. as you do here. [...]
--- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h
[...]
@@ -37,4 +45,14 @@ virCryptoHashString(virCryptoHash hash, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
+bool virCryptoHaveCipher(virCryptoCipher algorithm); + +int virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey, size_t enckeylen, + uint8_t *iv, size_t ivlen, + uint8_t *data, size_t datalen, + uint8_t **ciphertext, size_t *ciphertextlen) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(8) ATTRIBUTE_RETURN_CHECK;
ciphertext and ciphertextlen must be nonnull too.
+ #endif /* __VIR_CRYPTO_H__ */ diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index bfc47db..1d719d9 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c
[...]
@@ -56,10 +58,82 @@ testCryptoHash(const void *opaque) }
+struct testCryptoEncryptData { + virCryptoCipher algorithm; + uint8_t *input; + size_t inputlen; + uint8_t *ciphertext; + size_t ciphertextlen; +}; + +static int +testCryptoEncrypt(const void *opaque) +{ + const struct testCryptoEncryptData *data = opaque; + size_t i; + uint8_t *enckey = NULL; + size_t enckeylen = 32; + uint8_t *iv = NULL; + size_t ivlen = 16; + uint8_t *ciphertext = NULL; + size_t ciphertextlen = 0; + int ret = -1; + + if (!virCryptoHaveCipher(data->algorithm)) { + fprintf(stderr, "Invalid cipher algorithm=%d\n", data->algorithm); + return -1; + } + + if (VIR_ALLOC_N(enckey, enckeylen) < 0 || + VIR_ALLOC_N(iv, ivlen) < 0) + goto cleanup; + + if (virRandomBytes(enckey, enckeylen) < 0 || + virRandomBytes(iv, ivlen) < 0) + goto cleanup; + + if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen, + data->input, data->inputlen, + &ciphertext, &ciphertextlen) < 0) + goto cleanup; + + if (data->ciphertextlen != ciphertextlen) { + fprintf(stderr, "Expected ciphertextlen(%zu) doesn't match (%zu)\n", + data->ciphertextlen, ciphertextlen); + goto cleanup; + } + + if (memcmp(data->ciphertext, ciphertext, ciphertextlen)) { + fprintf(stderr, "Expected ciphertext doesn't match\n"); + for (i = 0; i < ciphertextlen; i++) { + if (data->ciphertext[i] != ciphertext[i]) { + fprintf(stderr, "expected[%zu]=%x ciphertext[%zu]=%x\n", + i, data->ciphertext[i], + i, ciphertext[i]);
Last time I was pointing out that it doesn't make much sense to output the difference whether encoded to base64 or not. [...]
@@ -84,7 +158,31 @@ mymain(void) VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "The quick brown fox", "a2004f37730b9445670a738fa0fc9ee5"); VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "The quick brown fox", "5cac4f980fedc3d3f1f99b4be3472c9b30d56523e632d151237ec9309048bda9");
+#undef VIR_CRYPTO_HASH + +#define VIR_CRYPTO_ENCRYPT(a, n, i, il, c, cl) \ + do { \ + struct testCryptoEncryptData data = { \ + .algorithm = a, \ + .input = i, \ + .inputlen = il, \ + .ciphertext = c, \ + .ciphertextlen = cl, \ + }; \ + if (virtTestRun("Encrypt " n, testCryptoEncrypt, &data) < 0) \ + ret = -1; \ + } while (0) + + memset(&secretdata, 0, 8); + memcpy(&secretdata, "letmein", 7); + + VIR_CRYPTO_ENCRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes265cbc", + secretdata, 7, expected_ciphertext, 16); +
This test will fail on builds without gnutls. ACK with the nits fixed.
On 05/20/2016 08:56 AM, Peter Krempa wrote:
On Thu, May 19, 2016 at 16:29:01 -0400, John Ferlan wrote:
Introduce virCryptoHaveCipher and virCryptoEncryptData to handle performing encryption.
virCryptoHaveCipher: Boolean function to determine whether the requested cipher algorithm is available. It's expected this API will be called prior to virCryptoEncryptdata. It will return true/false.
virCryptoEncryptData: Based on the requested cipher type, call the specific encryption API to encrypt the data.
Currently the only algorithm support is the AES 256 CBC encryption.
Adjust tests for the API's
Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 1 + src/libvirt_private.syms | 2 + src/util/vircrypto.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 20 ++++- tests/vircryptotest.c | 100 +++++++++++++++++++++++- 5 files changed, 312 insertions(+), 3 deletions(-)
[...]
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 39a479a..b8f5554 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c
[...]
@@ -76,3 +85,184 @@ virCryptoHashString(virCryptoHash hash,
return 0; } + + +/* virCryptoHaveCipher: + * @algorithm: Specific cipher algorithm desired + * + * Expected to be called prior to virCryptoEncryptData in order + * to determine whether the requested encryption option is available, + * so that "other" alternatives can be taken if the algorithm is + * not available. + * + * Returns true if we can support the encryption. + */ +bool +virCryptoHaveCipher(virCryptoCipher algorithm) +{ + switch (algorithm) { + + case VIR_CRYPTO_CIPHER_AES256CBC: +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + return true; +#else + return false; +#endif + + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + }; + + return false; +} + + +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT +/* virCryptoEncryptDataAESgntuls: + * + * Performs the AES gnutls encryption + * + * Same input as virCryptoEncryptData, except the algoritm is replaced + * by the specific gnutls algorithm. + * + * Encrypts the @data buffer using the @enckey and if available the @iv + * + * Returns 0 on success with the ciphertext being filled. It is the + * caller's responsibility to clear and free it. Returns -1 on failure + * w/ error set. + */ +static int +virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, + uint8_t *enckey, + size_t enckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **ciphertextret, + size_t *ciphertextlenret) +{ + int rc; + size_t i; + gnutls_cipher_hd_t handle = NULL; + gnutls_datum_t enc_key; + gnutls_datum_t iv_buf; + uint8_t *ciphertext; + size_t ciphertextlen; + + /* Allocate a padded buffer, copy in the data */ + ciphertextlen = VIR_ROUND_UP(datalen, 16); + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0) + return -1; + memcpy(ciphertext, data, datalen); + + /* Fill in the padding of the buffer with the size of the padding + * which is required for decryption. */ + for (i = datalen; i < ciphertextlen; i++) + ciphertext[i] = ciphertextlen - datalen; + + /* Initialize the gnutls cipher */ + enc_key.size = enckeylen; + enc_key.data = enckey; + if (iv) { + iv_buf.size = ivlen; + iv_buf.data = iv; + } + if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg, + &enc_key, &iv_buf)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + /* Encrypt the data and free the memory for cipher operations */ + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); + gnutls_cipher_deinit(handle);
memset(&enc_key, 0, sizeof(gnutls_datum_t)); memset(&iv_key, 0, sizeof(gnutls_datum_t));
+ if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to encrypt the data: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + *ciphertextret = ciphertext; + *ciphertextlenret = ciphertextlen; + return 0; + + error: + VIR_DISPOSE_N(ciphertext, ciphertextlen); + return -1;
In both cases you should clear the stack'd copy of the encryption key.
+} +#endif + + +/* virCryptoEncryptData: + * @algorithm: algoritm desired for encryption + * @enckey: encryption key + * @enckeylen: encription key length + * @iv: initialization vector + * @ivlen: length of initialization vector + * @data: data to encrypt + * @datalen: length of data + * @ciphertext: stream of bytes allocated to store ciphertext + * @ciphertextlen: size of the stream of bytes + * + * If available, attempt and return the requested encryption type + * using the parameters passed. + * + * Returns 0 on success, -1 on failure with error set + */ +int +virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey, + size_t enckeylen, + uint8_t *iv, + size_t ivlen, + uint8_t *data, + size_t datalen, + uint8_t **ciphertext, + size_t *ciphertextlen) +{ + switch (algorithm) { + case VIR_CRYPTO_CIPHER_AES256CBC: + if (enckeylen != 32) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC encryption invalid keylen=%zu"), + enckeylen); + return -1; + } + + if (ivlen != 16) { + virReportError(VIR_ERR_INVALID_ARG, + _("AES256CBC initialization vector invalid len=%zu"), + ivlen); + return -1; + } + +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + /* + * Encrypt the data buffer using an encryption key and + * initialization vector via the gnutls_cipher_encrypt API + * for GNUTLS_CIPHER_AES_256_CBC. + */ + return virCryptoEncryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC, + enckey, enckeylen, iv, ivlen, + data, datalen, + ciphertext, ciphertextlen); +#else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("no encryption algorithm exists"));
That is quite a strong message. I'd say that the chosen algorithm is not supported.
OK - I'll replace with a break; to fall into message below
+ return -1; +#endif + + case VIR_CRYPTO_CIPHER_NONE: + case VIR_CRYPTO_CIPHER_LAST: + break; + } + + virReportError(VIR_ERR_INVALID_ARG, + _("algorithm=%d is not supported"), algorithm);
e.g. as you do here.
[...]
--- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h
[...]
@@ -37,4 +45,14 @@ virCryptoHashString(virCryptoHash hash, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
+bool virCryptoHaveCipher(virCryptoCipher algorithm); + +int virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey, size_t enckeylen, + uint8_t *iv, size_t ivlen, + uint8_t *data, size_t datalen, + uint8_t **ciphertext, size_t *ciphertextlen) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(8) ATTRIBUTE_RETURN_CHECK;
ciphertext and ciphertextlen must be nonnull too.
OK - ciphertext is (8)... added (9) for ciphertextlen
+ #endif /* __VIR_CRYPTO_H__ */ diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index bfc47db..1d719d9 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c
[...]
@@ -56,10 +58,82 @@ testCryptoHash(const void *opaque) }
+struct testCryptoEncryptData { + virCryptoCipher algorithm; + uint8_t *input; + size_t inputlen; + uint8_t *ciphertext; + size_t ciphertextlen; +}; + +static int +testCryptoEncrypt(const void *opaque) +{ + const struct testCryptoEncryptData *data = opaque; + size_t i; + uint8_t *enckey = NULL; + size_t enckeylen = 32; + uint8_t *iv = NULL; + size_t ivlen = 16; + uint8_t *ciphertext = NULL; + size_t ciphertextlen = 0; + int ret = -1; + + if (!virCryptoHaveCipher(data->algorithm)) { + fprintf(stderr, "Invalid cipher algorithm=%d\n", data->algorithm);
Would adding "return EXIT_AM_SKIP;" be acceptable?
+ return -1; + } + + if (VIR_ALLOC_N(enckey, enckeylen) < 0 || + VIR_ALLOC_N(iv, ivlen) < 0) + goto cleanup; + + if (virRandomBytes(enckey, enckeylen) < 0 || + virRandomBytes(iv, ivlen) < 0) + goto cleanup; + + if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen, + data->input, data->inputlen, + &ciphertext, &ciphertextlen) < 0) + goto cleanup; + + if (data->ciphertextlen != ciphertextlen) { + fprintf(stderr, "Expected ciphertextlen(%zu) doesn't match (%zu)\n", + data->ciphertextlen, ciphertextlen); + goto cleanup; + } + + if (memcmp(data->ciphertext, ciphertext, ciphertextlen)) { + fprintf(stderr, "Expected ciphertext doesn't match\n"); + for (i = 0; i < ciphertextlen; i++) { + if (data->ciphertext[i] != ciphertext[i]) { + fprintf(stderr, "expected[%zu]=%x ciphertext[%zu]=%x\n", + i, data->ciphertext[i], + i, ciphertext[i]);
Last time I was pointing out that it doesn't make much sense to output the difference whether encoded to base64 or not.
[...]
I used it mainly for "testing", but OK I'll remove it.
@@ -84,7 +158,31 @@ mymain(void) VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "The quick brown fox", "a2004f37730b9445670a738fa0fc9ee5"); VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "The quick brown fox", "5cac4f980fedc3d3f1f99b4be3472c9b30d56523e632d151237ec9309048bda9");
+#undef VIR_CRYPTO_HASH + +#define VIR_CRYPTO_ENCRYPT(a, n, i, il, c, cl) \ + do { \ + struct testCryptoEncryptData data = { \ + .algorithm = a, \ + .input = i, \ + .inputlen = il, \ + .ciphertext = c, \ + .ciphertextlen = cl, \ + }; \ + if (virtTestRun("Encrypt " n, testCryptoEncrypt, &data) < 0) \ + ret = -1; \ + } while (0) + + memset(&secretdata, 0, 8); + memcpy(&secretdata, "letmein", 7); + + VIR_CRYPTO_ENCRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes265cbc", + secretdata, 7, expected_ciphertext, 16); +
This test will fail on builds without gnutls.
<sigh> Thanks John
ACK with the nits fixed.
On Fri, May 20, 2016 at 10:03:27 -0400, John Ferlan wrote:
On 05/20/2016 08:56 AM, Peter Krempa wrote:
On Thu, May 19, 2016 at 16:29:01 -0400, John Ferlan wrote:
Introduce virCryptoHaveCipher and virCryptoEncryptData to handle performing encryption.
virCryptoHaveCipher: Boolean function to determine whether the requested cipher algorithm is available. It's expected this API will be called prior to virCryptoEncryptdata. It will return true/false.
virCryptoEncryptData: Based on the requested cipher type, call the specific encryption API to encrypt the data.
Currently the only algorithm support is the AES 256 CBC encryption.
Adjust tests for the API's
Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 1 + src/libvirt_private.syms | 2 + src/util/vircrypto.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 20 ++++- tests/vircryptotest.c | 100 +++++++++++++++++++++++- 5 files changed, 312 insertions(+), 3 deletions(-)
[...]
+ /* Fill in the padding of the buffer with the size of the padding + * which is required for decryption. */ + for (i = datalen; i < ciphertextlen; i++) + ciphertext[i] = ciphertextlen - datalen; + + /* Initialize the gnutls cipher */ + enc_key.size = enckeylen; + enc_key.data = enckey; + if (iv) { + iv_buf.size = ivlen; + iv_buf.data = iv; + } + if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg, + &enc_key, &iv_buf)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + /* Encrypt the data and free the memory for cipher operations */ + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); + gnutls_cipher_deinit(handle);
memset(&enc_key, 0, sizeof(gnutls_datum_t)); memset(&iv_key, 0, sizeof(gnutls_datum_t));
Note that this doesn't catch the cleanup path if gnutls_cipher_init fails.
+ if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to encrypt the data: '%s'"), + gnutls_strerror(rc)); + goto error; + } + + *ciphertextret = ciphertext; + *ciphertextlenret = ciphertextlen; + return 0; + + error: + VIR_DISPOSE_N(ciphertext, ciphertextlen); + return -1;
In both cases you should clear the stack'd copy of the encryption key.
+} +#endif +
Move the logic from qemuDomainGenerateRandomKey into this new function, altering the comments, variable names, and error messages to keep things more generic. NB: Although perhaps more reasonable to add soemthing to virrandom.c. The virrandom.c was included in the setuid_rpc_client, so I chose placement in vircrypto. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 53 ++---------------------------------------------- src/util/vircrypto.c | 41 +++++++++++++++++++++++++++++++++++++ src/util/vircrypto.h | 2 ++ 4 files changed, 46 insertions(+), 51 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6c02b10..fb5b419 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1395,6 +1395,7 @@ virConfWriteMem; # util/vircrypto.h virCryptoEncryptData; +virCryptoGenerateRandom; virCryptoHashString; virCryptoHaveCipher; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0cec340..f038450 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -45,15 +45,8 @@ #include "virthreadjob.h" #include "viratomic.h" #include "virprocess.h" -#include "virrandom.h" +#include "vircrypto.h" #include "secret_util.h" -#include "base64.h" -#ifdef WITH_GNUTLS -# include <gnutls/gnutls.h> -# if HAVE_GNUTLS_CRYPTO_H -# include <gnutls/crypto.h> -# endif -#endif #include "logging/log_manager.h" #include "locking/domain_lock.h" @@ -630,48 +623,6 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv) } -/* qemuDomainGenerateRandomKey - * @nbytes: Size in bytes of random key to generate - * - * Generate a random key of nbytes length and return it. - * - * Since the gnutls_rnd could be missing, provide an alternate less - * secure mechanism to at least have something. - * - * Returns pointer memory containing key on success, NULL on failure - */ -static uint8_t * -qemuDomainGenerateRandomKey(size_t nbytes) -{ - uint8_t *key; - int ret; - - if (VIR_ALLOC_N(key, nbytes) < 0) - return NULL; - -#if HAVE_GNUTLS_RND - /* Generate a master key using gnutls_rnd() if possible */ - if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to generate master key, ret=%d"), ret); - VIR_FREE(key); - return NULL; - } -#else - /* If we don't have gnutls_rnd(), we will generate a less cryptographically - * strong master key from /dev/urandom. - */ - if ((ret = virRandomBytes(key, nbytes)) < 0) { - virReportSystemError(ret, "%s", _("failed to generate master key")); - VIR_FREE(key); - return NULL; - } -#endif - - return key; -} - - /* qemuDomainMasterKeyRemove: * @priv: Pointer to the domain private object * @@ -718,7 +669,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm) return 0; if (!(priv->masterKey = - qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN))) return -1; priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index b8f5554..a2132bf 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -266,3 +266,44 @@ virCryptoEncryptData(virCryptoCipher algorithm, _("algorithm=%d is not supported"), algorithm); return -1; } + +/* virCryptoGenerateRandom: + * @nbytes: Size in bytes of random byte stream to generate + * + * Generate a random stream of nbytes length and return it. + * + * Since the gnutls_rnd could be missing, provide an alternate less + * secure mechanism to at least have something. + * + * Returns pointer memory containing byte stream on success, NULL on failure + */ +uint8_t * +virCryptoGenerateRandom(size_t nbytes) +{ + uint8_t *buf; + int ret; + + if (VIR_ALLOC_N(buf, nbytes) < 0) + return NULL; + +#if HAVE_GNUTLS_RND + /* Generate the byte stream using gnutls_rnd() if possible */ + if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to generate byte stream, ret=%d"), ret); + VIR_FREE(buf); + return NULL; + } +#else + /* If we don't have gnutls_rnd(), we will generate a less cryptographically + * strong master buf from /dev/urandom. + */ + if ((ret = virRandomBytes(buf, nbytes)) < 0) { + virReportSystemError(ret, "%s", _("failed to generate byte stream")); + VIR_FREE(buf); + return NULL; + } +#endif + + return buf; +} diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h index f0ec07b..1270414 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -55,4 +55,6 @@ int virCryptoEncryptData(virCryptoCipher algorithm, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(8) ATTRIBUTE_RETURN_CHECK; +uint8_t *virCryptoGenerateRandom(size_t nbytes); + #endif /* __VIR_CRYPTO_H__ */ -- 2.5.5
On Thu, May 19, 2016 at 16:29:02 -0400, John Ferlan wrote:
Move the logic from qemuDomainGenerateRandomKey into this new function, altering the comments, variable names, and error messages to keep things more generic.
NB: Although perhaps more reasonable to add soemthing to virrandom.c. The virrandom.c was included in the setuid_rpc_client, so I chose placement in vircrypto.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 53 ++---------------------------------------------- src/util/vircrypto.c | 41 +++++++++++++++++++++++++++++++++++++ src/util/vircrypto.h | 2 ++ 4 files changed, 46 insertions(+), 51 deletions(-)
ACK
Currently just a shim to call qemuDomainSecretPlainSetup, but soon to be more Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f038450..754536e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -835,6 +835,26 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, } +/* qemuDomainSecretSetup: + * @conn: Pointer to connection + * @secinfo: Pointer to secret info + * @protocol: Protocol for secret + * @authdef: Pointer to auth data + * + * A shim to call plain setup. + * + * Returns 0 on success, -1 on failure + */ +static int +qemuDomainSecretSetup(virConnectPtr conn, + qemuDomainSecretInfoPtr secinfo, + virStorageNetProtocol protocol, + virStorageAuthDefPtr authdef) +{ + return qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef); +} + + /* qemuDomainSecretDiskDestroy: * @disk: Pointer to a disk definition * @@ -880,8 +900,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol, - src->auth) < 0) + if (qemuDomainSecretSetup(conn, secinfo, src->protocol, + src->auth) < 0) goto error; diskPriv->secinfo = secinfo; @@ -945,9 +965,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretPlainSetup(conn, secinfo, - VIR_STORAGE_NET_PROTOCOL_ISCSI, - iscsisrc->auth) < 0) + if (qemuDomainSecretSetup(conn, secinfo, + VIR_STORAGE_NET_PROTOCOL_ISCSI, + iscsisrc->auth) < 0) goto error; hostdevPriv->secinfo = secinfo; -- 2.5.5
On Thu, May 19, 2016 at 16:29:03 -0400, John Ferlan wrote:
Currently just a shim to call qemuDomainSecretPlainSetup, but soon to be more
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
ACK
Adjust the model to allow callers to provide a comma separated list of libraries to be loaded. For those tests that may need more than one mock library loaded. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/testutils.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 9180e86..69cd77c 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -854,8 +854,16 @@ int virtTestMain(int argc, if (getenv("VIR_TEST_FILE_ACCESS")) VIRT_TEST_PRELOAD(TEST_MOCK); - if (lib) - VIRT_TEST_PRELOAD(lib); + if (lib) { + char **liblist; + size_t count = 0; + size_t i; + + liblist = virStringSplitCount(lib, ",", 0, &count); + for (i = 0; i < count; i++) + VIRT_TEST_PRELOAD(liblist[i]); + virStringFreeListCount(liblist, count); + } progname = last_component(argv[0]); if (STRPREFIX(progname, "lt-")) -- 2.5.5
On Thu, May 19, 2016 at 16:29:04 -0400, John Ferlan wrote:
Adjust the model to allow callers to provide a comma separated list of libraries to be loaded. For those tests that may need more than one mock library loaded.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/testutils.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index 9180e86..69cd77c 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -854,8 +854,16 @@ int virtTestMain(int argc, if (getenv("VIR_TEST_FILE_ACCESS")) VIRT_TEST_PRELOAD(TEST_MOCK);
- if (lib) - VIRT_TEST_PRELOAD(lib); + if (lib) { + char **liblist; + size_t count = 0; + size_t i; + + liblist = virStringSplitCount(lib, ",", 0, &count); + for (i = 0; i < count; i++) + VIRT_TEST_PRELOAD(liblist[i]);
I don't like this very much. I'll try to refactor the chaining of the mocking libs to prevent re-execing for every single .so you want to pre-load. I expect to use a variadic macro for that. Peter
On 05/20/2016 09:03 AM, Peter Krempa wrote:
On Thu, May 19, 2016 at 16:29:04 -0400, John Ferlan wrote:
Adjust the model to allow callers to provide a comma separated list of libraries to be loaded. For those tests that may need more than one mock library loaded.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/testutils.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index 9180e86..69cd77c 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -854,8 +854,16 @@ int virtTestMain(int argc, if (getenv("VIR_TEST_FILE_ACCESS")) VIRT_TEST_PRELOAD(TEST_MOCK);
- if (lib) - VIRT_TEST_PRELOAD(lib); + if (lib) { + char **liblist; + size_t count = 0; + size_t i; + + liblist = virStringSplitCount(lib, ",", 0, &count); + for (i = 0; i < count; i++) + VIRT_TEST_PRELOAD(liblist[i]);
I don't like this very much. I'll try to refactor the chaining of the mocking libs to prevent re-execing for every single .so you want to pre-load.
I expect to use a variadic macro for that.
OK - I'll wait for that before pushing patch 6 since it depends on it. John
https://bugzilla.redhat.com/show_bug.cgi?id=1182074 If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for RBD volumes instead of passing the base64 encoded secret on the command line. The goal is to make AES secrets the default and have no user interaction required in order to allow using the AES mechanism. If the mechanism is not available, then fall back to the current plain mechanism using a base64 encoded secret. New APIs: qemu_domain.c: qemuDomainGetSecretAESAlias: Generate/return the secret object alias for an AES Secret Info type. This will be called from qemuDomainSecretAESSetup. qemuDomainSecretAESSetup: (private) This API handles the details of the generation of the AES secret and saves the pieces that need to be passed to qemu in order for the secret to be decrypted. The encrypted secret based upon the domain master key, an initialization vector (16 byte random value), and the stored secret. Finally, the requirement from qemu is the IV and encrypted secret are to be base64 encoded. qemu_command.c: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the AES secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense. qemuBuildObjectSecretCommandLine (private) Generate and add to the command line the -object secret for the secret. This will be required for the subsequent RBD reference to the object. qemuBuildDiskSecinfoCommandLine (private) Handle adding the AES secret object. Adjustments: qemu_domain.c: The qemuDomainSecretSetup was altered to call either the AES or Plain Setup functions based upon whether AES secrets are possible (we have the encryption API) or not, we have secrets, and of course if the protocol source is RBD. qemu_command.c: Adjust the qemuBuildRBDSecinfoURI API's in order to generate the specific command options for an AES secret, such as: -object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ mon_host=mon1.example.org\:6321,password-secret=$alias,... where the 'id=' value is the secret object alias generated by concatenating the disk alias and "-aesKey0". The 'keyid= $masterKey' is the master key shared with qemu, and the -drive syntax will reference that alias as the 'password-secret'. For the -drive syntax, the 'id=myname' is kept to define the username, while the 'key=$base64 encoded secret' is removed. While according to the syntax described for qemu commit '60390a21' or as seen in the email archive: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html it is possible to pass a plaintext password via a file, the qemu commit 'ac1d8878' describes the more feature rich 'keyid=' option based upon the shared masterKey. Add tests for checking/comparing output. NB: For hotplug, since the hotplug code doesn't add command line arguments, passing the encoded secret directly to the monitor will suffice. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 23 ++++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 117 ++++++++++++++++++- src/qemu/qemu_domain.c | 127 +++++++++++++++++++-- ...muxml2argv-disk-drive-network-rbd-auth-AES.args | 31 +++++ ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml | 42 +++++++ tests/qemuxml2argvmock.c | 16 +++ tests/qemuxml2argvtest.c | 5 +- 8 files changed, 354 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index cb102ec..d624071 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -482,3 +482,26 @@ qemuDomainGetMasterKeyAlias(void) return alias; } + + +/* qemuDomainGetSecretAESAlias: + * + * Generate and return an alias for the encrypted secret + * + * Returns NULL or a string containing the alias + */ +char * +qemuDomainGetSecretAESAlias(const char *srcalias) +{ + char *alias; + + if (!srcalias) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("encrypted secret alias requires valid source alias")); + return NULL; + } + + ignore_value(virAsprintf(&alias, "%s-secret0", srcalias)); + + return alias; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 2d7bc9b..e328a9b 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -69,4 +69,6 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk); char *qemuDomainGetMasterKeyAlias(void); +char *qemuDomainGetSecretAESAlias(const char *srcalias); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a2fa1d..316aab9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -616,6 +616,106 @@ qemuNetworkDriveGetPort(int protocol, } +/** + * qemuBuildSecretInfoProps: + * @secinfo: pointer to the secret info object + * @type: returns a pointer to a character string for object name + * @props: json properties to return + * + * Build the JSON properties for the secret info type. + * + * Returns 0 on success with the filled in JSON property; otherwise, + * returns -1 on failure error message set. + */ +static int +qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, + const char **type, + virJSONValuePtr *propsret) +{ + int ret = -1; + char *keyid = NULL; + + *type = "secret"; + + if (!(keyid = qemuDomainGetMasterKeyAlias())) + return -1; + + if (virJSONValueObjectCreate(propsret, + "s:data", secinfo->s.aes.ciphertext, + "s:keyid", keyid, + "s:iv", secinfo->s.aes.iv, + "s:format", "base64", NULL) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(keyid); + + return ret; +} + + +/** + * qemuBuildObjectSecretCommandLine: + * @cmd: the command to modify + * @secinfo: pointer to the secret info object + * + * If the secinfo is available and associated with an AES secret, + * then format the command line for the secret object. This object + * will be referenced by the device that needs/uses it, so it needs + * to be in place first. + * + * Returns 0 on success, -1 w/ error message on failure + */ +static int +qemuBuildObjectSecretCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + int ret = -1; + virJSONValuePtr props = NULL; + const char *type; + char *tmp = NULL; + + if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) + return -1; + + if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.aes.alias, + props))) + goto cleanup; + + virCommandAddArgList(cmd, "-object", tmp, NULL); + ret = 0; + + cleanup: + virJSONValueFree(props); + VIR_FREE(tmp); + + return ret; +} + + +/* qemuBuildDiskSecinfoCommandLine: + * @cmd: Pointer to the command string + * @secinfo: Pointer to a possible secinfo + * + * Add the secret object for the disks that will be using it to perform + * their authentication. + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + /* Not necessary for non AES secrets */ + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_AES) + return 0; + + return qemuBuildObjectSecretCommandLine(cmd, secinfo); +} + + /* qemuBuildGeneralSecinfoURI: * @uri: Pointer to the URI structure to add to * @secinfo: Pointer to the secret info data (if present) @@ -697,6 +797,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, break; case VIR_DOMAIN_SECRET_INFO_TYPE_AES: + virBufferEscape(buf, '\\', ":", ":id=%s:auth_supported=cephx\\;none", + secinfo->s.aes.username); + break; + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -1094,6 +1198,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, char *source = NULL; int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1175,7 +1280,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; } - if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0) + if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0) goto error; if (source && @@ -1227,6 +1332,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, qemuBufferEscapeComma(&opt, source); virBufferAddLit(&opt, ","); + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + virBufferAsprintf(&opt, "password-secret=%s,", + secinfo->s.aes.alias); + } + if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) virBufferAsprintf(&opt, "format=%s,", @@ -1906,6 +2016,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk = def->disks[i]; bool withDeviceArg = false; bool deviceFlagMasked = false; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; /* Unless we have -device, then USB disks need special handling */ @@ -1948,6 +2060,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, break; } + if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); /* Unfortunately it is not possible to use diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 754536e..98fc337 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -835,23 +835,136 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, } +/* qemuDomainSecretAESSetup: + * @conn: Pointer to connection + * @priv: pointer to domain private object + * @secinfo: Pointer to secret info + * @srcalias: Alias of the disk/hostdev used to generate the secret alias + * @protocol: Protocol for secret + * @authdef: Pointer to auth data + * + * Taking a secinfo, fill in the AES specific information using the + * + * Returns 0 on success, -1 on failure with error message + */ +static int +qemuDomainSecretAESSetup(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + qemuDomainSecretInfoPtr secinfo, + const char *srcalias, + virStorageNetProtocol protocol, + virStorageAuthDefPtr authdef) +{ + int ret = -1; + uint8_t *raw_iv = NULL; + size_t ivlen = QEMU_DOMAIN_AES_IV_LEN; + uint8_t *secret = NULL; + size_t secretlen = 0; + uint8_t *ciphertext = NULL; + size_t ciphertextlen = 0; + int secretType = VIR_SECRET_USAGE_TYPE_NONE; + + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES; + if (VIR_STRDUP(secinfo->s.aes.username, authdef->username) < 0) + return -1; + + switch ((virStorageNetProtocol)protocol) { + case VIR_STORAGE_NET_PROTOCOL_RBD: + secretType = VIR_SECRET_USAGE_TYPE_CEPH; + break; + + case VIR_STORAGE_NET_PROTOCOL_NONE: + case VIR_STORAGE_NET_PROTOCOL_NBD: + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("protocol '%s' cannot be used for encrypted secrets"), + virStorageNetProtocolTypeToString(protocol)); + return -1; + } + + if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias))) + return -1; + + /* Create a random initialization vector */ + if (!(raw_iv = virCryptoGenerateRandom(ivlen))) + return -1; + + /* Encode the IV and save that since qemu will need it */ + if (!(secinfo->s.aes.iv = virStringEncodeBase64(raw_iv, ivlen))) + goto cleanup; + + /* Grab the unencoded secret */ + if (virSecretGetSecretString(conn, authdef, secretType, + &secret, &secretlen) < 0) + goto cleanup; + + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, + priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN, + raw_iv, ivlen, secret, secretlen, + &ciphertext, &ciphertextlen) < 0) + goto cleanup; + + /* Clear out the secret */ + memset(secret, 0, secretlen); + + /* Now encode the ciphertext and store to be passed to qemu */ + if (!(secinfo->s.aes.ciphertext = virStringEncodeBase64(ciphertext, + ciphertextlen))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_DISPOSE_N(raw_iv, ivlen); + VIR_DISPOSE_N(secret, secretlen); + VIR_DISPOSE_N(ciphertext, ciphertextlen); + + return ret; +} + + /* qemuDomainSecretSetup: * @conn: Pointer to connection + * @priv: pointer to domain private object * @secinfo: Pointer to secret info + * @srcalias: Alias of the disk/hostdev used to generate the secret alias * @protocol: Protocol for secret * @authdef: Pointer to auth data * - * A shim to call plain setup. + * If we have the encryption API present and can support a secret object, then + * build the AES secret; otherwise, build the Plain secret. This is the magic + * decision point for utilizing the AES secrets for an RBD disk. For now iSCSI + * disks and hostdevs will not be able to utilize this mechanism. * * Returns 0 on success, -1 on failure */ static int qemuDomainSecretSetup(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, qemuDomainSecretInfoPtr secinfo, + const char *srcalias, virStorageNetProtocol protocol, virStorageAuthDefPtr authdef) { - return qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef); + if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && + protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { + if (qemuDomainSecretAESSetup(conn, priv, secinfo, + srcalias, protocol, authdef) < 0) + return -1; + } else { + if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0) + return -1; + } + return 0; } @@ -883,7 +996,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) */ int qemuDomainSecretDiskPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, + qemuDomainObjPrivatePtr priv, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; @@ -900,8 +1013,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretSetup(conn, secinfo, src->protocol, - src->auth) < 0) + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + src->protocol, src->auth) < 0) goto error; diskPriv->secinfo = secinfo; @@ -944,7 +1057,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) */ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, + qemuDomainObjPrivatePtr priv, virDomainHostdevDefPtr hostdev) { virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; @@ -965,7 +1078,7 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretSetup(conn, secinfo, + if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias, VIR_STORAGE_NET_PROTOCOL_ISCSI, iscsisrc->auth) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args new file mode 100644 index 0000000..7100d2d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object secret,id=virtio-disk0-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ +password-secret=virtio-disk0-secret0,format=raw,if=none,id=drive-virtio-disk0' \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml new file mode 100644 index 0000000..ac2e942 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml @@ -0,0 +1,42 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..c6a1f98 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,11 +21,14 @@ #include <config.h> #include "internal.h" +#include "viralloc.h" #include "vircommand.h" +#include "vircrypto.h" #include "virmock.h" #include "virnetdev.h" #include "virnetdevtap.h" #include "virnuma.h" +#include "virrandom.h" #include "virscsi.h" #include "virstring.h" #include "virtpm.h" @@ -145,3 +148,16 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, { /* nada */ } + +uint8_t * +virCryptoGenerateRandom(size_t nbytes) +{ + uint8_t *buf; + + if (VIR_ALLOC_N(buf, nbytes) < 0) + return NULL; + + ignore_value(virRandomBytes(buf, nbytes)); + + return buf; +} diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 3bfb5c4..fd15186 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -799,6 +799,8 @@ mymain(void) DO_TEST("disk-drive-network-rbd", NONE); DO_TEST("disk-drive-network-sheepdog", NONE); DO_TEST("disk-drive-network-rbd-auth", NONE); + DO_TEST("disk-drive-network-rbd-auth-AES", + QEMU_CAPS_OBJECT_SECRET); DO_TEST("disk-drive-network-rbd-ipv6", NONE); DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); DO_TEST("disk-drive-no-boot", @@ -1980,7 +1982,8 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/qemuxml2argvmock.so") +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/qemuxml2argvmock.so," + abs_builddir "/.libs/virrandommock.so") #else -- 2.5.5
On Thu, May 19, 2016 at 16:29:05 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1182074
If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for RBD volumes instead of passing the base64 encoded secret on the command line.
The goal is to make AES secrets the default and have no user interaction required in order to allow using the AES mechanism. If the mechanism is not available, then fall back to the current plain mechanism using a base64 encoded secret.
New APIs:
qemu_domain.c: qemuDomainGetSecretAESAlias: Generate/return the secret object alias for an AES Secret Info type. This will be called from qemuDomainSecretAESSetup.
qemuDomainSecretAESSetup: (private) This API handles the details of the generation of the AES secret and saves the pieces that need to be passed to qemu in order for the secret to be decrypted. The encrypted secret based upon the domain master key, an initialization vector (16 byte random value), and the stored secret. Finally, the requirement from qemu is the IV and encrypted secret are to be base64 encoded.
qemu_command.c: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the AES secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense.
qemuBuildObjectSecretCommandLine (private) Generate and add to the command line the -object secret for the secret. This will be required for the subsequent RBD reference to the object.
qemuBuildDiskSecinfoCommandLine (private) Handle adding the AES secret object.
Adjustments:
qemu_domain.c: The qemuDomainSecretSetup was altered to call either the AES or Plain Setup functions based upon whether AES secrets are possible (we have the encryption API) or not, we have secrets, and of course if the protocol source is RBD.
qemu_command.c: Adjust the qemuBuildRBDSecinfoURI API's in order to generate the specific command options for an AES secret, such as:
-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ mon_host=mon1.example.org\:6321,password-secret=$alias,...
where the 'id=' value is the secret object alias generated by concatenating the disk alias and "-aesKey0". The 'keyid= $masterKey' is the master key shared with qemu, and the -drive syntax will reference that alias as the 'password-secret'. For the -drive syntax, the 'id=myname' is kept to define the username, while the 'key=$base64 encoded secret' is removed.
While according to the syntax described for qemu commit '60390a21' or as seen in the email archive:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
it is possible to pass a plaintext password via a file, the qemu commit 'ac1d8878' describes the more feature rich 'keyid=' option based upon the shared masterKey.
Add tests for checking/comparing output.
NB: For hotplug, since the hotplug code doesn't add command line arguments, passing the encoded secret directly to the monitor will suffice.
I didn't read the commit message.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
ACK
On 05/19/2016 04:28 PM, John Ferlan wrote: [...]
John Ferlan (6): tests: Add mock for virRandomBytes util: Introduce encryption APIs util: Introduce virCryptoGenerateRandom qemu: Introduce qemuDomainSecretSetup tests: Allow comma separate list of libs to preload qemu: Utilize qemu secret objects for RBD auth/secret
configure.ac | 1 + src/libvirt_private.syms | 3 + src/qemu/qemu_alias.c | 23 ++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 117 ++++++++++- src/qemu/qemu_domain.c | 200 +++++++++++++----- src/util/vircrypto.c | 233 ++++++++++++++++++++- src/util/vircrypto.h | 22 +- tests/Makefile.am | 12 ++ ...muxml2argv-disk-drive-network-rbd-auth-AES.args | 31 +++ ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml | 42 ++++ tests/qemuxml2argvmock.c | 16 ++ tests/qemuxml2argvtest.c | 5 +- tests/testutils.c | 12 +- tests/vircryptotest.c | 100 ++++++++- tests/virrandommock.c | 39 ++++ tests/virrandomtest.c | 86 ++++++++ 17 files changed, 879 insertions(+), 65 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml create mode 100644 tests/virrandommock.c create mode 100644 tests/virrandomtest.c
After replacing patch 5 w/ Peter's mock library list patch - I've pushed this series. Thanks for the time spent on multiple reviews - John
participants (2)
-
John Ferlan -
Peter Krempa