From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 2AD2E1FF391
for <inbox@lore.proxmox.com>; Wed, 12 Jun 2024 15:23:00 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 03683117D1;
Wed, 12 Jun 2024 15:23:37 +0200 (CEST)
From: Gabriel Goller <g.goller@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Wed, 12 Jun 2024 15:22:56 +0200
Message-ID: <20240612132300.352392-1-g.goller@proxmox.com>
X-Mailer: git-send-email 2.43.0
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.059 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
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. [datastore.rs, zfs.rs, directory.rs]
Subject: [pbs-devel] [PATCH proxmox-backup v2 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
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<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" <pbs-devel-bounces@lists.proxmox.com>
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)
are existing and have the correct permissions, then opens the
datastore.
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
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 | 49 ++++++++++++++++++++++++++++-
src/api2/config/datastore.rs | 54 +++++++++++++++++++++++++-------
src/api2/node/disks/directory.rs | 1 +
src/api2/node/disks/zfs.rs | 1 +
4 files changed, 93 insertions(+), 12 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 9f6289c9..077bc316 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -164,7 +164,7 @@ impl ChunkStore {
/// 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>>(
name: &str,
base: P,
sync_level: DatastoreFSyncLevel,
@@ -563,6 +563,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
+ {
+ 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 6b742acb..f1e80b2d 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;
@@ -70,23 +70,48 @@ pub(crate) fn do_create_datastore(
_lock: BackupLockGuard,
mut config: SectionConfigData,
datastore: DataStoreConfig,
+ reuse_datastore: bool,
worker: Option<&dyn WorkerTaskContext>,
) -> Result<(), Error> {
let path: PathBuf = datastore.path.clone().into();
+ if path.parent().is_none() {
+ bail!("cannot create datastore in root path");
+ }
+
+ let datastore_empty = std::fs::read_dir(path.clone())?.all(|dir| {
+ dir.map_or(false, |file| {
+ file.file_name().to_str().map_or(false, |name| {
+ (name.starts_with('.')
+ && !(name == ".chunks" || name == ".lock" || name == ".gc-status"))
+ || name.starts_with("lost+found")
+ })
+ })
+ });
+
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,
- worker,
- tuning.sync_level.unwrap_or_default(),
- )?;
+
+ if !datastore_empty && !reuse_datastore {
+ bail!("Directory is not empty!");
+ } else if !datastore_empty && reuse_datastore {
+ ChunkStore::verify_chunkstore(&path)?;
+
+ let _store =
+ ChunkStore::open(&datastore.name, path, tuning.sync_level.unwrap_or_default())?;
+ } else {
+ let backup_user = pbs_config::backup_user()?;
+ let _store = ChunkStore::create(
+ &datastore.name,
+ path,
+ backup_user.uid,
+ backup_user.gid,
+ worker,
+ tuning.sync_level.unwrap_or_default(),
+ )?;
+ }
config.set_data(&datastore.name, "datastore", &datastore)?;
@@ -103,6 +128,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: {
@@ -112,6 +143,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()?;
@@ -156,7 +188,7 @@ pub fn create_datastore(
auth_id.to_string(),
to_stdout,
move |worker| {
- do_create_datastore(lock, section_config, config, Some(&worker))?;
+ do_create_datastore(lock, section_config, config, reuse_datastore, Some(&worker))?;
if let Some(prune_job_config) = prune_job_config {
do_create_prune_job(prune_job_config, Some(&worker))
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 9f1112a9..25cfe784 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -217,6 +217,7 @@ pub fn create_datastore_disk(
lock,
config,
datastore,
+ false,
Some(&worker),
)?;
}
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 673dc1bf..ad2129fc 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -323,6 +323,7 @@ pub fn create_zpool(
lock,
config,
datastore,
+ false,
Some(&worker),
)?;
}
--
2.43.0
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel