Re: [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm

["DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>]

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
>