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