[PATCH v2 0/2] iotests: Fix crypto algorithm failures

[PATCH v2 0/2] iotests: Fix crypto algorithm failures

[Hanna Reitz]

Hi,

iotests 149, 206, and 210 fail when qemu uses the gnutls crypto backend
(which is the default as of 8bd0931f6) because they try to use
algorithms that this backend does not support.

Have 206 and 210 use different algorithms instead (patch 1), and let 149
be skipped when it encounters an unsupported algorithm (patch 2).


v2:
- Fixed the `check_cipher_support()` function introduced in patch 2
  (forgot to pass `config`, though it worked even without, apparently
  because `config` is a global-scope variable....)

- Also a good opportunity to CC Dan, who I forgot on v1


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[----] [--] 'iotests: Use aes-128-cbc'
002/2:[0008] [FC] 'iotests/149: Skip on unsupported ciphers'


Hanna Reitz (2):
  iotests: Use aes-128-cbc
  iotests/149: Skip on unsupported ciphers

 tests/qemu-iotests/149     | 23 ++++++++++++++++++-----
 tests/qemu-iotests/206     |  4 ++--
 tests/qemu-iotests/206.out |  6 +++---
 tests/qemu-iotests/210     |  4 ++--
 tests/qemu-iotests/210.out |  6 +++---
 5 files changed, 28 insertions(+), 15 deletions(-)

-- 
2.33.1

[PATCH v2 1/2] iotests: Use aes-128-cbc

[Hanna Reitz]

Our gnutls crypto backend (which is the default as of 8bd0931f6)
supports neither twofish-128 nor the CTR mode.  CBC and aes-128 are
supported by all of our backends (as far as I can tell), so use
aes-128-cbc in our iotests.

(We could also use e.g. aes-256-cbc, but the different key sizes would
lead to different key slot offsets and so change the reference output
more, which is why I went with aes-128.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/206     | 4 ++--
 tests/qemu-iotests/206.out | 6 +++---
 tests/qemu-iotests/210     | 4 ++--
 tests/qemu-iotests/210.out | 6 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index c3cdad4ce4..10eff343f7 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -162,8 +162,8 @@ with iotests.FilePath('t.qcow2') as disk_path, \
                          'encrypt': {
                              'format': 'luks',
                              'key-secret': 'keysec0',
-                             'cipher-alg': 'twofish-128',
-                             'cipher-mode': 'ctr',
+                             'cipher-alg': 'aes-128',
+                             'cipher-mode': 'cbc',
                              'ivgen-alg': 'plain64',
                              'ivgen-hash-alg': 'md5',
                              'hash-alg': 'sha1',
diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out
index 3593e8e9c2..80cd274223 100644
--- a/tests/qemu-iotests/206.out
+++ b/tests/qemu-iotests/206.out
@@ -97,7 +97,7 @@ Format specific information:
 
 === Successful image creation (encrypted) ===
 
-{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "qcow2", "encrypt": {"cipher-alg": "twofish-128", "cipher-mode": "ctr", "format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file": {"driver": "file", "filename": "TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "qcow2", "encrypt": {"cipher-alg": "aes-128", "cipher-mode": "cbc", "format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file": {"driver": "file", "filename": "TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
@@ -115,10 +115,10 @@ Format specific information:
     encrypt:
         ivgen alg: plain64
         hash alg: sha1
-        cipher alg: twofish-128
+        cipher alg: aes-128
         uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
         format: luks
-        cipher mode: ctr
+        cipher mode: cbc
         slots:
             [0]:
                 active: true
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 5a62ed4dd1..a4dcc5fe59 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -83,8 +83,8 @@ with iotests.FilePath('t.luks') as disk_path, \
                          },
                          'size': size,
                          'key-secret': 'keysec0',
-                         'cipher-alg': 'twofish-128',
-                         'cipher-mode': 'ctr',
+                         'cipher-alg': 'aes-128',
+                         'cipher-mode': 'cbc',
                          'ivgen-alg': 'plain64',
                          'ivgen-hash-alg': 'md5',
                          'hash-alg': 'sha1',
diff --git a/tests/qemu-iotests/210.out b/tests/qemu-iotests/210.out
index 55c0844370..96d9f749dd 100644
--- a/tests/qemu-iotests/210.out
+++ b/tests/qemu-iotests/210.out
@@ -59,7 +59,7 @@ Format specific information:
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
 
-{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"cipher-alg": "twofish-128", "cipher-mode": "ctr", "driver": "luks", "file": {"driver": "file", "filename": "TEST_DIR/PID-t.luks"}, "hash-alg": "sha1", "iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0", "size": 67108864}}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"cipher-alg": "aes-128", "cipher-mode": "cbc", "driver": "luks", "file": {"driver": "file", "filename": "TEST_DIR/PID-t.luks"}, "hash-alg": "sha1", "iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0", "size": 67108864}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
@@ -71,9 +71,9 @@ encrypted: yes
 Format specific information:
     ivgen alg: plain64
     hash alg: sha1
-    cipher alg: twofish-128
+    cipher alg: aes-128
     uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
-    cipher mode: ctr
+    cipher mode: cbc
     slots:
         [0]:
             active: true
-- 
2.33.1

Re: [PATCH v2 1/2] iotests: Use aes-128-cbc

[Daniel P. Berrangé]

On Wed, Nov 17, 2021 at 04:17:06PM +0100, Hanna Reitz wrote:
> Our gnutls crypto backend (which is the default as of 8bd0931f6)
> supports neither twofish-128 nor the CTR mode.  CBC and aes-128 are
> supported by all of our backends (as far as I can tell), so use
> aes-128-cbc in our iotests.

Yes, AES is guarnateed by all backends, as is ECB,CBC & XTS modes.

> 
> (We could also use e.g. aes-256-cbc, but the different key sizes would
> lead to different key slot offsets and so change the reference output
> more, which is why I went with aes-128.)
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  tests/qemu-iotests/206     | 4 ++--
>  tests/qemu-iotests/206.out | 6 +++---
>  tests/qemu-iotests/210     | 4 ++--
>  tests/qemu-iotests/210.out | 6 +++---
>  4 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v2 1/2] iotests: Use aes-128-cbc

[Thomas Huth]

On 17/11/2021 16.17, Hanna Reitz wrote:
> Our gnutls crypto backend (which is the default as of 8bd0931f6)
> supports neither twofish-128 nor the CTR mode.  CBC and aes-128 are
> supported by all of our backends (as far as I can tell), so use
> aes-128-cbc in our iotests.
> 
> (We could also use e.g. aes-256-cbc, but the different key sizes would
> lead to different key slot offsets and so change the reference output
> more, which is why I went with aes-128.)
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>   tests/qemu-iotests/206     | 4 ++--
>   tests/qemu-iotests/206.out | 6 +++---
>   tests/qemu-iotests/210     | 4 ++--
>   tests/qemu-iotests/210.out | 6 +++---
>   4 files changed, 10 insertions(+), 10 deletions(-)

Thanks, this fixes the failure on my system!

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

[PATCH v2 2/2] iotests/149: Skip on unsupported ciphers

[Hanna Reitz]

Whenever qemu-img or qemu-io report that some cipher is unsupported,
skip the whole test, because that is probably because qemu has been
configured with the gnutls crypto backend.

We could taylor the algorithm list to what gnutls supports, but this is
a test that is run rather rarely anyway (because it requires
password-less sudo), and so it seems better and easier to skip it.  When
this test is intentionally run to check LUKS compatibility, it seems
better not to limit the algorithms but keep the list extensive.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/149 | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 328fd05a4c..d49646ca60 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -230,6 +230,18 @@ def create_image(config, size_mb):
         fn.truncate(size_mb * 1024 * 1024)
 
 
+def check_cipher_support(config, output):
+    """Check the output of qemu-img or qemu-io for mention of the respective
+    cipher algorithm being unsupported, and if so, skip this test.
+    (Returns `output` for convenience.)"""
+
+    if 'Unsupported cipher algorithm' in output:
+        iotests.notrun('Unsupported cipher algorithm '
+                       f'{config.cipher}-{config.keylen}-{config.mode}; '
+                       'consider configuring qemu with a different crypto '
+                       'backend')
+    return output
+
 def qemu_img_create(config, size_mb):
     """Create and format a disk image with LUKS using qemu-img"""
 
@@ -253,7 +265,8 @@ def qemu_img_create(config, size_mb):
             "%dM" % size_mb]
 
     iotests.log("qemu-img " + " ".join(args), filters=[iotests.filter_test_dir])
-    iotests.log(iotests.qemu_img_pipe(*args), filters=[iotests.filter_test_dir])
+    iotests.log(check_cipher_support(config, iotests.qemu_img_pipe(*args)),
+                filters=[iotests.filter_test_dir])
 
 def qemu_io_image_args(config, dev=False):
     """Get the args for access an image or device with qemu-io"""
@@ -279,8 +292,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb, size_mb, dev=False):
     args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
     args.extend(qemu_io_image_args(config, dev))
     iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-    iotests.log(iotests.qemu_io(*args), filters=[iotests.filter_test_dir,
-                                                 iotests.filter_qemu_io])
+    iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+                filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
 def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
@@ -291,8 +304,8 @@ def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
     args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
     args.extend(qemu_io_image_args(config, dev))
     iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-    iotests.log(iotests.qemu_io(*args), filters=[iotests.filter_test_dir,
-                                                 iotests.filter_qemu_io])
+    iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+                filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
 def test_once(config, qemu_img=False):
-- 
2.33.1

Re: [PATCH v2 2/2] iotests/149: Skip on unsupported ciphers

[Daniel P. Berrangé]

On Wed, Nov 17, 2021 at 04:17:07PM +0100, Hanna Reitz wrote:
> Whenever qemu-img or qemu-io report that some cipher is unsupported,
> skip the whole test, because that is probably because qemu has been
> configured with the gnutls crypto backend.
> 
> We could taylor the algorithm list to what gnutls supports, but this is
> a test that is run rather rarely anyway (because it requires
> password-less sudo), and so it seems better and easier to skip it.  When
> this test is intentionally run to check LUKS compatibility, it seems
> better not to limit the algorithms but keep the list extensive.

I'd really like to figure out a way to be able to partially run
this test. When I have hit problems in the past, I needed to
run specific tests, but then the expected output always contains
everything.  I've thought of a few options

 - Split it into many stanadlone tests - eg
     tests/qemu-iotests/tests/luks-host-$ALG

 - Split only the expected output eg
 
     149-$SUBTEST

  and have a way to indicate which of expected output files
  we need to concatenate for the set of subtests that we
  run.

 - Introduce some template syntax in expected output
   tha can be used to munge the output.

 - Stop comparing expected output entirely and just
   then this into a normal python unit test.

 - Insert your idea here ?

> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  tests/qemu-iotests/149 | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v2 2/2] iotests/149: Skip on unsupported ciphers

[Hanna Reitz]

On 17.11.21 16:46, Daniel P. Berrangé wrote:
> On Wed, Nov 17, 2021 at 04:17:07PM +0100, Hanna Reitz wrote:
>> Whenever qemu-img or qemu-io report that some cipher is unsupported,
>> skip the whole test, because that is probably because qemu has been
>> configured with the gnutls crypto backend.
>>
>> We could taylor the algorithm list to what gnutls supports, but this is
>> a test that is run rather rarely anyway (because it requires
>> password-less sudo), and so it seems better and easier to skip it.  When
>> this test is intentionally run to check LUKS compatibility, it seems
>> better not to limit the algorithms but keep the list extensive.
> I'd really like to figure out a way to be able to partially run
> this test. When I have hit problems in the past, I needed to
> run specific tests, but then the expected output always contains
> everything.  I've thought of a few options
>
>   - Split it into many stanadlone tests - eg
>       tests/qemu-iotests/tests/luks-host-$ALG

I wouldn’t hate it, though we should have some common file where common 
code can be sourced from.

>   - Split only the expected output eg
>   
>       149-$SUBTEST
>
>    and have a way to indicate which of expected output files
>    we need to concatenate for the set of subtests that we
>    run.

I’d prefer it if the test could verify its own output so that the 
reference output is basically just the usual unittest output of dots, 
“Ran XX tests” and “OK”.

(Two reasons: You can then easily disable some tests with the reference 
output changing only slightly; and it makes reviewing a test much easier 
because then I don’t need to verify the reference output...)

>   - Introduce some template syntax in expected output
>     tha can be used to munge the output.
>
>   - Stop comparing expected output entirely and just
>     then this into a normal python unit test.

That’s something that might indeed be useful for unittest-style iotests.

Then again, we already allow them to skip any test case and it will be 
counted as success, is that not sufficient?

>   - Insert your idea here ?

I personally most prefer unittest-style tests, because with them you can 
just %s/def test_/def xtest_/, then reverse this change for all the 
cases you want to run, and then adjust the reference output to match the 
number of tests run.

So I suppose the best idea I have is to convert this test into unittest 
style, and then it should be more modular when it comes to what subtests 
it wants to run.

I mean, it doesn’t have to truly be an iotests.QMPTestCase.  It would be 
sufficient if the test itself verified the output of every command it 
invokes (instead of leaving that to a separate reference output file) 
and then printed something like “OK” afterwards.  Then we could 
trivially skip some cases just by printing “OK” even if they weren’t run.

Hanna