[pve-devel] [PATCH cluster 2/4] fix #4886: SSH: pin node's host key if available

[Esi Y]

On Mon, Jan 15, 2024 at 12:51:48PM +0100, Fabian Grünbichler wrote:
> > On Thu, Jan 11, 2024 at 11:51:16AM +0100, Fabian Grünbichler wrote:
> > > if the target node has already stored their SSH host key on pmxcfs, pin it and
> > > ignore the global known hosts information.
> > > 
> > > Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> > > ---
> > >  src/PVE/SSHInfo.pm | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/PVE/SSHInfo.pm b/src/PVE/SSHInfo.pm
> > > index c351148..fad23bf 100644
> > > --- a/src/PVE/SSHInfo.pm
> > > +++ b/src/PVE/SSHInfo.pm
> > > @@ -49,11 +49,24 @@ sub get_ssh_info {
> > >  
> > >  sub ssh_info_to_command_base {
> > >      my ($info, @extra_options) = @_;
> > > +
> > > +    my $nodename = $info->{name};
> > > +
> > > +    my $known_hosts_file = "/etc/pve/nodes/$nodename/ssh_known_hosts";
> > > +    my $known_hosts_options = undef;
> > > +    if (-f $known_hosts_file) {
> > > +	$known_hosts_options = [
> > > +	    '-o', "UserKnownHostsFile=$known_hosts_file",
> > > +	    '-o', 'GlobalKnownHostsFile=none',
> > 
> > why does Global need to be none, even as this only applies if the snippet exists?
> 
> because we want to only let SSH look at our pinned file, not the regular one, which might contain bogus information. since our pinned file contains an entry for our host key alias which must match, the global file can never improve the situation, but it can cause a verification failure.

This might not work as expected.

1. There will not be any verification failure if there is at least some valid key present. If wrong keys are present alongside a good one, it's a pass. If _only_ wrong keys are present, with StrictHostKeyChecking default (ask) it will outright stop.

2. The Global none does not improve anything there. If no keys are present it will try to ask (under SKHC default), but no use in BatchMode.

3. Using -o UserKHF alongside default SKHC, e.g. if run by someone even manually after a failed script without BatchMode, will have it crash for them because the pinned file cannot be updated by ssh properly due to the same issue as mentioned before regarding ssh-keygen -R. In this case the pmxcfs will cause it to crash again on link-unlink-rename() again [1].

[1] https://github.com/openssh/openssh-portable/blob/50080fa42f5f744b798ee29400c0710f1b59f50e/hostfile.c#L695

4. I suppose you did not like my suggestion re KnownHostsCommand [2] instead of "pinning", but giving -o's to ssh code where the files reside on pmxcfs is just creating the same problem (that e.g. keygen -R had) elsewhere depending if you plan e.g. multiline.

[2] https://lists.proxmox.com/pipermail/pve-devel/2024-January/061329.html

> > > +	];
> > > +    } 
> > > +
> > >      return [
> > >  	'/usr/bin/ssh',
> > >  	'-e', 'none',
> > >  	'-o', 'BatchMode=yes',
> > > -	'-o', 'HostKeyAlias='.$info->{name},
> > > +	'-o', 'HostKeyAlias='.$nodename,
> > > +	defined($known_hosts_options) ? @$known_hosts_options : (),
> > >  	@extra_options
> > >      ];
> > >  }
> > > -- 
> > > 2.39.2
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > pve-devel mailing list
> > > pve-devel at lists.proxmox.com
> > > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>