[pve-devel] [PATCH v1 pve-storage 0/2] SSHFS Example Storage Plugin

[pve-devel] [PATCH v1 pve-storage 0/2] SSHFS Example Storage Plugin

[Max Carrara]

SSHFS Example Storage Plugin - v1
=================================

Add a custom storage plugin based on SSHFS [0] to serve as an example
for an upcoming storage plugin development guide. This plugin should
also be ready for production usage, though it would be nice to get some
more testing (and potentially performance-tuning) done.

The plugin demonstrates how the following things should be handled:
- handling sensitive properties (shout-out to Fiona [1])
  - the private key is copied to /etc/pve/priv/storage/${PRIV_KEY}.key
    instead of being put into /etc/pve/storage.cfg
  - on removal of the storage, the private key is also removed 
  - though not really that nicely supported via the CLI, updating the
    private key file is also handled
- making use of /etc/pve/priv/known_hosts for easy setup on a cluster
  and marking the storage as shared automatically
- connection checking (since it's a network storage)
- conditionally creating the mountpoint and mounting the storage if not
  already mounted
- unmounting the storage on removal (not deactivation!)
- handling "notes" and "protected" attributes for backups

Furthermore, the plugin lives in its own subdir inside `pve-storage`,
complete with its own `debian/` dir and `Makefile` in order to
demonstrate how to package a plugin for Debian.

NOTE: The plugin is *not* built via the top-level `Makefile` at the
moment.

How to Use & Test
-----------------

You'll need some kind guest you can SSH into with pubkey auth.
On that host, it's best to create some directory for SSHFS, e.g.:

    mkdir -p /srv/sshfs && chmod 750 /srv/sshfs

The plugin can easily be built via `make deb` inside
`example/sshfs-plugin`. Once built and installed on a different PVE host
of your choice, you should first deploy a private key for SSHFS, e.g.:

  ssh-copy-id -i ~/.ssh/id_my_private_key \
    -o UserKnownHostsFile=/etc/pve/priv/known_hosts [USER]@[HOST]

Then, using the deployed key, the storage can be added as follows:

  pvesm add sshfs [STOREID] \
    --username [USER] \
    --server [HOST] \
    --sshfs-remote-path /srv/sshfs \
    --path /mnt/path/to/storage \
    --sshfs-private-key ~/.ssh/id_my_private_key

The storage should then pop up in the UI. It should be functionally
equivalent to most other dir-based network storages, except that imports
aren't supported in order to keep the example from becoming too complex.

Would be nice if somebody could give this a spin! I think there's
definitely room for some performance improvements, but so far the
storage was doing *alright*. IOPS are terrible, though.

References
----------

[0]: https://github.com/libfuse/sshfs
[1]: https://lore.proxmox.com/pve-devel/20250404133204.239783-6-f.ebner@proxmox.com/

Older Versions
--------------

rfc-v1: https://lore.proxmox.com/pve-devel/20250328171209.503132-1-m.carrara@proxmox.com/

Summary of Changes
------------------

Max Carrara (2):
  example: sshfs plugin: add custom storage plugin for SSHFS
  example: sshfs plugin: package SSHFSPlugin.pm

 example/sshfs-plugin/Makefile                 |  71 ++++
 example/sshfs-plugin/debian/changelog         |   5 +
 example/sshfs-plugin/debian/control           |  22 +
 example/sshfs-plugin/debian/copyright         |  21 +
 example/sshfs-plugin/debian/rules             |  26 ++
 example/sshfs-plugin/debian/source/format     |   1 +
 example/sshfs-plugin/debian/triggers          |   1 +
 .../lib/PVE/Storage/Custom/SSHFSPlugin.pm     | 398 ++++++++++++++++++
 8 files changed, 545 insertions(+)
 create mode 100644 example/sshfs-plugin/Makefile
 create mode 100644 example/sshfs-plugin/debian/changelog
 create mode 100644 example/sshfs-plugin/debian/control
 create mode 100644 example/sshfs-plugin/debian/copyright
 create mode 100755 example/sshfs-plugin/debian/rules
 create mode 100644 example/sshfs-plugin/debian/source/format
 create mode 100644 example/sshfs-plugin/debian/triggers
 create mode 100644 example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm

-- 
2.39.5



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

[pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Max Carrara]

This commit adds an example implementation of a custom storage plugin
that uses SSHFS [0] as the underlying filesystem.

The implementation is very similar to that of the NFS plugin; as a
prerequisite, it is currently necessary to use pubkey auth and have
the host's root user's public key deployed to the remote host like so:

  ssh-copy-id -i ~/.ssh/id_my_private_key \
    -o UserKnownHostsFile=/etc/pve/priv/known_hosts [USER]@[HOST]

Then, the storage can be added as follows:

  pvesm add sshfs [STOREID] \
    --username [USER] \
    --server [HOST] \
    --sshfs-remote-path [ABS PATH ON REMOTE] \
    --path /mnt/path/to/storage \
    --sshfs-private-key ~/.ssh/id_my_private_key

If the host is part of a cluster, other nodes may connect to the
remote without any additional setup required. This is because we copy
the private key to `/etc/pve/priv/storage/$KEYNAME.key` and use the
cluster-wide `/etc/pve/priv/known_hosts` file. Also mark each SSHFS
storage as `shared` by default in order to make use of this.

Note: Because there's currently no way to officially and permanently
mark a storage as shared (like some built-in plugins [1]) set
`$scfg->{shared} = 1;` in `on_add_hook`. This has almost the same
effect as modifying `@PVE::Storage::Plugin::SHARED_STORAGE` directly,
except that the `shared` is written to `/etc/pve/storage.cfg`.

[0]: https://github.com/libfuse/sshfs
[1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4e16420f667f196e8eb99ae7c9f3f1d3e13791fb;hb=refs/heads/master#l37

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes rfc-v1 --> v1:
  * rework most of the plugin
  * cease to call methods of DirPlugin (plugins should be isolated;
    we don't want to encourage third-party devs to do that)
  * handle SSH private key as sensitive property and place it on pmxcfs
  * make storage shared
  * manually implement attribute handling ("notes", "protected" for
    backups)

 .../lib/PVE/Storage/Custom/SSHFSPlugin.pm     | 398 ++++++++++++++++++
 1 file changed, 398 insertions(+)
 create mode 100644 example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm

diff --git a/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
new file mode 100644
index 0000000..75b29c1
--- /dev/null
+++ b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
@@ -0,0 +1,398 @@
+package PVE::Storage::Custom::SSHFSPlugin;
+
+use strict;
+use warnings;
+
+use feature 'signatures';
+
+use Cwd qw();
+use Encode qw(decode encode);
+use File::Path qw(make_path);
+use File::Basename qw(dirname);
+use IO::File;
+use POSIX;
+
+use PVE::ProcFSTools;
+use PVE::Tools qw(
+    file_copy
+    file_get_contents
+    file_set_contents
+    run_command
+);
+
+use base qw(PVE::Storage::Plugin);
+
+my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";
+
+# Plugin Definition
+
+sub api {
+    return 11;
+}
+
+sub type {
+    return 'sshfs';
+}
+
+sub plugindata {
+    return {
+	content => [
+	    {
+		images => 1,
+		rootdir => 1,
+		vztmpl => 1,
+		iso => 1,
+		backup => 1,
+		snippets => 1,
+		none => 1,
+	    },
+	    {
+		images => 1,
+		rootdir => 1,
+	    },
+	],
+	format => [
+	    {
+		raw => 1,
+		qcow2 => 1,
+		vmdk => 1,
+	    },
+	    'qcow2',
+	],
+	'sensitive-properties' => {
+	    'sshfs-private-key' => 1,
+	},
+    };
+}
+
+sub properties {
+    return {
+	'sshfs-remote-path' => {
+	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
+	    type => 'string',
+	    format => 'pve-storage-path',
+	},
+	'sshfs-private-key' => {
+	    description => "Path to the private key to use for SSHFS.",
+	    type => 'string',
+	    format => 'pve-storage-path',
+	}
+    };
+}
+
+sub options {
+    return {
+	disable => { optional => 1 },
+	path => { fixed => 1 },
+	'create-base-path' => { optional => 1 },
+	content => { optional => 1 },
+	'create-subdirs' => { optional => 1 },
+	'content-dirs' => { optional => 1 },
+	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
+	format => { optional => 1 },
+	bwlimit => { optional => 1 },
+	preallocation => { optional => 1 },
+	nodes => { optional => 1 },
+	shared => { optional => 1 },
+
+	# SSHFS Options
+	username => {},
+	server => {},
+	'sshfs-remote-path' => {},
+	port => { optional => 1 },
+	'sshfs-private-key' => { optional => 1 },
+   };
+}
+
+# SSHFS Helpers
+
+my sub sshfs_remote_from_config: prototype($) ($scfg) {
+    my ($user, $host, $remote_path) = $scfg->@{qw(username server sshfs-remote-path)};
+    return "${user}\@${host}:${remote_path}";
+}
+
+my sub sshfs_private_key_path: prototype($) ($storeid) {
+    return "/etc/pve/priv/storage/$storeid.key";
+}
+
+my sub sshfs_common_ssh_opts: prototype($) ($storeid) {
+    my $private_key_path = sshfs_private_key_path($storeid);
+
+    my @common_opts = (
+	'-o', "UserKnownHostsFile=${CLUSTER_KNOWN_HOSTS}",
+	'-o', 'GlobalKnownHostsFile=none',
+	'-o', "IdentityFile=${private_key_path}",
+    );
+
+    return @common_opts;
+}
+
+my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
+    die "path of private key file not specified" if !defined($src_key_path);
+    die "path of private key file does not exist" if ! -e $src_key_path;
+    die "path of private key file does not point to a file" if ! -f $src_key_path;
+
+    my $dest_key_path = sshfs_private_key_path($storeid);
+
+    my $dest_key_parent_dir = dirname($dest_key_path);
+    if (! -e $dest_key_parent_dir) {
+	make_path($dest_key_parent_dir, { chmod => 0700 });
+    } else {
+	die "'$dest_key_path' already exists" if -e $dest_key_path;
+    }
+
+    file_copy($src_key_path, $dest_key_path, undef, 600);
+
+    return undef;
+}
+
+my sub sshfs_remove_private_key: prototype($) ($storeid) {
+    my $key_path = sshfs_private_key_path($storeid);
+    unlink($key_path) or $! == ENOENT or die "failed to remove private key '$key_path' - $!\n";
+
+    return undef;
+}
+
+my sub sshfs_is_mounted: prototype($) ($scfg) {
+    my $remote = sshfs_remote_from_config($scfg);
+
+    my $mountpoint = Cwd::realpath($scfg->{path}); # Resolve symlinks
+    return 0 if !defined($mountpoint);
+
+    my $mountdata = PVE::ProcFSTools::parse_proc_mounts();
+
+    my $has_found_mountpoint = grep {
+	$_->[0] =~ m|^\Q${remote}\E$|
+	&& $_->[1] eq $mountpoint
+	&& $_->[2] eq 'fuse.sshfs'
+    } $mountdata->@*;
+
+    return $has_found_mountpoint != 0;
+}
+
+my sub sshfs_mount: prototype($$) ($scfg, $storeid) {
+    my $remote = sshfs_remote_from_config($scfg);
+    my ($port, $mountpoint) = $scfg->@{qw(port path)};
+
+    my @common_opts = sshfs_common_ssh_opts($storeid);
+    my $cmd = [
+	'/usr/bin/sshfs', @common_opts,
+	'-o', 'noatime',
+    ];
+
+    push($cmd->@*, '-p', $port) if $port;
+    push($cmd->@*, $remote, $mountpoint);
+
+    eval {
+	run_command(
+	    $cmd,
+	    timeout => 10,
+	    errfunc => sub { warn "$_[0]\n"; },
+	);
+    };
+    if (my $err = $@) {
+	die "failed to mount SSHFS storage '$remote' at '$mountpoint': $@\n";
+    }
+
+    die "SSHFS storage '$remote' not mounted at '$mountpoint' despite reported success\n"
+	if ! sshfs_is_mounted($scfg);
+
+    return;
+}
+
+my sub sshfs_umount: prototype($) ($scfg) {
+    my $mountpoint = $scfg->{path};
+
+    my $cmd = ['/usr/bin/umount', $mountpoint];
+
+    eval {
+	run_command(
+	    $cmd,
+	    timeout => 10,
+	    errfunc => sub { warn "$_[0]\n"; },
+	);
+    };
+    if (my $err = $@) {
+	die "failed to unmount SSHFS at '$mountpoint': $err\n";
+    }
+
+    return;
+}
+
+# Storage Implementation
+
+sub on_add_hook ($class, $storeid, $scfg, %sensitive) {
+    $scfg->{shared} = 1; # mark SSHFS storages as shared by default
+
+    eval {
+	my $src_key_path = $sensitive{'sshfs-private-key'};
+	sshfs_set_private_key($storeid, $src_key_path);
+    };
+    die "error while adding SSHFS storage '${storeid}': $@\n" if $@;
+
+    return undef;
+}
+
+sub on_update_hook ($class, $storeid, $scfg, %sensitive) {
+    return undef if !exists($sensitive{'sshfs-private-key'});
+
+    my $src_key_path = $sensitive{'sshfs-private-key'};
+
+    if (!defined($src_key_path)) {
+	warn "removing private key for SSHFS storage '${storeid}'";
+	warn "the storage might not be mountable without a private key!";
+
+	eval { sshfs_remove_private_key($storeid); };
+	die $@ if $@;
+
+	return undef;
+    }
+
+    my $dest_key_path = sshfs_private_key_path($storeid);
+    my $dest_key_path_tmp = "${dest_key_path}.old";
+
+    file_copy($dest_key_path, $dest_key_path_tmp) if -e $dest_key_path;
+
+    eval { file_copy($src_key_path, $dest_key_path, undef, 600); };
+
+    if (my $err = $@) {
+	if (-e $dest_key_path_tmp) {
+	    warn "attempting to restore previous private key for storage '${storeid}'\n";
+	    eval { file_copy($dest_key_path_tmp, $dest_key_path, undef, 600); };
+	    warn "$@\n" if $@;
+
+	    unlink $dest_key_path_tmp;
+	}
+
+	die "failed to set private key for SSHFS storage '${storeid}': $err\n";
+    }
+
+    unlink $dest_key_path_tmp;
+
+    return undef;
+}
+
+sub on_delete_hook ($class, $storeid, $scfg) {
+    eval { sshfs_remove_private_key($storeid); };
+    warn $@ if $@;
+
+    eval { sshfs_umount($scfg) if sshfs_is_mounted($scfg); };
+    warn $@ if $@;
+
+    return undef;
+}
+
+sub check_connection ($class, $storeid, $scfg) {
+    my ($user, $host, $port) = $scfg->@{qw(username server port)};
+
+    my @common_opts = sshfs_common_ssh_opts($storeid);
+    my $cmd = [
+	'/usr/bin/ssh',
+	'-T',
+	@common_opts,
+	'-o', 'BatchMode=yes',
+	'-o', 'ConnectTimeout=5',
+    ];
+
+    push($cmd->@*, "-p", $port) if $port;
+    push($cmd->@*, "${user}\@${host}", 'exit 0');
+
+    eval {
+	run_command(
+	    $cmd,
+	    timeout => 10,
+	    errfunc => sub { warn "$_[0]\n"; },
+	);
+    };
+    if (my $err = $@) {
+	warn "$err";
+	return 0;
+    }
+
+    return 1;
+}
+
+sub activate_storage ($class, $storeid, $scfg, $cache) {
+    my $mountpoint = $scfg->{path};
+
+    if (!sshfs_is_mounted($scfg)) {
+	if ($scfg->{'create-base-path'} // 1) {
+	    make_path($mountpoint);
+	}
+
+	die "unable to activate storage '$storeid' - directory '$mountpoint' does not exist\n"
+	    if ! -d $mountpoint;
+
+	sshfs_mount($scfg, $storeid);
+    }
+
+    $class->SUPER::activate_storage($storeid, $scfg, $cache);
+    return;
+}
+
+sub get_volume_attribute ($class, $scfg, $storeid, $volname, $attribute) {
+    my ($vtype) = $class->parse_volname($volname);
+    return if $vtype ne 'backup';
+
+    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
+
+    if ($attribute eq 'notes') {
+	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
+
+	if (-f $notes_path) {
+	    my $notes = file_get_contents($notes_path);
+	    return eval { decode('UTF-8', $notes, 1) } // $notes;
+	}
+
+	return "";
+    }
+
+    if ($attribute eq 'protected') {
+	return -e PVE::Storage::protection_file_path($volume_path) ? 1 : 0;
+    }
+
+    return;
+}
+
+sub update_volume_attribute ($class, $scfg, $storeid, $volname, $attribute, $value) {
+    my ($vtype, $name) = $class->parse_volname($volname);
+    die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
+
+    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
+
+    if ($attribute eq 'notes') {
+	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
+
+	if (defined($value)) {
+	    my $encoded_notes = encode('UTF-8', $value);
+	    file_set_contents($notes_path, $encoded_notes);
+	} else {
+	    unlink $notes_path or $! == ENOENT or die "could not delete notes - $!\n";
+	}
+
+	return;
+    }
+
+    if ($attribute eq 'protected') {
+	my $protection_path = PVE::Storage::protection_file_path($volume_path);
+
+	# Protection already set or unset
+	return if !((-e $protection_path) xor $value);
+
+	if ($value) {
+	    my $fh = IO::File->new($protection_path, O_CREAT, 0644)
+		or die "unable to create protection file '$protection_path' - $!\n";
+	    close($fh);
+	} else {
+	    unlink $protection_path or $! == ENOENT
+		or die "could not delete protection file '$protection_path' - $!\n";
+	}
+
+	return;
+    }
+
+    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
+}
+
+1;
-- 
2.39.5



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

Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Max Carrara]

On Wed Apr 16, 2025 at 2:47 PM CEST, Max Carrara wrote:
> This commit adds an example implementation of a custom storage plugin
> that uses SSHFS [0] as the underlying filesystem.
>
> The implementation is very similar to that of the NFS plugin; as a
> prerequisite, it is currently necessary to use pubkey auth and have
> the host's root user's public key deployed to the remote host like so:
>
>   ssh-copy-id -i ~/.ssh/id_my_private_key \
>     -o UserKnownHostsFile=/etc/pve/priv/known_hosts [USER]@[HOST]
>
> Then, the storage can be added as follows:
>
>   pvesm add sshfs [STOREID] \
>     --username [USER] \
>     --server [HOST] \
>     --sshfs-remote-path [ABS PATH ON REMOTE] \
>     --path /mnt/path/to/storage \
>     --sshfs-private-key ~/.ssh/id_my_private_key
>
> If the host is part of a cluster, other nodes may connect to the
> remote without any additional setup required. This is because we copy
> the private key to `/etc/pve/priv/storage/$KEYNAME.key` and use the
> cluster-wide `/etc/pve/priv/known_hosts` file. Also mark each SSHFS
> storage as `shared` by default in order to make use of this.
>
> Note: Because there's currently no way to officially and permanently
> mark a storage as shared (like some built-in plugins [1]) set
> `$scfg->{shared} = 1;` in `on_add_hook`. This has almost the same
> effect as modifying `@PVE::Storage::Plugin::SHARED_STORAGE` directly,
> except that the `shared` is written to `/etc/pve/storage.cfg`.
>
> [0]: https://github.com/libfuse/sshfs
> [1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4e16420f667f196e8eb99ae7c9f3f1d3e13791fb;hb=refs/heads/master#l37
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes rfc-v1 --> v1:
>   * rework most of the plugin
>   * cease to call methods of DirPlugin (plugins should be isolated;
>     we don't want to encourage third-party devs to do that)
>   * handle SSH private key as sensitive property and place it on pmxcfs
>   * make storage shared
>   * manually implement attribute handling ("notes", "protected" for
>     backups)
>
>  .../lib/PVE/Storage/Custom/SSHFSPlugin.pm     | 398 ++++++++++++++++++
>  1 file changed, 398 insertions(+)
>  create mode 100644 example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
>
> diff --git a/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> new file mode 100644
> index 0000000..75b29c1
> --- /dev/null
> +++ b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> @@ -0,0 +1,398 @@
> +package PVE::Storage::Custom::SSHFSPlugin;
> +
> +use strict;
> +use warnings;
> +
> +use feature 'signatures';
> +
> +use Cwd qw();
> +use Encode qw(decode encode);
> +use File::Path qw(make_path);
> +use File::Basename qw(dirname);
> +use IO::File;
> +use POSIX;
> +
> +use PVE::ProcFSTools;
> +use PVE::Tools qw(
> +    file_copy
> +    file_get_contents
> +    file_set_contents
> +    run_command
> +);
> +
> +use base qw(PVE::Storage::Plugin);
> +
> +my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";
> +
> +# Plugin Definition
> +
> +sub api {
> +    return 11;
> +}
> +
> +sub type {
> +    return 'sshfs';
> +}
> +
> +sub plugindata {
> +    return {
> +	content => [
> +	    {
> +		images => 1,
> +		rootdir => 1,
> +		vztmpl => 1,
> +		iso => 1,
> +		backup => 1,
> +		snippets => 1,
> +		none => 1,

Argh, there's one thing I just noticed: The `none` content type above
should not be declared here and needs to be removed, because it's for
internal use only (for now, at least).

Even though we'll ship this example ourselves, it should of course still
adhere to what third-party plugin devs should do. >.>

For now it doesn't hurt if it's here and can just be removed in a
follow-up, but I'll already intentionally exclude it from the upcoming
plugin dev guide.

> +	    },
> +	    {
> +		images => 1,
> +		rootdir => 1,
> +	    },
> +	],
> +	format => [
> +	    {
> +		raw => 1,
> +		qcow2 => 1,
> +		vmdk => 1,
> +	    },
> +	    'qcow2',
> +	],
> +	'sensitive-properties' => {
> +	    'sshfs-private-key' => 1,
> +	},
> +    };
> +}
> +
> +sub properties {
> +    return {
> +	'sshfs-remote-path' => {
> +	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
> +	    type => 'string',
> +	    format => 'pve-storage-path',
> +	},
> +	'sshfs-private-key' => {
> +	    description => "Path to the private key to use for SSHFS.",
> +	    type => 'string',
> +	    format => 'pve-storage-path',
> +	}
> +    };
> +}
> +
> +sub options {
> +    return {
> +	disable => { optional => 1 },
> +	path => { fixed => 1 },
> +	'create-base-path' => { optional => 1 },
> +	content => { optional => 1 },
> +	'create-subdirs' => { optional => 1 },
> +	'content-dirs' => { optional => 1 },
> +	'prune-backups' => { optional => 1 },
> +	'max-protected-backups' => { optional => 1 },
> +	format => { optional => 1 },
> +	bwlimit => { optional => 1 },
> +	preallocation => { optional => 1 },
> +	nodes => { optional => 1 },
> +	shared => { optional => 1 },
> +
> +	# SSHFS Options
> +	username => {},
> +	server => {},
> +	'sshfs-remote-path' => {},
> +	port => { optional => 1 },
> +	'sshfs-private-key' => { optional => 1 },
> +   };
> +}
> +
> +# SSHFS Helpers
> +
> +my sub sshfs_remote_from_config: prototype($) ($scfg) {
> +    my ($user, $host, $remote_path) = $scfg->@{qw(username server sshfs-remote-path)};
> +    return "${user}\@${host}:${remote_path}";
> +}
> +
> +my sub sshfs_private_key_path: prototype($) ($storeid) {
> +    return "/etc/pve/priv/storage/$storeid.key";
> +}
> +
> +my sub sshfs_common_ssh_opts: prototype($) ($storeid) {
> +    my $private_key_path = sshfs_private_key_path($storeid);
> +
> +    my @common_opts = (
> +	'-o', "UserKnownHostsFile=${CLUSTER_KNOWN_HOSTS}",
> +	'-o', 'GlobalKnownHostsFile=none',
> +	'-o', "IdentityFile=${private_key_path}",
> +    );
> +
> +    return @common_opts;
> +}
> +
> +my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
> +    die "path of private key file not specified" if !defined($src_key_path);
> +    die "path of private key file does not exist" if ! -e $src_key_path;
> +    die "path of private key file does not point to a file" if ! -f $src_key_path;
> +
> +    my $dest_key_path = sshfs_private_key_path($storeid);
> +
> +    my $dest_key_parent_dir = dirname($dest_key_path);
> +    if (! -e $dest_key_parent_dir) {
> +	make_path($dest_key_parent_dir, { chmod => 0700 });
> +    } else {
> +	die "'$dest_key_path' already exists" if -e $dest_key_path;
> +    }
> +
> +    file_copy($src_key_path, $dest_key_path, undef, 600);
> +
> +    return undef;
> +}
> +
> +my sub sshfs_remove_private_key: prototype($) ($storeid) {
> +    my $key_path = sshfs_private_key_path($storeid);
> +    unlink($key_path) or $! == ENOENT or die "failed to remove private key '$key_path' - $!\n";
> +
> +    return undef;
> +}
> +
> +my sub sshfs_is_mounted: prototype($) ($scfg) {
> +    my $remote = sshfs_remote_from_config($scfg);
> +
> +    my $mountpoint = Cwd::realpath($scfg->{path}); # Resolve symlinks
> +    return 0 if !defined($mountpoint);
> +
> +    my $mountdata = PVE::ProcFSTools::parse_proc_mounts();
> +
> +    my $has_found_mountpoint = grep {
> +	$_->[0] =~ m|^\Q${remote}\E$|
> +	&& $_->[1] eq $mountpoint
> +	&& $_->[2] eq 'fuse.sshfs'
> +    } $mountdata->@*;
> +
> +    return $has_found_mountpoint != 0;
> +}
> +
> +my sub sshfs_mount: prototype($$) ($scfg, $storeid) {
> +    my $remote = sshfs_remote_from_config($scfg);
> +    my ($port, $mountpoint) = $scfg->@{qw(port path)};
> +
> +    my @common_opts = sshfs_common_ssh_opts($storeid);
> +    my $cmd = [
> +	'/usr/bin/sshfs', @common_opts,
> +	'-o', 'noatime',
> +    ];
> +
> +    push($cmd->@*, '-p', $port) if $port;
> +    push($cmd->@*, $remote, $mountpoint);
> +
> +    eval {
> +	run_command(
> +	    $cmd,
> +	    timeout => 10,
> +	    errfunc => sub { warn "$_[0]\n"; },
> +	);
> +    };
> +    if (my $err = $@) {
> +	die "failed to mount SSHFS storage '$remote' at '$mountpoint': $@\n";
> +    }
> +
> +    die "SSHFS storage '$remote' not mounted at '$mountpoint' despite reported success\n"
> +	if ! sshfs_is_mounted($scfg);
> +
> +    return;
> +}
> +
> +my sub sshfs_umount: prototype($) ($scfg) {
> +    my $mountpoint = $scfg->{path};
> +
> +    my $cmd = ['/usr/bin/umount', $mountpoint];
> +
> +    eval {
> +	run_command(
> +	    $cmd,
> +	    timeout => 10,
> +	    errfunc => sub { warn "$_[0]\n"; },
> +	);
> +    };
> +    if (my $err = $@) {
> +	die "failed to unmount SSHFS at '$mountpoint': $err\n";
> +    }
> +
> +    return;
> +}
> +
> +# Storage Implementation
> +
> +sub on_add_hook ($class, $storeid, $scfg, %sensitive) {
> +    $scfg->{shared} = 1; # mark SSHFS storages as shared by default
> +
> +    eval {
> +	my $src_key_path = $sensitive{'sshfs-private-key'};
> +	sshfs_set_private_key($storeid, $src_key_path);
> +    };
> +    die "error while adding SSHFS storage '${storeid}': $@\n" if $@;
> +
> +    return undef;
> +}
> +
> +sub on_update_hook ($class, $storeid, $scfg, %sensitive) {
> +    return undef if !exists($sensitive{'sshfs-private-key'});
> +
> +    my $src_key_path = $sensitive{'sshfs-private-key'};
> +
> +    if (!defined($src_key_path)) {
> +	warn "removing private key for SSHFS storage '${storeid}'";
> +	warn "the storage might not be mountable without a private key!";
> +
> +	eval { sshfs_remove_private_key($storeid); };
> +	die $@ if $@;
> +
> +	return undef;
> +    }
> +
> +    my $dest_key_path = sshfs_private_key_path($storeid);
> +    my $dest_key_path_tmp = "${dest_key_path}.old";
> +
> +    file_copy($dest_key_path, $dest_key_path_tmp) if -e $dest_key_path;
> +
> +    eval { file_copy($src_key_path, $dest_key_path, undef, 600); };
> +
> +    if (my $err = $@) {
> +	if (-e $dest_key_path_tmp) {
> +	    warn "attempting to restore previous private key for storage '${storeid}'\n";
> +	    eval { file_copy($dest_key_path_tmp, $dest_key_path, undef, 600); };
> +	    warn "$@\n" if $@;
> +
> +	    unlink $dest_key_path_tmp;
> +	}
> +
> +	die "failed to set private key for SSHFS storage '${storeid}': $err\n";
> +    }
> +
> +    unlink $dest_key_path_tmp;
> +
> +    return undef;
> +}
> +
> +sub on_delete_hook ($class, $storeid, $scfg) {
> +    eval { sshfs_remove_private_key($storeid); };
> +    warn $@ if $@;
> +
> +    eval { sshfs_umount($scfg) if sshfs_is_mounted($scfg); };
> +    warn $@ if $@;
> +
> +    return undef;
> +}
> +
> +sub check_connection ($class, $storeid, $scfg) {
> +    my ($user, $host, $port) = $scfg->@{qw(username server port)};
> +
> +    my @common_opts = sshfs_common_ssh_opts($storeid);
> +    my $cmd = [
> +	'/usr/bin/ssh',
> +	'-T',
> +	@common_opts,
> +	'-o', 'BatchMode=yes',
> +	'-o', 'ConnectTimeout=5',
> +    ];
> +
> +    push($cmd->@*, "-p", $port) if $port;
> +    push($cmd->@*, "${user}\@${host}", 'exit 0');
> +
> +    eval {
> +	run_command(
> +	    $cmd,
> +	    timeout => 10,
> +	    errfunc => sub { warn "$_[0]\n"; },
> +	);
> +    };
> +    if (my $err = $@) {
> +	warn "$err";
> +	return 0;
> +    }
> +
> +    return 1;
> +}
> +
> +sub activate_storage ($class, $storeid, $scfg, $cache) {
> +    my $mountpoint = $scfg->{path};
> +
> +    if (!sshfs_is_mounted($scfg)) {
> +	if ($scfg->{'create-base-path'} // 1) {
> +	    make_path($mountpoint);
> +	}
> +
> +	die "unable to activate storage '$storeid' - directory '$mountpoint' does not exist\n"
> +	    if ! -d $mountpoint;
> +
> +	sshfs_mount($scfg, $storeid);
> +    }
> +
> +    $class->SUPER::activate_storage($storeid, $scfg, $cache);
> +    return;
> +}
> +
> +sub get_volume_attribute ($class, $scfg, $storeid, $volname, $attribute) {
> +    my ($vtype) = $class->parse_volname($volname);
> +    return if $vtype ne 'backup';
> +
> +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> +
> +    if ($attribute eq 'notes') {
> +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> +
> +	if (-f $notes_path) {
> +	    my $notes = file_get_contents($notes_path);
> +	    return eval { decode('UTF-8', $notes, 1) } // $notes;
> +	}
> +
> +	return "";
> +    }
> +
> +    if ($attribute eq 'protected') {
> +	return -e PVE::Storage::protection_file_path($volume_path) ? 1 : 0;
> +    }
> +
> +    return;
> +}
> +
> +sub update_volume_attribute ($class, $scfg, $storeid, $volname, $attribute, $value) {
> +    my ($vtype, $name) = $class->parse_volname($volname);
> +    die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
> +
> +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> +
> +    if ($attribute eq 'notes') {
> +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> +
> +	if (defined($value)) {
> +	    my $encoded_notes = encode('UTF-8', $value);
> +	    file_set_contents($notes_path, $encoded_notes);
> +	} else {
> +	    unlink $notes_path or $! == ENOENT or die "could not delete notes - $!\n";
> +	}
> +
> +	return;
> +    }
> +
> +    if ($attribute eq 'protected') {
> +	my $protection_path = PVE::Storage::protection_file_path($volume_path);
> +
> +	# Protection already set or unset
> +	return if !((-e $protection_path) xor $value);
> +
> +	if ($value) {
> +	    my $fh = IO::File->new($protection_path, O_CREAT, 0644)
> +		or die "unable to create protection file '$protection_path' - $!\n";
> +	    close($fh);
> +	} else {
> +	    unlink $protection_path or $! == ENOENT
> +		or die "could not delete protection file '$protection_path' - $!\n";
> +	}
> +
> +	return;
> +    }
> +
> +    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> +}
> +
> +1;



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

Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Thomas Lamprecht]

Am 16.04.25 um 14:47 schrieb Max Carrara:
> This commit adds an example implementation of a custom storage plugin
> that uses SSHFS [0] as the underlying filesystem.
> 
> The implementation is very similar to that of the NFS plugin; as a
> prerequisite, it is currently necessary to use pubkey auth and have
> the host's root user's public key deployed to the remote host like so:
> 
>   ssh-copy-id -i ~/.ssh/id_my_private_key \
>     -o UserKnownHostsFile=/etc/pve/priv/known_hosts [USER]@[HOST]
> 
> Then, the storage can be added as follows:
> 
>   pvesm add sshfs [STOREID] \
>     --username [USER] \
>     --server [HOST] \
>     --sshfs-remote-path [ABS PATH ON REMOTE] \
>     --path /mnt/path/to/storage \
>     --sshfs-private-key ~/.ssh/id_my_private_key
> 
> If the host is part of a cluster, other nodes may connect to the
> remote without any additional setup required. This is because we copy
> the private key to `/etc/pve/priv/storage/$KEYNAME.key` and use the
> cluster-wide `/etc/pve/priv/known_hosts` file. Also mark each SSHFS
> storage as `shared` by default in order to make use of this.
> 
> Note: Because there's currently no way to officially and permanently
> mark a storage as shared (like some built-in plugins [1]) set
> `$scfg->{shared} = 1;` in `on_add_hook`. This has almost the same
> effect as modifying `@PVE::Storage::Plugin::SHARED_STORAGE` directly,
> except that the `shared` is written to `/etc/pve/storage.cfg`.
> 
> [0]: https://github.com/libfuse/sshfs
> [1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4e16420f667f196e8eb99ae7c9f3f1d3e13791fb;hb=refs/heads/master#l37

not a fully complete review but some comments inline, and there is now also a
new repo that would be a great fit for hosting this example:

https://git.proxmox.com/?p=pve-storage-plugin-examples.git;a=summary

> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes rfc-v1 --> v1:
>   * rework most of the plugin
>   * cease to call methods of DirPlugin (plugins should be isolated;
>     we don't want to encourage third-party devs to do that)
>   * handle SSH private key as sensitive property and place it on pmxcfs
>   * make storage shared
>   * manually implement attribute handling ("notes", "protected" for
>     backups)
> 
>  .../lib/PVE/Storage/Custom/SSHFSPlugin.pm     | 398 ++++++++++++++++++
>  1 file changed, 398 insertions(+)
>  create mode 100644 example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> 
> diff --git a/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> new file mode 100644
> index 0000000..75b29c1
> --- /dev/null
> +++ b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> @@ -0,0 +1,398 @@
> +package PVE::Storage::Custom::SSHFSPlugin;
> +
> +use strict;
> +use warnings;
> +
> +use feature 'signatures';

FYI: you can replace above four lines with:

use v5.36;

As that version implies strict, warnings and enables the signatures feature.

> +
> +use Cwd qw();
> +use Encode qw(decode encode);
> +use File::Path qw(make_path);
> +use File::Basename qw(dirname);
> +use IO::File;
> +use POSIX;

The POSIX module exports a lot of stuff by default, even overriding some existing
perl symbols IIRC, while most of the time one just needs some error constants or
format time printing or the like, so explicitly importing what one uses is almost
always the safer choice. There are some export groups for sets of exports, so that
one can e.g. import qw(:errno_h) instead of every error number constant explicitly.

> +
> +use PVE::ProcFSTools;
> +use PVE::Tools qw(
> +    file_copy
> +    file_get_contents
> +    file_set_contents
> +    run_command
> +);
> +
> +use base qw(PVE::Storage::Plugin);
> +
> +my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";

For intra-cluster ssh this shared known_host file is being deprecated in
favor of using a per-node file, i.e. /etc/pve/nodes/NODENAME/ssh_known_hosts,
it might be better to not re-use that here and rather use a dedicated file
for this storage or directly encode it in the config entry (either passed by
the user or TOFU on storage addition).

> +
> +# Plugin Definition
> +

> +sub properties {
> +    return {
> +	'sshfs-remote-path' => {
> +	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
> +	    type => 'string',
> +	    format => 'pve-storage-path',
> +	},
> +	'sshfs-private-key' => {
> +	    description => "Path to the private key to use for SSHFS.",
> +	    type => 'string',
> +	    format => 'pve-storage-path',
> +	}

We normally do not add the storage type as prefix for properties.
FWIW, we could treat the private-key like a password and manage it ourselves

> +    };
> +}
> +
> +sub options {
> +    return {
> +	disable => { optional => 1 },
> +	path => { fixed => 1 },
> +	'create-base-path' => { optional => 1 },
> +	content => { optional => 1 },
> +	'create-subdirs' => { optional => 1 },
> +	'content-dirs' => { optional => 1 },
> +	'prune-backups' => { optional => 1 },
> +	'max-protected-backups' => { optional => 1 },
> +	format => { optional => 1 },
> +	bwlimit => { optional => 1 },
> +	preallocation => { optional => 1 },
> +	nodes => { optional => 1 },
> +	shared => { optional => 1 },
> +
> +	# SSHFS Options
> +	username => {},
> +	server => {},
> +	'sshfs-remote-path' => {},
> +	port => { optional => 1 },
> +	'sshfs-private-key' => { optional => 1 },
> +   };
> +}
> +
> +# SSHFS Helpers
> +
> +my sub sshfs_remote_from_config: prototype($) ($scfg) {
> +    my ($user, $host, $remote_path) = $scfg->@{qw(username server sshfs-remote-path)};
> +    return "${user}\@${host}:${remote_path}";
> +}
> +
> +my sub sshfs_private_key_path: prototype($) ($storeid) {
> +    return "/etc/pve/priv/storage/$storeid.key";
> +}
> +
> +my sub sshfs_common_ssh_opts: prototype($) ($storeid) {
> +    my $private_key_path = sshfs_private_key_path($storeid);
> +
> +    my @common_opts = (
> +	'-o', "UserKnownHostsFile=${CLUSTER_KNOWN_HOSTS}",
> +	'-o', 'GlobalKnownHostsFile=none',
> +	'-o', "IdentityFile=${private_key_path}",
> +    );
> +
> +    return @common_opts;
> +}
> +
> +my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
> +    die "path of private key file not specified" if !defined($src_key_path);
> +    die "path of private key file does not exist" if ! -e $src_key_path;
> +    die "path of private key file does not point to a file" if ! -f $src_key_path;

I mean, fine as this is more for better UX, but not really safe as there is
a TOCTOU race here. If we really want to make it safe(r) we probably should
not allow copying arbitrary files from the system and allow either passing a
existing key to use as value, not the best UX, but it's not _that_ worse then
the status quo IMO.

> +
> +    my $dest_key_path = sshfs_private_key_path($storeid);
> +
> +    my $dest_key_parent_dir = dirname($dest_key_path);
> +    if (! -e $dest_key_parent_dir) {
> +	make_path($dest_key_parent_dir, { chmod => 0700 });

chmod is useless for pmxcfs, as it controls those file attributes itself. While
as is it shouldn't hurt, it might be confusing for devs basing off this plugin
and wanting some other mode (on another path) and then wonder why this suddenly
errors out; so maybe dropping the chmod and adding a comment that the file will
reside below /etc/pve/priv, of which  pmxcfs will always set the mode to 0700 for.

btw., does make_path even dies explicitly, or would you need to check the return
value?

> +    } else {
> +	die "'$dest_key_path' already exists" if -e $dest_key_path;

Being able to override this on add might be nice though? And FWIW, there is
some code deduplication with writing this file in the on_update_hook that
might be avoidable I think.

> +    }
> +
> +    file_copy($src_key_path, $dest_key_path, undef, 600);
> +
> +    return undef;
> +}
> +
> +my sub sshfs_remove_private_key: prototype($) ($storeid) {
> +    my $key_path = sshfs_private_key_path($storeid);
> +    unlink($key_path) or $! == ENOENT or die "failed to remove private key '$key_path' - $!\n";
> +
> +    return undef;
> +}
> +
> +my sub sshfs_is_mounted: prototype($) ($scfg) {
> +    my $remote = sshfs_remote_from_config($scfg);
> +
> +    my $mountpoint = Cwd::realpath($scfg->{path}); # Resolve symlinks
> +    return 0 if !defined($mountpoint);
> +
> +    my $mountdata = PVE::ProcFSTools::parse_proc_mounts();
> +
> +    my $has_found_mountpoint = grep {
> +	$_->[0] =~ m|^\Q${remote}\E$|
> +	&& $_->[1] eq $mountpoint
> +	&& $_->[2] eq 'fuse.sshfs'
> +    } $mountdata->@*;
> +
> +    return $has_found_mountpoint != 0;
> +}
> +
> +my sub sshfs_mount: prototype($$) ($scfg, $storeid) {
> +    my $remote = sshfs_remote_from_config($scfg);
> +    my ($port, $mountpoint) = $scfg->@{qw(port path)};
> +
> +    my @common_opts = sshfs_common_ssh_opts($storeid);
> +    my $cmd = [
> +	'/usr/bin/sshfs', @common_opts,
> +	'-o', 'noatime',
> +    ];
> +
> +    push($cmd->@*, '-p', $port) if $port;
> +    push($cmd->@*, $remote, $mountpoint);
> +
> +    eval {
> +	run_command(
> +	    $cmd,
> +	    timeout => 10,
> +	    errfunc => sub { warn "$_[0]\n"; },
> +	);
> +    };
> +    if (my $err = $@) {
> +	die "failed to mount SSHFS storage '$remote' at '$mountpoint': $@\n";
> +    }
> +
> +    die "SSHFS storage '$remote' not mounted at '$mountpoint' despite reported success\n"
> +	if ! sshfs_is_mounted($scfg);
> +
> +    return;
> +}
> +
> +my sub sshfs_umount: prototype($) ($scfg) {
> +    my $mountpoint = $scfg->{path};
> +
> +    my $cmd = ['/usr/bin/umount', $mountpoint];
> +
> +    eval {
> +	run_command(
> +	    $cmd,
> +	    timeout => 10,
> +	    errfunc => sub { warn "$_[0]\n"; },
> +	);
> +    };
> +    if (my $err = $@) {
> +	die "failed to unmount SSHFS at '$mountpoint': $err\n";
> +    }
> +
> +    return;
> +}
> +
> +# Storage Implementation
> +
> +sub on_add_hook ($class, $storeid, $scfg, %sensitive) {
> +    $scfg->{shared} = 1; # mark SSHFS storages as shared by default
> +
> +    eval {
> +	my $src_key_path = $sensitive{'sshfs-private-key'};
> +	sshfs_set_private_key($storeid, $src_key_path);
> +    };
> +    die "error while adding SSHFS storage '${storeid}': $@\n" if $@;
> +
> +    return undef;
> +}
> +
> +sub on_update_hook ($class, $storeid, $scfg, %sensitive) {
> +    return undef if !exists($sensitive{'sshfs-private-key'});
> +
> +    my $src_key_path = $sensitive{'sshfs-private-key'};
> +
> +    if (!defined($src_key_path)) {
> +	warn "removing private key for SSHFS storage '${storeid}'";
> +	warn "the storage might not be mountable without a private key!";
> +
> +	eval { sshfs_remove_private_key($storeid); };
> +	die $@ if $@;
> +
> +	return undef;
> +    }
> +
> +    my $dest_key_path = sshfs_private_key_path($storeid);
> +    my $dest_key_path_tmp = "${dest_key_path}.old";
> +
> +    file_copy($dest_key_path, $dest_key_path_tmp) if -e $dest_key_path;
> +
> +    eval { file_copy($src_key_path, $dest_key_path, undef, 600); };
> +
> +    if (my $err = $@) {
> +	if (-e $dest_key_path_tmp) {
> +	    warn "attempting to restore previous private key for storage '${storeid}'\n";
> +	    eval { file_copy($dest_key_path_tmp, $dest_key_path, undef, 600); };
> +	    warn "$@\n" if $@;
> +
> +	    unlink $dest_key_path_tmp;
> +	}
> +
> +	die "failed to set private key for SSHFS storage '${storeid}': $err\n";
> +    }
> +
> +    unlink $dest_key_path_tmp;
> +
> +    return undef;
> +}
> +
> +sub on_delete_hook ($class, $storeid, $scfg) {
> +    eval { sshfs_remove_private_key($storeid); };
> +    warn $@ if $@;
> +
> +    eval { sshfs_umount($scfg) if sshfs_is_mounted($scfg); };
> +    warn $@ if $@;
> +
> +    return undef;
> +}
> +
> +sub check_connection ($class, $storeid, $scfg) {
> +    my ($user, $host, $port) = $scfg->@{qw(username server port)};
> +
> +    my @common_opts = sshfs_common_ssh_opts($storeid);
> +    my $cmd = [
> +	'/usr/bin/ssh',
> +	'-T',
> +	@common_opts,
> +	'-o', 'BatchMode=yes',
> +	'-o', 'ConnectTimeout=5',
> +    ];
> +
> +    push($cmd->@*, "-p", $port) if $port;
> +    push($cmd->@*, "${user}\@${host}", 'exit 0');
> +
> +    eval {
> +	run_command(
> +	    $cmd,
> +	    timeout => 10,
> +	    errfunc => sub { warn "$_[0]\n"; },
> +	);
> +    };
> +    if (my $err = $@) {
> +	warn "$err";
> +	return 0;
> +    }
> +
> +    return 1;
> +}
> +
> +sub activate_storage ($class, $storeid, $scfg, $cache) {
> +    my $mountpoint = $scfg->{path};
> +
> +    if (!sshfs_is_mounted($scfg)) {
> +	if ($scfg->{'create-base-path'} // 1) {
> +	    make_path($mountpoint);
> +	}
> +
> +	die "unable to activate storage '$storeid' - directory '$mountpoint' does not exist\n"
> +	    if ! -d $mountpoint;
> +
> +	sshfs_mount($scfg, $storeid);
> +    }
> +
> +    $class->SUPER::activate_storage($storeid, $scfg, $cache);
> +    return;
> +}
> +
> +sub get_volume_attribute ($class, $scfg, $storeid, $volname, $attribute) {
> +    my ($vtype) = $class->parse_volname($volname);
> +    return if $vtype ne 'backup';
> +
> +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> +
> +    if ($attribute eq 'notes') {
> +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> +
> +	if (-f $notes_path) {
> +	    my $notes = file_get_contents($notes_path);
> +	    return eval { decode('UTF-8', $notes, 1) } // $notes;
> +	}
> +
> +	return "";
> +    }
> +
> +    if ($attribute eq 'protected') {
> +	return -e PVE::Storage::protection_file_path($volume_path) ? 1 : 0;
> +    }
> +
> +    return;
> +}
> +
> +sub update_volume_attribute ($class, $scfg, $storeid, $volname, $attribute, $value) {
> +    my ($vtype, $name) = $class->parse_volname($volname);
> +    die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
> +
> +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> +
> +    if ($attribute eq 'notes') {
> +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> +
> +	if (defined($value)) {
> +	    my $encoded_notes = encode('UTF-8', $value);
> +	    file_set_contents($notes_path, $encoded_notes);
> +	} else {
> +	    unlink $notes_path or $! == ENOENT or die "could not delete notes - $!\n";
> +	}
> +
> +	return;
> +    }
> +
> +    if ($attribute eq 'protected') {
> +	my $protection_path = PVE::Storage::protection_file_path($volume_path);
> +
> +	# Protection already set or unset
> +	return if !((-e $protection_path) xor $value);
> +
> +	if ($value) {
> +	    my $fh = IO::File->new($protection_path, O_CREAT, 0644)
> +		or die "unable to create protection file '$protection_path' - $!\n";
> +	    close($fh);
> +	} else {
> +	    unlink $protection_path or $! == ENOENT
> +		or die "could not delete protection file '$protection_path' - $!\n";
> +	}
> +
> +	return;
> +    }
> +
> +    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> +}
> +
> +1;



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

Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Max Carrara]

On Wed Jul 2, 2025 at 10:14 PM CEST, Thomas Lamprecht wrote:
> Am 16.04.25 um 14:47 schrieb Max Carrara:
> > This commit adds an example implementation of a custom storage plugin
> > that uses SSHFS [0] as the underlying filesystem.
> > 
> > The implementation is very similar to that of the NFS plugin; as a
> > prerequisite, it is currently necessary to use pubkey auth and have
> > the host's root user's public key deployed to the remote host like so:
> > 
> >   ssh-copy-id -i ~/.ssh/id_my_private_key \
> >     -o UserKnownHostsFile=/etc/pve/priv/known_hosts [USER]@[HOST]
> > 
> > Then, the storage can be added as follows:
> > 
> >   pvesm add sshfs [STOREID] \
> >     --username [USER] \
> >     --server [HOST] \
> >     --sshfs-remote-path [ABS PATH ON REMOTE] \
> >     --path /mnt/path/to/storage \
> >     --sshfs-private-key ~/.ssh/id_my_private_key
> > 
> > If the host is part of a cluster, other nodes may connect to the
> > remote without any additional setup required. This is because we copy
> > the private key to `/etc/pve/priv/storage/$KEYNAME.key` and use the
> > cluster-wide `/etc/pve/priv/known_hosts` file. Also mark each SSHFS
> > storage as `shared` by default in order to make use of this.
> > 
> > Note: Because there's currently no way to officially and permanently
> > mark a storage as shared (like some built-in plugins [1]) set
> > `$scfg->{shared} = 1;` in `on_add_hook`. This has almost the same
> > effect as modifying `@PVE::Storage::Plugin::SHARED_STORAGE` directly,
> > except that the `shared` is written to `/etc/pve/storage.cfg`.
> > 
> > [0]: https://github.com/libfuse/sshfs
> > [1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4e16420f667f196e8eb99ae7c9f3f1d3e13791fb;hb=refs/heads/master#l37
>
> not a fully complete review but some comments inline, and there is now also a
> new repo that would be a great fit for hosting this example:
>
> https://git.proxmox.com/?p=pve-storage-plugin-examples.git;a=summary

Thanks for the pointer; v2 will be for that repo then!

>
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> > Changes rfc-v1 --> v1:
> >   * rework most of the plugin
> >   * cease to call methods of DirPlugin (plugins should be isolated;
> >     we don't want to encourage third-party devs to do that)
> >   * handle SSH private key as sensitive property and place it on pmxcfs
> >   * make storage shared
> >   * manually implement attribute handling ("notes", "protected" for
> >     backups)
> > 
> >  .../lib/PVE/Storage/Custom/SSHFSPlugin.pm     | 398 ++++++++++++++++++
> >  1 file changed, 398 insertions(+)
> >  create mode 100644 example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> > 
> > diff --git a/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> > new file mode 100644
> > index 0000000..75b29c1
> > --- /dev/null
> > +++ b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> > @@ -0,0 +1,398 @@
> > +package PVE::Storage::Custom::SSHFSPlugin;
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use feature 'signatures';
>
> FYI: you can replace above four lines with:
>
> use v5.36;
>
> As that version implies strict, warnings and enables the signatures feature.

Oh, that's nice. Will change the above to that in v2.

>
> > +
> > +use Cwd qw();
> > +use Encode qw(decode encode);
> > +use File::Path qw(make_path);
> > +use File::Basename qw(dirname);
> > +use IO::File;
> > +use POSIX;
>
> The POSIX module exports a lot of stuff by default, even overriding some existing
> perl symbols IIRC, while most of the time one just needs some error constants or
> format time printing or the like, so explicitly importing what one uses is almost
> always the safer choice. There are some export groups for sets of exports, so that
> one can e.g. import qw(:errno_h) instead of every error number constant explicitly.

Oh good point, I hadn't considered that the POSIX module imports that
much.

>
> > +
> > +use PVE::ProcFSTools;
> > +use PVE::Tools qw(
> > +    file_copy
> > +    file_get_contents
> > +    file_set_contents
> > +    run_command
> > +);
> > +
> > +use base qw(PVE::Storage::Plugin);
> > +
> > +my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";
>
> For intra-cluster ssh this shared known_host file is being deprecated in
> favor of using a per-node file, i.e. /etc/pve/nodes/NODENAME/ssh_known_hosts,
> it might be better to not re-use that here and rather use a dedicated file
> for this storage or directly encode it in the config entry (either passed by
> the user or TOFU on storage addition).

Do you mean a dedicated file for the entire storage plugin or one for
each configured SSHFS storage? Asking just in case because the
terminology can sometimes be a bit ambiguous.

I'm not sure if we should encode it in the config entry though as
there's no way to pass a single known_hosts entry directly
to `ssh` / `sshfs` AFAIK.

>
> > +
> > +# Plugin Definition
> > +
>
> > +sub properties {
> > +    return {
> > +	'sshfs-remote-path' => {
> > +	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
> > +	    type => 'string',
> > +	    format => 'pve-storage-path',
> > +	},
> > +	'sshfs-private-key' => {
> > +	    description => "Path to the private key to use for SSHFS.",
> > +	    type => 'string',
> > +	    format => 'pve-storage-path',
> > +	}
>
> We normally do not add the storage type as prefix for properties.
> FWIW, we could treat the private-key like a password and manage it ourselves

Yeah this was more catered towards external developers; my idea here was
that prefixing the property name in some way (doesn't necessarily need
to be the storage type) helps avoiding conflicts.

Specifically, if a plugin introduces a property that already exists it
will `die` on the call to `->init()`.

While the chance is low that users will install many different plugins
all at once (I assume), I've seen some plugins introduce properties with
names like "multipath", "apikey", "protocol", etc. that to me seem
common enough to eventually cause breakage if we introduce properties
with such names ourselves.

>
> > +    };
> > +}
> > +
> > +sub options {
> > +    return {
> > +	disable => { optional => 1 },
> > +	path => { fixed => 1 },
> > +	'create-base-path' => { optional => 1 },
> > +	content => { optional => 1 },
> > +	'create-subdirs' => { optional => 1 },
> > +	'content-dirs' => { optional => 1 },
> > +	'prune-backups' => { optional => 1 },
> > +	'max-protected-backups' => { optional => 1 },
> > +	format => { optional => 1 },
> > +	bwlimit => { optional => 1 },
> > +	preallocation => { optional => 1 },
> > +	nodes => { optional => 1 },
> > +	shared => { optional => 1 },
> > +
> > +	# SSHFS Options
> > +	username => {},
> > +	server => {},
> > +	'sshfs-remote-path' => {},
> > +	port => { optional => 1 },
> > +	'sshfs-private-key' => { optional => 1 },
> > +   };
> > +}
> > +
> > +# SSHFS Helpers
> > +
> > +my sub sshfs_remote_from_config: prototype($) ($scfg) {
> > +    my ($user, $host, $remote_path) = $scfg->@{qw(username server sshfs-remote-path)};
> > +    return "${user}\@${host}:${remote_path}";
> > +}
> > +
> > +my sub sshfs_private_key_path: prototype($) ($storeid) {
> > +    return "/etc/pve/priv/storage/$storeid.key";
> > +}
> > +
> > +my sub sshfs_common_ssh_opts: prototype($) ($storeid) {
> > +    my $private_key_path = sshfs_private_key_path($storeid);
> > +
> > +    my @common_opts = (
> > +	'-o', "UserKnownHostsFile=${CLUSTER_KNOWN_HOSTS}",
> > +	'-o', 'GlobalKnownHostsFile=none',
> > +	'-o', "IdentityFile=${private_key_path}",
> > +    );
> > +
> > +    return @common_opts;
> > +}
> > +
> > +my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
> > +    die "path of private key file not specified" if !defined($src_key_path);
> > +    die "path of private key file does not exist" if ! -e $src_key_path;
> > +    die "path of private key file does not point to a file" if ! -f $src_key_path;
>
> I mean, fine as this is more for better UX, but not really safe as there is
> a TOCTOU race here. If we really want to make it safe(r) we probably should
> not allow copying arbitrary files from the system and allow either passing a
> existing key to use as value, not the best UX, but it's not _that_ worse then
> the status quo IMO.

Oh yeah you're right, I hadn't considered that there's a TOCTOU race
here... I mean, I guess passing the key as value should work just fine
then, the user could always just do something like e.g.

    pvesm add sshfs sshfs-storage [...] --sshfs-private-key $(cat ~/.ssh/id_sshfs-storage)

(Unless there's also something I'm overlooking here.)

>
> > +
> > +    my $dest_key_path = sshfs_private_key_path($storeid);
> > +
> > +    my $dest_key_parent_dir = dirname($dest_key_path);
> > +    if (! -e $dest_key_parent_dir) {
> > +	make_path($dest_key_parent_dir, { chmod => 0700 });
>
> chmod is useless for pmxcfs, as it controls those file attributes itself. While
> as is it shouldn't hurt, it might be confusing for devs basing off this plugin
> and wanting some other mode (on another path) and then wonder why this suddenly
> errors out; so maybe dropping the chmod and adding a comment that the file will
> reside below /etc/pve/priv, of which  pmxcfs will always set the mode to 0700 for.

ACK, will remove the mode and add a comment. Will also do that below
for the call to `file_copy`.

>
> btw., does make_path even dies explicitly, or would you need to check the return
> value?

make_path will `die` on fatal errors, yes. All non-fatal errors will
emit a warning.

I double-checked just to be sure; `make_path` has only one "severe"
error that becomes fatal if it isn't trapped.
See: https://perldoc.perl.org/File::Path#ERROR-HANDLING

Even though none of the non-fatal errors for `make_path` seem to apply
in our case, I guess we could still use its trapping mechanism and
always `die` on our end. That would probably be the safest option.

>
> > +    } else {
> > +	die "'$dest_key_path' already exists" if -e $dest_key_path;
>
> Being able to override this on add might be nice though? And FWIW, there is
> some code deduplication with writing this file in the on_update_hook that
> might be avoidable I think.

You mean leaving the user to choose whether to overwrite the private key
or not? I mean, we could make the sshfs-private-key property optional,
and then still `die` if the private key doesn't exist after the storage
was added.

>
> > +    }
> > +
> > +    file_copy($src_key_path, $dest_key_path, undef, 600);
> > +
> > +    return undef;
> > +}
> > +
> > +my sub sshfs_remove_private_key: prototype($) ($storeid) {
> > +    my $key_path = sshfs_private_key_path($storeid);
> > +    unlink($key_path) or $! == ENOENT or die "failed to remove private key '$key_path' - $!\n";
> > +
> > +    return undef;
> > +}
> > +
> > +my sub sshfs_is_mounted: prototype($) ($scfg) {
> > +    my $remote = sshfs_remote_from_config($scfg);
> > +
> > +    my $mountpoint = Cwd::realpath($scfg->{path}); # Resolve symlinks
> > +    return 0 if !defined($mountpoint);
> > +
> > +    my $mountdata = PVE::ProcFSTools::parse_proc_mounts();
> > +
> > +    my $has_found_mountpoint = grep {
> > +	$_->[0] =~ m|^\Q${remote}\E$|
> > +	&& $_->[1] eq $mountpoint
> > +	&& $_->[2] eq 'fuse.sshfs'
> > +    } $mountdata->@*;
> > +
> > +    return $has_found_mountpoint != 0;
> > +}
> > +
> > +my sub sshfs_mount: prototype($$) ($scfg, $storeid) {
> > +    my $remote = sshfs_remote_from_config($scfg);
> > +    my ($port, $mountpoint) = $scfg->@{qw(port path)};
> > +
> > +    my @common_opts = sshfs_common_ssh_opts($storeid);
> > +    my $cmd = [
> > +	'/usr/bin/sshfs', @common_opts,
> > +	'-o', 'noatime',
> > +    ];
> > +
> > +    push($cmd->@*, '-p', $port) if $port;
> > +    push($cmd->@*, $remote, $mountpoint);
> > +
> > +    eval {
> > +	run_command(
> > +	    $cmd,
> > +	    timeout => 10,
> > +	    errfunc => sub { warn "$_[0]\n"; },
> > +	);
> > +    };
> > +    if (my $err = $@) {
> > +	die "failed to mount SSHFS storage '$remote' at '$mountpoint': $@\n";
> > +    }
> > +
> > +    die "SSHFS storage '$remote' not mounted at '$mountpoint' despite reported success\n"
> > +	if ! sshfs_is_mounted($scfg);
> > +
> > +    return;
> > +}
> > +
> > +my sub sshfs_umount: prototype($) ($scfg) {
> > +    my $mountpoint = $scfg->{path};
> > +
> > +    my $cmd = ['/usr/bin/umount', $mountpoint];
> > +
> > +    eval {
> > +	run_command(
> > +	    $cmd,
> > +	    timeout => 10,
> > +	    errfunc => sub { warn "$_[0]\n"; },
> > +	);
> > +    };
> > +    if (my $err = $@) {
> > +	die "failed to unmount SSHFS at '$mountpoint': $err\n";
> > +    }
> > +
> > +    return;
> > +}
> > +
> > +# Storage Implementation
> > +
> > +sub on_add_hook ($class, $storeid, $scfg, %sensitive) {
> > +    $scfg->{shared} = 1; # mark SSHFS storages as shared by default
> > +
> > +    eval {
> > +	my $src_key_path = $sensitive{'sshfs-private-key'};
> > +	sshfs_set_private_key($storeid, $src_key_path);
> > +    };
> > +    die "error while adding SSHFS storage '${storeid}': $@\n" if $@;
> > +
> > +    return undef;
> > +}
> > +
> > +sub on_update_hook ($class, $storeid, $scfg, %sensitive) {
> > +    return undef if !exists($sensitive{'sshfs-private-key'});
> > +
> > +    my $src_key_path = $sensitive{'sshfs-private-key'};
> > +
> > +    if (!defined($src_key_path)) {
> > +	warn "removing private key for SSHFS storage '${storeid}'";
> > +	warn "the storage might not be mountable without a private key!";
> > +
> > +	eval { sshfs_remove_private_key($storeid); };
> > +	die $@ if $@;
> > +
> > +	return undef;
> > +    }
> > +
> > +    my $dest_key_path = sshfs_private_key_path($storeid);
> > +    my $dest_key_path_tmp = "${dest_key_path}.old";
> > +
> > +    file_copy($dest_key_path, $dest_key_path_tmp) if -e $dest_key_path;
> > +
> > +    eval { file_copy($src_key_path, $dest_key_path, undef, 600); };
> > +
> > +    if (my $err = $@) {
> > +	if (-e $dest_key_path_tmp) {
> > +	    warn "attempting to restore previous private key for storage '${storeid}'\n";
> > +	    eval { file_copy($dest_key_path_tmp, $dest_key_path, undef, 600); };
> > +	    warn "$@\n" if $@;
> > +
> > +	    unlink $dest_key_path_tmp;
> > +	}
> > +
> > +	die "failed to set private key for SSHFS storage '${storeid}': $err\n";
> > +    }
> > +
> > +    unlink $dest_key_path_tmp;
> > +
> > +    return undef;
> > +}
> > +
> > +sub on_delete_hook ($class, $storeid, $scfg) {
> > +    eval { sshfs_remove_private_key($storeid); };
> > +    warn $@ if $@;
> > +
> > +    eval { sshfs_umount($scfg) if sshfs_is_mounted($scfg); };
> > +    warn $@ if $@;
> > +
> > +    return undef;
> > +}
> > +
> > +sub check_connection ($class, $storeid, $scfg) {
> > +    my ($user, $host, $port) = $scfg->@{qw(username server port)};
> > +
> > +    my @common_opts = sshfs_common_ssh_opts($storeid);
> > +    my $cmd = [
> > +	'/usr/bin/ssh',
> > +	'-T',
> > +	@common_opts,
> > +	'-o', 'BatchMode=yes',
> > +	'-o', 'ConnectTimeout=5',
> > +    ];
> > +
> > +    push($cmd->@*, "-p", $port) if $port;
> > +    push($cmd->@*, "${user}\@${host}", 'exit 0');
> > +
> > +    eval {
> > +	run_command(
> > +	    $cmd,
> > +	    timeout => 10,
> > +	    errfunc => sub { warn "$_[0]\n"; },
> > +	);
> > +    };
> > +    if (my $err = $@) {
> > +	warn "$err";
> > +	return 0;
> > +    }
> > +
> > +    return 1;
> > +}
> > +
> > +sub activate_storage ($class, $storeid, $scfg, $cache) {
> > +    my $mountpoint = $scfg->{path};
> > +
> > +    if (!sshfs_is_mounted($scfg)) {
> > +	if ($scfg->{'create-base-path'} // 1) {
> > +	    make_path($mountpoint);
> > +	}
> > +
> > +	die "unable to activate storage '$storeid' - directory '$mountpoint' does not exist\n"
> > +	    if ! -d $mountpoint;
> > +
> > +	sshfs_mount($scfg, $storeid);
> > +    }
> > +
> > +    $class->SUPER::activate_storage($storeid, $scfg, $cache);
> > +    return;
> > +}
> > +
> > +sub get_volume_attribute ($class, $scfg, $storeid, $volname, $attribute) {
> > +    my ($vtype) = $class->parse_volname($volname);
> > +    return if $vtype ne 'backup';
> > +
> > +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> > +
> > +    if ($attribute eq 'notes') {
> > +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> > +
> > +	if (-f $notes_path) {
> > +	    my $notes = file_get_contents($notes_path);
> > +	    return eval { decode('UTF-8', $notes, 1) } // $notes;
> > +	}
> > +
> > +	return "";
> > +    }
> > +
> > +    if ($attribute eq 'protected') {
> > +	return -e PVE::Storage::protection_file_path($volume_path) ? 1 : 0;
> > +    }
> > +
> > +    return;
> > +}
> > +
> > +sub update_volume_attribute ($class, $scfg, $storeid, $volname, $attribute, $value) {
> > +    my ($vtype, $name) = $class->parse_volname($volname);
> > +    die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
> > +
> > +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> > +
> > +    if ($attribute eq 'notes') {
> > +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> > +
> > +	if (defined($value)) {
> > +	    my $encoded_notes = encode('UTF-8', $value);
> > +	    file_set_contents($notes_path, $encoded_notes);
> > +	} else {
> > +	    unlink $notes_path or $! == ENOENT or die "could not delete notes - $!\n";
> > +	}
> > +
> > +	return;
> > +    }
> > +
> > +    if ($attribute eq 'protected') {
> > +	my $protection_path = PVE::Storage::protection_file_path($volume_path);
> > +
> > +	# Protection already set or unset
> > +	return if !((-e $protection_path) xor $value);
> > +
> > +	if ($value) {
> > +	    my $fh = IO::File->new($protection_path, O_CREAT, 0644)
> > +		or die "unable to create protection file '$protection_path' - $!\n";
> > +	    close($fh);
> > +	} else {
> > +	    unlink $protection_path or $! == ENOENT
> > +		or die "could not delete protection file '$protection_path' - $!\n";
> > +	}
> > +
> > +	return;
> > +    }
> > +
> > +    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> > +}
> > +
> > +1;



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

Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Max Carrara]

On Fri Jul 4, 2025 at 2:33 PM CEST, Max Carrara wrote:
> On Wed Jul 2, 2025 at 10:14 PM CEST, Thomas Lamprecht wrote:
> > Am 16.04.25 um 14:47 schrieb Max Carrara:
> > > This commit adds an example implementation of a custom storage plugin
> > > that uses SSHFS [0] as the underlying filesystem.
> > > 
> > > The implementation is very similar to that of the NFS plugin; as a
> > > prerequisite, it is currently necessary to use pubkey auth and have
> > > the host's root user's public key deployed to the remote host like so:
> > > 
> > >   ssh-copy-id -i ~/.ssh/id_my_private_key \
> > >     -o UserKnownHostsFile=/etc/pve/priv/known_hosts [USER]@[HOST]
> > > 
> > > Then, the storage can be added as follows:
> > > 
> > >   pvesm add sshfs [STOREID] \
> > >     --username [USER] \
> > >     --server [HOST] \
> > >     --sshfs-remote-path [ABS PATH ON REMOTE] \
> > >     --path /mnt/path/to/storage \
> > >     --sshfs-private-key ~/.ssh/id_my_private_key
> > > 
> > > If the host is part of a cluster, other nodes may connect to the
> > > remote without any additional setup required. This is because we copy
> > > the private key to `/etc/pve/priv/storage/$KEYNAME.key` and use the
> > > cluster-wide `/etc/pve/priv/known_hosts` file. Also mark each SSHFS
> > > storage as `shared` by default in order to make use of this.
> > > 
> > > Note: Because there's currently no way to officially and permanently
> > > mark a storage as shared (like some built-in plugins [1]) set
> > > `$scfg->{shared} = 1;` in `on_add_hook`. This has almost the same
> > > effect as modifying `@PVE::Storage::Plugin::SHARED_STORAGE` directly,
> > > except that the `shared` is written to `/etc/pve/storage.cfg`.
> > > 
> > > [0]: https://github.com/libfuse/sshfs
> > > [1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4e16420f667f196e8eb99ae7c9f3f1d3e13791fb;hb=refs/heads/master#l37
> >
> > not a fully complete review but some comments inline, and there is now also a
> > new repo that would be a great fit for hosting this example:
> >
> > https://git.proxmox.com/?p=pve-storage-plugin-examples.git;a=summary
>
> Thanks for the pointer; v2 will be for that repo then!
>
> >
> > > 
> > > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > > ---
> > > Changes rfc-v1 --> v1:
> > >   * rework most of the plugin
> > >   * cease to call methods of DirPlugin (plugins should be isolated;
> > >     we don't want to encourage third-party devs to do that)
> > >   * handle SSH private key as sensitive property and place it on pmxcfs
> > >   * make storage shared
> > >   * manually implement attribute handling ("notes", "protected" for
> > >     backups)
> > > 
> > >  .../lib/PVE/Storage/Custom/SSHFSPlugin.pm     | 398 ++++++++++++++++++
> > >  1 file changed, 398 insertions(+)
> > >  create mode 100644 example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> > > 
> > > diff --git a/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> > > new file mode 100644
> > > index 0000000..75b29c1
> > > --- /dev/null
> > > +++ b/example/sshfs-plugin/lib/PVE/Storage/Custom/SSHFSPlugin.pm
> > > @@ -0,0 +1,398 @@
> > > +package PVE::Storage::Custom::SSHFSPlugin;
> > > +
> > > +use strict;
> > > +use warnings;
> > > +
> > > +use feature 'signatures';
> >
> > FYI: you can replace above four lines with:
> >
> > use v5.36;
> >
> > As that version implies strict, warnings and enables the signatures feature.
>
> Oh, that's nice. Will change the above to that in v2.
>
> >
> > > +
> > > +use Cwd qw();
> > > +use Encode qw(decode encode);
> > > +use File::Path qw(make_path);
> > > +use File::Basename qw(dirname);
> > > +use IO::File;
> > > +use POSIX;
> >
> > The POSIX module exports a lot of stuff by default, even overriding some existing
> > perl symbols IIRC, while most of the time one just needs some error constants or
> > format time printing or the like, so explicitly importing what one uses is almost
> > always the safer choice. There are some export groups for sets of exports, so that
> > one can e.g. import qw(:errno_h) instead of every error number constant explicitly.
>
> Oh good point, I hadn't considered that the POSIX module imports that
> much.
>
> >
> > > +
> > > +use PVE::ProcFSTools;
> > > +use PVE::Tools qw(
> > > +    file_copy
> > > +    file_get_contents
> > > +    file_set_contents
> > > +    run_command
> > > +);
> > > +
> > > +use base qw(PVE::Storage::Plugin);
> > > +
> > > +my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";
> >
> > For intra-cluster ssh this shared known_host file is being deprecated in
> > favor of using a per-node file, i.e. /etc/pve/nodes/NODENAME/ssh_known_hosts,
> > it might be better to not re-use that here and rather use a dedicated file
> > for this storage or directly encode it in the config entry (either passed by
> > the user or TOFU on storage addition).
>
> Do you mean a dedicated file for the entire storage plugin or one for
> each configured SSHFS storage? Asking just in case because the
> terminology can sometimes be a bit ambiguous.
>
> I'm not sure if we should encode it in the config entry though as
> there's no way to pass a single known_hosts entry directly
> to `ssh` / `sshfs` AFAIK.
>
> >
> > > +
> > > +# Plugin Definition
> > > +
> >
> > > +sub properties {
> > > +    return {
> > > +	'sshfs-remote-path' => {
> > > +	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
> > > +	    type => 'string',
> > > +	    format => 'pve-storage-path',
> > > +	},
> > > +	'sshfs-private-key' => {
> > > +	    description => "Path to the private key to use for SSHFS.",
> > > +	    type => 'string',
> > > +	    format => 'pve-storage-path',
> > > +	}
> >
> > We normally do not add the storage type as prefix for properties.
> > FWIW, we could treat the private-key like a password and manage it ourselves
>
> Yeah this was more catered towards external developers; my idea here was
> that prefixing the property name in some way (doesn't necessarily need
> to be the storage type) helps avoiding conflicts.
>
> Specifically, if a plugin introduces a property that already exists it
> will `die` on the call to `->init()`.
>
> While the chance is low that users will install many different plugins
> all at once (I assume), I've seen some plugins introduce properties with
> names like "multipath", "apikey", "protocol", etc. that to me seem
> common enough to eventually cause breakage if we introduce properties
> with such names ourselves.
>
> >
> > > +    };
> > > +}
> > > +
> > > +sub options {
> > > +    return {
> > > +	disable => { optional => 1 },
> > > +	path => { fixed => 1 },
> > > +	'create-base-path' => { optional => 1 },
> > > +	content => { optional => 1 },
> > > +	'create-subdirs' => { optional => 1 },
> > > +	'content-dirs' => { optional => 1 },
> > > +	'prune-backups' => { optional => 1 },
> > > +	'max-protected-backups' => { optional => 1 },
> > > +	format => { optional => 1 },
> > > +	bwlimit => { optional => 1 },
> > > +	preallocation => { optional => 1 },
> > > +	nodes => { optional => 1 },
> > > +	shared => { optional => 1 },
> > > +
> > > +	# SSHFS Options
> > > +	username => {},
> > > +	server => {},
> > > +	'sshfs-remote-path' => {},
> > > +	port => { optional => 1 },
> > > +	'sshfs-private-key' => { optional => 1 },
> > > +   };
> > > +}
> > > +
> > > +# SSHFS Helpers
> > > +
> > > +my sub sshfs_remote_from_config: prototype($) ($scfg) {
> > > +    my ($user, $host, $remote_path) = $scfg->@{qw(username server sshfs-remote-path)};
> > > +    return "${user}\@${host}:${remote_path}";
> > > +}
> > > +
> > > +my sub sshfs_private_key_path: prototype($) ($storeid) {
> > > +    return "/etc/pve/priv/storage/$storeid.key";
> > > +}
> > > +
> > > +my sub sshfs_common_ssh_opts: prototype($) ($storeid) {
> > > +    my $private_key_path = sshfs_private_key_path($storeid);
> > > +
> > > +    my @common_opts = (
> > > +	'-o', "UserKnownHostsFile=${CLUSTER_KNOWN_HOSTS}",
> > > +	'-o', 'GlobalKnownHostsFile=none',
> > > +	'-o', "IdentityFile=${private_key_path}",
> > > +    );
> > > +
> > > +    return @common_opts;
> > > +}
> > > +
> > > +my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
> > > +    die "path of private key file not specified" if !defined($src_key_path);
> > > +    die "path of private key file does not exist" if ! -e $src_key_path;
> > > +    die "path of private key file does not point to a file" if ! -f $src_key_path;
> >
> > I mean, fine as this is more for better UX, but not really safe as there is
> > a TOCTOU race here. If we really want to make it safe(r) we probably should
> > not allow copying arbitrary files from the system and allow either passing a
> > existing key to use as value, not the best UX, but it's not _that_ worse then
> > the status quo IMO.
>
> Oh yeah you're right, I hadn't considered that there's a TOCTOU race
> here... I mean, I guess passing the key as value should work just fine
> then, the user could always just do something like e.g.
>
>     pvesm add sshfs sshfs-storage [...] --sshfs-private-key $(cat ~/.ssh/id_sshfs-storage)
>
> (Unless there's also something I'm overlooking here.)
>
> >
> > > +
> > > +    my $dest_key_path = sshfs_private_key_path($storeid);
> > > +
> > > +    my $dest_key_parent_dir = dirname($dest_key_path);
> > > +    if (! -e $dest_key_parent_dir) {
> > > +	make_path($dest_key_parent_dir, { chmod => 0700 });
> >
> > chmod is useless for pmxcfs, as it controls those file attributes itself. While
> > as is it shouldn't hurt, it might be confusing for devs basing off this plugin
> > and wanting some other mode (on another path) and then wonder why this suddenly
> > errors out; so maybe dropping the chmod and adding a comment that the file will
> > reside below /etc/pve/priv, of which  pmxcfs will always set the mode to 0700 for.
>
> ACK, will remove the mode and add a comment. Will also do that below
> for the call to `file_copy`.
>
> >
> > btw., does make_path even dies explicitly, or would you need to check the return
> > value?
>
> make_path will `die` on fatal errors, yes. All non-fatal errors will
> emit a warning.
>
> I double-checked just to be sure; `make_path` has only one "severe"
> error that becomes fatal if it isn't trapped.
> See: https://perldoc.perl.org/File::Path#ERROR-HANDLING
>
> Even though none of the non-fatal errors for `make_path` seem to apply
> in our case, I guess we could still use its trapping mechanism and
> always `die` on our end. That would probably be the safest option.
>
> >
> > > +    } else {
> > > +	die "'$dest_key_path' already exists" if -e $dest_key_path;
> >
> > Being able to override this on add might be nice though? And FWIW, there is
> > some code deduplication with writing this file in the on_update_hook that
> > might be avoidable I think.
>
> You mean leaving the user to choose whether to overwrite the private key
> or not? I mean, we could make the sshfs-private-key property optional,
> and then still `die` if the private key doesn't exist after the storage
> was added.

Small correction: The sshfs-private-key property is already optional.

>
> >
> > > +    }
> > > +
> > > +    file_copy($src_key_path, $dest_key_path, undef, 600);
> > > +
> > > +    return undef;
> > > +}
> > > +
> > > +my sub sshfs_remove_private_key: prototype($) ($storeid) {
> > > +    my $key_path = sshfs_private_key_path($storeid);
> > > +    unlink($key_path) or $! == ENOENT or die "failed to remove private key '$key_path' - $!\n";
> > > +
> > > +    return undef;
> > > +}
> > > +
> > > +my sub sshfs_is_mounted: prototype($) ($scfg) {
> > > +    my $remote = sshfs_remote_from_config($scfg);
> > > +
> > > +    my $mountpoint = Cwd::realpath($scfg->{path}); # Resolve symlinks
> > > +    return 0 if !defined($mountpoint);
> > > +
> > > +    my $mountdata = PVE::ProcFSTools::parse_proc_mounts();
> > > +
> > > +    my $has_found_mountpoint = grep {
> > > +	$_->[0] =~ m|^\Q${remote}\E$|
> > > +	&& $_->[1] eq $mountpoint
> > > +	&& $_->[2] eq 'fuse.sshfs'
> > > +    } $mountdata->@*;
> > > +
> > > +    return $has_found_mountpoint != 0;
> > > +}
> > > +
> > > +my sub sshfs_mount: prototype($$) ($scfg, $storeid) {
> > > +    my $remote = sshfs_remote_from_config($scfg);
> > > +    my ($port, $mountpoint) = $scfg->@{qw(port path)};
> > > +
> > > +    my @common_opts = sshfs_common_ssh_opts($storeid);
> > > +    my $cmd = [
> > > +	'/usr/bin/sshfs', @common_opts,
> > > +	'-o', 'noatime',
> > > +    ];
> > > +
> > > +    push($cmd->@*, '-p', $port) if $port;
> > > +    push($cmd->@*, $remote, $mountpoint);
> > > +
> > > +    eval {
> > > +	run_command(
> > > +	    $cmd,
> > > +	    timeout => 10,
> > > +	    errfunc => sub { warn "$_[0]\n"; },
> > > +	);
> > > +    };
> > > +    if (my $err = $@) {
> > > +	die "failed to mount SSHFS storage '$remote' at '$mountpoint': $@\n";
> > > +    }
> > > +
> > > +    die "SSHFS storage '$remote' not mounted at '$mountpoint' despite reported success\n"
> > > +	if ! sshfs_is_mounted($scfg);
> > > +
> > > +    return;
> > > +}
> > > +
> > > +my sub sshfs_umount: prototype($) ($scfg) {
> > > +    my $mountpoint = $scfg->{path};
> > > +
> > > +    my $cmd = ['/usr/bin/umount', $mountpoint];
> > > +
> > > +    eval {
> > > +	run_command(
> > > +	    $cmd,
> > > +	    timeout => 10,
> > > +	    errfunc => sub { warn "$_[0]\n"; },
> > > +	);
> > > +    };
> > > +    if (my $err = $@) {
> > > +	die "failed to unmount SSHFS at '$mountpoint': $err\n";
> > > +    }
> > > +
> > > +    return;
> > > +}
> > > +
> > > +# Storage Implementation
> > > +
> > > +sub on_add_hook ($class, $storeid, $scfg, %sensitive) {
> > > +    $scfg->{shared} = 1; # mark SSHFS storages as shared by default
> > > +
> > > +    eval {
> > > +	my $src_key_path = $sensitive{'sshfs-private-key'};
> > > +	sshfs_set_private_key($storeid, $src_key_path);
> > > +    };
> > > +    die "error while adding SSHFS storage '${storeid}': $@\n" if $@;
> > > +
> > > +    return undef;
> > > +}
> > > +
> > > +sub on_update_hook ($class, $storeid, $scfg, %sensitive) {
> > > +    return undef if !exists($sensitive{'sshfs-private-key'});
> > > +
> > > +    my $src_key_path = $sensitive{'sshfs-private-key'};
> > > +
> > > +    if (!defined($src_key_path)) {
> > > +	warn "removing private key for SSHFS storage '${storeid}'";
> > > +	warn "the storage might not be mountable without a private key!";
> > > +
> > > +	eval { sshfs_remove_private_key($storeid); };
> > > +	die $@ if $@;
> > > +
> > > +	return undef;
> > > +    }
> > > +
> > > +    my $dest_key_path = sshfs_private_key_path($storeid);
> > > +    my $dest_key_path_tmp = "${dest_key_path}.old";
> > > +
> > > +    file_copy($dest_key_path, $dest_key_path_tmp) if -e $dest_key_path;
> > > +
> > > +    eval { file_copy($src_key_path, $dest_key_path, undef, 600); };
> > > +
> > > +    if (my $err = $@) {
> > > +	if (-e $dest_key_path_tmp) {
> > > +	    warn "attempting to restore previous private key for storage '${storeid}'\n";
> > > +	    eval { file_copy($dest_key_path_tmp, $dest_key_path, undef, 600); };
> > > +	    warn "$@\n" if $@;
> > > +
> > > +	    unlink $dest_key_path_tmp;
> > > +	}
> > > +
> > > +	die "failed to set private key for SSHFS storage '${storeid}': $err\n";
> > > +    }
> > > +
> > > +    unlink $dest_key_path_tmp;
> > > +
> > > +    return undef;
> > > +}
> > > +
> > > +sub on_delete_hook ($class, $storeid, $scfg) {
> > > +    eval { sshfs_remove_private_key($storeid); };
> > > +    warn $@ if $@;
> > > +
> > > +    eval { sshfs_umount($scfg) if sshfs_is_mounted($scfg); };
> > > +    warn $@ if $@;
> > > +
> > > +    return undef;
> > > +}
> > > +
> > > +sub check_connection ($class, $storeid, $scfg) {
> > > +    my ($user, $host, $port) = $scfg->@{qw(username server port)};
> > > +
> > > +    my @common_opts = sshfs_common_ssh_opts($storeid);
> > > +    my $cmd = [
> > > +	'/usr/bin/ssh',
> > > +	'-T',
> > > +	@common_opts,
> > > +	'-o', 'BatchMode=yes',
> > > +	'-o', 'ConnectTimeout=5',
> > > +    ];
> > > +
> > > +    push($cmd->@*, "-p", $port) if $port;
> > > +    push($cmd->@*, "${user}\@${host}", 'exit 0');
> > > +
> > > +    eval {
> > > +	run_command(
> > > +	    $cmd,
> > > +	    timeout => 10,
> > > +	    errfunc => sub { warn "$_[0]\n"; },
> > > +	);
> > > +    };
> > > +    if (my $err = $@) {
> > > +	warn "$err";
> > > +	return 0;
> > > +    }
> > > +
> > > +    return 1;
> > > +}
> > > +
> > > +sub activate_storage ($class, $storeid, $scfg, $cache) {
> > > +    my $mountpoint = $scfg->{path};
> > > +
> > > +    if (!sshfs_is_mounted($scfg)) {
> > > +	if ($scfg->{'create-base-path'} // 1) {
> > > +	    make_path($mountpoint);
> > > +	}
> > > +
> > > +	die "unable to activate storage '$storeid' - directory '$mountpoint' does not exist\n"
> > > +	    if ! -d $mountpoint;
> > > +
> > > +	sshfs_mount($scfg, $storeid);
> > > +    }
> > > +
> > > +    $class->SUPER::activate_storage($storeid, $scfg, $cache);
> > > +    return;
> > > +}
> > > +
> > > +sub get_volume_attribute ($class, $scfg, $storeid, $volname, $attribute) {
> > > +    my ($vtype) = $class->parse_volname($volname);
> > > +    return if $vtype ne 'backup';
> > > +
> > > +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> > > +
> > > +    if ($attribute eq 'notes') {
> > > +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> > > +
> > > +	if (-f $notes_path) {
> > > +	    my $notes = file_get_contents($notes_path);
> > > +	    return eval { decode('UTF-8', $notes, 1) } // $notes;
> > > +	}
> > > +
> > > +	return "";
> > > +    }
> > > +
> > > +    if ($attribute eq 'protected') {
> > > +	return -e PVE::Storage::protection_file_path($volume_path) ? 1 : 0;
> > > +    }
> > > +
> > > +    return;
> > > +}
> > > +
> > > +sub update_volume_attribute ($class, $scfg, $storeid, $volname, $attribute, $value) {
> > > +    my ($vtype, $name) = $class->parse_volname($volname);
> > > +    die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
> > > +
> > > +    my $volume_path = PVE::Storage::Plugin->filesystem_path($scfg, $volname);
> > > +
> > > +    if ($attribute eq 'notes') {
> > > +	my $notes_path = $volume_path . $class->SUPER::NOTES_EXT;
> > > +
> > > +	if (defined($value)) {
> > > +	    my $encoded_notes = encode('UTF-8', $value);
> > > +	    file_set_contents($notes_path, $encoded_notes);
> > > +	} else {
> > > +	    unlink $notes_path or $! == ENOENT or die "could not delete notes - $!\n";
> > > +	}
> > > +
> > > +	return;
> > > +    }
> > > +
> > > +    if ($attribute eq 'protected') {
> > > +	my $protection_path = PVE::Storage::protection_file_path($volume_path);
> > > +
> > > +	# Protection already set or unset
> > > +	return if !((-e $protection_path) xor $value);
> > > +
> > > +	if ($value) {
> > > +	    my $fh = IO::File->new($protection_path, O_CREAT, 0644)
> > > +		or die "unable to create protection file '$protection_path' - $!\n";
> > > +	    close($fh);
> > > +	} else {
> > > +	    unlink $protection_path or $! == ENOENT
> > > +		or die "could not delete protection file '$protection_path' - $!\n";
> > > +	}
> > > +
> > > +	return;
> > > +    }
> > > +
> > > +    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> > > +}
> > > +
> > > +1;



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

Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Thomas Lamprecht]

Am 04.07.25 um 14:33 schrieb Max Carrara:
> On Wed Jul 2, 2025 at 10:14 PM CEST, Thomas Lamprecht wrote:
>> Am 16.04.25 um 14:47 schrieb Max Carrara:
>>> +my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";
>>
>> For intra-cluster ssh this shared known_host file is being deprecated in
>> favor of using a per-node file, i.e. /etc/pve/nodes/NODENAME/ssh_known_hosts,
>> it might be better to not re-use that here and rather use a dedicated file
>> for this storage or directly encode it in the config entry (either passed by
>> the user or TOFU on storage addition).
> 
> Do you mean a dedicated file for the entire storage plugin or one for
> each configured SSHFS storage? Asking just in case because the
> terminology can sometimes be a bit ambiguous.

We normally namespace such (secret) files with the storage ID included in the
file names, and I'd keep that here to cleanly partition different storage entries.

> I'm not sure if we should encode it in the config entry though as
> there's no way to pass a single known_hosts entry directly
> to `ssh` / `sshfs` AFAIK.

We could create an anonymous file, e.g. using open's O_TMPFILE flag.
memfd_create could be another alternative, but we do not expose that
through our syscall perl module yet, and O_TMPFILE should be enough
for this use case here.

> 
>>
>>> +
>>> +# Plugin Definition
>>> +
>>
>>> +sub properties {
>>> +    return {
>>> +	'sshfs-remote-path' => {
>>> +	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
>>> +	    type => 'string',
>>> +	    format => 'pve-storage-path',
>>> +	},
>>> +	'sshfs-private-key' => {
>>> +	    description => "Path to the private key to use for SSHFS.",
>>> +	    type => 'string',
>>> +	    format => 'pve-storage-path',
>>> +	}
>>
>> We normally do not add the storage type as prefix for properties.
>> FWIW, we could treat the private-key like a password and manage it ourselves
> 
> Yeah this was more catered towards external developers; my idea here was
> that prefixing the property name in some way (doesn't necessarily need
> to be the storage type) helps avoiding conflicts.
> 
> Specifically, if a plugin introduces a property that already exists it
> will `die` on the call to `->init()`.

Which does get detected with any trivial testing this though.
I checked a few external plugins and did not yet see any that do this,
so at least currently this doesn't seem a concern.

> While the chance is low that users will install many different plugins
> all at once (I assume), I've seen some plugins introduce properties with
> names like "multipath", "apikey", "protocol", etc. that to me seem
> common enough to eventually cause breakage if we introduce properties
> with such names ourselves.

IMO, in the midterm, it would be better to work towards enabling the
relatively new property_isolation from section config, e.g. with a major
release in the midterm, as that avoids this and other issues and UX,
and is a bit more explicit compared to rather subtle naming prefix,
which on its own won't signal the reasoning you mention here.
So while I get where you come from I'd avoid this here and rather have
something in the (yet to be extended) storage plugin dev docs/wiki.

>>> +my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
>>> +    die "path of private key file not specified" if !defined($src_key_path);
>>> +    die "path of private key file does not exist" if ! -e $src_key_path;
>>> +    die "path of private key file does not point to a file" if ! -f $src_key_path;
>>
>> I mean, fine as this is more for better UX, but not really safe as there is
>> a TOCTOU race here. If we really want to make it safe(r) we probably should
>> not allow copying arbitrary files from the system and allow either passing a
>> existing key to use as value, not the best UX, but it's not _that_ worse then
>> the status quo IMO.
>
> 
> Oh yeah you're right, I hadn't considered that there's a TOCTOU race
> here... I mean, I guess passing the key as value should work just fine
> then, the user could always just do something like e.g.
> 
>     pvesm add sshfs sshfs-storage [...] --sshfs-private-key $(cat ~/.ssh/id_sshfs-storage)
> 
> (Unless there's also something I'm overlooking here.)

FWIW, one reason I mentioned the TOCTOU was that the -e and -f check
together have some redundancy without gaining much. It won't make a
big difference if we or the shell read the file, if it's a path that
can be controlled by other, untrusted users/programs, that will never
be safe, as it leaks the key in any case and can be intercepted with
some timing luck.

That's why it probably also doesn't matter that much that it's not
perfect, root executes that command and we have to assume that they
chose a safe enough path, this is a private key after all.


>>> +    } else {
>>> +	die "'$dest_key_path' already exists" if -e $dest_key_path;
>>
>> Being able to override this on add might be nice though? And FWIW, there is
>> some code deduplication with writing this file in the on_update_hook that
>> might be avoidable I think.
> 
> You mean leaving the user to choose whether to overwrite the private key
> or not? I mean, we could make the sshfs-private-key property optional,
> and then still `die` if the private key doesn't exist after the storage
> was added.

I mean mostly being able to continue adding a storage after a botched
delete of a (SSHFS) storage with the same ID that had a file here that
wasn't cleaned up, as not allowing to override that if the storage ID
is free doesn't really gain the user anything IMO.



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

Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS

[Max Carrara]

On Fri Jul 4, 2025 at 3:30 PM CEST, Thomas Lamprecht wrote:
> Am 04.07.25 um 14:33 schrieb Max Carrara:
> > On Wed Jul 2, 2025 at 10:14 PM CEST, Thomas Lamprecht wrote:
> >> Am 16.04.25 um 14:47 schrieb Max Carrara:
> >>> +my $CLUSTER_KNOWN_HOSTS = "/etc/pve/priv/known_hosts";
> >>
> >> For intra-cluster ssh this shared known_host file is being deprecated in
> >> favor of using a per-node file, i.e. /etc/pve/nodes/NODENAME/ssh_known_hosts,
> >> it might be better to not re-use that here and rather use a dedicated file
> >> for this storage or directly encode it in the config entry (either passed by
> >> the user or TOFU on storage addition).
> > 
> > Do you mean a dedicated file for the entire storage plugin or one for
> > each configured SSHFS storage? Asking just in case because the
> > terminology can sometimes be a bit ambiguous.
>
> We normally namespace such (secret) files with the storage ID included in the
> file names, and I'd keep that here to cleanly partition different storage entries.
>
> > I'm not sure if we should encode it in the config entry though as
> > there's no way to pass a single known_hosts entry directly
> > to `ssh` / `sshfs` AFAIK.
>
> We could create an anonymous file, e.g. using open's O_TMPFILE flag.
> memfd_create could be another alternative, but we do not expose that
> through our syscall perl module yet, and O_TMPFILE should be enough
> for this use case here.
>
> > 
> >>
> >>> +
> >>> +# Plugin Definition
> >>> +
> >>
> >>> +sub properties {
> >>> +    return {
> >>> +	'sshfs-remote-path' => {
> >>> +	    description => "Path on the remote filesystem used for SSHFS. Must be absolute.",
> >>> +	    type => 'string',
> >>> +	    format => 'pve-storage-path',
> >>> +	},
> >>> +	'sshfs-private-key' => {
> >>> +	    description => "Path to the private key to use for SSHFS.",
> >>> +	    type => 'string',
> >>> +	    format => 'pve-storage-path',
> >>> +	}
> >>
> >> We normally do not add the storage type as prefix for properties.
> >> FWIW, we could treat the private-key like a password and manage it ourselves
> > 
> > Yeah this was more catered towards external developers; my idea here was
> > that prefixing the property name in some way (doesn't necessarily need
> > to be the storage type) helps avoiding conflicts.
> > 
> > Specifically, if a plugin introduces a property that already exists it
> > will `die` on the call to `->init()`.
>
> Which does get detected with any trivial testing this though.
> I checked a few external plugins and did not yet see any that do this,
> so at least currently this doesn't seem a concern.
>
> > While the chance is low that users will install many different plugins
> > all at once (I assume), I've seen some plugins introduce properties with
> > names like "multipath", "apikey", "protocol", etc. that to me seem
> > common enough to eventually cause breakage if we introduce properties
> > with such names ourselves.
>
> IMO, in the midterm, it would be better to work towards enabling the
> relatively new property_isolation from section config, e.g. with a major
> release in the midterm, as that avoids this and other issues and UX,
> and is a bit more explicit compared to rather subtle naming prefix,
> which on its own won't signal the reasoning you mention here.
> So while I get where you come from I'd avoid this here and rather have
> something in the (yet to be extended) storage plugin dev docs/wiki.

ACK--I understand now; thanks for elaborating.

(FWIW I do mention this in the upcoming docs; will send a draft soon
once the plugin's done. Will have to update the code snippets etc.)

FYI: While checking the other examples I noticed that we already define
a ssh-private-key property in one of them [1], so I'll just change
sshfs-remote-path to remote-path and leave the other one as it is ---
assuming that we're going to package that example.

I would normally be inclined to just merge those two properties and move
them into Plugin.pm, *but* I want to explicitly show how to declare a
sensitive property and use it in the docs; have the example be
self-sufficient, basically.

>
> >>> +my sub sshfs_set_private_key: prototype($$) ($storeid, $src_key_path) {
> >>> +    die "path of private key file not specified" if !defined($src_key_path);
> >>> +    die "path of private key file does not exist" if ! -e $src_key_path;
> >>> +    die "path of private key file does not point to a file" if ! -f $src_key_path;
> >>
> >> I mean, fine as this is more for better UX, but not really safe as there is
> >> a TOCTOU race here. If we really want to make it safe(r) we probably should
> >> not allow copying arbitrary files from the system and allow either passing a
> >> existing key to use as value, not the best UX, but it's not _that_ worse then
> >> the status quo IMO.
> >
> > 
> > Oh yeah you're right, I hadn't considered that there's a TOCTOU race
> > here... I mean, I guess passing the key as value should work just fine
> > then, the user could always just do something like e.g.
> > 
> >     pvesm add sshfs sshfs-storage [...] --sshfs-private-key $(cat ~/.ssh/id_sshfs-storage)
> > 
> > (Unless there's also something I'm overlooking here.)
>
> FWIW, one reason I mentioned the TOCTOU was that the -e and -f check
> together have some redundancy without gaining much. It won't make a
> big difference if we or the shell read the file, if it's a path that
> can be controlled by other, untrusted users/programs, that will never
> be safe, as it leaks the key in any case and can be intercepted with
> some timing luck.
>
> That's why it probably also doesn't matter that much that it's not
> perfect, root executes that command and we have to assume that they
> chose a safe enough path, this is a private key after all.

Ah okay, I see. I'll opt for passing the key as a value then instead of
as path, as we do that in another example too [1].

FWIW we could maybe also extend the handling of sensitive parameters to
the `pvesm` CLI, since we handle some of them explicitly there at the
moment [2]. So while the parameters aren't paths, `pvesm` expects some
of them to be provided as a path, and then reads from that file.

Probably also something for the midterm; just wanted to mention the idea
here before it fizzles away.

(Some more context: These parameters there used to be part of the set of
pre-defined params that were regarded as sensitive, before the
introduction of marking them as such in the `plugindata()` method, IIRC.)

>
>
> >>> +    } else {
> >>> +	die "'$dest_key_path' already exists" if -e $dest_key_path;
> >>
> >> Being able to override this on add might be nice though? And FWIW, there is
> >> some code deduplication with writing this file in the on_update_hook that
> >> might be avoidable I think.
> > 
> > You mean leaving the user to choose whether to overwrite the private key
> > or not? I mean, we could make the sshfs-private-key property optional,
> > and then still `die` if the private key doesn't exist after the storage
> > was added.
>
> I mean mostly being able to continue adding a storage after a botched
> delete of a (SSHFS) storage with the same ID that had a file here that
> wasn't cleaned up, as not allowing to override that if the storage ID
> is free doesn't really gain the user anything IMO.

Oh right, that makes sense. :P

[1]: https://git.proxmox.com/?p=pve-storage-plugin-examples.git;a=blob;f=backup-provider-borg/src/PVE/Storage/Custom/BorgBackupPlugin.pm;h=64fc24399fce6637129eb69af0b06e9fa8ea2d4d;hb=refs/heads/master#l315
[2]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/CLI/pvesm.pm;h=860e46f884bcf02ca5faa00bb13bea3f9d34ff90;hb=refs/heads/master#l35


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

[pve-devel] [PATCH v1 pve-storage 2/2] example: sshfs plugin: package SSHFSPlugin.pm

[Max Carrara]

Package the new SSHFS plugin separately from the libpve-storage-perl
source. This avoids mixing up the example packaging code with the
pre-existing one.

Note that the Makefile this commit adds currently isn't being called
by the top-level one in the repository root.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes rfc-v1 --> v1:
  * none

 example/sshfs-plugin/Makefile             | 71 +++++++++++++++++++++++
 example/sshfs-plugin/debian/changelog     |  5 ++
 example/sshfs-plugin/debian/control       | 22 +++++++
 example/sshfs-plugin/debian/copyright     | 21 +++++++
 example/sshfs-plugin/debian/rules         | 26 +++++++++
 example/sshfs-plugin/debian/source/format |  1 +
 example/sshfs-plugin/debian/triggers      |  1 +
 7 files changed, 147 insertions(+)
 create mode 100644 example/sshfs-plugin/Makefile
 create mode 100644 example/sshfs-plugin/debian/changelog
 create mode 100644 example/sshfs-plugin/debian/control
 create mode 100644 example/sshfs-plugin/debian/copyright
 create mode 100755 example/sshfs-plugin/debian/rules
 create mode 100644 example/sshfs-plugin/debian/source/format
 create mode 100644 example/sshfs-plugin/debian/triggers

diff --git a/example/sshfs-plugin/Makefile b/example/sshfs-plugin/Makefile
new file mode 100644
index 0000000..cbd0691
--- /dev/null
+++ b/example/sshfs-plugin/Makefile
@@ -0,0 +1,71 @@
+# Makes useful Debian-specific variables available
+include /usr/share/dpkg/default.mk
+
+# --- Useful variables for convenience
+
+# Note that variables can be overridden, e.g. `make install DESTDIR='./foo'`
+DESTDIR=
+PACKAGE=pve-storage-sshfs-plugin
+
+BINDIR=${DESTDIR}/usr/bin
+PERLLIBDIR=${DESTDIR}/usr/share/perl5
+MAN1DIR=${DESTDIR}/usr/share/man/man1
+MAN8DIR=${DESTDIR}/usr/share/man/man8
+CRONDAILYDIR=${DESTDIR}/etc/cron.daily
+INITDBINDIR=${DESTDIR}/etc/init.d
+SERVICEDIR=${DESTDIR}/lib/systemd/system
+BASHCOMPLDIR=${DESTDIR}/usr/share/bash-completion/completions/
+ZSHCOMPLDIR=${DESTDIR}/usr/share/zsh/vendor-completions/
+HARADIR=${DESTDIR}/usr/share/cluster
+DOCDIR=${DESTDIR}/usr/share/doc/${PACKAGE}
+PODDIR=${DESTDIR}/usr/share/doc/${PACKAGE}/pod
+USRSHARE=${DESTDIR}/usr/share/${PACKAGE}
+
+# Directory where our custom plugin has to go
+PLUGINDIR=${PERLLIBDIR}/PVE/Storage/Custom
+
+export VERSION = $(DEB_VERSION_UPSTREAM_REVISION)
+
+BUILDDIR = $(PACKAGE)-$(DEB_VERSION_UPSTREAM)
+
+DSC=$(PACKAGE)_$(DEB_VERSION).dsc
+DEB=$(PACKAGE)_$(DEB_VERSION)_$(DEB_HOST_ARCH).deb
+
+# --- Targets
+
+$(BUILDDIR):
+	rm -rf $@ $@.tmp
+	mkdir $@.tmp
+	rsync -a * $@.tmp
+	# You can add additional commands instead of this comment if you need to
+	# e.g. create additional files inside the build directory etc.
+	mv $@.tmp $@
+
+# Creates a .deb package for installation
+.PHONY: deb
+deb: $(DEB)
+$(DEB): $(BUILDDIR)
+	cd $(BUILDDIR); dpkg-buildpackage -b -us -uc
+	lintian $(DEB)
+
+# Creates a .dsc (Debian source) package
+.PHONY: dsc
+dsc:
+	rm -rf $(BUILDDIR) $(DSC)
+	$(MAKE) $(DSC)
+	lintian $(DSC)
+
+$(DSC): $(BUILDDIR)
+	cd $(BUILDDIR); dpkg-buildpackage -S -us -uc -d
+
+# Target used to place files and directories at expected locations, with expected permissions
+.PHONY: install
+install:
+	install -D -m 0644 lib/PVE/Storage/Custom/SSHFSPlugin.pm $(PLUGINDIR)/SSHFSPlugin.pm
+
+# Used to clean up your builds
+.PHONY: clean
+clean:
+	rm -f $(PACKAGE)*.tar* *.deb *.dsc *.build *.buildinfo *.changes
+	rm -rf dest $(PACKAGE)-[0-9]*/
+
diff --git a/example/sshfs-plugin/debian/changelog b/example/sshfs-plugin/debian/changelog
new file mode 100644
index 0000000..7372095
--- /dev/null
+++ b/example/sshfs-plugin/debian/changelog
@@ -0,0 +1,5 @@
+pve-storage-sshfs-plugin (1.0.0) UNRELEASED; urgency=medium
+
+  * Initial release.
+
+ -- Proxmox Support Team <support@proxmox.com>  Wed, 09 Apr 2025 16:13:26 +0200
diff --git a/example/sshfs-plugin/debian/control b/example/sshfs-plugin/debian/control
new file mode 100644
index 0000000..5732cd6
--- /dev/null
+++ b/example/sshfs-plugin/debian/control
@@ -0,0 +1,22 @@
+Source: pve-storage-sshfs-plugin
+Section: perl
+Priority: optional
+Maintainer: Proxmox Support Team <support@proxmox.com>
+Rules-Requires-Root: no
+Build-Depends:
+ debhelper-compat (= 13),
+ libpve-storage-perl (>= 8.3.3),
+ lintian,
+ perl,
+ rsync,
+Standards-Version: 4.6.2
+
+Package: pve-storage-sshfs-plugin
+Architecture: any
+Depends:
+ ${misc:Depends},
+ ${perl:Depends},
+ libpve-storage-perl (>= 8.3.3),
+ sshfs (>= 3.7.3),
+Description: SSHFS storage plugin for Proxmox Virtual Environment.
+ Used to demonstrate plugin development.
diff --git a/example/sshfs-plugin/debian/copyright b/example/sshfs-plugin/debian/copyright
new file mode 100644
index 0000000..c26ed02
--- /dev/null
+++ b/example/sshfs-plugin/debian/copyright
@@ -0,0 +1,21 @@
+Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
+Source: https://git.proxmox.com/?p=pve-storage.git
+
+Files:
+ *
+Copyright:
+ 2025 Proxmox Server Solutions GmbH <support@proxmox.com>
+License: AGPL-3.0+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU Affero General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU Affero General Public License for more details.
+
+ You should have received a copy of the GNU Affero General Public License
+ along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
diff --git a/example/sshfs-plugin/debian/rules b/example/sshfs-plugin/debian/rules
new file mode 100755
index 0000000..f1d1d25
--- /dev/null
+++ b/example/sshfs-plugin/debian/rules
@@ -0,0 +1,26 @@
+#!/usr/bin/make -f
+
+# See debhelper(7) (uncomment to enable).
+# Output every command that modifies files on the build system.
+#export DH_VERBOSE = 1
+
+
+# See FEATURE AREAS in dpkg-buildflags(1).
+#export DEB_BUILD_MAINT_OPTIONS = hardening=+all
+
+# See ENVIRONMENT in dpkg-buildflags(1).
+# Package maintainers to append CFLAGS.
+#export DEB_CFLAGS_MAINT_APPEND  = -Wall -pedantic
+# Package maintainers to append LDFLAGS.
+#export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
+
+
+%:
+	dh $@
+
+
+# dh_make generated override targets.
+# This is an example for Cmake (see <https://bugs.debian.org/641051>).
+#override_dh_auto_configure:
+#	dh_auto_configure -- \
+#	-DCMAKE_LIBRARY_PATH=$(DEB_HOST_MULTIARCH)
diff --git a/example/sshfs-plugin/debian/source/format b/example/sshfs-plugin/debian/source/format
new file mode 100644
index 0000000..89ae9db
--- /dev/null
+++ b/example/sshfs-plugin/debian/source/format
@@ -0,0 +1 @@
+3.0 (native)
diff --git a/example/sshfs-plugin/debian/triggers b/example/sshfs-plugin/debian/triggers
new file mode 100644
index 0000000..59dd688
--- /dev/null
+++ b/example/sshfs-plugin/debian/triggers
@@ -0,0 +1 @@
+activate-noawait pve-api-updates
-- 
2.39.5



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