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