On 05/19/2016 03:32 AM, Peter Krempa wrote:

On Wed, May 18, 2016 at 19:52:30 -0400, John Ferlan wrote:

...

Create a mock for virRandomBytes to generate a not so random value that can be used by the tests to ensure the generation of an encrypted secret by masterKey and random initialization vector can produce an expected result. 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/qemuxml2argvmock.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..dade748 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c

[...]

...

@@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, { /* nada */ } + +int +virRandomBytes(unsigned char *buf, + size_t buflen) +{ + size_t i; + + for (i = 0; i < buflen; i++) + buf[i] = i; + + return 0; +} + +#ifdef WITH_GNUTLS +int +gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED, + void *data, + size_t len) +{ + return virRandomBytes(data, len); +#endif

As I've pointed out last time, this won't compile without gnutls.

Beyond the merge issue with putting the } after the #endif, I agree - it won't compile, but I can only assume that's not your issue; otherwise, I would think that your initial review would have just pointed out that the } needs to be inside the #endif. If one checks who would actually call this: qemuDomainGenerateRandomKey(): #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, ... So is your issue that 1. The incorrect placement of #endif 2. "#ifdef HAVE_GNUTLS_RND" should have been used 3. you want an "#else" 4. you don't want to see the #ifdef? I could see value in #2 to follow the caller, but the others I don't see value in. But certainly none of those options encompasses the catch-all this won't compile without gnutls when I read the review. In any case, between patch 2 and the cover - I pointed out that I wasn't sure putting this into a file named "qemuxml2argvmock.c" was the right choice given that as you pointed out previously that qemu isn't required. So I started down the path of creating a virrandommock.c and a virrandomtest.c; however, there is just something about that mock environment that I don't understand well enough to get things to work as I expected. I posted this with the hope that someone would be able to look and provide some assistance with the magic words to write in Makefile.am. OH and BTW: In patch 2, the }/#endif issue was already handled properly in virrandommock.c. John

On Wed, May 18, 2016 at 19:52:31 -0400, John Ferlan wrote:

What I'd like to do is merge this and the previous patch together; however, I'm missing something (hopefully obvious) with the building of the mocked function.

Since this won't be a qemu specific function, the test wouldn't belong in qemuxml2argvmock - rather a separate mock would seemingly have to be created and then a separate virrandomtest; however, I'm lost in the maze of Makefile magic - so hopefully someone can help... The current code works, but generates a nefarious:

*** Warning: Linking the executable virrandomtest against the loadable module *** virrandommock.so is not portable!

during the build...

diff --git a/tests/Makefile.am b/tests/Makefile.am index 6ba6c5b..9d1a2ea 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1085,7 +1085,6 @@ vircryptotest_LDADD = $(LDADDS) virrandomtest_SOURCES = \ virrandomtest.c testutils.h testutils.c virrandomtest_LDADD = \ - virrandommock.la \ ../src/libvirt.la \ ../src/libvirt_util.la \ $(GNULIB_LIBS) diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c index 76e187c..bf1f46e 100644 --- a/tests/virrandomtest.c +++ b/tests/virrandomtest.c @@ -73,7 +73,7 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIRT_TEST_MAIN(mymain) +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virrandommock.so") #else

On 05/19/2016 05:00 AM, Peter Krempa wrote:

On Wed, May 18, 2016 at 19:52:32 -0400, John Ferlan wrote:

...

Introduce virCryptoHaveEncrypt and virCryptoEncryptSecret to handle performing encryption.

virCryptoHaveEncrypt: Boolean function to determine whether the requested encryption API is available. It's expected this API will be called prior to virCryptoEncryptSecret. It will return true/false.

virCryptoEncryptSecret: Based on the requested encryption 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 | 189 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/vircrypto.h | 20 ++++- tests/vircryptotest.c | 86 +++++++++++++++++++++ 5 files changed, 296 insertions(+), 2 deletions(-)

[...]

...

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 39a479a..05bd167 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c

[...]

...

@@ -76,3 +85,181 @@ virCryptoHashString(virCryptoHash hash,

return 0; } + + +/* virCryptoHaveEncrypt: + * @algorithm: Specific encryption 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 +virCryptoHaveEncrypt(virCryptoEncrypt algorithm)

Cipher rather than Encrypt

Fine -

...

+{ + switch (algorithm) { + + case VIR_CRYPTO_ENCRYPT_AES256CBC: +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + return true; +#else + return false; +#endif + + case VIR_CRYPTO_ENCRYPT_NONE: + case VIR_CRYPTO_ENCRYPT_LAST: + break; + }; + + return false; +} + + +/* virCryptoEncryptDataAESgntuls: + * + * Performs the AES gnutls encryption - parameters essentially the + * same as virCryptoEncryptSecret, except the libvirt algorithm is + * converted to the gnutls_cipher_algorithm_t + * + * Same input as virCryptoEncryptData, but ensures encryption key and + * initialization vector lengths are correct. + * + * The data buffer will be cleared as soon as it has been prepared for + * encryption. + * + * 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_key;

As pointed out last time, even here it won't compile with gnutls.

I was more focused on getting the framework right. I'll add #ifdef HAVE_GNUTLS_CIPHER_ENCRYPT around the virCryptoEncryptDataAESgnutls function and around the calling spot where the #else will be a VIR_ERR_OPERATION_UNSUPPORTED error. I did point out in my cover that this probably should have been an RFC, but seeing as it's the setup for the other AES patches - I just went with a v1.

...

+ uint8_t *ciphertext; + size_t ciphertextlen; + + /* Allocate a padded buffer, copy in the data, padding the buffer with ^^^^^^^^^^^^^^^^^ + * the size of the padding size which is required for decryption, then ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + * clear the data buffer when we're done. */ + ciphertextlen = VIR_ROUND_UP(datalen, 16); + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0) + return -1; + memcpy(ciphertext, data, datalen); + for (i = datalen; i < ciphertextlen; i++)

And this still isn't documented.

It was, but I'll move it to make it more obvious.

...

+ ciphertext[i] = ciphertextlen - datalen; + memset(data, 0, datalen);

The caller should ensure sanitization. Especially sinc its _NOT_ done if the above allocation fails. In such case the caller still needs to do it.

Fine

...

+ + /* Initialize the gnutls cipher */ + enc_key.size = enckeylen; + enc_key.data = enckey; + if (iv) { + iv_key.size = ivlen; + iv_key.data = iv; + } + if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg, + &enc_key, &iv_key)) < 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; +} + + +/* 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(virCryptoEncrypt 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_ENCRYPT_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, + _("AES246CBC initialization vector invalid len=%zu"),

AES256CBC

OK

...

+ ivlen); + return -1; + } + + /* + * 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); + + case VIR_CRYPTO_ENCRYPT_NONE: + case VIR_CRYPTO_ENCRYPT_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..7d0829e 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h

[...]

...

@@ -30,6 +30,14 @@ typedef enum { VIR_CRYPTO_HASH_LAST } virCryptoHash;

+ +typedef enum { + VIR_CRYPTO_ENCRYPT_NONE = 0, + VIR_CRYPTO_ENCRYPT_AES256CBC, + + VIR_CRYPTO_ENCRYPT_LAST +} virCryptoEncrypt;

This can be used for decryption too. Thus virCryptoCipher.

Sure and the symbols change toe _CIPHER_ as well. That's fine.

...

+ int virCryptoHashString(virCryptoHash hash, const char *input,

[...]

...

diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index bfc47db..038eb7a 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c

[...]

...

@@ -56,10 +57,74 @@ testCryptoHash(const void *opaque) }

+struct testCryptoEncryptData { + virCryptoEncrypt algorithm; + uint8_t *input; + size_t inputlen; + const char *base64; +}; + +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; + char *actual = NULL; + int ret = -1; + + if (!virCryptoHaveEncrypt(data->algorithm)) { + fprintf(stderr, "Invalid encryption algorithm=%d\n", data->algorithm); + return -1; + } + + if (VIR_ALLOC_N(enckey, enckeylen) < 0 || + VIR_ALLOC_N(iv, ivlen) < 0) + goto cleanup; + + /* To be replaced by mock if I can get it to work */

Why not keep it this way? This is testing the encryption not the key generation.

Well I was *hoping* to get the mock to work... Something covered between patch 2 and the cover (which IIRC you don't read covers).

...

+ for (i = 0; i < enckeylen; i++) + enckey[i] = i; + for (i = 0; i < ivlen; i++) + iv[i] = i; + + if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen, + data->input, data->inputlen, + &ciphertext, &ciphertextlen) < 0) + goto cleanup; + + /* Comparing the encoded ciphertext would be what is desired in + * the long run and easier to read/format the expected ciphertext */

Read? nobody is going to read it if it doesn't match.

OK - so I took the other route - passing in an expected cipher value.

...

+ if (!(actual = virStringEncodeBase64(ciphertext, ciphertextlen))) + goto cleanup; + + if (STRNEQ(data->base64, actual)) { + fprintf(stderr, "Expected encoded encryption '%s' but got '%s'\n", + data->base64, NULLSTR(actual));

and it definitely doesn't make sense to print it. It's just binary garbage.

I forgot the STRNEQ_NULLABLE too.. hrmph.

...

+ goto cleanup; + } + + ret = 0; + cleanup: + VIR_DISPOSE_N(enckey, enckeylen); + VIR_DISPOSE_N(iv, ivlen); + VIR_DISPOSE_N(ciphertext, ciphertextlen);

This isn't really necessary for demo keys. You can also lose the lenght variables.

OK

...

+ VIR_FREE(actual); +

I won't review further versions until you fix the issue with compiling without gnutls. I've pointed it out far too many times.

I didn't expect v1 to be the last - figured it was more important to get the structure as expected. John