From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 2B1A41FF166 for ; Wed, 28 Aug 2024 15:48:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D30ADAFF9; Wed, 28 Aug 2024 15:48:48 +0200 (CEST) Date: Wed, 28 Aug 2024 15:48:43 +0200 From: Wolfgang Bumiller To: Gabriel Goller Message-ID: References: <20240814085712.174810-1-g.goller@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240814085712.174810-1-g.goller@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.087 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.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_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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [zfs.rs, datastore.rs, directory.rs, proxmox.com] Subject: Re: [pbs-devel] [PATCH proxmox-backup v4 1/2] fix #5439: allow to reuse existing datastore X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Cc: pbs-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Wed, Aug 14, 2024 at 10:57:11AM GMT, Gabriel Goller wrote: > Disallow creating datastores in non-empty directories. Allow adding > existing datastores via a 'reuse-datastore' checkmark. This only checks > if all the necessary directories (.chunks + subdirectories and .lock) > exist and have the correct permissions. Note that the reuse-datastore > path does not open the datastore, so that we don't drop the > ProcessLocker of an existing datastore. > > Signed-off-by: Gabriel Goller > --- > v4, thanks @Thomas: > - move reuse-datastore checkbox to "advanced options" > > v3, thanks @Fabian: > - don't open chunkstore on existing datastore, as this drops the > previous ProcessLocker > - factor out `ChunkStore::open` checks and call them in reuse-datastore > path as well > > v2, thanks @Fabian: > - also check on frontend for root > - forbid datastore creation if dir not empty > - add reuse-datastore option > - verify chunkstore directories permissions and owners > > pbs-datastore/src/chunk_store.rs | 75 +++++++++++++++++++++++++++----- > src/api2/config/datastore.rs | 52 +++++++++++++++++----- > src/api2/node/disks/directory.rs | 4 +- > src/api2/node/disks/zfs.rs | 4 +- > 4 files changed, 112 insertions(+), 23 deletions(-) > > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs > index dd0061ea56ca..1a150fd92579 100644 > --- a/pbs-datastore/src/chunk_store.rs > +++ b/pbs-datastore/src/chunk_store.rs > @@ -156,27 +156,35 @@ impl ChunkStore { > lockfile_path > } > > + /// Check if the chunkstore path is absolute and that we can > + /// access it. Returns the absolute '.chunks' path on success. > + pub fn chunk_dir_accessible(base: &Path) -> Result { > + if !base.is_absolute() { > + bail!("expected absolute path - got {:?}", base); > + } > + > + let chunk_dir = Self::chunk_dir(base); > + > + if let Err(err) = std::fs::metadata(&chunk_dir) { > + bail!("unable to open chunk store at {chunk_dir:?} - {err}"); > + } > + > + Ok(chunk_dir) > + } > + > /// Opens the chunk store with a new process locker. > /// > /// Note that this must be used with care, as it's dangerous to create two instances on the > /// same base path, as closing the underlying ProcessLocker drops all locks from this process > /// on the lockfile (even if separate FDs) > - pub(crate) fn open>( > + pub fn open>( ^ This is not used and should be dropped. > name: &str, > base: P, > sync_level: DatastoreFSyncLevel, > ) -> Result { > let base: PathBuf = base.into(); > > - if !base.is_absolute() { > - bail!("expected absolute path - got {:?}", base); > - } > - > - let chunk_dir = Self::chunk_dir(&base); > - > - if let Err(err) = std::fs::metadata(&chunk_dir) { > - bail!("unable to open chunk store '{name}' at {chunk_dir:?} - {err}"); > - } > + let chunk_dir = ChunkStore::chunk_dir_accessible(&base)?; > > let lockfile_path = Self::lockfile_path(&base); > > @@ -561,6 +569,53 @@ impl ChunkStore { > // unwrap: only `None` in unit tests > ProcessLocker::try_exclusive_lock(self.locker.clone().unwrap()) > } > + > + /// Checks permissions and owner of passed path. > + fn check_permissions>(path: T, file_mode: u32) -> Result<(), Error> { > + match nix::sys::stat::stat(path.as_ref()) { > + Ok(stat) => { > + if stat.st_uid != u32::from(pbs_config::backup_user()?.uid) > + || stat.st_gid != u32::from(pbs_config::backup_group()?.gid) > + || stat.st_mode & 0o700 != file_mode Either be exact: st_mode != file_mode or only check the required bits: (st_mode & file_mode) != file_mode (This is one of those rare cases where I'd rather go with the first option. If users modified the permissions via the shell, they can just fix them up, too.) as your current code would for instance fail if the lock file had *more* permissions for the *user* (u+x) but would ignore more permissions for *others* (o+rwx or g+w). > + { > + bail!( > + "unable to open existing chunk store path {:?} - permissions or owner not correct", > + path.as_ref(), > + ); > + } > + } > + Err(err) => { > + bail!( > + "unable to open existing chunk store path {:?} - {err}", > + path.as_ref(), > + ); > + } > + } > + Ok(()) > + } > + > + /// Verify vital files in datastore. Checks the owner and permissions of: the chunkstore, it's > + /// subdirectories and the lock file. > + pub fn verify_chunkstore>(path: T) -> Result<(), Error> { > + // Check datastore root path perm/owner > + ChunkStore::check_permissions(path.as_ref(), 0o700)?; > + > + let chunk_dir = Self::chunk_dir(path.as_ref()); > + // Check datastore .chunks path perm/owner > + ChunkStore::check_permissions(&chunk_dir, 0o700)?; > + > + // Check all .chunks subdirectories > + for i in 0..64 * 1024 { > + let mut l1path = chunk_dir.clone(); > + l1path.push(format!("{:04x}", i)); > + ChunkStore::check_permissions(&l1path, 0o700)?; > + } > + > + // Check .lock file > + let lockfile_path = Self::lockfile_path(path.as_ref()); > + ChunkStore::check_permissions(lockfile_path, 0o600)?; > + Ok(()) > + } > } > > #[test] > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index ca6edf05acd1..120b20cf48df 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -1,7 +1,7 @@ > use std::path::PathBuf; > > use ::serde::{Deserialize, Serialize}; > -use anyhow::Error; > +use anyhow::{bail, Error}; > use hex::FromHex; > use serde_json::Value; > use tracing::warn; > @@ -70,21 +70,44 @@ pub(crate) fn do_create_datastore( > _lock: BackupLockGuard, > mut config: SectionConfigData, > datastore: DataStoreConfig, > + reuse_datastore: bool, > ) -> Result<(), Error> { > let path: PathBuf = datastore.path.clone().into(); > > + if path.parent().is_none() { > + bail!("cannot create datastore in root path"); > + } > + > let tuning: DatastoreTuning = serde_json::from_value( > DatastoreTuning::API_SCHEMA > .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?, > )?; > - let backup_user = pbs_config::backup_user()?; > - let _store = ChunkStore::create( > - &datastore.name, > - path, > - backup_user.uid, > - backup_user.gid, > - tuning.sync_level.unwrap_or_default(), > - )?; > + > + if reuse_datastore { > + ChunkStore::verify_chunkstore(&path)?; ^ If this is successful > + ChunkStore::chunk_dir_accessible(&path)?; ^ then this will be, too, no? I think this can just be dropped? Or am I missing something. The root check is already up there in this hunk, and other than that this just tries to call `fs::metadata()` which is also just a `stat()` which `verify_chunkstore()` already does. > + } else { > + let datastore_empty = std::fs::read_dir(path.clone()).map_or(true, |mut d| { > + d.all(|dir| { > + dir.map_or(false, |file| { > + file.file_name() > + .to_str() > + .map_or(false, |name| name.starts_with('.')) > + }) > + }) > + }); > + if !datastore_empty { > + bail!("path not empty!"); > + } > + let backup_user = pbs_config::backup_user()?; > + let _store = ChunkStore::create( > + &datastore.name, > + path, > + backup_user.uid, > + backup_user.gid, > + tuning.sync_level.unwrap_or_default(), > + )?; > + } > > config.set_data(&datastore.name, "datastore", &datastore)?; > > @@ -101,6 +124,12 @@ pub(crate) fn do_create_datastore( > type: DataStoreConfig, > flatten: true, > }, > + "reuse-datastore": { > + type: Boolean, > + optional: true, > + default: false, > + description: "Re-use existing datastore directory." > + } > }, > }, > access: { > @@ -110,6 +139,7 @@ pub(crate) fn do_create_datastore( > /// Create new datastore config. > pub fn create_datastore( > config: DataStoreConfig, > + reuse_datastore: bool, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result { > let lock = pbs_config::datastore::lock_config()?; > @@ -153,8 +183,8 @@ pub fn create_datastore( > Some(config.name.to_string()), > auth_id.to_string(), > to_stdout, > - move |_worker| { > - do_create_datastore(lock, section_config, config)?; > + move |worker| { `worker` is not used > + do_create_datastore(lock, section_config, config, reuse_datastore)?; > > if let Some(prune_job_config) = prune_job_config { > do_create_prune_job(prune_job_config) > diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs > index 2aa9571d7150..fe6dccaad57b 100644 > --- a/src/api2/node/disks/directory.rs > +++ b/src/api2/node/disks/directory.rs > @@ -213,7 +213,9 @@ pub fn create_datastore_disk( > bail!("datastore '{}' already exists.", datastore.name); > } > > - crate::api2::config::datastore::do_create_datastore(lock, config, datastore)?; > + crate::api2::config::datastore::do_create_datastore( > + lock, config, datastore, false, > + )?; > } > > Ok(()) > diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs > index 666fe3e82cfe..a31320c5451a 100644 > --- a/src/api2/node/disks/zfs.rs > +++ b/src/api2/node/disks/zfs.rs > @@ -313,7 +313,9 @@ pub fn create_zpool( > bail!("datastore '{}' already exists.", datastore.name); > } > > - crate::api2::config::datastore::do_create_datastore(lock, config, datastore)?; > + crate::api2::config::datastore::do_create_datastore( > + lock, config, datastore, false, > + )?; > } > > Ok(()) > -- > 2.39.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel