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 [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id B0CE21FF15E
for <inbox@lore.proxmox.com>; Tue, 25 Mar 2025 11:13:46 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 5B1D0616D;
Tue, 25 Mar 2025 11:13:42 +0100 (CET)
Mime-Version: 1.0
Date: Tue, 25 Mar 2025 11:13:09 +0100
Message-Id: <D8P9MR9B7MIV.182GHZ9TXWZOC@proxmox.com>
To: "Christian Ebner" <c.ebner@proxmox.com>, "Proxmox Backup Server
development discussion" <pbs-devel@lists.proxmox.com>
From: "Shannon Sterz" <s.sterz@proxmox.com>
X-Mailer: aerc 0.20.1-0-g2ecb8770224a-dirty
References: <20250324125157.165976-1-s.sterz@proxmox.com>
<20250324125157.165976-5-s.sterz@proxmox.com>
<5ace7ae9-232b-42d3-b48b-9c4b44bdbe45@proxmox.com>
In-Reply-To: <5ace7ae9-232b-42d3-b48b-9c4b44bdbe45@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.017 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
Subject: Re: [pbs-devel] [PATCH proxmox-backup v8 4/4] fix: api: avoid race
condition in set_backup_owner
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>
On Tue Mar 25, 2025 at 11:00 AM CET, Christian Ebner wrote:
> On 3/24/25 13:51, Shannon Sterz wrote:
>> when two clients change the owner of a backup store, a race condition
>> arose. add locking to avoid this.
>>
>> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>> ---
>> src/api2/admin/datastore.rs | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 483e595c..3e532636 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -7,7 +7,7 @@ use std::os::unix::ffi::OsStrExt;
>> use std::path::{Path, PathBuf};
>> use std::sync::Arc;
>>
>> -use anyhow::{bail, format_err, Error};
>> +use anyhow::{bail, format_err, Context, Error};
>> use futures::*;
>> use hyper::http::request::Parts;
>> use hyper::{header, Body, Response, StatusCode};
>> @@ -2347,10 +2347,9 @@ pub async fn set_backup_owner(
>> let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
>>
>> let backup_group = datastore.backup_group(ns, backup_group);
>> + let owner = backup_group.get_owner()?;
>
> this needs to read the content from the owner file
>
>>
>> if owner_check_required {
>> - let owner = backup_group.get_owner()?;
>> -
>> let allowed = match (owner.is_token(), new_owner.is_token()) {
>> (true, true) => {
>> // API token to API token, owned by same user
>> @@ -2397,6 +2396,14 @@ pub async fn set_backup_owner(
>> );
>> }
>>
>> + let _guard = backup_group
>> + .lock()
>> + .with_context(|| format!("while setting the owner of group '{backup_group:?}'"))?;
>> +
>> + if owner != backup_group.get_owner()? {
>
> same here.
>
>> + bail!("{owner} does not own this group anymore");
>> + }
>> +
>> backup_group.set_owner(&new_owner, true)?;
>>
>> Ok(())
>
> reading from the filesystem is more expensive than the ownership checks
> if required, so IMO it is better to lock the whole operation instead of
> reading the owner file twice. Or is this done to avoid locking over and
> over by unauthorized users?
yes that function has `Permission::Anybody` if you locked the file right
away, creating a dos attack by *any* logged in user for *any* group
would be trivial. getting the owner might be a bit more expensive, but
this way we can still handle multiple requests in parallel easily and
only hold the lock for the part that actually requires it. hence, the
dos potential is lower (imo) and group owners should not change *that*
often in normal operations. so the performance hit is acceptable.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel