Bug 2027673

I knew about this one but haven't fixed it yet.  It's related to this change in openssh
which changed how quoting happens on the command line:

https://github.com/openssh/openssh-portable/commit/197e29f1cca190d767c4b2b63a662f9a9e5da0b3

That change only happened upstream in OpenSSH 8.8, but in Fedora and RHEL 9
we're carrying a downstream-only patch which enables it:

https://gitlab.com/redhat/centos-stream/rpms/openssh/-/blob/c9s/openssh-8.7p1-sftp-default-protocol.patch

The fix involves changing this code so it doesn't do the double quoting:

https://github.com/libguestfs/virt-v2v/blob/e626913907ab89e325174b4588e585730f277437/input/parse_domain_from_vmx.ml#L86

(There may be other places where the change needs to be made)

Note this change would be incompatible with OpenSSH upstream versions < 8.8.

Upstream fix:
https://github.com/libguestfs/virt-v2v/commit/e2af12ba69c4463bb73d30db63290a887cdd41eb

I think there are three cases to support:

- if option "M" is known: use "-M sftp", no double quoting
- else, if option "T" is known: use "-T", with double quoting (bug 1733168)
- otherwise, just use double quoting

I went with "just support openssh 8.8".  Precise reasons given in the
commit message.  Actually it still works with < 8.8 in the case where the
path doesn't need quoting, so it's kind of fine.

(In reply to Richard W.M. Jones from comment #2)
> Upstream fix:
> https://github.com/libguestfs/virt-v2v/commit/e2af12ba69c4463bb73d30db63290a887cdd41eb

I understand the argument about "latest OpenSSH only". However, my interpretation is different:

(1) I think that we should always pass "-M sftp", because, from <https://github.com/openssh/openssh-portable/commit/197e29f1cca190d767c4b2b63a662f9a9e5da0b3>:

> .It Fl M Ar scp | sftp
> Specifies a mode which will be used to transfer files.
> The default is to use the original
> .Cm scp
> protocol.
> Alternately, experimental support for using the
> .Cm sftp
> protocol is available.

(2) Given "-M sftp", the "-T" option should be a no-op (sftp is not vulnerable to filename tricks), so "-T" should be removed together with the double quoting.

About -T, it was added for https://bugzilla.redhat.com/show_bug.cgi?id=1733168
However reading the comments on that bug I agree that we can now drop it.

I didn't spot that upstream, openssh is still defaulting to scp.  Sounds like
we do need -M sftp.  However about this?

diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml
index 05bab40b48..e8bf9d4a74 100644
--- a/input/parse_domain_from_vmx.ml
+++ b/input/parse_domain_from_vmx.ml
@@ -68,7 +68,7 @@ let scp_from_remote_to_temporary uri tmpdir filename =
   let localfile = tmpdir // filename in
 
   let cmd =
-    sprintf "scp -T%s%s %s%s:%s %s"
+    sprintf "scp -M sftp%s%s %s%s:%s %s"
             (if verbose () then "" else " -q")
             (match port_of_uri uri with
              | None -> ""

Just check that works ...

s/However/How/

(In reply to Richard W.M. Jones from comment #1)
> I knew about this one but haven't fixed it yet.  It's related to this change
> in openssh
> which changed how quoting happens on the command line:
> 
> https://github.com/openssh/openssh-portable/commit/197e29f1cca190d767c4b2b63a662f9a9e5da0b3
> 
> That change only happened upstream in OpenSSH 8.8, but in Fedora and RHEL 9
> we're carrying a downstream-only patch which enables it:
> 
> https://gitlab.com/redhat/centos-stream/rpms/openssh/-/blob/c9s/openssh-8.7p1-sftp-default-protocol.patch

The downstream variant of the upstream commit is perplexing:

- it changes the default mode from scp to sftp --> therefore, diverges from upstream, meaning that virt-v2v will work differently against upstream scp vs. downstream scp, unless we force sftp mode explicitly

- it removes an option called "-s", which I guess used to be the downstream-only guesswork of what would turn out as "-M sftp" upstream.

- it still does not recognize "-M sftp".

(In reply to Richard W.M. Jones from comment #6)
> About -T, it was added for
> https://bugzilla.redhat.com/show_bug.cgi?id=1733168
> However reading the comments on that bug I agree that we can now drop it.
> 
> I didn't spot that upstream, openssh is still defaulting to scp.  Sounds like
> we do need -M sftp.  However about this?
> 
> diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml
> index 05bab40b48..e8bf9d4a74 100644
> --- a/input/parse_domain_from_vmx.ml
> +++ b/input/parse_domain_from_vmx.ml
> @@ -68,7 +68,7 @@ let scp_from_remote_to_temporary uri tmpdir filename =
>    let localfile = tmpdir // filename in
>  
>    let cmd =
> -    sprintf "scp -T%s%s %s%s:%s %s"
> +    sprintf "scp -M sftp%s%s %s%s:%s %s"
>              (if verbose () then "" else " -q")
>              (match port_of_uri uri with
>               | None -> ""
> 
> Just check that works ...

This looks good to me, *assuming* downstream scp actually understand "-M sftp". From <https://gitlab.com/redhat/centos-stream/rpms/openssh/-/blob/c9s/openssh-8.7p1-sftp-default-protocol.patch>, that doesn't seem to be the case. (Well I guess once RHEL / c9s adopts OpenSSH 8.8, virt-v2v will "start" working, but until then, we'll need a downstream-only change in virt-v2v too.)

My patch above doesn't work because the -M option doesn't exist any more.  It's now
-O (old) or -s (sftp), and the documentation says that -s will be removed "soon" and
should not be used in scripts!

I think what we have might be OK.

Do we expect that "scp" default to the sftp method on all distros, starting with OpenSSH 8.8?

(If that's the case, -T still seems superfluous...)

Debian isn't carrying any downstream patches related to this, but at the same
time is only on 8.7 (8.8 was released 2 months ago):
https://sources.debian.org/patches/openssh/1:8.7p1-2/

My guess therefore is that Debian will eventually move to 8.8 which defaults
to the old protocol and has the -O / -s flags.

Ubuntu is likely to follow Debian.

Arch has 8.8 and seems vanilla, so same as Debian.
https://github.com/archlinux/svntogit-packages/tree/packages/openssh/trunk

So probably this change will not work on Debian/Ubuntu/Arch *if* the filename
has spaces / quotable characters, but also there's no change I can see that
will make it compatible with those and RHEL/Fedora.

And yes I agree that -T is superfluous, but only when all distros move to sftp.
We can leave it there for the moment.

OK, thanks!

Test the bug with below builds:
virt-v2v-1.45.93-1.el9.x86_64
libguestfs-1.46.0-5.el9.x86_64
guestfs-tools-1.46.1-5.el9.x86_64
nbdkit-1.28.2-2.el9.x86_64
libvirt-libs-7.9.0-1.el9.x86_64
qemu-img-6.1.0-7.el9.x86_64
openssh-8.7p1-5.el9.x86_64

Steps:
1.Update virt-v2v to latest version, openssh version will be updated with virt-v2v
# yum update virt-v2v -y
.....
Last metadata expiration check: 1:00:32 ago on Mon 06 Dec 2021 01:58:39 PM CST.
virt-v2v-1.45.93-1.el9.x86_64.rpm                                                        380 kB/s | 1.2 MB     00:03    
Dependencies resolved.
=========================================================================================================================
 Package                        Architecture          Version                          Repository                   Size
=========================================================================================================================
Upgrading:
 openssh                        x86_64                8.7p1-5.el9                      rhel9-base                  452 k
 openssh-clients                x86_64                8.7p1-5.el9                      rhel9-base                  700 k
 openssh-server                 x86_64                8.7p1-5.el9                      rhel9-base                  459 k
 virt-v2v                       x86_64                1:1.45.93-1.el9                  @commandline                1.2 M

Transaction Summary
=========================================================================================================================
Upgrade  4 Packages
.......

# rpm -Rq virt-v2v-1.45.93-1.el9.x86_64 |grep openssh
openssh-clients >= 8.7p1

2.Convert guest from VMware via vmx+ssh by v2v
# virt-v2v -i vmx -it ssh ssh://root.75.219/vmfs/volumes/esx6.7-matrix/esx6.7-rhel8.4-x86_64/esx6.7-rhel8.4-x86_64.vmx -o rhv-upload -of qcow2 -oc https://dell-per740-22.lab.eng.pek2.redhat.com/ovirt-engine/api -op /home/rhvpasswd  -os nfs_data  -b ovirtmgmt  -ip /home/esxpw
[  24.4] Opening the source
[  35.8] Inspecting the source
[  46.5] Checking for sufficient free disk space in the guest
[  46.5] Converting Red Hat Enterprise Linux 8.4 (Ootpa) to run on KVM
virt-v2v: This guest has virtio drivers installed.
[ 130.1] Mapping filesystem data to avoid copying unused and blank areas
[ 131.6] Closing the overlay
[ 131.9] Assigning disks to buses
[ 131.9] Checking if the guest needs BIOS or UEFI to boot
[ 131.9] Copying disk 1/1
█ 100% [****************************************]
[ 378.6] Creating output metadata
[ 441.1] Finishing off

3. Check the guest after v2v conversion, the checkpoints of guest are passed

Verify the bug with below builds:
virt-v2v-1.45.97-1.el9.x86_64
libguestfs-1.46.1-2.el9.x86_64
guestfs-tools-1.46.1-6.el9.x86_64
libvirt-libs-8.0.0-1.el9.x86_64
qemu-img-6.2.0-4.el9.x86_64
nbdkit-1.28.4-2.el9.x86_64
libnbd-1.10.3-1.el9.x86_64
openssh-8.7p1-6.el9.x86_64

Steps:
1.Check the dependence status of virt-v2v about openssh
# rpm -Rq virt-v2v-1.45.97-1.el9.x86_64 |grep openssh
openssh-clients >= 8.7p1

2.Convert guest from VMware via vmx+ssh by v2v
# virt-v2v -i vmx -it ssh ssh://root.75.219/vmfs/volumes/esx6.7-matrix/esx6.7-rhel8.5-x86_64/esx6.7-rhel8.5-x86_64.vmx -o rhv-upload -of qcow2 -oc https://dell-per740-22.lab.eng.pek2.redhat.com/ovirt-engine/api -op /home/rhvpasswd  -os nfs_data  -b ovirtmgmt  -ip /home/esxpw
[   1.7] Opening the source
[  10.1] Inspecting the source
[  24.4] Checking for sufficient free disk space in the guest
[  24.4] Converting Red Hat Enterprise Linux 8.5 (Ootpa) to run on KVM
virt-v2v: This guest has virtio drivers installed.
[ 162.1] Mapping filesystem data to avoid copying unused and blank areas
[ 164.6] Closing the overlay
[ 164.9] Assigning disks to buses
[ 164.9] Checking if the guest needs BIOS or UEFI to boot
[ 177.8] Copying disk 1/1
█ 100% [****************************************]
[ 890.3] Creating output metadata
[ 953.4] Finishing off

3. Check the guest after v2v conversion, the checkpoints of guest are passed except bug2042479

Result:
   The bug has been fixed, move bug from ON_QA to VERIFIED

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (new packages: virt-v2v), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2022:2566