[Qemu-devel] [PATCH] crypto: extend unit tests to cover decryption too

[Qemu-devel] [PATCH] crypto: extend unit tests to cover decryption too

[Daniel P. Berrange]

The current unit test only verify the encryption API, which
resulted in us missing a recently introduced bug in the
decryption API from commit d3462e3. It was fortunately
later discovered & fixed by commit bd0959 thanks to the
QEMU I/O tests for qcow2 encryption, but we should really
detect this directly in the crypto unit tests.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-crypto-cipher.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tests/test-crypto-cipher.c b/tests/test-crypto-cipher.c
index f9b1a03..4485886 100644
--- a/tests/test-crypto-cipher.c
+++ b/tests/test-crypto-cipher.c
@@ -231,7 +231,6 @@ static void test_cipher(const void *opaque)
     size_t nkey, niv, nciphertext, nplaintext;
     char *outtexthex;
 
-    g_test_message("foo");
     nkey = unhex_string(data->key, &key);
     niv = unhex_string(data->iv, &iv);
     nciphertext = unhex_string(data->ciphertext, &ciphertext);
@@ -266,6 +265,25 @@ static void test_cipher(const void *opaque)
 
     g_assert_cmpstr(outtexthex, ==, data->ciphertext);
 
+    g_free(outtexthex);
+
+    if (iv) {
+        g_assert(qcrypto_cipher_setiv(cipher,
+                                      iv, niv,
+                                      &err) == 0);
+        g_assert(err == NULL);
+    }
+    g_assert(qcrypto_cipher_decrypt(cipher,
+                                    ciphertext,
+                                    outtext,
+                                    nplaintext,
+                                    &err) == 0);
+    g_assert(err == NULL);
+
+    outtexthex = hex_string(outtext, nplaintext);
+
+    g_assert_cmpstr(outtexthex, ==, data->plaintext);
+
     g_free(outtext);
     g_free(outtexthex);
     g_free(key);
-- 
2.4.3

Re: [Qemu-devel] [PATCH] crypto: extend unit tests to cover decryption too

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On 07/20/2015 11:28 AM, Daniel P. Berrange wrote:
> The current unit test only verify the encryption API, which

grammatical mismatch; you want either:
tests only verify
test only verifies

> resulted in us missing a recently introduced bug in the
> decryption API from commit d3462e3. It was fortunately
> later discovered & fixed by commit bd0959 thanks to the

Is a 6-byte SHA going to stay unambiguous long enough? git defaults to
7-byte to reduce chances of collisions by 16.

> QEMU I/O tests for qcow2 encryption, but we should really
> detect this directly in the crypto unit tests.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/test-crypto-cipher.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 

> @@ -266,6 +265,25 @@ static void test_cipher(const void *opaque)
>  
>      g_assert_cmpstr(outtexthex, ==, data->ciphertext);
>  
> +    g_free(outtexthex);
> +
> +    if (iv) {
> +        g_assert(qcrypto_cipher_setiv(cipher,
> +                                      iv, niv,
> +                                      &err) == 0);
> +        g_assert(err == NULL);

g_assert(qcrypto_cipher_setiv(cipher, iv, niv, &error_abort);

saves you a call to g_assert().

> +    }
> +    g_assert(qcrypto_cipher_decrypt(cipher,
> +                                    ciphertext,
> +                                    outtext,
> +                                    nplaintext,
> +                                    &err) == 0);
> +    g_assert(err == NULL);

and again

But that's style, not correctness, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] crypto: extend unit tests to cover decryption too

[Peter Maydell]

On 20 July 2015 at 21:57, Eric Blake <eblake@redhat.com> wrote:
> On 07/20/2015 11:28 AM, Daniel P. Berrange wrote:
>> The current unit test only verify the encryption API, which
>
> grammatical mismatch; you want either:
> tests only verify
> test only verifies
>
>> resulted in us missing a recently introduced bug in the
>> decryption API from commit d3462e3. It was fortunately
>> later discovered & fixed by commit bd0959 thanks to the
>
> Is a 6-byte SHA going to stay unambiguous long enough? git defaults to
> 7-byte to reduce chances of collisions by 16.

Fortunately causality allows us to disambiguate by knowing
that Daniel can only be referring to a commit that's
already happened rather than one in the future :-)

-- PMM

Re: [Qemu-devel] [PATCH] crypto: extend unit tests to cover decryption too

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2315 bytes --]

On 07/20/2015 03:37 PM, Peter Maydell wrote:
> On 20 July 2015 at 21:57, Eric Blake <eblake@redhat.com> wrote:
>> On 07/20/2015 11:28 AM, Daniel P. Berrange wrote:
>>> The current unit test only verify the encryption API, which
>>
>> grammatical mismatch; you want either:
>> tests only verify
>> test only verifies
>>
>>> resulted in us missing a recently introduced bug in the
>>> decryption API from commit d3462e3. It was fortunately
>>> later discovered & fixed by commit bd0959 thanks to the
>>
>> Is a 6-byte SHA going to stay unambiguous long enough? git defaults to
>> 7-byte to reduce chances of collisions by 16.
> 
> Fortunately causality allows us to disambiguate by knowing
> that Daniel can only be referring to a commit that's
> already happened rather than one in the future :-)

True, but that doesn't mean git makes it easy to disambiguate.  When, in
the future, resolving the short name becomes ambiguous, how do you coax
git into showing the full SHA of both (or all) candidates, as well as
the date they were created, so you can then compare dates to the commit
message you are reading and deduce which one was meant?

$ git rev-parse ffff
error: short SHA1 ffff is ambiguous.
ffff
error: short SHA1 ffff is ambiguous.
fatal: ambiguous argument 'ffff': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

/me goes and reads the man page

Hmm - this is something I've never used before:

$ git rev-parse --disambiguate=ffff
ffffb8c0bd544ab479a7a32a208d9de13aff62d9
ffffbf79aa11e013e8fd14ffb134faee6489b42c
ffffff01466a0180de7632842cf583c8a9cbf959
ffffbb369f3ed9bca5ff2867143f76d0c6e069c0

But that still doesn't tell you that two of those are blobs, and two of
them commits, nor what dates the commits have, in a way that can easily
determine if one of the commits was an ancestor of the point where a
(hypothetical) commit message referred to a (once-non-ambiguous) short
reference.

And (straying off-topic for this list) why does 'git rev-parse
--disambiguate ffff' not behave the same as 'git rev-parse
--disambiguate=ffff'?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]