[PATCH 0/4] crypto: handle gcrypt ciphers being disabled

[PATCH 0/4] crypto: handle gcrypt ciphers being disabled

[Daniel P. Berrangé]

This fixes testing on RHEL with libgcrypt cipher backends

Daniel P. Berrangé (4):
  crypto: factor out conversion of QAPI to gcrypt constants
  crypto: query gcrypt for cipher availability
  crypto: use error_abort for unexpected failures
  crypto: report which ciphers are being skipped during tests

 crypto/cipher-gcrypt.c.inc      | 121 +++++++++++++++++---------------
 tests/unit/test-crypto-cipher.c |   7 +-
 2 files changed, 70 insertions(+), 58 deletions(-)

-- 
2.43.0

[PATCH 1/4] crypto: factor out conversion of QAPI to gcrypt constants

[Daniel P. Berrangé]

The conversion of cipher mode will shortly be required in more
than one place.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/cipher-gcrypt.c.inc | 116 +++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 56 deletions(-)

diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
index 1377cbaf14..6b82280f90 100644
--- a/crypto/cipher-gcrypt.c.inc
+++ b/crypto/cipher-gcrypt.c.inc
@@ -20,6 +20,56 @@
 
 #include <gcrypt.h>
 
+static int qcrypto_cipher_alg_to_gcry_alg(QCryptoCipherAlgorithm alg)
+{
+    switch (alg) {
+    case QCRYPTO_CIPHER_ALG_DES:
+        return GCRY_CIPHER_DES;
+    case QCRYPTO_CIPHER_ALG_3DES:
+        return GCRY_CIPHER_3DES;
+    case QCRYPTO_CIPHER_ALG_AES_128:
+        return GCRY_CIPHER_AES128;
+    case QCRYPTO_CIPHER_ALG_AES_192:
+        return GCRY_CIPHER_AES192;
+    case QCRYPTO_CIPHER_ALG_AES_256:
+        return GCRY_CIPHER_AES256;
+    case QCRYPTO_CIPHER_ALG_CAST5_128:
+        return GCRY_CIPHER_CAST5;
+    case QCRYPTO_CIPHER_ALG_SERPENT_128:
+        return GCRY_CIPHER_SERPENT128;
+    case QCRYPTO_CIPHER_ALG_SERPENT_192:
+        return GCRY_CIPHER_SERPENT192;
+    case QCRYPTO_CIPHER_ALG_SERPENT_256:
+        return GCRY_CIPHER_SERPENT256;
+    case QCRYPTO_CIPHER_ALG_TWOFISH_128:
+        return GCRY_CIPHER_TWOFISH128;
+    case QCRYPTO_CIPHER_ALG_TWOFISH_256:
+        return GCRY_CIPHER_TWOFISH;
+#ifdef CONFIG_CRYPTO_SM4
+    case QCRYPTO_CIPHER_ALG_SM4:
+        return GCRY_CIPHER_SM4;
+#endif
+    default:
+        return GCRY_CIPHER_NONE;
+    }
+}
+
+static int qcrypto_cipher_mode_to_gcry_mode(QCryptoCipherMode mode)
+{
+    switch (mode) {
+    case QCRYPTO_CIPHER_MODE_ECB:
+        return GCRY_CIPHER_MODE_ECB;
+    case QCRYPTO_CIPHER_MODE_XTS:
+        return GCRY_CIPHER_MODE_XTS;
+    case QCRYPTO_CIPHER_MODE_CBC:
+        return GCRY_CIPHER_MODE_CBC;
+    case QCRYPTO_CIPHER_MODE_CTR:
+        return GCRY_CIPHER_MODE_CTR;
+    default:
+        return GCRY_CIPHER_MODE_NONE;
+    }
+}
+
 bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
                              QCryptoCipherMode mode)
 {
@@ -188,72 +238,26 @@ static QCryptoCipher *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
         return NULL;
     }
 
-    switch (alg) {
-    case QCRYPTO_CIPHER_ALG_DES:
-        gcryalg = GCRY_CIPHER_DES;
-        break;
-    case QCRYPTO_CIPHER_ALG_3DES:
-        gcryalg = GCRY_CIPHER_3DES;
-        break;
-    case QCRYPTO_CIPHER_ALG_AES_128:
-        gcryalg = GCRY_CIPHER_AES128;
-        break;
-    case QCRYPTO_CIPHER_ALG_AES_192:
-        gcryalg = GCRY_CIPHER_AES192;
-        break;
-    case QCRYPTO_CIPHER_ALG_AES_256:
-        gcryalg = GCRY_CIPHER_AES256;
-        break;
-    case QCRYPTO_CIPHER_ALG_CAST5_128:
-        gcryalg = GCRY_CIPHER_CAST5;
-        break;
-    case QCRYPTO_CIPHER_ALG_SERPENT_128:
-        gcryalg = GCRY_CIPHER_SERPENT128;
-        break;
-    case QCRYPTO_CIPHER_ALG_SERPENT_192:
-        gcryalg = GCRY_CIPHER_SERPENT192;
-        break;
-    case QCRYPTO_CIPHER_ALG_SERPENT_256:
-        gcryalg = GCRY_CIPHER_SERPENT256;
-        break;
-    case QCRYPTO_CIPHER_ALG_TWOFISH_128:
-        gcryalg = GCRY_CIPHER_TWOFISH128;
-        break;
-    case QCRYPTO_CIPHER_ALG_TWOFISH_256:
-        gcryalg = GCRY_CIPHER_TWOFISH;
-        break;
-#ifdef CONFIG_CRYPTO_SM4
-    case QCRYPTO_CIPHER_ALG_SM4:
-        gcryalg = GCRY_CIPHER_SM4;
-        break;
-#endif
-    default:
+    gcryalg = qcrypto_cipher_alg_to_gcry_alg(alg);
+    if (gcryalg == GCRY_CIPHER_NONE) {
         error_setg(errp, "Unsupported cipher algorithm %s",
                    QCryptoCipherAlgorithm_str(alg));
         return NULL;
     }
 
-    drv = &qcrypto_gcrypt_driver;
-    switch (mode) {
-    case QCRYPTO_CIPHER_MODE_ECB:
-        gcrymode = GCRY_CIPHER_MODE_ECB;
-        break;
-    case QCRYPTO_CIPHER_MODE_XTS:
-        gcrymode = GCRY_CIPHER_MODE_XTS;
-        break;
-    case QCRYPTO_CIPHER_MODE_CBC:
-        gcrymode = GCRY_CIPHER_MODE_CBC;
-        break;
-    case QCRYPTO_CIPHER_MODE_CTR:
-        drv = &qcrypto_gcrypt_ctr_driver;
-        gcrymode = GCRY_CIPHER_MODE_CTR;
-        break;
-    default:
+    gcrymode = qcrypto_cipher_mode_to_gcry_mode(mode);
+    if (gcrymode == GCRY_CIPHER_MODE_NONE) {
         error_setg(errp, "Unsupported cipher mode %s",
                    QCryptoCipherMode_str(mode));
         return NULL;
     }
 
+    if (mode == QCRYPTO_CIPHER_MODE_CTR) {
+        drv = &qcrypto_gcrypt_ctr_driver;
+    } else {
+        drv = &qcrypto_gcrypt_driver;
+    }
+
     ctx = g_new0(QCryptoCipherGcrypt, 1);
     ctx->base.driver = drv;
 
-- 
2.43.0

Re: [PATCH 1/4] crypto: factor out conversion of QAPI to gcrypt constants

[Thomas Huth]

On 11/03/2024 13.19, Daniel P. Berrangé wrote:
> The conversion of cipher mode will shortly be required in more
> than one place.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/cipher-gcrypt.c.inc | 116 +++++++++++++++++++------------------
>   1 file changed, 60 insertions(+), 56 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/4] crypto: query gcrypt for cipher availability

[Daniel P. Berrangé]

Just because a cipher is defined in the gcrypt header file, does not
imply that it can be used. Distros can filter the list of ciphers when
building gcrypt. For example, RHEL-9 disables the SM4 cipher. It is
also possible that running in FIPS mode might dynamically change what
ciphers are available at runtime.

qcrypto_cipher_supports must therefore query gcrypt directly to check
for cipher availability.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/cipher-gcrypt.c.inc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
index 6b82280f90..4a8314746d 100644
--- a/crypto/cipher-gcrypt.c.inc
+++ b/crypto/cipher-gcrypt.c.inc
@@ -93,6 +93,11 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
         return false;
     }
 
+    if (gcry_cipher_algo_info(qcrypto_cipher_alg_to_gcry_alg(alg),
+                              GCRYCTL_TEST_ALGO, NULL, NULL) != 0) {
+        return false;
+    }
+
     switch (mode) {
     case QCRYPTO_CIPHER_MODE_ECB:
     case QCRYPTO_CIPHER_MODE_CBC:
-- 
2.43.0

Re: [PATCH 2/4] crypto: query gcrypt for cipher availability

[Thomas Huth]

On 11/03/2024 13.19, Daniel P. Berrangé wrote:
> Just because a cipher is defined in the gcrypt header file, does not
> imply that it can be used. Distros can filter the list of ciphers when
> building gcrypt. For example, RHEL-9 disables the SM4 cipher. It is
> also possible that running in FIPS mode might dynamically change what
> ciphers are available at runtime.
> 
> qcrypto_cipher_supports must therefore query gcrypt directly to check
> for cipher availability.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/cipher-gcrypt.c.inc | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
> index 6b82280f90..4a8314746d 100644
> --- a/crypto/cipher-gcrypt.c.inc
> +++ b/crypto/cipher-gcrypt.c.inc
> @@ -93,6 +93,11 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
>           return false;
>       }
>   
> +    if (gcry_cipher_algo_info(qcrypto_cipher_alg_to_gcry_alg(alg),
> +                              GCRYCTL_TEST_ALGO, NULL, NULL) != 0) {
> +        return false;
> +    }
> +
>       switch (mode) {
>       case QCRYPTO_CIPHER_MODE_ECB:
>       case QCRYPTO_CIPHER_MODE_CBC:

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 3/4] crypto: use error_abort for unexpected failures

[Daniel P. Berrangé]

This improves the error diagnosis from the unit test when a cipher
is unexpected not available from

ERROR:../tests/unit/test-crypto-cipher.c:683:test_cipher: assertion failed: (err == NULL)
Bail out! ERROR:../tests/unit/test-crypto-cipher.c:683:test_cipher: assertion failed: (err == NULL)
Aborted (core dumped)

to

Unexpected error in qcrypto_cipher_ctx_new() at ../crypto/cipher-gcrypt.c.inc:262:
./build//tests/unit/test-crypto-cipher: Cannot initialize cipher: Invalid cipher algorithm
Aborted (core dumped)

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/test-crypto-cipher.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c
index 11ab1a54fc..d0ea7b4d8e 100644
--- a/tests/unit/test-crypto-cipher.c
+++ b/tests/unit/test-crypto-cipher.c
@@ -676,9 +676,8 @@ static void test_cipher(const void *opaque)
     cipher = qcrypto_cipher_new(
         data->alg, data->mode,
         key, nkey,
-        &err);
+        data->plaintext ? &error_abort : &err);
     if (data->plaintext) {
-        g_assert(err == NULL);
         g_assert(cipher != NULL);
     } else {
         error_free_or_abort(&err);
-- 
2.43.0

Re: [PATCH 3/4] crypto: use error_abort for unexpected failures

[Thomas Huth]

On 11/03/2024 13.19, Daniel P. Berrangé wrote:
> This improves the error diagnosis from the unit test when a cipher
> is unexpected not available from
> 
> ERROR:../tests/unit/test-crypto-cipher.c:683:test_cipher: assertion failed: (err == NULL)
> Bail out! ERROR:../tests/unit/test-crypto-cipher.c:683:test_cipher: assertion failed: (err == NULL)
> Aborted (core dumped)
> 
> to
> 
> Unexpected error in qcrypto_cipher_ctx_new() at ../crypto/cipher-gcrypt.c.inc:262:
> ./build//tests/unit/test-crypto-cipher: Cannot initialize cipher: Invalid cipher algorithm
> Aborted (core dumped)
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/unit/test-crypto-cipher.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c
> index 11ab1a54fc..d0ea7b4d8e 100644
> --- a/tests/unit/test-crypto-cipher.c
> +++ b/tests/unit/test-crypto-cipher.c
> @@ -676,9 +676,8 @@ static void test_cipher(const void *opaque)
>       cipher = qcrypto_cipher_new(
>           data->alg, data->mode,
>           key, nkey,
> -        &err);
> +        data->plaintext ? &error_abort : &err);
>       if (data->plaintext) {
> -        g_assert(err == NULL);
>           g_assert(cipher != NULL);
>       } else {
>           error_free_or_abort(&err);

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 4/4] crypto: report which ciphers are being skipped during tests

[Daniel P. Berrangé]

Since the ciphers can be dynamically disabled at runtime, when running
unit tests it is helpful to report which ciphers we can skipped for
testing.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/test-crypto-cipher.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c
index d0ea7b4d8e..f5152e569d 100644
--- a/tests/unit/test-crypto-cipher.c
+++ b/tests/unit/test-crypto-cipher.c
@@ -821,6 +821,10 @@ int main(int argc, char **argv)
     for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
         if (qcrypto_cipher_supports(test_data[i].alg, test_data[i].mode)) {
             g_test_add_data_func(test_data[i].path, &test_data[i], test_cipher);
+        } else {
+            g_printerr("# skip unsupported %s:%s\n",
+                       QCryptoCipherAlgorithm_str(test_data[i].alg),
+                       QCryptoCipherMode_str(test_data[i].mode));
         }
     }
 
-- 
2.43.0

Re: [PATCH 4/4] crypto: report which ciphers are being skipped during tests

[Thomas Huth]

On 11/03/2024 13.19, Daniel P. Berrangé wrote:
> Since the ciphers can be dynamically disabled at runtime, when running
> unit tests it is helpful to report which ciphers we can skipped for
> testing.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/unit/test-crypto-cipher.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c
> index d0ea7b4d8e..f5152e569d 100644
> --- a/tests/unit/test-crypto-cipher.c
> +++ b/tests/unit/test-crypto-cipher.c
> @@ -821,6 +821,10 @@ int main(int argc, char **argv)
>       for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
>           if (qcrypto_cipher_supports(test_data[i].alg, test_data[i].mode)) {
>               g_test_add_data_func(test_data[i].path, &test_data[i], test_cipher);
> +        } else {
> +            g_printerr("# skip unsupported %s:%s\n",
> +                       QCryptoCipherAlgorithm_str(test_data[i].alg),
> +                       QCryptoCipherMode_str(test_data[i].mode));
>           }
>       }
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/4] crypto: handle gcrypt ciphers being disabled

[Philippe Mathieu-Daudé]

On 11/3/24 13:19, Daniel P. Berrangé wrote:
> This fixes testing on RHEL with libgcrypt cipher backends
> 
> Daniel P. Berrangé (4):
>    crypto: factor out conversion of QAPI to gcrypt constants
>    crypto: query gcrypt for cipher availability
>    crypto: use error_abort for unexpected failures
>    crypto: report which ciphers are being skipped during tests

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