* [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores
@ 2024-11-26 14:28 Hannes Laimer
2024-11-26 15:12 ` Fabian Grünbichler
2024-11-26 15:25 ` [pbs-devel] applied: " Thomas Lamprecht
0 siblings, 2 replies; 4+ messages in thread
From: Hannes Laimer @ 2024-11-26 14:28 UTC (permalink / raw)
To: pbs-devel
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),
+ &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),
+ &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)?;
+ }
+
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)?;
+ }
+
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores
2024-11-26 14:28 [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores Hannes Laimer
@ 2024-11-26 15:12 ` Fabian Grünbichler
2024-11-26 15:20 ` Hannes Laimer
2024-11-26 15:25 ` [pbs-devel] applied: " Thomas Lamprecht
1 sibling, 1 reply; 4+ messages in thread
From: Fabian Grünbichler @ 2024-11-26 15:12 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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
> + &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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores
2024-11-26 15:12 ` Fabian Grünbichler
@ 2024-11-26 15:20 ` Hannes Laimer
0 siblings, 0 replies; 4+ messages in thread
From: Hannes Laimer @ 2024-11-26 15:20 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [pbs-devel] applied: [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores
2024-11-26 14:28 [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores Hannes Laimer
2024-11-26 15:12 ` Fabian Grünbichler
@ 2024-11-26 15:25 ` Thomas Lamprecht
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2024-11-26 15:25 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Hannes Laimer
Am 26.11.24 um 15:28 schrieb Hannes Laimer:
> 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(-)
>
>
applied, with subject slightly improved and the descriptions for the dynamic
access checks squashed in, thanks!
You can send Fabian's other suggestion to switch to Datastore.Modify as
follow-up.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-11-26 15:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-26 14:28 [pbs-devel] [PATCH proxmox-backup v2] api: add Sys.Modify on /system/disks as permission to endpoints handling removable datastores Hannes Laimer
2024-11-26 15:12 ` Fabian Grünbichler
2024-11-26 15:20 ` Hannes Laimer
2024-11-26 15:25 ` [pbs-devel] applied: " Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox