From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 1764D1FF17A for ; Fri, 4 Jul 2025 16:22:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5578B39AF8; Fri, 4 Jul 2025 16:23:23 +0200 (CEST) Mime-Version: 1.0 Date: Fri, 04 Jul 2025 16:22:48 +0200 Message-Id: To: "Thomas Lamprecht" , "Proxmox VE development discussion" From: "Max Carrara" X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20250416124735.320256-1-m.carrara@proxmox.com> <20250416124735.320256-2-m.carrara@proxmox.com> <6016e664-5c81-4a32-a749-db807d4e56b0@proxmox.com> In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.032 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.218 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH v1 pve-storage 1/2] example: sshfs plugin: add custom storage plugin for SSHFS X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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