From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D77A89D117 for ; Fri, 2 Jun 2023 13:43:30 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BFCBE29887 for ; Fri, 2 Jun 2023 13:43:30 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 2 Jun 2023 13:43:30 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9C0FA4847D for ; Fri, 2 Jun 2023 13:43:29 +0200 (CEST) Date: Fri, 02 Jun 2023 13:43:23 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230526073347.1615981-1-aderumier@odiso.com> <20230526073347.1615981-2-aderumier@odiso.com> In-Reply-To: <20230526073347.1615981-2-aderumier@odiso.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1685705445.0kkr7b9mhh.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.073 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH qemu-server 1/1] api2: add check_bridge_access for create/update vm X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Jun 2023 11:43:30 -0000 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 > --- > PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) >=20 > 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; > =20 > +my $have_sdn; > +eval { > + require PVE::Network::SDN; > + $have_sdn =3D 1; > +}; > + > BEGIN { > if (!$ENV{PVE_GENERATING_DOCS}) { > require PVE::HA::Env::PVE2; > @@ -601,6 +607,33 @@ my $check_vm_create_usb_perm =3D sub { > return 1; > }; > =20 > +my $check_bridge_access =3D sub { > + my ($rpcenv, $authuser, $param) =3D @_; > + > + return 1 if $authuser eq 'root@pam'; > + > + foreach my $opt (keys %{$param}) { > + next if $opt !~ m/^net\d+$/; > + my $net =3D PVE::QemuServer::parse_net($param->{$opt}); > + my $bridge =3D $net->{bridge}; > + my $tag =3D $net->{tag}; > + my $zone =3D 'local'; > + > + if ($have_sdn) { > + my $vnet_cfg =3D PVE::Network::SDN::Vnets::config(); > + if (defined(my $vnet =3D PVE::Network::SDN::Vnets::sdn_vnets_config= ($vnet_cfg, $bridge, 1))) { > + $zone =3D $vnet->{zone}; > + } > + } > + return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Aud= it', '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 =3D sub { > my ($rpcenv, $authuser, $vmid, $pool, $key_list) =3D @_; > =20 > @@ -878,7 +911,7 @@ __PACKAGE__->register_method({ > =20 > &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $pa= ram); > &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param= ); > - > + &$check_bridge_access($rpcenv, $authuser, $param); > &$check_cpu_model_access($rpcenv, $authuser, $param); > =20 > $check_drive_param->($param, $storecfg); > @@ -1578,6 +1611,8 @@ my $update_vm_api =3D sub { > =20 > &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param)= ; > =20 > + &$check_bridge_access($rpcenv, $authuser, $param); > + > my $updatefn =3D sub { > =20 > my $conf =3D PVE::QemuConfig->load_config($vmid); > --=20 > 2.30.2 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20