* [pve-devel] applied: [PATCH 1/2] disk api: avoid using unrelated Datastore.Audit priv for disk management
@ 2023-06-17 13:32 Thomas Lamprecht
2023-06-17 13:32 ` [pve-devel] applied: [PATCH 2/2] disk api: only ask for Datastore.Allocate if adding to storage config Thomas Lamprecht
0 siblings, 1 reply; 2+ messages in thread
From: Thomas Lamprecht @ 2023-06-17 13:32 UTC (permalink / raw)
To: pve-devel
Local disk and storage creation and listing is something rather
different than the Proxmox VE storage client ABI that provides an
abstract access to a variety of storage types, specifically targeted
to virtual guests images, templates and backups.
The Datastore.* privilege group is specifically made for auditing the
abstract configuration, here the name must be interpreted in context
and not just assumed that due to "datastore" sounding like it could
have to do something with disks or creation of local storage it just
must be a good fit.
Luckily, Sys.Audit was already used too, which is the correct one
here, this is for node specific (HW) details, not some config for
accessing datastore in a restricted way.
This is a step in splitting the disk manage code out of the
pve-storage package, and maybe even repository.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
src/PVE/API2/Disks.pm | 7 ++-----
src/PVE/API2/Disks/Directory.pm | 2 +-
src/PVE/API2/Disks/LVM.pm | 2 +-
src/PVE/API2/Disks/LVMThin.pm | 2 +-
src/PVE/API2/Disks/ZFS.pm | 4 ++--
5 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/src/PVE/API2/Disks.pm b/src/PVE/API2/Disks.pm
index bde6132..f0e3fc4 100644
--- a/src/PVE/API2/Disks.pm
+++ b/src/PVE/API2/Disks.pm
@@ -85,10 +85,7 @@ __PACKAGE__->register_method ({
protected => 1,
proxyto => 'node',
permissions => {
- check => ['or',
- ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
- ['perm', '/nodes/{node}', ['Sys.Audit', 'Datastore.Audit'], any => 1],
- ],
+ check => ['or', ['perm', '/', ['Sys.Audit']], ['perm', '/nodes/{node}', ['Sys.Audit']]],
},
parameters => {
additionalProperties => 0,
@@ -183,7 +180,7 @@ __PACKAGE__->register_method ({
protected => 1,
proxyto => "node",
permissions => {
- check => ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
+ check => ['perm', '/', ['Sys.Audit']],
},
parameters => {
additionalProperties => 0,
diff --git a/src/PVE/API2/Disks/Directory.pm b/src/PVE/API2/Disks/Directory.pm
index efc701a..5cddddb 100644
--- a/src/PVE/API2/Disks/Directory.pm
+++ b/src/PVE/API2/Disks/Directory.pm
@@ -97,7 +97,7 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
+ check => ['perm', '/', ['Sys.Audit']],
},
description => "PVE Managed Directory storages.",
parameters => {
diff --git a/src/PVE/API2/Disks/LVM.pm b/src/PVE/API2/Disks/LVM.pm
index c091e37..f0419f6 100644
--- a/src/PVE/API2/Disks/LVM.pm
+++ b/src/PVE/API2/Disks/LVM.pm
@@ -21,7 +21,7 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
+ check => ['perm', '/', ['Sys.Audit']],
},
description => "List LVM Volume Groups",
parameters => {
diff --git a/src/PVE/API2/Disks/LVMThin.pm b/src/PVE/API2/Disks/LVMThin.pm
index 89b6401..bd6d47b 100644
--- a/src/PVE/API2/Disks/LVMThin.pm
+++ b/src/PVE/API2/Disks/LVMThin.pm
@@ -22,7 +22,7 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
+ check => ['perm', '/', ['Sys.Audit']],
},
description => "List LVM thinpools",
parameters => {
diff --git a/src/PVE/API2/Disks/ZFS.pm b/src/PVE/API2/Disks/ZFS.pm
index afd9a3e..6a4381c 100644
--- a/src/PVE/API2/Disks/ZFS.pm
+++ b/src/PVE/API2/Disks/ZFS.pm
@@ -57,7 +57,7 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
+ check => ['perm', '/', ['Sys.Audit']],
},
description => "List Zpools.",
parameters => {
@@ -130,7 +130,7 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Audit', 'Datastore.Audit'], any => 1],
+ check => ['perm', '/', ['Sys.Audit']],
},
description => "Get details about a zpool.",
parameters => {
--
2.39.2
^ permalink raw reply [flat|nested] 2+ messages in thread
* [pve-devel] applied: [PATCH 2/2] disk api: only ask for Datastore.Allocate if adding to storage config
2023-06-17 13:32 [pve-devel] applied: [PATCH 1/2] disk api: avoid using unrelated Datastore.Audit priv for disk management Thomas Lamprecht
@ 2023-06-17 13:32 ` Thomas Lamprecht
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2023-06-17 13:32 UTC (permalink / raw)
To: pve-devel
The Proxmox VE storage systems doesn't cares at all if the
Datastore.Allocate privilege is present if no Proxmox VE storage will
be allocated.
Note, if we want to restrict this further as Sys.Modify on /, which
is already quite a powerful permission, we should probably add a new
one under the Sys. space, e.g., Sys.Disk.Use or the like.
This is a step in splitting the disk manage code out of the
pve-storage package, and maybe even repository
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
src/PVE/API2/Disks/Directory.pm | 10 ++++++++--
src/PVE/API2/Disks/LVM.pm | 10 ++++++++--
src/PVE/API2/Disks/LVMThin.pm | 10 ++++++++--
src/PVE/API2/Disks/ZFS.pm | 10 ++++++++--
4 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/src/PVE/API2/Disks/Directory.pm b/src/PVE/API2/Disks/Directory.pm
index 5cddddb..099afae 100644
--- a/src/PVE/API2/Disks/Directory.pm
+++ b/src/PVE/API2/Disks/Directory.pm
@@ -165,7 +165,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'add_storage'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Create a Filesystem on an unused disk. Will be mounted under '/mnt/pve/NAME'.",
parameters => {
@@ -221,6 +222,8 @@ __PACKAGE__->register_method ({
my $verify_params = [qw(path)];
if ($param->{add_storage}) {
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']);
+
# reserve the name and add as disabled, will be enabled below if creation works out
PVE::API2::Storage::Config->create_or_update(
$name, $node, $storage_params, $verify_params, 1);
@@ -317,7 +320,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'cleanup-config'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Unmounts the storage and removes the mount unit.",
parameters => {
@@ -347,6 +351,8 @@ __PACKAGE__->register_method ({
my $rpcenv = PVE::RPCEnvironment::get();
my $user = $rpcenv->get_user();
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']) if $param->{'cleanup-config'};
+
my $name = $param->{name};
my $node = $param->{node};
diff --git a/src/PVE/API2/Disks/LVM.pm b/src/PVE/API2/Disks/LVM.pm
index f0419f6..3c5bdfa 100644
--- a/src/PVE/API2/Disks/LVM.pm
+++ b/src/PVE/API2/Disks/LVM.pm
@@ -117,7 +117,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'add_storage'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Create an LVM Volume Group",
parameters => {
@@ -162,6 +163,8 @@ __PACKAGE__->register_method ({
my $verify_params = [qw(vgname)];
if ($param->{add_storage}) {
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']);
+
# reserve the name and add as disabled, will be enabled below if creation works out
PVE::API2::Storage::Config->create_or_update(
$name, $node, $storage_params, $verify_params, 1);
@@ -199,7 +202,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'cleanup-config'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Remove an LVM Volume Group.",
parameters => {
@@ -229,6 +233,8 @@ __PACKAGE__->register_method ({
my $rpcenv = PVE::RPCEnvironment::get();
my $user = $rpcenv->get_user();
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']) if $param->{'cleanup-config'};
+
my $name = $param->{name};
my $node = $param->{node};
diff --git a/src/PVE/API2/Disks/LVMThin.pm b/src/PVE/API2/Disks/LVMThin.pm
index bd6d47b..f1c3957 100644
--- a/src/PVE/API2/Disks/LVMThin.pm
+++ b/src/PVE/API2/Disks/LVMThin.pm
@@ -75,7 +75,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'add_storage'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Create an LVM thinpool",
parameters => {
@@ -120,6 +121,8 @@ __PACKAGE__->register_method ({
my $verify_params = [qw(vgname thinpool)];
if ($param->{add_storage}) {
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']);
+
# reserve the name and add as disabled, will be enabled below if creation works out
PVE::API2::Storage::Config->create_or_update(
$name, $node, $storage_params, $verify_params, 1);
@@ -178,7 +181,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'cleanup-config'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Remove an LVM thin pool.",
parameters => {
@@ -209,6 +213,8 @@ __PACKAGE__->register_method ({
my $rpcenv = PVE::RPCEnvironment::get();
my $user = $rpcenv->get_user();
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']) if $param->{'cleanup-config'};
+
my $vg = $param->{'volume-group'};
my $lv = $param->{name};
my $node = $param->{node};
diff --git a/src/PVE/API2/Disks/ZFS.pm b/src/PVE/API2/Disks/ZFS.pm
index 6a4381c..6fb6bd6 100644
--- a/src/PVE/API2/Disks/ZFS.pm
+++ b/src/PVE/API2/Disks/ZFS.pm
@@ -300,7 +300,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'add_storage'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Create a ZFS pool.",
parameters => {
@@ -383,6 +384,8 @@ __PACKAGE__->register_method ({
my $verify_params = [qw(pool)];
if ($param->{add_storage}) {
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']);
+
# reserve the name and add as disabled, will be enabled below if creation works out
PVE::API2::Storage::Config->create_or_update(
$name, $node, $storage_params, $verify_params, 1);
@@ -507,7 +510,8 @@ __PACKAGE__->register_method ({
proxyto => 'node',
protected => 1,
permissions => {
- check => ['perm', '/', ['Sys.Modify', 'Datastore.Allocate']],
+ description => "Requires additionally 'Datastore.Allocate' on /storage when setting 'cleanup-config'",
+ check => ['perm', '/', ['Sys.Modify']],
},
description => "Destroy a ZFS pool.",
parameters => {
@@ -537,6 +541,8 @@ __PACKAGE__->register_method ({
my $rpcenv = PVE::RPCEnvironment::get();
my $user = $rpcenv->get_user();
+ $rpcenv->check($user, "/storage", ['Datastore.Allocate']) if $param->{'cleanup-config'};
+
my $name = $param->{name};
my $node = $param->{node};
--
2.39.2
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-06-17 13:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-17 13:32 [pve-devel] applied: [PATCH 1/2] disk api: avoid using unrelated Datastore.Audit priv for disk management Thomas Lamprecht
2023-06-17 13:32 ` [pve-devel] applied: [PATCH 2/2] disk api: only ask for Datastore.Allocate if adding to storage config Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox