Re: [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores

[Hannes Laimer <h.laimer@proxmox.com>]

On 11/26/24 16:12, Fabian Grünbichler wrote:
> two small (easily done as follow-up) nits below, otherwise:
> 
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> 
> On November 26, 2024 3:28 pm, Hannes Laimer wrote:
>> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> changes since v1:
>>   * config: create/delete: only check for Sys.Modify on /system/disks if
>>      removable
>>
>>   src/api2/admin/datastore.rs  | 12 +++++++++---
>>   src/api2/config/datastore.rs | 13 ++++++++++++-
>>   2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 1c939bc20..cae7eb89c 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -45,7 +45,7 @@ use pbs_api_types::{
>>       BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
>>       IGNORE_VERIFIED_BACKUPS_SCHEMA, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
>>       PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
>> -    PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID, UPID_SCHEMA,
>> +    PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, UPID, UPID_SCHEMA,
>>       VERIFICATION_OUTDATED_AFTER_SCHEMA,
>>   };
>>   use pbs_client::pxar::{create_tar, create_zip};
>> @@ -2512,7 +2512,10 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
>>           schema: UPID_SCHEMA,
>>       },
>>       access: {
>> -        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
>> +        permission: &Permission::And(&[
>> +            &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
> 
> so should we adapt this AUDIT to be MODIFY as well, so that this
> 

we can, my reasoning was that if we make this MODIFY there is basically
no "can only view" for removable datastores, being allowed to look at
something kind of implies being allowed to open it. But I get why MODIFY
would also make sense

>> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
>> +        ]),
>>       },
>>   )]
>>   /// Mount removable datastore.
>> @@ -2625,7 +2628,10 @@ fn do_unmount_device(
>>           schema: UPID_SCHEMA,
>>       },
>>       access: {
>> -        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
>> +        permission: &Permission::And(&[
>> +            &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> 
> and this lines up?
> 
>> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
>> +        ]),
>>       }
>>   )]
>>   /// Unmount a removable device that is associated with the datastore
>> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
>> index 121222c40..d23d7c455 100644
>> --- a/src/api2/config/datastore.rs
>> +++ b/src/api2/config/datastore.rs
>> @@ -14,7 +14,7 @@ use proxmox_uuid::Uuid;
>>   use pbs_api_types::{
>>       Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreNotify, DatastoreTuning, KeepOptions,
>>       MaintenanceMode, PruneJobConfig, PruneJobOptions, SyncDirection, DATASTORE_SCHEMA,
>> -    PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY,
>> +    PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY,
>>       PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
>>   };
>>   use pbs_config::BackupLockGuard;
>> @@ -204,6 +204,11 @@ pub fn create_datastore(
>>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>>   
>> +    if config.backing_device.is_some() {
>> +        let user_info = CachedUserInfo::new()?;
>> +        user_info.check_privs(&auth_id, &["system", "disks"], PRIV_SYS_MODIFY, false)?;
>> +    }
> 
> this might be added to the permissions description in the schema, so
> that it's contained in the api-viewer
> 
>> +
>>       let mut prune_job_config = None;
>>       if config.keep.keeps_something() || !has_prune_job(&config.name)? {
>>           prune_job_config = config.prune_schedule.as_ref().map(|schedule| {
>> @@ -580,6 +585,12 @@ pub async fn delete_datastore(
>>   
>>       let store_config: DataStoreConfig = config.lookup("datastore", &name)?;
>>   
>> +    if store_config.backing_device.is_some() {
>> +        let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>> +        let user_info = CachedUserInfo::new()?;
>> +        user_info.check_privs(&auth_id, &["system", "disks"], PRIV_SYS_MODIFY, false)?;
>> +    }
>> +
> 
> same here
> 
>>       if destroy_data && get_datastore_mount_status(&store_config) == Some(false) {
>>           http_bail!(
>>               BAD_REQUEST,
>> -- 
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> 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



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel