[pbs-devel] [PATCH proxmox-backup 0/5] removable datastore follow-up

[pbs-devel] [PATCH proxmox-backup 0/5] removable datastore follow-up

[Hannes Laimer]

Some follow-ups for removable datastore, including
 * fix sync with older pbs versions
 * add Sys.Modify permission for endpoints
 * add docs
 * allow resetting 'unmount' maintenance mode trough API/CLI
    last patch also allows it through UI, just put it in, in case
    we want to allow that 


Hannes Laimer (5):
  api: mainatenance: allow setting of maintenance mode if 'unmounting'
  api: add Sys.Modify on /system/disks as permission to endpoints
    handling removable datastores
  api: types: add 'mount_status' to schema
  docs: add information for removable datastores
  ui: allow resetting unmounting maintenance

 docs/storage.rst                 | 31 ++++++++++++++++++++++++++-----
 pbs-api-types/src/datastore.rs   | 10 +++++++++-
 src/api2/admin/datastore.rs      | 12 +++++++++---
 src/api2/config/datastore.rs     | 12 +++++++++---
 www/window/MaintenanceOptions.js |  3 +--
 5 files changed, 54 insertions(+), 14 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 1/5] api: mainatenance: allow setting of maintenance mode if 'unmounting'

[Hannes Laimer]

So it is possible to reset it after a failed unmount, or abort an
unmount task by restting it through the API.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 4927f3724..203e75e38 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -416,7 +416,7 @@ impl DataStoreConfig {
             Some(MaintenanceType::ReadOnly) => { /* always OK  */ }
             Some(MaintenanceType::Offline) => { /* always OK  */ }
             Some(MaintenanceType::Unmount) => {
-                bail!("datastore is being unmounted");
+                /* used to reset it after failed unmount, or alternative for aborting unmount task */
             }
             Some(MaintenanceType::Delete) => {
                 match new_type {
-- 
2.39.5



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

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

[Hannes Laimer]

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/admin/datastore.rs  | 12 +++++++++---
 src/api2/config/datastore.rs | 12 +++++++++---
 2 files changed, 18 insertions(+), 6 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..359b676a5 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;
@@ -173,7 +173,10 @@ pub(crate) fn do_create_datastore(
         },
     },
     access: {
-        permission: &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
+        permission: &Permission::And(&[
+            &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
+            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
+        ]),
     },
 )]
 /// Create new datastore config.
@@ -551,7 +554,10 @@ pub fn update_datastore(
         },
     },
     access: {
-        permission: &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
+        permission: &Permission::And(&[
+            &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
+            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
+        ]),
     },
     returns: {
         schema: UPID_SCHEMA,
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 3/5] api: types: add 'mount_status' to schema

[Hannes Laimer]

... and deserialize with default if field is missing in data.

Reported-by: Aaron Lauterer <a.lauterer@proxmox.com>
Fixes: 76609915d6 ("pbs-api-types: add mount_status field to DataStoreListItem")
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 203e75e38..90f1195bf 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -452,6 +452,9 @@ impl DataStoreConfig {
             optional: true,
             schema: SINGLE_LINE_COMMENT_SCHEMA,
         },
+        "mount-status": {
+            type: DataStoreMountStatus,
+        },
         maintenance: {
             optional: true,
             format: &ApiStringFormat::PropertyString(&MaintenanceMode::API_SCHEMA),
@@ -465,6 +468,7 @@ impl DataStoreConfig {
 pub struct DataStoreListItem {
     pub store: String,
     pub comment: Option<String>,
+    #[serde(default)]
     pub mount_status: DataStoreMountStatus,
     /// If the datastore is in maintenance mode, information about it
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -1447,6 +1451,9 @@ pub struct DataStoreStatus {
         store: {
             schema: DATASTORE_SCHEMA,
         },
+        "mount-status": {
+            type: DataStoreMountStatus,
+        },
         history: {
             type: Array,
             optional: true,
@@ -1471,6 +1478,7 @@ pub struct DataStoreStatusListItem {
     /// The available bytes of the underlying storage. (-1 on error)
     #[serde(skip_serializing_if = "Option::is_none")]
     pub avail: Option<u64>,
+    #[serde(default)]
     pub mount_status: DataStoreMountStatus,
     /// A list of usages of the past (last Month).
     #[serde(skip_serializing_if = "Option::is_none")]
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 4/5] docs: add information for removable datastores

[Hannes Laimer]

Specifically about jobs and how they behave when the datastore is not
mounted, how to create and use deivices with multiple datatstores on
multiple PBS instances and options how to handle failed unmounts.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 docs/storage.rst | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/docs/storage.rst b/docs/storage.rst
index 361af4420..5cd8704c4 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -176,16 +176,32 @@ datastores, should be either ``ext4`` or ``xfs``.  It is also possible to create
 on completely unused disks through "Administration" > "Disks / Storage" > "Directory",
 using this method the disk will be partitioned and formatted automatically for the datastore.
 
-Devices with only one datastore on them will be mounted automatically. It is possible to create a
-removable datastore on one PBS and use it on multiple instances, the device just has to be added
-on each instance as a removable datastore by checking "reuse datastore" on creation.
-If the device already contains a datastore at the specified path it'll just be added as
-a new datastore to the PBS instance and will be mounted whenever plugged in. Unmounting has
+Devices with only one datastore on them will be mounted automatically. Unmounting has
 to be done through the UI by clicking "Unmount" on the summary page or using the CLI.
+If unmounting should fail, the reason is logged in the unmount-task, and the datastore
+will stay in maintenance mode ``unmounting``, which prevents any IO operations. If that should
+happen, the maintenace mode has to be reset manually using:
+
+.. code-block:: console
+
+  # proxmox-backup-manager datastore update --maintenance-mode offline
+
+to prevent any IO, or to clear it use:
+
+.. code-block:: console
+
+  # proxmox-backup-manager datastore update --delete maintenance-mode
+
 
 A single device can house multiple datastores, they only limitation is that they are not
 allowed to be nested.
 
+Removable datastores are created on the the device with the given relative path that is specified
+on creation. In order to use a datastore on multiple PBS instances, it has to be created on one,
+and added with ``Reuse existing datastore`` checked on the others. The path you set on creation
+is how multiple datastores on a signle device are identified. So When adding on a new PBS instance,
+it has to match what was set on creation.
+
 .. code-block:: console
 
   # proxmox-backup-manager datastore unmount store1
@@ -202,6 +218,11 @@ All datastores present on a device can be listed using ``proxmox-backup-debug``.
   # proxmox-backup-debug inspect device /dev/...
 
 
+Verify jobs are skipped if the removable datastore should not be mounted when they are scheduled,
+Sync jobs start, but fail with an error saying the datastore was not mounted. The reason is that
+syncs not happening as schduled should at least be noticable. GC and pruning, like verification,
+is skipped without a failed task if the datastore should not be mounted.
+
 
 Managing Datastores
 ^^^^^^^^^^^^^^^^^^^
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 5/5] ui: allow resetting unmounting maintenance

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
optional, just added it in case we want it

 www/window/MaintenanceOptions.js | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
index 896d6a58e..8a88ba2b7 100644
--- a/www/window/MaintenanceOptions.js
+++ b/www/window/MaintenanceOptions.js
@@ -89,12 +89,11 @@ Ext.define('PBS.window.MaintenanceOptions', {
         let unmounting = options['maintenance-type'] === 'unmount';
         let defaultType = options['maintenance-type'] === '__default__';
         if (unmounting) {
-            options['maintenance-type'] = '';
+            options['maintenance-type'] = gettext('Unmounting');
         }
 
 	me.callParent([options]);
 
-        me.lookupReference('type-field').setDisabled(unmounting);
         me.lookupReference('message-field').setDisabled(unmounting || defaultType);
     },
 });
-- 
2.39.5



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

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

[Fabian Grünbichler]

On November 26, 2024 12:43 pm, Hannes Laimer wrote:
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/admin/datastore.rs  | 12 +++++++++---
>  src/api2/config/datastore.rs | 12 +++++++++---
>  2 files changed, 18 insertions(+), 6 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)
> +        ]),

I am not 100% sure this part should require Sys.Modify.. somebody needs
to have set up the datastore already, just mounting seems benign in that
case?

>      },
>  )]
>  /// 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)
> +        ]),

same logic would apply here..

>      }
>  )]
>  /// 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..359b676a5 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;
> @@ -173,7 +173,10 @@ pub(crate) fn do_create_datastore(
>          },
>      },
>      access: {
> -        permission: &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
> +        permission: &Permission::And(&[
> +            &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
> +        ]),

this now affects regular datastores as well, it should probably be
inside the API handler and conditionalized on backing_device being set?

>      },
>  )]
>  /// Create new datastore config.
> @@ -551,7 +554,10 @@ pub fn update_datastore(
>          },
>      },
>      access: {
> -        permission: &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
> +        permission: &Permission::And(&[
> +            &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
> +        ]),

and this is not needed at all, since path and backing_device are fixed
after creation?

>      },
>      returns: {
>          schema: UPID_SCHEMA,
> -- 
> 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] partially applied: [PATCH proxmox-backup 0/5] removable datastore follow-up

[Fabian Grünbichler]

all but patch #2 and #5 and with a small follow-up for the docs patch
and some typos in commit one fixed up ;)

On November 26, 2024 12:43 pm, Hannes Laimer wrote:
> Some follow-ups for removable datastore, including
>  * fix sync with older pbs versions
>  * add Sys.Modify permission for endpoints
>  * add docs
>  * allow resetting 'unmount' maintenance mode trough API/CLI
>     last patch also allows it through UI, just put it in, in case
>     we want to allow that 
> 
> 
> Hannes Laimer (5):
>   api: mainatenance: allow setting of maintenance mode if 'unmounting'
>   api: add Sys.Modify on /system/disks as permission to endpoints
>     handling removable datastores
>   api: types: add 'mount_status' to schema
>   docs: add information for removable datastores
>   ui: allow resetting unmounting maintenance
> 
>  docs/storage.rst                 | 31 ++++++++++++++++++++++++++-----
>  pbs-api-types/src/datastore.rs   | 10 +++++++++-
>  src/api2/admin/datastore.rs      | 12 +++++++++---
>  src/api2/config/datastore.rs     | 12 +++++++++---
>  www/window/MaintenanceOptions.js |  3 +--
>  5 files changed, 54 insertions(+), 14 deletions(-)
> 
> -- 
> 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

Re: [pbs-devel] [PATCH proxmox-backup 5/5] ui: allow resetting unmounting maintenance

[Fabian Grünbichler]

Acked-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

but not applied in case somebody else wants to keep this away from the
UI for now ;)

On November 26, 2024 12:43 pm, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> optional, just added it in case we want it
> 
>  www/window/MaintenanceOptions.js | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
> index 896d6a58e..8a88ba2b7 100644
> --- a/www/window/MaintenanceOptions.js
> +++ b/www/window/MaintenanceOptions.js
> @@ -89,12 +89,11 @@ Ext.define('PBS.window.MaintenanceOptions', {
>          let unmounting = options['maintenance-type'] === 'unmount';
>          let defaultType = options['maintenance-type'] === '__default__';
>          if (unmounting) {
> -            options['maintenance-type'] = '';
> +            options['maintenance-type'] = gettext('Unmounting');
>          }
>  
>  	me.callParent([options]);
>  
> -        me.lookupReference('type-field').setDisabled(unmounting);
>          me.lookupReference('message-field').setDisabled(unmounting || defaultType);
>      },
>  });
> -- 
> 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

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

[Thomas Lamprecht]

This is missing a commit message explaining the rationale.

Am 26.11.24 um 13:07 schrieb Fabian Grünbichler:
>> @@ -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)
>> +        ]),
> I am not 100% sure this part should require Sys.Modify.. somebody needs
> to have set up the datastore already, just mounting seems benign in that
> case?

Mounting is always a bit of an involved operation as it can result in
IO hangs, just requiring audit on the store seems IMO rather to low of a
requirement. The Audit privs are not for things that alter the system state,
but rather for pure observation. Sys.Modify might not be ideal but IMO
definitively better than the status quo, "Datastore.Modify" might be fine too
though.

>>      },
>>  )]
>>  /// 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)
>> +        ]),
> same logic would apply here..
> 
>>      }
>>  )]
>>  /// Unmount a removable device that is associated with the datastore

here the status quo requires "Datastore.Modify", which is better, but if we
go for Sys.Modify above I'd not have any objection to use it also here.


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

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

[Hannes Laimer]

On 11/26/24 13:07, Fabian Grünbichler wrote:
> On November 26, 2024 12:43 pm, Hannes Laimer wrote:
>> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs  | 12 +++++++++---
>>   src/api2/config/datastore.rs | 12 +++++++++---
>>   2 files changed, 18 insertions(+), 6 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)
>> +        ]),
> 
> I am not 100% sure this part should require Sys.Modify.. somebody needs
> to have set up the datastore already, just mounting seems benign in that
> case?
> 
>>       },
>>   )]
>>   /// 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)
>> +        ]),
> 
> same logic would apply here..
> 
>>       }
>>   )]
>>   /// 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..359b676a5 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;
>> @@ -173,7 +173,10 @@ pub(crate) fn do_create_datastore(
>>           },
>>       },
>>       access: {
>> -        permission: &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
>> +        permission: &Permission::And(&[
>> +            &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
>> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
>> +        ]),
> 
> this now affects regular datastores as well, it should probably be
> inside the API handler and conditionalized on backing_device being set?
> 
>>       },
>>   )]
>>   /// Create new datastore config.
>> @@ -551,7 +554,10 @@ pub fn update_datastore(
>>           },
>>       },
>>       access: {
>> -        permission: &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
>> +        permission: &Permission::And(&[
>> +            &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
>> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
>> +        ]),
> 
> and this is not needed at all, since path and backing_device are fixed
> after creation?
> 

not sure why git diff shows `update_datastore` this is for the delete
endpoint. But I'll chnage that to only check when it is actually
removable(as above)
>>       },
>>       returns: {
>>           schema: UPID_SCHEMA,
>> -- 
>> 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

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

[Fabian Grünbichler]

On November 26, 2024 2:53 pm, Hannes Laimer wrote:
> 
> 
> On 11/26/24 13:07, Fabian Grünbichler wrote:
>> On November 26, 2024 12:43 pm, Hannes Laimer wrote:
>>> @@ -551,7 +554,10 @@ pub fn update_datastore(
>>>           },
>>>       },
>>>       access: {
>>> -        permission: &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
>>> +        permission: &Permission::And(&[
>>> +            &Permission::Privilege(&["datastore", "{name}"], PRIV_DATASTORE_ALLOCATE, false),
>>> +            &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false)
>>> +        ]),
>> 
>> and this is not needed at all, since path and backing_device are fixed
>> after creation?
>> 
> 
> not sure why git diff shows `update_datastore` this is for the delete
> endpoint. But I'll chnage that to only check when it is actually
> removable(as above)

oh, missed that. yeah, for deletion one can argue that mirroring the
creation ACL checks makes sense..

I think the API macro often confuses `git diff/format-patch` and gets
the context wrong, not sure whether a different diff algorithm or other
settings might help?


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

[pbs-devel] applied: [PATCH proxmox-backup 5/5] ui: allow resetting unmounting maintenance

[Thomas Lamprecht]

Am 26.11.24 um 12:43 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> optional, just added it in case we want it
> 
>  www/window/MaintenanceOptions.js | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
>

applied, thanks!


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