Skip to content

Commit

Permalink
input: -i vmx: Remove support for openssh scp < 8.8
Browse files Browse the repository at this point in the history
Older openssh scp (< 8.8) had a strange double-quoting syntax for
paths because it passed the literal parameter to the remote side where
it was interpreted again by the remote shell.  In openssh 8.8 scp was
changed to use the sftp protocol instead.

Since we now require openssh >= 8.8, the -T option will always be
present so we don't need to test for it.

There are a few reasons why this change does not attempt to support
the older scp:

 * Distros like to keep up with OpenSSH for security reasons.

 * The older double quoting format was crazy, and probably insecure.
   Replacing it with a sane alternative is something we should
   encourage.

 * Import from -i vmx is fairly niche, and there are workable
   alternatives (VDDK or HTTPS) in case anyone is affected by this.

 * In fact it _does_ support the older version, at least in the case
   where the filename doesn't contain spaces or quoted characters.

Note that we require sftp in nbdkit-ssh-plugin, so we don't need to
worry about the remote server not supporting it because conversion
would fail anyway.

Reported-by: Ming Xie
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2027673
  • Loading branch information
rwmjones committed Nov 30, 2021
1 parent e626913 commit e2af12b
Showing 1 changed file with 3 additions and 14 deletions.
17 changes: 3 additions & 14 deletions input/parse_domain_from_vmx.ml
Original file line number Diff line number Diff line change
Expand Up @@ -61,33 +61,23 @@ let server_of_uri { Xml.uri_server } =
let path_of_uri { Xml.uri_path } =
match uri_path with None -> assert false | Some p -> p

let scp_supports_T_option = lazy (
let cmd = "LANG=C scp -T |& grep \"unknown option\"" in
shell_command cmd <> 0
)

(* 'scp' a remote file into a temporary local file, returning the path
* of the temporary local file.
*)
let scp_from_remote_to_temporary uri tmpdir filename =
let localfile = tmpdir // filename in

let cmd =
sprintf "scp%s%s%s %s%s:%s %s"
sprintf "scp -T%s%s %s%s:%s %s"
(if verbose () then "" else " -q")
(if Lazy.force scp_supports_T_option then " -T" else "")
(match port_of_uri uri with
| None -> ""
| Some port -> sprintf " -P %d" port)
(match uri.Xml.uri_user with
| None -> ""
| Some user -> quote user ^ "@")
(quote (server_of_uri uri))
(* The double quoting of the path is counter-intuitive
* but correct, see:
* https://stackoverflow.com/questions/19858176/how-to-escape-spaces-in-path-during-scp-copy-in-linux
*)
(quote (quote (path_of_uri uri)))
(quote (path_of_uri uri))
(quote localfile) in
if verbose () then
eprintf "%s\n%!" cmd;
Expand All @@ -106,8 +96,7 @@ let remote_file_exists uri path =
| None -> ""
| Some user -> quote user ^ "@")
(quote (server_of_uri uri))
(* Double quoting is necessary here, see above. *)
(quote (quote path)) in
(quote path) in
if verbose () then
eprintf "%s\n%!" cmd;
Sys.command cmd = 0
Expand Down

0 comments on commit e2af12b

Please sign in to comment.