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 598739F070 for ; Wed, 7 Jun 2023 16:56:43 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2B1871B9DB for ; Wed, 7 Jun 2023 16:56:43 +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 ; Wed, 7 Jun 2023 16:56:42 +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 BA0B241F73 for ; Wed, 7 Jun 2023 16:56:41 +0200 (CEST) Date: Wed, 07 Jun 2023 16:56:35 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230607120357.4177891-1-aderumier@odiso.com> <20230607120357.4177891-5-aderumier@odiso.com> In-Reply-To: <20230607120357.4177891-5-aderumier@odiso.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1686149560.i7ewotwm93.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.078 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 POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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 pve-network 1/1] get_local_vnets: fix permission path && perm 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: Wed, 07 Jun 2023 14:56:43 -0000 pve-network requires more work: - there is a lot of /sdn/vnets/.. permission checks leftover (all of the vn= et/subnet code!) - there are /sdn/vnets/../subnets/.. ACL paths that need to be dropped, or they clash with /sdn/zones//[/] - the GUI seems to be broken when "Advanced" is not ticked I started off, but then I realized we might also want to re-evaluate: - whether we even care about potentially leaking the vnet<->zone binding in case the ACL checks fail - whether we want to move the whole API tree as well to have vnets below zones instead of next to eachother, so we always have the zone as (path) parameter? anyhow, here's a half-diff of some potentially relevant changes ;) ``` diff --git a/src/PVE/API2/Network/SDN/Subnets.pm b/src/PVE/API2/Network/SDN= /Subnets.pm index 377a568..fbe2c46 100644 --- a/src/PVE/API2/Network/SDN/Subnets.pm +++ b/src/PVE/API2/Network/SDN/Subnets.pm @@ -39,7 +39,7 @@ __PACKAGE__->register_method ({ method =3D> 'GET', description =3D> "SDN subnets index.", permissions =3D> { - description =3D> "Only list entries where you have 'SDN.Audit' or 'SDN.Al= locate' permissions on '/sdn/subnets/'", + description =3D> "Only list entries where you have 'SDN.Audit', 'SDN.Use'= or 'SDN.Allocate' permissions on '/sdn/subnets/'", user =3D> 'all', }, parameters =3D> { @@ -89,7 +89,7 @@ __PACKAGE__->register_method ({ my @sids =3D PVE::Network::SDN::Subnets::sdn_subnets_ids($cfg); my $res =3D []; foreach my $id (@sids) { - my $privs =3D [ 'SDN.Audit', 'SDN.Allocate' ]; + my $privs =3D [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ]; next if !$rpcenv->check_any($authuser, "/sdn/vnets/$vnetid/subnets/$i= d", $privs, 1); =20 my $scfg =3D &$api_sdn_subnets_config($cfg, $id); diff --git a/src/PVE/API2/Network/SDN/Vnets.pm b/src/PVE/API2/Network/SDN/V= nets.pm index 811a2e8..eaa3a04 100644 --- a/src/PVE/API2/Network/SDN/Vnets.pm +++ b/src/PVE/API2/Network/SDN/Vnets.pm @@ -50,6 +50,13 @@ my $api_sdn_vnets_deleted_config =3D sub { } }; =20 +# checks access, but masks zone to avoid info leak.. +my $check_vnet_access =3D sub { + sub ($rpcenv, $authuser, $zone, $vnet, $privs) =3D @_; + $rpcenv->check_any($authuser, "/sdn/zones//$vnet", $privs) + if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet", $privs, 1); +} + __PACKAGE__->register_method ({ name =3D> 'index', path =3D> '', @@ -57,7 +64,7 @@ __PACKAGE__->register_method ({ description =3D> "SDN vnets index.", permissions =3D> { description =3D> "Only list entries where you have 'SDN.Audit' or 'SDN.Al= locate'" - ." permissions on '/sdn/vnets/'", + ." permissions on '/sdn/zones//'", user =3D> 'all', }, parameters =3D> { @@ -104,8 +111,10 @@ __PACKAGE__->register_method ({ my @sids =3D PVE::Network::SDN::Vnets::sdn_vnets_ids($cfg); my $res =3D []; foreach my $id (@sids) { - my $privs =3D [ 'SDN.Audit', 'SDN.Allocate' ]; - next if !$rpcenv->check_any($authuser, "/sdn/vnets/$id", $privs, 1); + my $privs =3D [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ]; + my $zone =3D $cfg->{$id}->{zone}; + next if !$zone; + next if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$id", $privs= , 1); =20 my $scfg =3D &$api_sdn_vnets_config($cfg, $id); push @$res, $scfg; @@ -120,8 +129,9 @@ __PACKAGE__->register_method ({ method =3D> 'GET', description =3D> "Read sdn vnet configuration.", permissions =3D> { - check =3D> ['perm', '/sdn/vnets/{vnet}', ['SDN.Allocate']], - }, + description =3D> "Requires 'SDN.Allocate' permission on '/sdn/zones//'", + user =3D> 'all', + }, parameters =3D> { additionalProperties =3D> 0, properties =3D> { @@ -144,6 +154,9 @@ __PACKAGE__->register_method ({ code =3D> sub { my ($param) =3D @_; =20 + my $rpcenv =3D PVE::RPCEnvironment::get(); + my $authuser =3D $rpcenv->get_user(); + my $cfg =3D {}; if($param->{pending}) { my $running_cfg =3D PVE::Network::SDN::running_config(); @@ -156,6 +169,11 @@ __PACKAGE__->register_method ({ $cfg =3D PVE::Network::SDN::Vnets::config(); } =20 + my $zone =3D $cfg->{$vnet}->{zone}; + return if !$zone; + + $check_vnet_access($rpcenv, $authuser, $zone, $vnet, ['SDN.Allocate']); + return $api_sdn_vnets_config->($cfg, $param->{vnet}); }}); =20 @@ -166,7 +184,7 @@ __PACKAGE__->register_method ({ method =3D> 'POST', description =3D> "Create a new sdn vnet object.", permissions =3D> { - check =3D> ['perm', '/sdn/vnets', ['SDN.Allocate']], + check =3D> ['perm', '/sdn/zones/{zone}', ['SDN.Allocate']], }, parameters =3D> PVE::Network::SDN::VnetPlugin->createSchema(), returns =3D> { type =3D> 'null' }, @@ -210,24 +228,36 @@ __PACKAGE__->register_method ({ method =3D> 'PUT', description =3D> "Update sdn vnet object configuration.", permissions =3D> { - check =3D> ['perm', '/sdn/vnets', ['SDN.Allocate']], + description =3D> "Requires 'SDN.Allocate' permission on '/sdn/zones//'", + user =3D> 'all', }, parameters =3D> PVE::Network::SDN::VnetPlugin->updateSchema(), returns =3D> { type =3D> 'null' }, code =3D> sub { my ($param) =3D @_; =20 + my $rpcenv =3D PVE::RPCEnvironment::get(); + my $authuser =3D $rpcenv->get_user(); + my $id =3D extract_param($param, 'vnet'); my $digest =3D extract_param($param, 'digest'); =20 PVE::Network::SDN::lock_sdn_config(sub { my $cfg =3D PVE::Network::SDN::Vnets::config(); =20 - PVE::SectionConfig::assert_if_modified($cfg, $digest); + my $zone =3D $cfg->{ids}->{$id}->{zone} // $params->{zone}; + # TODO can this even happen? + raise_param_exc({ zone =3D> "missing zone" }) if !$zone; + + $check_vnet_access($rpcenv, $authuser, $zone, $id, ['SDN.Allocate']); + + if (my $new_zone =3D $params->{zone}) { + $rpcenv->check($authuser, "/sdn/zones/$new_zone/$id", ['SDN.Allocate']); + } =20 + PVE::SectionConfig::assert_if_modified($cfg, $digest); =20 my $opts =3D PVE::Network::SDN::VnetPlugin->check_config($id, $param,= 0, 1); - raise_param_exc({ zone =3D> "missing zone"}) if !$opts->{zone}; my $subnets =3D PVE::Network::SDN::Vnets::get_subnets($id); raise_param_exc({ zone =3D> "can't change zone if subnets exists"}) i= f($subnets && $opts->{zone} ne $cfg->{ids}->{$id}->{zone}); =20 @@ -256,7 +286,8 @@ __PACKAGE__->register_method ({ method =3D> 'DELETE', description =3D> "Delete sdn vnet object configuration.", permissions =3D> { - check =3D> ['perm', '/sdn/vnets', ['SDN.Allocate']], + description =3D> "Requires 'SDN.Allocate' permission on '/sdn/zones//'", + user =3D> 'all', }, parameters =3D> { additionalProperties =3D> 0, @@ -270,10 +301,19 @@ __PACKAGE__->register_method ({ code =3D> sub { my ($param) =3D @_; =20 + my $rpcenv =3D PVE::RPCEnvironment::get(); + my $authuser =3D $rpcenv->get_user(); + my $id =3D extract_param($param, 'vnet'); =20 PVE::Network::SDN::lock_sdn_config(sub { my $cfg =3D PVE::Network::SDN::Vnets::config(); + my $zone =3D $cfg->{ids}->{$id}->{zone}; + # TODO can this even happen? + raise_param_exc({ zone =3D> "missing zone" }) if !$zone; + + $check_vnet_access($rpcenv, $authuser, $zone, $id, ['SDN.Allocate']); + my $scfg =3D PVE::Network::SDN::Vnets::sdn_vnets_config($cfg, $id); #= check if exists my $vnet_cfg =3D PVE::Network::SDN::Vnets::config(); =20 ``` On June 7, 2023 2:03 pm, Alexandre Derumier wrote: > new path is /zones// >=20 > Signed-off-by: Alexandre Derumier > --- > PVE/Network/SDN.pm | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) >=20 > diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm > index b95dd5b..1ad85e5 100644 > --- a/PVE/Network/SDN.pm > +++ b/PVE/Network/SDN.pm > @@ -190,10 +190,10 @@ sub get_local_vnets { > my $zoneid =3D $vnet->{zone}; > my $comments =3D $vnet->{alias}; > =20 > - my $privs =3D [ 'SDN.Audit', 'SDN.Allocate' ]; > + my $privs =3D [ 'SDN.Audit', 'SDN.Use' ]; > =20 > next if !$zoneid; > - next if !$rpcenv->check_any($authuser, "/sdn/zones/$zoneid", $privs, 1)= && !$rpcenv->check_any($authuser, "/sdn/vnets/$vnetid", $privs, 1); > + next if !$rpcenv->check_sdn_bridge($authuser, $zoneid, $vnetid, $privs,= 1); > =20 > my $zone_config =3D PVE::Network::SDN::Zones::sdn_zones_config($zones_c= fg, $zoneid); > =20 > --=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