public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations
@ 2024-09-10  7:08 Dominik Csapak
  2024-11-12 20:15 ` Thomas Lamprecht
  2024-11-13 11:38 ` Dominik Csapak
  0 siblings, 2 replies; 5+ messages in thread
From: Dominik Csapak @ 2024-09-10  7:08 UTC (permalink / raw)
  To: pbs-devel

instead, require 'Tape.Write' on '/tape' path.
This makes it possible for a TapeAdmin or TapeOperator to
format/remove/vault tapes, instead of just root@pam.

I opted for the path '/tape' since we don't have a dedicated acl
structure for single tapes, just '/tape/pool' (which does not apply
since not all tapes have to have a pool), '/tape/device' (which is
intended for drives/changers) and '/tape/jobs' (which is for jobs only).

Alternatively we could invent a new scheme for tape media, e.g.
'/tape/media' for this.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/media.rs | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs
index a7c8483a..a480ae17 100644
--- a/src/api2/tape/media.rs
+++ b/src/api2/tape/media.rs
@@ -9,7 +9,7 @@ use proxmox_uuid::Uuid;
 use pbs_api_types::{
     Authid, MediaContentEntry, MediaContentListFilter, MediaListEntry, MediaPoolConfig,
     MediaSetListEntry, MediaStatus, CHANGER_NAME_SCHEMA, MEDIA_LABEL_SCHEMA,
-    MEDIA_POOL_NAME_SCHEMA, MEDIA_UUID_SCHEMA, PRIV_TAPE_AUDIT, VAULT_NAME_SCHEMA,
+    MEDIA_POOL_NAME_SCHEMA, MEDIA_UUID_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_WRITE, VAULT_NAME_SCHEMA,
 };
 use pbs_config::CachedUserInfo;
 
@@ -305,6 +305,9 @@ pub async fn list_media(
             },
         },
     },
+    access: {
+        permission: &Permission::Privilege(&["tape"], PRIV_TAPE_WRITE, false),
+    },
 )]
 /// Change Tape location to vault (if given), or offline.
 pub fn move_tape(
@@ -362,6 +365,9 @@ pub fn move_tape(
             },
         },
     },
+    access: {
+        permission: &Permission::Privilege(&["tape"], PRIV_TAPE_WRITE, false),
+    },
 )]
 /// Destroy media (completely remove from database)
 pub fn destroy_media(
@@ -553,6 +559,9 @@ pub fn get_media_status(uuid: Uuid) -> Result<MediaStatus, Error> {
             },
         },
     },
+    access: {
+        permission: &Permission::Privilege(&["tape"], PRIV_TAPE_WRITE, false),
+    },
 )]
 /// Update media status (None, 'full', 'damaged' or 'retired')
 ///
-- 
2.39.2



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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations
  2024-09-10  7:08 [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations Dominik Csapak
@ 2024-11-12 20:15 ` Thomas Lamprecht
  2024-11-13  7:42   ` Dominik Csapak
  2024-11-13 11:38 ` Dominik Csapak
  1 sibling, 1 reply; 5+ messages in thread
From: Thomas Lamprecht @ 2024-11-12 20:15 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Dominik Csapak

Am 10.09.24 um 09:08 schrieb Dominik Csapak:
> instead, require 'Tape.Write' on '/tape' path.
> This makes it possible for a TapeAdmin or TapeOperator to
> format/remove/vault tapes, instead of just root@pam.
> 
> I opted for the path '/tape' since we don't have a dedicated acl
> structure for single tapes, just '/tape/pool' (which does not apply
> since not all tapes have to have a pool), '/tape/device' (which is
> intended for drives/changers) and '/tape/jobs' (which is for jobs only).
> 
> Alternatively we could invent a new scheme for tape media, e.g.
> '/tape/media' for this.

the path is fine, but why Tape.Write over Tape.Modify?

> Tape.Modify
>     Tape.Modify allows a user to modify the configuration of tape drives, changers and backups.

vs 

> Tape.Write
>     Tape.Write allows a user to write to a tape media.

The former might be a better fit here as these calls alter not only the tape
content, or?

Noticed because Hannes' recent patch already switched the move-tape one to
Tape.Modify,



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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations
  2024-11-12 20:15 ` Thomas Lamprecht
@ 2024-11-13  7:42   ` Dominik Csapak
  2024-11-13  8:25     ` Thomas Lamprecht
  0 siblings, 1 reply; 5+ messages in thread
From: Dominik Csapak @ 2024-11-13  7:42 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox Backup Server development discussion

On 11/12/24 21:15, Thomas Lamprecht wrote:
> Am 10.09.24 um 09:08 schrieb Dominik Csapak:
>> instead, require 'Tape.Write' on '/tape' path.
>> This makes it possible for a TapeAdmin or TapeOperator to
>> format/remove/vault tapes, instead of just root@pam.
>>
>> I opted for the path '/tape' since we don't have a dedicated acl
>> structure for single tapes, just '/tape/pool' (which does not apply
>> since not all tapes have to have a pool), '/tape/device' (which is
>> intended for drives/changers) and '/tape/jobs' (which is for jobs only).
>>
>> Alternatively we could invent a new scheme for tape media, e.g.
>> '/tape/media' for this.
> 
> the path is fine, but why Tape.Write over Tape.Modify?
> 
>> Tape.Modify
>>      Tape.Modify allows a user to modify the configuration of tape drives, changers and backups.
> 
> vs
> 
>> Tape.Write
>>      Tape.Write allows a user to write to a tape media.
> 
> The former might be a better fit here as these calls alter not only the tape
> content, or?
> 
> Noticed because Hannes' recent patch already switched the move-tape one to
> Tape.Modify,
> 


mhmm... not sure why i chose Tape.Write exactly, but IMHO looking at it again, Modify would probably
fit better for the 'update_media_status' (also fits better for the move-tape as Hannes rightly
noticed)

for destroy I'd be inclined to still use Write. While it does update the inventory
(it removes it from there), It's basically the inverse of 'format_media' (as
in, that inserts it into the inventory) which also uses Write.

I'd send a rebased version for using Write for destroy, and Modify for update status if that's
fine with you.


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations
  2024-11-13  7:42   ` Dominik Csapak
@ 2024-11-13  8:25     ` Thomas Lamprecht
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2024-11-13  8:25 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox Backup Server development discussion

Am 13.11.24 um 08:42 schrieb Dominik Csapak:
> for destroy I'd be inclined to still use Write. While it does update the inventory
> (it removes it from there), It's basically the inverse of 'format_media' (as
> in, that inserts it into the inventory) which also uses Write.

We could also require both, but no hard feelings here..

> 
> I'd send a rebased version for using Write for destroy, and Modify for update status if that's
> fine with you.

Yeah, that's fine by me.


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations
  2024-09-10  7:08 [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations Dominik Csapak
  2024-11-12 20:15 ` Thomas Lamprecht
@ 2024-11-13 11:38 ` Dominik Csapak
  1 sibling, 0 replies; 5+ messages in thread
From: Dominik Csapak @ 2024-11-13 11:38 UTC (permalink / raw)
  To: pbs-devel

sent a v2:
https://lore.proxmox.com/pbs-devel/20241113113742.2278769-1-d.csapak@proxmox.com/


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-11-13 11:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-10  7:08 [pbs-devel] [PATCH proxmox-backup] fix #5233: don't require root for some tape operations Dominik Csapak
2024-11-12 20:15 ` Thomas Lamprecht
2024-11-13  7:42   ` Dominik Csapak
2024-11-13  8:25     ` Thomas Lamprecht
2024-11-13 11:38 ` Dominik Csapak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal