Re: [pbs-devel] [PATCH proxmox-backup v4 1/2] fix #5439: allow to reuse existing datastore

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <g.goller@proxmox.com>
> ---
> 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<PathBuf, Error> {
> +        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<P: Into<PathBuf>>(
> +    pub fn open<P: Into<PathBuf>>(

^ This is not used and should be dropped.

>          name: &str,
>          base: P,
>          sync_level: DatastoreFSyncLevel,
>      ) -> Result<Self, Error> {
>          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<T: AsRef<Path>>(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<T: AsRef<Path>>(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<String, Error> {
>      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