[pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Fabian Grünbichler]

this series replaces the old mechanism that used a cluster-wide merged known
hosts file with distributing of each node's host key via pmxcfs, and pinning
the distributed key explicitly for internal SSH connections.

the main changes in pve-cluster somewhat break the old manager and
storage versions, but only when such a partial upgrade is mixed with a
host key rotation of some sort.

pve-storage uses a newly introduced helper, so needs a versioned
dependency accordingly.

the last pve-docs patch has a placeholder for the actual version shipping the
changes which needs to be replaced when applying.

there's still some potential for follow-ups:
- 'pvecm ssh' wrapper to debug and/or re-use the host key pinning (and other
  future changes)
- also add non-RSA host keys
- key (and thus authorized keys) and/or sshd disentangling (this
  potentially also affects external access, so might be done on a major
  release to give more heads up)

cluster:

Fabian Grünbichler (4):
  fix #4886: write node SSH hostkey to pmxcfs
  fix #4886: SSH: pin node's host key if available
  ssh: expose SSH options on their own
  pvecm: stop merging SSH known hosts by default

 src/PVE/CLI/pvecm.pm     | 10 ++++++++--
 src/PVE/Cluster/Setup.pm | 24 +++++++++++++++++++++---
 src/PVE/SSHInfo.pm       | 31 +++++++++++++++++++++++++++----
 3 files changed, 56 insertions(+), 9 deletions(-)

docs:

Fabian Grünbichler (2):
  ssh: make pitfalls a regular section instead of block
  ssh: document PVE-specific setup

 pvecm.adoc | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

manager:

Fabian Grünbichler (2):
  vnc: use SSH command helper
  pvesh: use SSH command helper

 PVE/API2/Nodes.pm | 3 ++-
 PVE/CLI/pvesh.pm  | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

storage:

Fabian Grünbichler (1):
  upload: use SSH helper to get ssh/scp options

 src/PVE/API2/Storage/Status.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH cluster 1/4] fix #4886: write node SSH hostkey to pmxcfs

[Fabian Grünbichler]

so that we can explicitly pin just this key when doing intra-cluster SSH
connections. this works similar to the certificate cache we use for API
proxying, but without automatic invalidation, since node A doesn't have access
to node B's host key..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    we could store more than just the RSA one there, but that would have some
    potential for fallout.. the filename could also be changed to reflect what
    it contains, not what is used for - e.g., "ssh_host_keys"

 src/PVE/Cluster/Setup.pm | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
index 07020d7..4b6f013 100644
--- a/src/PVE/Cluster/Setup.pm
+++ b/src/PVE/Cluster/Setup.pm
@@ -220,6 +220,20 @@ sub ssh_unmerge_known_hosts {
     PVE::Tools::file_set_contents($ssh_system_known_hosts, $old);
 }
 
+sub ssh_create_node_known_hosts {
+    my ($nodename) = @_;
+
+    my $hostkey = PVE::Tools::file_get_contents($ssh_host_rsa_id);
+    # Note: file sometimes containe empty lines at start, so we use multiline match
+    die "can't parse $ssh_host_rsa_id" if $hostkey !~ m/^(ssh-rsa\s\S+)(\s.*)?$/m;
+    $hostkey = $1;
+
+    my $raw = "$nodename $hostkey";
+    PVE::Tools::file_set_contents("/etc/pve/nodes/$nodename/ssh_known_hosts", $raw);
+
+    # TODO: also setup custom keypair and client config here to disentangle entirely from /root/.ssh?
+}
+
 sub ssh_merge_known_hosts {
     my ($nodename, $ip_address, $createLink) = @_;
 
@@ -823,6 +837,7 @@ sub updatecerts_and_ssh {
     $p->("merge authorized SSH keys and known hosts");
     ssh_merge_keys();
     ssh_merge_known_hosts($nodename, $local_ip_address, 1);
+    ssh_create_node_known_hosts($nodename);
     gen_pve_vzdump_files();
 }
 
-- 
2.39.2

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

[Fabian Grünbichler]

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@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',
+	];
+    } 
+
     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] [PATCH cluster 3/4] ssh: expose SSH options on their own

[Fabian Grünbichler]

for example, to re-use with an scp command.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
this is used by pve-storage, versioned dependency needed accordingly.

 src/PVE/SSHInfo.pm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/PVE/SSHInfo.pm b/src/PVE/SSHInfo.pm
index fad23bf..a26ae31 100644
--- a/src/PVE/SSHInfo.pm
+++ b/src/PVE/SSHInfo.pm
@@ -47,7 +47,7 @@ sub get_ssh_info {
     };
 }
 
-sub ssh_info_to_command_base {
+sub ssh_info_to_ssh_opts {
     my ($info, @extra_options) = @_;
 
     my $nodename = $info->{name};
@@ -62,8 +62,6 @@ sub ssh_info_to_command_base {
     } 
 
     return [
-	'/usr/bin/ssh',
-	'-e', 'none',
 	'-o', 'BatchMode=yes',
 	'-o', 'HostKeyAlias='.$nodename,
 	defined($known_hosts_options) ? @$known_hosts_options : (),
@@ -71,6 +69,18 @@ sub ssh_info_to_command_base {
     ];
 }
 
+sub ssh_info_to_command_base {
+    my ($info, @extra_options) = @_;
+
+    my $opts = ssh_info_to_ssh_opts($info, @extra_options);
+
+    return [
+	'/usr/bin/ssh',
+	'-e', 'none', # only works for ssh, not scp!
+	$opts->@*,
+    ];
+}
+
 sub ssh_info_to_command {
     my ($info, @extra_options) = @_;
     my $cmd = ssh_info_to_command_base($info, @extra_options);
-- 
2.39.2

[pve-devel] [PATCH cluster 4/4] pvecm: stop merging SSH known hosts by default

[Fabian Grünbichler]

and allow explicitly unmerging to remove the symlink altogether.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/CLI/pvecm.pm     | 10 ++++++++--
 src/PVE/Cluster/Setup.pm |  9 ++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/PVE/CLI/pvecm.pm b/src/PVE/CLI/pvecm.pm
index 0005e4b..0e8ca8f 100755
--- a/src/PVE/CLI/pvecm.pm
+++ b/src/PVE/CLI/pvecm.pm
@@ -567,12 +567,18 @@ __PACKAGE__->register_method ({
 		type => 'boolean',
 		optional => 1,
 	    },
+	    'unmerge-known-hosts' => {
+		description => "Unmerge legacy SSH known hosts.",
+		type => 'boolean',
+		optional => 1,
+		default => 0,
+	    },
 	},
     },
     returns => { type => 'null' },
     code => sub {
 	my ($param) = @_;
-	my ($force_new_cert, $silent) = $param->@{qw(force silent)};
+	my ($force_new_cert, $silent, $unmerge) = $param->@{qw(force silent unmerge-known-hosts)};
 
 	# pveproxy's ExecStartPre calls this, and as we do IO (on /etc/pve) that can hang
 	# (uninterruptible D state) we could fail the whole service, rendering the API guaranteed
@@ -585,7 +591,7 @@ __PACKAGE__->register_method ({
 		usleep(100 * 1000);
 	    }
 
-	    PVE::Cluster::Setup::updatecerts_and_ssh($force_new_cert, $silent);
+	    PVE::Cluster::Setup::updatecerts_and_ssh($force_new_cert, $silent, $unmerge);
 	    PVE::Cluster::prepare_observed_file_basedirs();
 	});
 	if ($got_timeout) {
diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
index 4b6f013..42dff85 100644
--- a/src/PVE/Cluster/Setup.pm
+++ b/src/PVE/Cluster/Setup.pm
@@ -816,7 +816,7 @@ sub generate_local_files {
 }
 
 sub updatecerts_and_ssh {
-    my ($force_new_cert, $silent) = @_;
+    my ($force_new_cert, $silent, $unmerge_ssh) = @_;
 
     my $p = sub { print "$_[0]\n" if !$silent };
 
@@ -834,9 +834,12 @@ sub updatecerts_and_ssh {
     $p->("generate new node certificate") if $force_new_cert;
     gen_pve_node_files($nodename, $local_ip_address, $force_new_cert);
 
-    $p->("merge authorized SSH keys and known hosts");
+    $p->("merge authorized SSH keys");
     ssh_merge_keys();
-    ssh_merge_known_hosts($nodename, $local_ip_address, 1);
+    if ($unmerge_ssh) {
+	$p->("unmerge SSH known hosts");
+	ssh_unmerge_known_hosts();
+    }
     ssh_create_node_known_hosts($nodename);
     gen_pve_vzdump_files();
 }
-- 
2.39.2

[pve-devel] [PATCH docs 1/2] ssh: make pitfalls a regular section instead of block

[Fabian Grünbichler]

because we'll add another one before it, and formatting is off otherwise.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 pvecm.adoc | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/pvecm.adoc b/pvecm.adoc
index 1f78585..5b5b27b 100644
--- a/pvecm.adoc
+++ b/pvecm.adoc
@@ -918,9 +918,9 @@ transfer memory and disk contents.
 
 * Storage replication
 
-.Pitfalls due to automatic execution of `.bashrc` and siblings
-[IMPORTANT]
-====
+Pitfalls due to automatic execution of `.bashrc` and siblings
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 In case you have a custom `.bashrc`, or similar files that get executed on
 login by the configured shell, `ssh` will automatically run it once the session
 is established successfully. This can cause some unexpected behavior, as those
@@ -940,8 +940,6 @@ case $- in
       *) return;;
 esac
 ----
-====
-
 
 Corosync External Vote Support
 ------------------------------
-- 
2.39.2

[pve-devel] [PATCH docs 2/2] ssh: document PVE-specific setup

[Fabian Grünbichler]

such as adapted configs and managed files.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes: actual version needs to be inserted!

 pvecm.adoc | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/pvecm.adoc b/pvecm.adoc
index 5b5b27b..3a32cfb 100644
--- a/pvecm.adoc
+++ b/pvecm.adoc
@@ -918,6 +918,24 @@ transfer memory and disk contents.
 
 * Storage replication
 
+SSH setup
+~~~~~~~~~
+
+On {pve} systems, the following changes are made to the SSH configuration/setup:
+
+* the `root` user's SSH client config gets setup to prefer `AES` over `ChaCha20`
+
+* the `root` user's `authorized_keys` file gets linked to
+  `/etc/pve/priv/authorized_keys`, merging all authorized keys within a cluster
+
+* `sshd` is configured to allow logging in as root with a password
+
+NOTE: Older systems might also have `/etc/ssh/ssh_known_hosts` set up as symlink
+pointing to `/etc/pve/priv/known_hosts`, containing a merged version of all
+node host keys. This system was replaced with explicit host key pinning in
+`pve-cluster <<INSERT VERSION>>`, the symlink can be deconfigured if still in
+place by running `pvecm updatecerts --unmerge-known-hosts`.
+
 Pitfalls due to automatic execution of `.bashrc` and siblings
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.39.2

[pve-devel] [PATCH manager 1/2] vnc: use SSH command helper

[Fabian Grünbichler]

to benefit from future improvements there, like pinning the known host key.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/API2/Nodes.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 3619190de..39139d8a9 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -965,7 +965,8 @@ my $get_vnc_connection_info = sub {
     my ($remip, $family);
     if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {
 	($remip, $family) = PVE::Cluster::remote_node_ip($node);
-	$remote_cmd = ['/usr/bin/ssh', '-e', 'none', '-t', $remip , '--'];
+	$remote_cmd = PVE::SSHInfo::ssh_info_to_command({ ip => $remip, name => $node }, ('-t'));
+	push @$remote_cmd, '--';
     } else {
 	$family = PVE::Tools::get_host_address_family($node);
     }
-- 
2.39.2

[pve-devel] [PATCH manager 2/2] pvesh: use SSH command helper

[Fabian Grünbichler]

to benefit from future improvements like known host key pinning.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/CLI/pvesh.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/CLI/pvesh.pm b/PVE/CLI/pvesh.pm
index 44a65213c..d373ae29f 100755
--- a/PVE/CLI/pvesh.pm
+++ b/PVE/CLI/pvesh.pm
@@ -116,7 +116,7 @@ sub proxy_handler {
 	}
     }
 
-    my @ssh_tunnel_cmd = ('ssh', '-o', 'BatchMode=yes', "root\@$remip");
+    my $ssh_tunnel_cmd = PVE::SSHInfo::ssh_info_to_command({ ip => $remip, name => $node });
 
     my @pvesh_cmd = ('pvesh', '--noproxy', $cmd, $path, '--output-format', 'json');
     if (scalar(@$args)) {
@@ -126,7 +126,7 @@ sub proxy_handler {
 
     my $res = '';
     PVE::Tools::run_command(
-	[ @ssh_tunnel_cmd, '--', @pvesh_cmd ],
+	[ $ssh_tunnel_cmd->@*, '--', @pvesh_cmd ],
 	errmsg => "proxy handler failed",
 	outfunc => sub { $res .= shift },
     );
-- 
2.39.2

[pve-devel] [PATCH storage 1/1] upload: use SSH helper to get ssh/scp options

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
requires versioned dependency on libpve-cluster-perl with the new helper

 src/PVE/API2/Storage/Status.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index b2336e6..d6de7fb 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -466,9 +466,9 @@ __PACKAGE__->register_method ({
 	if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {
 	    my $remip = PVE::Cluster::remote_node_ip($node);
 
-	    my @ssh_options = ('-o', 'BatchMode=yes');
+	    my $ssh_options = PVE::SSHInfo::ssh_info_to_ssh_opts({ ip => $remip, name => $node });
 
-	    my @remcmd = ('/usr/bin/ssh', @ssh_options, $remip, '--');
+	    my @remcmd = ('/usr/bin/ssh', $ssh_options->@*, $remip, '--');
 
 	    eval { # activate remote storage
 		run_command([@remcmd, '/usr/sbin/pvesm', 'status', '--storage', $param->{storage}]);
@@ -480,7 +480,7 @@ __PACKAGE__->register_method ({
 		errmsg => "mkdir failed",
 	    );
  
-	    $cmd = ['/usr/bin/scp', @ssh_options, '-p', '--', $tmpfilename, "[$remip]:" . PVE::Tools::shell_quote($dest)];
+	    $cmd = ['/usr/bin/scp', $ssh_options->@*, '-p', '--', $tmpfilename, "[$remip]:" . PVE::Tools::shell_quote($dest)];
 
 	    $err_cleanup = sub { run_command([@remcmd, 'rm', '-f', '--', $dest]) };
 	} else {
-- 
2.39.2

Re: [pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Fabian Grünbichler]

On January 11, 2024 11:51 am, Fabian Grünbichler wrote:
> this series replaces the old mechanism that used a cluster-wide merged known
> hosts file with distributing of each node's host key via pmxcfs, and pinning
> the distributed key explicitly for internal SSH connections.
> 
> the main changes in pve-cluster somewhat break the old manager and
> storage versions, but only when such a partial upgrade is mixed with a
> host key rotation of some sort.
> 
> pve-storage uses a newly introduced helper, so needs a versioned
> dependency accordingly.
> 
> the last pve-docs patch has a placeholder for the actual version shipping the
> changes which needs to be replaced when applying.
> 
> there's still some potential for follow-ups:
> - 'pvecm ssh' wrapper to debug and/or re-use the host key pinning (and other
>   future changes)
> - also add non-RSA host keys
> - key (and thus authorized keys) and/or sshd disentangling (this
>   potentially also affects external access, so might be done on a major
>   release to give more heads up)

and one fixup that I just realized thanks to talking to Hannes D. - the
cluster create API call will also merge the known hosts, that call
should also be removed if we remove it from `pvecm updatecerts`.

Re: [pve-devel] [PATCH docs 2/2] ssh: document PVE-specific setup

[Fabian Grünbichler]

> Esi Y via pve-devel <pve-devel@lists.proxmox.com> hat am 12.01.2024 13:33 CET geschrieben:
> On Thu, Jan 11, 2024 at 11:51:20AM +0100, Fabian Grünbichler wrote:
> > such as adapted configs and managed files.
> > 
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> > ---
> > Notes: actual version needs to be inserted!
> > 
> >  pvecm.adoc | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/pvecm.adoc b/pvecm.adoc
> > index 5b5b27b..3a32cfb 100644
> > --- a/pvecm.adoc
> > +++ b/pvecm.adoc
> > @@ -918,6 +918,24 @@ transfer memory and disk contents.
> >  
> >  * Storage replication
> >  
> > +SSH setup
> > +~~~~~~~~~
> > +
> > +On {pve} systems, the following changes are made to the SSH configuration/setup:
> > +
> > +* the `root` user's SSH client config gets setup to prefer `AES` over `ChaCha20`
> > +
> > +* the `root` user's `authorized_keys` file gets linked to
> > +  `/etc/pve/priv/authorized_keys`, merging all authorized keys within a cluster
> 
> Will you be opening a new fix # thread on this one or intending to keep it as-is (even as the known_hosts changes are rolled out)?

see the cover letter - if this series gets applied in its current form, then changing the (client) key setup (both the keys used, and the authorized keys handling) would be a potential (but not required) follow-up. the main issue with that is that setups out there might rely on the current behaviour (e.g., ssh-copy-id to one node registering the key automatically with all nodes in the cluster), so it's likely only possible to switch by default on the next major bump, if we decide to go down that route.

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

[Fabian Grünbichler]

> 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@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.

> > +	];
> > +    } 
> > +
> >      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@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Hannes Dürr]

Tested cluster creation with three new nodes on 8.1 and the patches
Cluster creation and further ssh communication (eq. migration) worked 
flawless


Tested-by: Hannes Duerr <h.duerr@proxmox.com>

On 1/11/24 11:51, Fabian Grünbichler wrote:
> this series replaces the old mechanism that used a cluster-wide merged known
> hosts file with distributing of each node's host key via pmxcfs, and pinning
> the distributed key explicitly for internal SSH connections.
>
> the main changes in pve-cluster somewhat break the old manager and
> storage versions, but only when such a partial upgrade is mixed with a
> host key rotation of some sort.
>
> pve-storage uses a newly introduced helper, so needs a versioned
> dependency accordingly.
>
> the last pve-docs patch has a placeholder for the actual version shipping the
> changes which needs to be replaced when applying.
>
> there's still some potential for follow-ups:
> - 'pvecm ssh' wrapper to debug and/or re-use the host key pinning (and other
>    future changes)
> - also add non-RSA host keys
> - key (and thus authorized keys) and/or sshd disentangling (this
>    potentially also affects external access, so might be done on a major
>    release to give more heads up)
>
> cluster:
>
> Fabian Grünbichler (4):
>    fix #4886: write node SSH hostkey to pmxcfs
>    fix #4886: SSH: pin node's host key if available
>    ssh: expose SSH options on their own
>    pvecm: stop merging SSH known hosts by default
>
>   src/PVE/CLI/pvecm.pm     | 10 ++++++++--
>   src/PVE/Cluster/Setup.pm | 24 +++++++++++++++++++++---
>   src/PVE/SSHInfo.pm       | 31 +++++++++++++++++++++++++++----
>   3 files changed, 56 insertions(+), 9 deletions(-)
>
> docs:
>
> Fabian Grünbichler (2):
>    ssh: make pitfalls a regular section instead of block
>    ssh: document PVE-specific setup
>
>   pvecm.adoc | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
>
> manager:
>
> Fabian Grünbichler (2):
>    vnc: use SSH command helper
>    pvesh: use SSH command helper
>
>   PVE/API2/Nodes.pm | 3 ++-
>   PVE/CLI/pvesh.pm  | 4 ++--
>   2 files changed, 4 insertions(+), 3 deletions(-)
>
> storage:
>
> Fabian Grünbichler (1):
>    upload: use SSH helper to get ssh/scp options
>
>   src/PVE/API2/Storage/Status.pm | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>

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

[Fabian Grünbichler]

> Esi Y via pve-devel <pve-devel@lists.proxmox.com> hat am 15.01.2024 15:31 CET geschrieben:
> 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@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.

technically true, but doesn't really matter for our use case. we only want to use our own pinned key (or maybe, keys, at some point in the future) for internal connections.

> 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

it doesn't crash, it just fails to work. and this is not the same issue as the original one at all, since previously running the suggest command would break the PVE setup by removing our symlink, whereas now it creates an empty temp file but preserves our setup.

> 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.

the only advantage of a KnownHostsCommand would be to avoid the above (tiny) issue in interactive use cases. our use case is by definition not interactive. the only situation where this should arise in practice is if you manually rotate the SSH host key of a node already in the cluster. even then, it will solve itself after a reboot (or manual invocation of pvecm updatecerts, which should definitely be noted in a yet-to-be-written "keys/secrets and rotating them" section of the docs).

the command approach has similar problems though:
- if it outputs a non-matching host key line, the connection will be aborted (so this is stricter than the file based solution! which is especially problematic if we extend this to handle all key types, since then a rotation of one of them would already trip it up)
- it internally treats the command option as if it were a file, leading to very nice output like this:

Offending RSA key in KnownHostsCommand-HOSTNAME:2
  remove with:
  ssh-keygen -f "KnownHostsCommand-HOSTNAME" -R "XXX"
Host key for XXX has changed and you have requested strict checking.
Host key verification failed.

(XXX is my hostname, the rest is output exactly like it is!)

last but not least, switching to a command is always possible as follow-up since it's entirely on the client side anyway and requires no coordination across the cluster - the command would just output the contents of the file anyhow.

Re: [pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Thomas Lamprecht]

Am 15/01/2024 um 16:53 schrieb Hannes Dürr:
> Tested cluster creation with three new nodes on 8.1 and the patches
> Cluster creation and further ssh communication (eq. migration) worked 
> flawless
> 
> Tested-by: Hannes Duerr <h.duerr@proxmox.com>

What about the reinstallation of an existing node, or replacing
one, while keeping the same nodename scenario?

As that was one of the main original reasons for this change here
in the first place.

For the removal you could play through the documented procedure
and send a patch for update it accordingly, as e.g., the part
about the node’s SSH keys remaining in the pmxcfs authorized_key
file would need some change to reflect that this is not true
for newer setups (once this series is applied and the respective
packages got bumped and released).

https://pve.proxmox.com/pve-docs/chapter-pvecm.html#pvecm_separate_node_without_reinstall

Re: [pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 16.01.2024 11:34 CET geschrieben:
> 
>  
> Am 15/01/2024 um 16:53 schrieb Hannes Dürr:
> > Tested cluster creation with three new nodes on 8.1 and the patches
> > Cluster creation and further ssh communication (eq. migration) worked 
> > flawless
> > 
> > Tested-by: Hannes Duerr <h.duerr@proxmox.com>
> 
> What about the reinstallation of an existing node, or replacing
> one, while keeping the same nodename scenario?

on (re)join, pvecm updatecerts is called, and the (new) host key is written to the node directory (and picked up by the other nodes) from there.

> As that was one of the main original reasons for this change here
> in the first place.
> 
> For the removal you could play through the documented procedure
> and send a patch for update it accordingly, as e.g., the part
> about the node’s SSH keys remaining in the pmxcfs authorized_key
> file would need some change to reflect that this is not true
> for newer setups (once this series is applied and the respective
> packages got bumped and released).

authorized_keys are not touched by this series at all, see the cover letter ;) this is purely known_hosts so far..

Re: [pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Thomas Lamprecht]

Am 16/01/2024 um 11:40 schrieb Fabian Grünbichler:
> 
>> Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 16.01.2024 11:34 CET geschrieben:
>>
>>  
>> Am 15/01/2024 um 16:53 schrieb Hannes Dürr:
>>> Tested cluster creation with three new nodes on 8.1 and the patches
>>> Cluster creation and further ssh communication (eq. migration) worked 
>>> flawless
>>>
>>> Tested-by: Hannes Duerr <h.duerr@proxmox.com>
>>
>> What about the reinstallation of an existing node, or replacing
>> one, while keeping the same nodename scenario?
> 
> on (re)join, pvecm updatecerts is called, and the (new) host key is written to the node directory (and picked up by the other nodes) from there.

Yeah, I saw that, but I didn't see it listed in the things tested by
Hannes.

>> As that was one of the main original reasons for this change here
>> in the first place.
>>
>> For the removal you could play through the documented procedure
>> and send a patch for update it accordingly, as e.g., the part
>> about the node’s SSH keys remaining in the pmxcfs authorized_key
>> file would need some change to reflect that this is not true
>> for newer setups (once this series is applied and the respective
>> packages got bumped and released).
> 
> authorized_keys are not touched by this series at all, see the cover letter ;) this is purely known_hosts so far..

argh, yeah sure, ignore me then here.

Re: [pve-devel] [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Hannes Dürr]

On 1/16/24 11:34, Thomas Lamprecht wrote:
> Am 15/01/2024 um 16:53 schrieb Hannes Dürr:
>> Tested cluster creation with three new nodes on 8.1 and the patches
>> Cluster creation and further ssh communication (eq. migration) worked
>> flawless
>>
>> Tested-by: Hannes Duerr <h.duerr@proxmox.com>
> What about the reinstallation of an existing node, or replacing
> one, while keeping the same nodename scenario?
I have covered removal and rejoin of a node as well.
> As that was one of the main original reasons for this change here
> in the first place.
>
> For the removal you could play through the documented procedure
> and send a patch for update it accordingly, as e.g., the part
> about the node’s SSH keys remaining in the pmxcfs authorized_key
> file would need some change to reflect that this is not true
> for newer setups (once this series is applied and the respective
> packages got bumped and released).
>
> https://pve.proxmox.com/pve-docs/chapter-pvecm.html#pvecm_separate_node_without_reinstall

[pve-devel] applied-series: [PATCH cluster/manager/storage/docs 0/9] fix #4886: improve SSH handling

[Thomas Lamprecht]

Am 11/01/2024 um 11:51 schrieb Fabian Grünbichler:
> this series replaces the old mechanism that used a cluster-wide merged known
> hosts file with distributing of each node's host key via pmxcfs, and pinning
> the distributed key explicitly for internal SSH connections.
> 
> the main changes in pve-cluster somewhat break the old manager and
> storage versions, but only when such a partial upgrade is mixed with a
> host key rotation of some sort.
> 
> pve-storage uses a newly introduced helper, so needs a versioned
> dependency accordingly.
> 
> the last pve-docs patch has a placeholder for the actual version shipping the
> changes which needs to be replaced when applying.
> 
> there's still some potential for follow-ups:
> - 'pvecm ssh' wrapper to debug and/or re-use the host key pinning (and other
>   future changes)
> - also add non-RSA host keys
> - key (and thus authorized keys) and/or sshd disentangling (this
>   potentially also affects external access, so might be done on a major
>   release to give more heads up)
> 
> cluster:
> 
> Fabian Grünbichler (4):
>   fix #4886: write node SSH hostkey to pmxcfs
>   fix #4886: SSH: pin node's host key if available
>   ssh: expose SSH options on their own
>   pvecm: stop merging SSH known hosts by default
> 
>  src/PVE/CLI/pvecm.pm     | 10 ++++++++--
>  src/PVE/Cluster/Setup.pm | 24 +++++++++++++++++++++---
>  src/PVE/SSHInfo.pm       | 31 +++++++++++++++++++++++++++----
>  3 files changed, 56 insertions(+), 9 deletions(-)
> 
> docs:
> 
> Fabian Grünbichler (2):
>   ssh: make pitfalls a regular section instead of block
>   ssh: document PVE-specific setup
> 
>  pvecm.adoc | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> manager:
> 
> Fabian Grünbichler (2):
>   vnc: use SSH command helper
>   pvesh: use SSH command helper
> 
>  PVE/API2/Nodes.pm | 3 ++-
>  PVE/CLI/pvesh.pm  | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> storage:
> 
> Fabian Grünbichler (1):
>   upload: use SSH helper to get ssh/scp options
> 
>  src/PVE/API2/Storage/Status.pm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 


applied series, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel