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
