[pbs-devel] [PATCH proxmox-backup v8 4/4] fix: api: avoid race condition in set_backup_owner

[Christian Ebner]

On 3/25/25 11:13, Shannon Sterz wrote:
> 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 at 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.

Acked, given that  information it makes sense to me. Maybe the commit 
message or a dedicated comment could explicitly mention this so this is 
not changed unintentionally.