[pve-devel] [PATCH cluster 1/3] add ssh command helpers

[Wolfgang Bumiller]

On Fri, Jan 20, 2017 at 04:13:11PM +0100, Thomas Lamprecht wrote:
> Add two helpers regarding ssh commands:
> * get_ssh_base_cmd: returns a ssh command with various
>   standard options set, useful if only the base ssh command is
>   needed
> * get_ssh_cmd: returns an string array for executing a command in a
>   safe way on another pve node
> 
> Both function ensure that we use root as login name, use the
> HostKeyAlias ssh option to ensure that host key verification uses
> the provided hostname and not the IP address when checking if the
> target node is a known_host, this avoids problems if we connect to a
> node over another network (useful for dedicated migration network,
> or if the user changes the cluster network).
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
>  data/PVE/Cluster.pm | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index 0edcc0a..35cc4d8 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -1078,6 +1078,52 @@ sub get_local_migration_ip {
>  
>  # ssh related utility functions
>  
> +# get_ssh_base_cmd is a helper for getting a ssh command with some common
> +# options set. E.g.: It uses the HostKeyAlias option to identify a peer over
> +# its node name instead of its IP. This allows connecting to cluster member
> +# nodes over different networks without altering the known_host file.
> +sub get_ssh_base_cmd {
> +    my ($node, $host_addr) = @_;
> +
> +    my $ssh_cmd = ['/usr/bin/ssh', '-l', 'root', '-o', 'BatchMode=yes'];
> +
> +    if (defined($node)) {
> +	my $host_alias = 'HostKeyAlias=' . PVE::Tools::shell_quote($node);
> +	push @$ssh_cmd, '-o', $host_alias;

I don't think this is the correct way to quote ssh options. shell_quote
seems to only ever use single-quotes while ssh_config(5) says:

    Configuration options may be separated by whitespace or optional
    whitespace and exactly one ‘=’; the latter format is useful to
    avoid the need to quote whitespace when specifying configuration
    options using the ssh, scp, and sftp -o option.

This tells me that when it sees a '=' the rest of the arugment is
considered a single string without the need for quoting.

It further states that:

    Arguments may optionally be enclosed in double quotes (") in order
    to represent arguments containing spaces.

Since shell_quote() uses single-quotes this would not work. Also it
means that if we don't quote and the string starts with double quotes
ssh probably interprets it in a wrong way. (So we should be careful how
we use this. In this case it might break host key validation?)

> +    }
> +
> +    if (defined($host_addr)) {
> +	my $host = PVE::Tools::shell_quote($host_addr);

Why are we quoting when building an array?

> +	push @$ssh_cmd, $host;
> +    }
> +
> +    return $ssh_cmd;
> +}
> +
> +# prepares a command to run on a cluster member over ssh in a safe way
> +sub get_ssh_cmd {
> +    my ($node, $cmd, $host_addr) = @_;
> +
> +    die "node parameter not defined in get_ssh_cmd\n" if (!defined($node));
> +
> +    if (!defined($host_addr)) {
> +	$host_addr = remote_node_ip($node);
> +    }
> +
> +    my $ssh_cmd = get_ssh_base_cmd($node, $host_addr);
> +
> +    if (!defined($cmd)) {
> +	return $ssh_cmd;
> +    } elsif (ref($cmd) && ref($cmd->[0])) {
> +	die "array of arrays unsuported in ssh_command!\n";
> +    }
> +
> +    my $cmdstr = PVE::Tools::cmd2string($cmd);
> +    push @$ssh_cmd, '--', $cmdstr;
> +
> +    return $ssh_cmd;
> +}
> +
>  sub ssh_merge_keys {
>      # remove duplicate keys in $sshauthkeys
>      # ssh-copy-id simply add keys, so the file can grow to large
> -- 
> 2.1.4