[PATCH 0/2] iotests/207: Filter host fingerprint

[PATCH 0/2] iotests/207: Filter host fingerprint

[Hanna Reitz]

Hi,

Commit e3296cc796aeaf (“block: print the server key type and fingerprint
on failure”) improved the verbosity of our ssh block driver's error
messages for fingerprint mismatches.  However, iotest 207, which tests
such errors, has not been adjusted accordingly.

Since the fingerprint will differ between hosts, we need to filter it
(and can’t just statically adjust the reference output).  The problem is
that the error condition is printed by iotest.py’s VM.run_job(), so we
need to pass the filter to that function.  Right now, VM.run_job()
doesn’t support any filters, though, so patch 1 adds a filter parameter
and makes VM.run_job() use it.

Patch 2 then has the fix for iotest 207.


Hanna Reitz (2):
  iotests.py: Filters for VM.run_job()
  iotests/207: Filter host fingerprint

 tests/qemu-iotests/207        |  7 ++++++-
 tests/qemu-iotests/207.out    |  6 +++---
 tests/qemu-iotests/iotests.py | 26 ++++++++++++++++----------
 3 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH 1/2] iotests.py: Filters for VM.run_job()

[Hanna Reitz]

Allow filters for VM.run_job(), and pass the filters given to
VM.blockdev_create() to it.

(Use this opportunity to annotate VM.run_job()'s parameter types;
unfortunately, for the filter, I could not come up with anything better
than Callable[[Any], Any] that would pass mypy's scrutiny.)

At one point, a plain string is logged, so the filters passed to it must
work fine with plain strings.  The only filters passed to it at this
point are the ones from VM.blockdev_create(), which are
filter_qmp_test_files() (by default) and 207's filter_hash().  Both
cannot handle plain strings yet, but we can make them by amending
filter_qmp() to treat them as plain values with a None key.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 508adade9e..ad62d1f641 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -521,8 +521,10 @@ def filter_qmp(qmsg, filter_fn):
     # Iterate through either lists or dicts;
     if isinstance(qmsg, list):
         items = enumerate(qmsg)
-    else:
+    elif isinstance(qmsg, dict):
         items = qmsg.items()
+    else:
+        return filter_fn(None, qmsg)
 
     for k, v in items:
         if isinstance(v, (dict, list)):
@@ -858,8 +860,12 @@ def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
         return result
 
     # Returns None on success, and an error string on failure
-    def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-                pre_finalize=None, cancel=False, wait=60.0):
+    def run_job(self, job: str, auto_finalize: bool = True,
+                auto_dismiss: bool = False,
+                pre_finalize: Optional[Callable[[], None]] = None,
+                cancel: bool = False, wait: float = 60.0,
+                filters: Iterable[Callable[[Any], Any]] = (),
+                ) -> Optional[str]:
         """
         run_job moves a job from creation through to dismissal.
 
@@ -889,7 +895,7 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
         while True:
             ev = filter_qmp_event(self.events_wait(events, timeout=wait))
             if ev['event'] != 'JOB_STATUS_CHANGE':
-                log(ev)
+                log(ev, filters=filters)
                 continue
             status = ev['data']['status']
             if status == 'aborting':
@@ -897,18 +903,18 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
                 for j in result['return']:
                     if j['id'] == job:
                         error = j['error']
-                        log('Job failed: %s' % (j['error']))
+                        log('Job failed: %s' % (j['error']), filters=filters)
             elif status == 'ready':
-                self.qmp_log('job-complete', id=job)
+                self.qmp_log('job-complete', id=job, filters=filters)
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
                 if cancel:
-                    self.qmp_log('job-cancel', id=job)
+                    self.qmp_log('job-cancel', id=job, filters=filters)
                 else:
-                    self.qmp_log('job-finalize', id=job)
+                    self.qmp_log('job-finalize', id=job, filters=filters)
             elif status == 'concluded' and not auto_dismiss:
-                self.qmp_log('job-dismiss', id=job)
+                self.qmp_log('job-dismiss', id=job, filters=filters)
             elif status == 'null':
                 return error
 
@@ -921,7 +927,7 @@ def blockdev_create(self, options, job_id='job0', filters=None):
 
         if 'return' in result:
             assert result['return'] == {}
-            job_result = self.run_job(job_id)
+            job_result = self.run_job(job_id, filters=filters)
         else:
             job_result = result['error']
 
-- 
2.35.1

Re: [PATCH 1/2] iotests.py: Filters for VM.run_job()

[John Snow]

On Fri, Mar 18, 2022 at 8:53 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> Allow filters for VM.run_job(), and pass the filters given to
> VM.blockdev_create() to it.
>
> (Use this opportunity to annotate VM.run_job()'s parameter types;
> unfortunately, for the filter, I could not come up with anything better
> than Callable[[Any], Any] that would pass mypy's scrutiny.)
>

Yeah, I wrote some of this stuff ... before I started using mypy, and
I'd do it differently if I had to again.

(And might still do so: pulling out things like a generalized Job
Runner is still on my Someday pile, especially now that I have an
async QMP module to play with.)

Long story short: Yeah, sure, cool, I don't want to do any better than
this right now either.

> At one point, a plain string is logged, so the filters passed to it must
> work fine with plain strings.  The only filters passed to it at this
> point are the ones from VM.blockdev_create(), which are
> filter_qmp_test_files() (by default) and 207's filter_hash().  Both
> cannot handle plain strings yet, but we can make them by amending
> filter_qmp() to treat them as plain values with a None key.
>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Looks fine enough to me for now.

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>  tests/qemu-iotests/iotests.py | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 508adade9e..ad62d1f641 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -521,8 +521,10 @@ def filter_qmp(qmsg, filter_fn):
>      # Iterate through either lists or dicts;
>      if isinstance(qmsg, list):
>          items = enumerate(qmsg)
> -    else:
> +    elif isinstance(qmsg, dict):
>          items = qmsg.items()
> +    else:
> +        return filter_fn(None, qmsg)
>
>      for k, v in items:
>          if isinstance(v, (dict, list)):
> @@ -858,8 +860,12 @@ def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
>          return result
>
>      # Returns None on success, and an error string on failure
> -    def run_job(self, job, auto_finalize=True, auto_dismiss=False,
> -                pre_finalize=None, cancel=False, wait=60.0):
> +    def run_job(self, job: str, auto_finalize: bool = True,
> +                auto_dismiss: bool = False,
> +                pre_finalize: Optional[Callable[[], None]] = None,
> +                cancel: bool = False, wait: float = 60.0,
> +                filters: Iterable[Callable[[Any], Any]] = (),
> +                ) -> Optional[str]:
>          """
>          run_job moves a job from creation through to dismissal.
>
> @@ -889,7 +895,7 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
>          while True:
>              ev = filter_qmp_event(self.events_wait(events, timeout=wait))
>              if ev['event'] != 'JOB_STATUS_CHANGE':
> -                log(ev)
> +                log(ev, filters=filters)
>                  continue
>              status = ev['data']['status']
>              if status == 'aborting':
> @@ -897,18 +903,18 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
>                  for j in result['return']:
>                      if j['id'] == job:
>                          error = j['error']
> -                        log('Job failed: %s' % (j['error']))
> +                        log('Job failed: %s' % (j['error']), filters=filters)
>              elif status == 'ready':
> -                self.qmp_log('job-complete', id=job)
> +                self.qmp_log('job-complete', id=job, filters=filters)
>              elif status == 'pending' and not auto_finalize:
>                  if pre_finalize:
>                      pre_finalize()
>                  if cancel:
> -                    self.qmp_log('job-cancel', id=job)
> +                    self.qmp_log('job-cancel', id=job, filters=filters)
>                  else:
> -                    self.qmp_log('job-finalize', id=job)
> +                    self.qmp_log('job-finalize', id=job, filters=filters)
>              elif status == 'concluded' and not auto_dismiss:
> -                self.qmp_log('job-dismiss', id=job)
> +                self.qmp_log('job-dismiss', id=job, filters=filters)
>              elif status == 'null':
>                  return error
>
> @@ -921,7 +927,7 @@ def blockdev_create(self, options, job_id='job0', filters=None):
>
>          if 'return' in result:
>              assert result['return'] == {}
> -            job_result = self.run_job(job_id)
> +            job_result = self.run_job(job_id, filters=filters)
>          else:
>              job_result = result['error']
>
> --
> 2.35.1
>

[PATCH 2/2] iotests/207: Filter host fingerprint

[Hanna Reitz]

Commit e3296cc796aeaf319f3ed4e064ec309baf5e4da4 made the ssh block
driver's error message for fingerprint mismatches more verbose, so it
now prints the actual host key fingerprint and the key type.

iotest 207 tests such errors, but was not amended to filter that
fingerprint (which is host-specific), so do it now.  Filter the key
type, too, because I guess this too can differ depending on the host
configuration.

Fixes: e3296cc796aeaf319f3ed4e064ec309baf5e4da4
       ("block: print the server key type and fingerprint on failure")
Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/207     | 7 ++++++-
 tests/qemu-iotests/207.out | 6 +++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 0f5c4bc8a0..41dcf3ff55 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -35,7 +35,12 @@ def filter_hash(qmsg):
         if key == 'hash' and re.match('[0-9a-f]+', value):
             return 'HASH'
         return value
-    return iotests.filter_qmp(qmsg, _filter)
+    if isinstance(qmsg, str):
+        # Strip key type and fingerprint
+        p = r"\S+ (key fingerprint) '(md5|sha1|sha256):[0-9a-f]+'"
+        return re.sub(p, r"\1 '\2:HASH'", qmsg)
+    else:
+        return iotests.filter_qmp(qmsg, _filter)
 
 def blockdev_create(vm, options):
     vm.blockdev_create(options, filters=[iotests.filter_qmp_testfiles, filter_hash])
diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
index aeb8569d77..05cf753283 100644
--- a/tests/qemu-iotests/207.out
+++ b/tests/qemu-iotests/207.out
@@ -42,7 +42,7 @@ virtual size: 4 MiB (4194304 bytes)
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "md5"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}}
 {"return": {}}
-Job failed: remote host key does not match host_key_check 'wrong'
+Job failed: remote host key fingerprint 'md5:HASH' does not match host_key_check 'md5:wrong'
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
 
@@ -59,7 +59,7 @@ virtual size: 8 MiB (8388608 bytes)
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "sha1"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}}
 {"return": {}}
-Job failed: remote host key does not match host_key_check 'wrong'
+Job failed: remote host key fingerprint 'sha1:HASH' does not match host_key_check 'sha1:wrong'
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
 
@@ -76,7 +76,7 @@ virtual size: 4 MiB (4194304 bytes)
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "sha256"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}}
 {"return": {}}
-Job failed: remote host key does not match host_key_check 'wrong'
+Job failed: remote host key fingerprint 'sha256:HASH' does not match host_key_check 'sha256:wrong'
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
 
-- 
2.35.1

Re: [PATCH 2/2] iotests/207: Filter host fingerprint

[John Snow]

On Fri, Mar 18, 2022 at 8:53 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> Commit e3296cc796aeaf319f3ed4e064ec309baf5e4da4 made the ssh block
> driver's error message for fingerprint mismatches more verbose, so it
> now prints the actual host key fingerprint and the key type.
>
> iotest 207 tests such errors, but was not amended to filter that
> fingerprint (which is host-specific), so do it now.  Filter the key
> type, too, because I guess this too can differ depending on the host
> configuration.
>

Oh, neat.

(Not that neat.)

> Fixes: e3296cc796aeaf319f3ed4e064ec309baf5e4da4
>        ("block: print the server key type and fingerprint on failure")
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  tests/qemu-iotests/207     | 7 ++++++-
>  tests/qemu-iotests/207.out | 6 +++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
> index 0f5c4bc8a0..41dcf3ff55 100755
> --- a/tests/qemu-iotests/207
> +++ b/tests/qemu-iotests/207
> @@ -35,7 +35,12 @@ def filter_hash(qmsg):
>          if key == 'hash' and re.match('[0-9a-f]+', value):
>              return 'HASH'
>          return value
> -    return iotests.filter_qmp(qmsg, _filter)
> +    if isinstance(qmsg, str):
> +        # Strip key type and fingerprint
> +        p = r"\S+ (key fingerprint) '(md5|sha1|sha256):[0-9a-f]+'"
> +        return re.sub(p, r"\1 '\2:HASH'", qmsg)
> +    else:
> +        return iotests.filter_qmp(qmsg, _filter)
>
>  def blockdev_create(vm, options):
>      vm.blockdev_create(options, filters=[iotests.filter_qmp_testfiles, filter_hash])
> diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
> index aeb8569d77..05cf753283 100644
> --- a/tests/qemu-iotests/207.out
> +++ b/tests/qemu-iotests/207.out
> @@ -42,7 +42,7 @@ virtual size: 4 MiB (4194304 bytes)
>
>  {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "md5"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}}
>  {"return": {}}
> -Job failed: remote host key does not match host_key_check 'wrong'
> +Job failed: remote host key fingerprint 'md5:HASH' does not match host_key_check 'md5:wrong'
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
>
> @@ -59,7 +59,7 @@ virtual size: 8 MiB (8388608 bytes)
>
>  {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "sha1"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}}
>  {"return": {}}
> -Job failed: remote host key does not match host_key_check 'wrong'
> +Job failed: remote host key fingerprint 'sha1:HASH' does not match host_key_check 'sha1:wrong'
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
>
> @@ -76,7 +76,7 @@ virtual size: 4 MiB (4194304 bytes)
>
>  {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "sha256"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}}
>  {"return": {}}
> -Job failed: remote host key does not match host_key_check 'wrong'
> +Job failed: remote host key fingerprint 'sha256:HASH' does not match host_key_check 'sha256:wrong'
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
>
> --
> 2.35.1
>

sankyuu~

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 0/2] iotests/207: Filter host fingerprint

[Hanna Reitz]

On 18.03.22 13:53, Hanna Reitz wrote:
> Hi,
>
> Commit e3296cc796aeaf (“block: print the server key type and fingerprint
> on failure”) improved the verbosity of our ssh block driver's error
> messages for fingerprint mismatches.  However, iotest 207, which tests
> such errors, has not been adjusted accordingly.
>
> Since the fingerprint will differ between hosts, we need to filter it
> (and can’t just statically adjust the reference output).  The problem is
> that the error condition is printed by iotest.py’s VM.run_job(), so we
> need to pass the filter to that function.  Right now, VM.run_job()
> doesn’t support any filters, though, so patch 1 adds a filter parameter
> and makes VM.run_job() use it.
>
> Patch 2 then has the fix for iotest 207.

Thanks for the review, applied to my block branch.

Hanna