* [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access @ 2023-05-26 7:33 Alexandre Derumier 2023-05-26 7:33 ` [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm Alexandre Derumier 2023-06-02 11:43 ` [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access Fabian Grünbichler 0 siblings, 2 replies; 7+ messages in thread From: Alexandre Derumier @ 2023-05-26 7:33 UTC (permalink / raw) To: pve-devel For proxmox 8, following the pve-manager patch serie https://lists.proxmox.com/pipermail/pve-devel/2023-May/056970.html This patch serie add check of permissions for bridge/vnets access (currently only at vm create/update, I'm note sureif they are other places where it should be added) if user have access to a zone, it have access to all vnets + vnet vlans if user have access to a vnet, it have access to the vnet + vnet vlans if user have access to a specific vnet+vlan, it have access to the vlan only Alexandre Derumier (1): api2: add check_bridge_access for create/update vm PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm 2023-05-26 7:33 [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access Alexandre Derumier @ 2023-05-26 7:33 ` Alexandre Derumier 2023-06-02 11:43 ` Fabian Grünbichler 2023-06-02 11:43 ` [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access Fabian Grünbichler 1 sibling, 1 reply; 7+ messages in thread From: Alexandre Derumier @ 2023-05-26 7:33 UTC (permalink / raw) To: pve-devel Signed-off-by: Alexandre Derumier <aderumier@odiso.com> --- PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index 587bb22..ebef93f 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -46,6 +46,12 @@ use PVE::SSHInfo; use PVE::Replication; use PVE::StorageTunnel; +my $have_sdn; +eval { + require PVE::Network::SDN; + $have_sdn = 1; +}; + BEGIN { if (!$ENV{PVE_GENERATING_DOCS}) { require PVE::HA::Env::PVE2; @@ -601,6 +607,33 @@ my $check_vm_create_usb_perm = sub { return 1; }; +my $check_bridge_access = sub { + my ($rpcenv, $authuser, $param) = @_; + + return 1 if $authuser eq 'root@pam'; + + foreach my $opt (keys %{$param}) { + next if $opt !~ m/^net\d+$/; + my $net = PVE::QemuServer::parse_net($param->{$opt}); + my $bridge = $net->{bridge}; + my $tag = $net->{tag}; + my $zone = 'local'; + + if ($have_sdn) { + my $vnet_cfg = PVE::Network::SDN::Vnets::config(); + if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) { + $zone = $vnet->{zone}; + } + } + return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1); + + return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge.$tag", ['SDN.Audit', 'SDN.Allocate'], 1); + + $rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']); + } + return 1; +}; + my $check_vm_modify_config_perm = sub { my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_; @@ -878,7 +911,7 @@ __PACKAGE__->register_method({ &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param); &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param); - + &$check_bridge_access($rpcenv, $authuser, $param); &$check_cpu_model_access($rpcenv, $authuser, $param); $check_drive_param->($param, $storecfg); @@ -1578,6 +1611,8 @@ my $update_vm_api = sub { &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param); + &$check_bridge_access($rpcenv, $authuser, $param); + my $updatefn = sub { my $conf = PVE::QemuConfig->load_config($vmid); -- 2.30.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm 2023-05-26 7:33 ` [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm Alexandre Derumier @ 2023-06-02 11:43 ` Fabian Grünbichler 2023-06-02 12:12 ` DERUMIER, Alexandre 0 siblings, 1 reply; 7+ messages in thread From: Fabian Grünbichler @ 2023-06-02 11:43 UTC (permalink / raw) To: Proxmox VE development discussion a few more places that come to my mind that might warrant further thinking or discussion: - restoring a backup - cloning a VM On May 26, 2023 9:33 am, Alexandre Derumier wrote: > Signed-off-by: Alexandre Derumier <aderumier@odiso.com> > --- > PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index 587bb22..ebef93f 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -46,6 +46,12 @@ use PVE::SSHInfo; > use PVE::Replication; > use PVE::StorageTunnel; > > +my $have_sdn; > +eval { > + require PVE::Network::SDN; > + $have_sdn = 1; > +}; > + > BEGIN { > if (!$ENV{PVE_GENERATING_DOCS}) { > require PVE::HA::Env::PVE2; > @@ -601,6 +607,33 @@ my $check_vm_create_usb_perm = sub { > return 1; > }; > > +my $check_bridge_access = sub { > + my ($rpcenv, $authuser, $param) = @_; > + > + return 1 if $authuser eq 'root@pam'; > + > + foreach my $opt (keys %{$param}) { > + next if $opt !~ m/^net\d+$/; > + my $net = PVE::QemuServer::parse_net($param->{$opt}); > + my $bridge = $net->{bridge}; > + my $tag = $net->{tag}; > + my $zone = 'local'; > + > + if ($have_sdn) { > + my $vnet_cfg = PVE::Network::SDN::Vnets::config(); > + if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) { > + $zone = $vnet->{zone}; > + } > + } > + return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1); why is this $noerr? should be a hard fail AFAICT! a, re-reading the cover letter, this is intentional. might be worth a callout here in a comment (and once this is finalized, in the docs ;)) > + return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge.$tag", ['SDN.Audit', 'SDN.Allocate'], 1); same here, I think I'd prefer /sdn/vnets/$bridge/$tag if we go down that route! then this would also be improved, since having access to the tag also checks access to the bridge, and this could be turned into a hard fail, which it should be! what about trunking? > + > + $rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']); for the whole block of checks here: what are the semantics of Audit and Allocate here? do we need another level between "sees bridge/vnet" and "can administer/reconfigure bridge/vnet" that says "can use bridge/vnet"? > + } > + return 1; > +}; > + > my $check_vm_modify_config_perm = sub { > my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_; > > @@ -878,7 +911,7 @@ __PACKAGE__->register_method({ > > &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param); > &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param); > - > + &$check_bridge_access($rpcenv, $authuser, $param); > &$check_cpu_model_access($rpcenv, $authuser, $param); > > $check_drive_param->($param, $storecfg); > @@ -1578,6 +1611,8 @@ my $update_vm_api = sub { > > &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param); > > + &$check_bridge_access($rpcenv, $authuser, $param); > + > my $updatefn = sub { > > my $conf = PVE::QemuConfig->load_config($vmid); > -- > 2.30.2 > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm 2023-06-02 11:43 ` Fabian Grünbichler @ 2023-06-02 12:12 ` DERUMIER, Alexandre 2023-06-05 7:24 ` Fabian Grünbichler 0 siblings, 1 reply; 7+ messages in thread From: DERUMIER, Alexandre @ 2023-06-02 12:12 UTC (permalink / raw) To: pve-devel Le vendredi 02 juin 2023 à 13:43 +0200, Fabian Grünbichler a écrit : > a few more places that come to my mind that might warrant further > thinking or discussion: > - restoring a backup doesn't it also use create_vm ? __PACKAGE__->register_method({ name => 'create_vm', path => '', method => 'POST', description => "Create or restore a virtual machine.", > - cloning a VM for cloning, we can't change the target bridge, so if user have access to the clone, isn't it enough ? > > On May 26, 2023 9:33 am, Alexandre Derumier wrote: > > Signed-off-by: Alexandre Derumier <aderumier@odiso.com> > > --- > > PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > > index 587bb22..ebef93f 100644 > > --- a/PVE/API2/Qemu.pm > > +++ b/PVE/API2/Qemu.pm > > @@ -46,6 +46,12 @@ use PVE::SSHInfo; > > use PVE::Replication; > > use PVE::StorageTunnel; > > > > +my $have_sdn; > > +eval { > > + require PVE::Network::SDN; > > + $have_sdn = 1; > > +}; > > + > > BEGIN { > > if (!$ENV{PVE_GENERATING_DOCS}) { > > require PVE::HA::Env::PVE2; > > @@ -601,6 +607,33 @@ my $check_vm_create_usb_perm = sub { > > return 1; > > }; > > > > +my $check_bridge_access = sub { > > + my ($rpcenv, $authuser, $param) = @_; > > + > > + return 1 if $authuser eq 'root@pam'; > > + > > + foreach my $opt (keys %{$param}) { > > + next if $opt !~ m/^net\d+$/; > > + my $net = PVE::QemuServer::parse_net($param->{$opt}); > > + my $bridge = $net->{bridge}; > > + my $tag = $net->{tag}; > > + my $zone = 'local'; > > + > > + if ($have_sdn) { > > + my $vnet_cfg = PVE::Network::SDN::Vnets::config(); > > + if (defined(my $vnet = > > PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) > > { > > + $zone = $vnet->{zone}; > > + } > > + } > > + return 1 if $rpcenv->check_any($authuser, > > "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1); > > why is this $noerr? should be a hard fail AFAICT! a, re-reading the > cover letter, this is intentional. might be worth a callout here in a > comment (and once this is finalized, in the docs ;)) yes, because we need to check later if we have permissoins on bridge/tag > > > + return 1 if $tag && $rpcenv->check_any($authuser, > > "/sdn/vnets/$bridge.$tag", ['SDN.Audit', 'SDN.Allocate'], 1); > > same here, I think I'd prefer /sdn/vnets/$bridge/$tag if we go down > that > route! then this would also be improved, since having access to the > tag > also checks access to the bridge, and this could be turned into a > hard > fail, which it should be! > I had tried this way, I don't remember, I think I had a problem somewhere, but I'll retry it again, as it's cleaner indeed. > what about trunking? > not implemented indeed, but I think we could compare the trunk vlans list to the vlan path permissions. Or maybe we just want that user have full bridge permissions,to defined the any vlans in the trunk ? > > + > > + $rpcenv->check_any($authuser, "/sdn/vnets/$bridge", > > ['SDN.Audit', 'SDN.Allocate']); > > for the whole block of checks here: > > what are the semantics of Audit and Allocate here? do we need another > level between "sees bridge/vnet" and "can administer/reconfigure > bridge/vnet" that says "can use bridge/vnet"? mmmm, the SDN.Allocate on /sdn/vnets indeed don't make sense. (it's only on the /sdn/zones, where you allocate the vnets). it should be SDN.Audit only. > > > + } > > + return 1; > > +}; > > + > > my $check_vm_modify_config_perm = sub { > > my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_; > > > > @@ -878,7 +911,7 @@ __PACKAGE__->register_method({ > > > > &$check_vm_create_serial_perm($rpcenv, $authuser, > > $vmid, $pool, $param); > > &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, > > $pool, $param); > > - > > + &$check_bridge_access($rpcenv, $authuser, $param); > > &$check_cpu_model_access($rpcenv, $authuser, $param); > > > > $check_drive_param->($param, $storecfg); > > @@ -1578,6 +1611,8 @@ my $update_vm_api = sub { > > > > &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, > > $param); > > > > + &$check_bridge_access($rpcenv, $authuser, $param); > > + > > my $updatefn = sub { > > > > my $conf = PVE::QemuConfig->load_config($vmid); > > -- > > 2.30.2 > > > > > > _______________________________________________ > > pve-devel mailing list > > pve-devel@lists.proxmox.com > > https://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPu0fK2BfWNnEHaDLDwG0rtDedpluZBIffSL1M5cj3F&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU > > > > > > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPu0fK2BfWNnEHaDLDwG0rtDedpluZBIffSL1M5cj3F&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm 2023-06-02 12:12 ` DERUMIER, Alexandre @ 2023-06-05 7:24 ` Fabian Grünbichler 2023-06-06 4:38 ` DERUMIER, Alexandre 0 siblings, 1 reply; 7+ messages in thread From: Fabian Grünbichler @ 2023-06-05 7:24 UTC (permalink / raw) To: Proxmox VE development discussion On June 2, 2023 2:12 pm, DERUMIER, Alexandre wrote: > Le vendredi 02 juin 2023 à 13:43 +0200, Fabian Grünbichler a écrit : >> a few more places that come to my mind that might warrant further >> thinking or discussion: >> - restoring a backup > doesn't it also use create_vm ? yes, but the potentially problematic parameters are coming from the backup in that case, not $param :) we do check the storage permissions at least, if we view nics on bridges/vnet as being the same kind of entity as volumes on storages than it would make sense to also check vnet permissions there (PVE::QemuServer -> $parse_backup_hints , but probably that is not the best place for network related checks ;)) > __PACKAGE__->register_method({ > name => 'create_vm', > path => '', > method => 'POST', > description => "Create or restore a virtual machine.", > > >> - cloning a VM > > for cloning, we can't change the target bridge, so if user have access > to the clone, isn't it enough ? same as above - if we treat "volume on storage" and "nic in vnet" as being equivalent, then cloning would also need to check whether I am allowed to add new nics to a vnet via cloning (like we do for volumes, even without a storage override set!). $check_storage_access_clone is the current helper, a similar one could be added for nics. note: we'd also need a similar patch for pve-container ;) but if you want, I can handle that one once this series is done or almost done, the current approach is guest-agnostic anyway so I don't expect any changes required for container support. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm 2023-06-05 7:24 ` Fabian Grünbichler @ 2023-06-06 4:38 ` DERUMIER, Alexandre 0 siblings, 0 replies; 7+ messages in thread From: DERUMIER, Alexandre @ 2023-06-06 4:38 UTC (permalink / raw) To: pve-devel > > same as above - if we treat "volume on storage" and "nic in vnet" as > being equivalent, then cloning would also need to check whether I am > allowed to add new nics to a vnet via cloning (like we do for > volumes, > even without a storage override set!). $check_storage_access_clone is > the current helper, a similar one could be added for nics. > ok got it! thanks. > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://antiphishing.cetsi.fr/proxy/v3?i=MlZSTzBhZFZ6Nzl4c3EyN7fbSKDePLMxi5u5_onpAoI&r=cm1qVmRYUWk2WXhYZVFHWA0PXtTaYxz7-FIOTkZBm34_dHdSch-gXn7ST9eGhQLN&f=S1Zkd042VWdrZG5qQUxxWk5ps4t67kNuHsBZzdzhpquLKuXqTZLIq2K1DfKr9N61yBafm7AuAITd6bHtRU4zEQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=F1is ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access 2023-05-26 7:33 [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access Alexandre Derumier 2023-05-26 7:33 ` [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm Alexandre Derumier @ 2023-06-02 11:43 ` Fabian Grünbichler 1 sibling, 0 replies; 7+ messages in thread From: Fabian Grünbichler @ 2023-06-02 11:43 UTC (permalink / raw) To: Proxmox VE development discussion On May 26, 2023 9:33 am, Alexandre Derumier wrote: > For proxmox 8, following the pve-manager patch serie > https://lists.proxmox.com/pipermail/pve-devel/2023-May/056970.html > > This patch serie add check of permissions for bridge/vnets access > (currently only at vm create/update, I'm note sureif they are other > places where it should be added) > > if user have access to a zone, it have access to all vnets + vnet vlans > if user have access to a vnet, it have access to the vnet + vnet vlans > if user have access to a specific vnet+vlan, it have access to the vlan only the last part could be solved more elegantly IMHO by making tags children of vnets (and delegating the propagation the propagation bit of the ACL), see comments on individual patches. nit: if you send a single commit, no need for a cover letter - and then please include this information in the commit message, as series cover letters are not included once the patch is applied! > > Alexandre Derumier (1): > api2: add check_bridge_access for create/update vm > > PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > -- > 2.30.2 > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-06 4:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-26 7:33 [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access Alexandre Derumier 2023-05-26 7:33 ` [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm Alexandre Derumier 2023-06-02 11:43 ` Fabian Grünbichler 2023-06-02 12:12 ` DERUMIER, Alexandre 2023-06-05 7:24 ` Fabian Grünbichler 2023-06-06 4:38 ` DERUMIER, Alexandre 2023-06-02 11:43 ` [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access Fabian Grünbichler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox