crypto: misc pbkdf fixes for testing & algorithm compat

[PATCH 2/2] tests/unit: always build the pbkdf crypto unit test

Posted by Daniel P. Berrangé 2 months, 3 weeks ago

The meson rules were excluding the pbkdf crypto test when gnutls was the
crypto backend. It was then excluded again in #if statements in the test
file.

Rather than update these conditions, remove them all, and use the result
of the qcrypto_pbkdf_supports() function to determine whether to skip
test registration.

Also add CONFIG_DARWIN to the remaining condition, since we have a way
to measure CPU time on this platform since commit bf98afc75efedf1.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/meson.build         | 4 +---
 tests/unit/test-crypto-pbkdf.c | 9 ++++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 490ab8182d..972d792883 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -121,9 +121,7 @@ if have_block
   if config_host_data.get('CONFIG_REPLICATION')
     tests += {'test-replication': [testblock]}
   endif
-  if nettle.found() or gcrypt.found()
-    tests += {'test-crypto-pbkdf': [io]}
-  endif
+  tests += {'test-crypto-pbkdf': [io]}
 endif
 
 if have_system
diff --git a/tests/unit/test-crypto-pbkdf.c b/tests/unit/test-crypto-pbkdf.c
index 43c417f6b4..034bb02422 100644
--- a/tests/unit/test-crypto-pbkdf.c
+++ b/tests/unit/test-crypto-pbkdf.c
@@ -25,8 +25,7 @@
 #include <sys/resource.h>
 #endif
 
-#if ((defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT)) && \
-     (defined(_WIN32) || defined(RUSAGE_THREAD)))
+#if defined(_WIN32) || defined(RUSAGE_THREAD) || defined(CONFIG_DARWIN)
 #include "crypto/pbkdf.h"
 
 typedef struct QCryptoPbkdfTestData QCryptoPbkdfTestData;
@@ -422,13 +421,17 @@ int main(int argc, char **argv)
     g_assert(qcrypto_init(NULL) == 0);
 
     for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
+        if (!qcrypto_pbkdf2_supports(test_data[i].hash)) {
+            continue;
+        }
+
         if (!test_data[i].slow ||
             g_test_slow()) {
             g_test_add_data_func(test_data[i].path, &test_data[i], test_pbkdf);
         }
     }
 
-    if (g_test_slow()) {
+    if (g_test_slow() && qcrypto_pbkdf2_supports(QCRYPTO_HASH_ALG_SHA256)) {
         g_test_add_func("/crypt0/pbkdf/timing", test_pbkdf_timing);
     }
 
-- 
2.45.2

Re: [PATCH 2/2] tests/unit: always build the pbkdf crypto unit test

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

On 30/8/24 13:05, Daniel P. Berrangé wrote:
> The meson rules were excluding the pbkdf crypto test when gnutls was the
> crypto backend. It was then excluded again in #if statements in the test
> file.
> 
> Rather than update these conditions, remove them all, and use the result
> of the qcrypto_pbkdf_supports() function to determine whether to skip
> test registration.
> 
> Also add CONFIG_DARWIN to the remaining condition, since we have a way
> to measure CPU time on this platform since commit bf98afc75efedf1.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/unit/meson.build         | 4 +---
>   tests/unit/test-crypto-pbkdf.c | 9 ++++++---
>   2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 490ab8182d..972d792883 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -121,9 +121,7 @@ if have_block
>     if config_host_data.get('CONFIG_REPLICATION')
>       tests += {'test-replication': [testblock]}
>     endif
> -  if nettle.found() or gcrypt.found()
> -    tests += {'test-crypto-pbkdf': [io]}
> -  endif
> +  tests += {'test-crypto-pbkdf': [io]}
>   endif
>   
>   if have_system
> diff --git a/tests/unit/test-crypto-pbkdf.c b/tests/unit/test-crypto-pbkdf.c
> index 43c417f6b4..034bb02422 100644
> --- a/tests/unit/test-crypto-pbkdf.c
> +++ b/tests/unit/test-crypto-pbkdf.c
> @@ -25,8 +25,7 @@
>   #include <sys/resource.h>
>   #endif
>   
> -#if ((defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT)) && \
> -     (defined(_WIN32) || defined(RUSAGE_THREAD)))
> +#if defined(_WIN32) || defined(RUSAGE_THREAD) || defined(CONFIG_DARWIN)

Add CONFIG_DARWIN in a subsequent commit?

>   #include "crypto/pbkdf.h"
>   
>   typedef struct QCryptoPbkdfTestData QCryptoPbkdfTestData;
> @@ -422,13 +421,17 @@ int main(int argc, char **argv)
>       g_assert(qcrypto_init(NULL) == 0);
>   
>       for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
> +        if (!qcrypto_pbkdf2_supports(test_data[i].hash)) {
> +            continue;
> +        }
> +
>           if (!test_data[i].slow ||
>               g_test_slow()) {
>               g_test_add_data_func(test_data[i].path, &test_data[i], test_pbkdf);
>           }
>       }
>   
> -    if (g_test_slow()) {
> +    if (g_test_slow() && qcrypto_pbkdf2_supports(QCRYPTO_HASH_ALG_SHA256)) {
>           g_test_add_func("/crypt0/pbkdf/timing", test_pbkdf_timing);

While here, rename test_pbkdf_timing -> test_pbkdf_sha256_timing?

>       }
>   

Anyway,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/2] tests/unit: always build the pbkdf crypto unit test

Posted by Daniel P. Berrangé 2 months, 2 weeks ago

On Mon, Sep 02, 2024 at 09:45:04PM +0200, Philippe Mathieu-Daudé wrote:
> On 30/8/24 13:05, Daniel P. Berrangé wrote:
> > The meson rules were excluding the pbkdf crypto test when gnutls was the
> > crypto backend. It was then excluded again in #if statements in the test
> > file.
> > 
> > Rather than update these conditions, remove them all, and use the result
> > of the qcrypto_pbkdf_supports() function to determine whether to skip
> > test registration.
> > 
> > Also add CONFIG_DARWIN to the remaining condition, since we have a way
> > to measure CPU time on this platform since commit bf98afc75efedf1.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/unit/meson.build         | 4 +---
> >   tests/unit/test-crypto-pbkdf.c | 9 ++++++---
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> > index 490ab8182d..972d792883 100644
> > --- a/tests/unit/meson.build
> > +++ b/tests/unit/meson.build
> > @@ -121,9 +121,7 @@ if have_block
> >     if config_host_data.get('CONFIG_REPLICATION')
> >       tests += {'test-replication': [testblock]}
> >     endif
> > -  if nettle.found() or gcrypt.found()
> > -    tests += {'test-crypto-pbkdf': [io]}
> > -  endif
> > +  tests += {'test-crypto-pbkdf': [io]}
> >   endif
> >   if have_system
> > diff --git a/tests/unit/test-crypto-pbkdf.c b/tests/unit/test-crypto-pbkdf.c
> > index 43c417f6b4..034bb02422 100644
> > --- a/tests/unit/test-crypto-pbkdf.c
> > +++ b/tests/unit/test-crypto-pbkdf.c
> > @@ -25,8 +25,7 @@
> >   #include <sys/resource.h>
> >   #endif
> > -#if ((defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT)) && \
> > -     (defined(_WIN32) || defined(RUSAGE_THREAD)))
> > +#if defined(_WIN32) || defined(RUSAGE_THREAD) || defined(CONFIG_DARWIN)
> 
> Add CONFIG_DARWIN in a subsequent commit?

Yes, classic trap. If you have "Also ..." in a commit message then you've
just told yourself it should have been a separate commit :-)

> 
> >   #include "crypto/pbkdf.h"
> >   typedef struct QCryptoPbkdfTestData QCryptoPbkdfTestData;
> > @@ -422,13 +421,17 @@ int main(int argc, char **argv)
> >       g_assert(qcrypto_init(NULL) == 0);
> >       for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
> > +        if (!qcrypto_pbkdf2_supports(test_data[i].hash)) {
> > +            continue;
> > +        }
> > +
> >           if (!test_data[i].slow ||
> >               g_test_slow()) {
> >               g_test_add_data_func(test_data[i].path, &test_data[i], test_pbkdf);
> >           }
> >       }
> > -    if (g_test_slow()) {
> > +    if (g_test_slow() && qcrypto_pbkdf2_supports(QCRYPTO_HASH_ALG_SHA256)) {
> >           g_test_add_func("/crypt0/pbkdf/timing", test_pbkdf_timing);
> 
> While here, rename test_pbkdf_timing -> test_pbkdf_sha256_timing?

Will do

> 
> >       }
> 
> Anyway,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|