Re: [pve-devel] [PATCH pve-network 1/1] get_local_vnets: fix permission path && perm

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

Le mercredi 07 juin 2023 à 16:56 +0200, Fabian Grünbichler a écrit :
> pve-network requires more work:
> 
> - there is a lot of /sdn/vnets/.. permission checks leftover (all of
> the vnet/subnet code!)
> - there are /sdn/vnets/../subnets/.. ACL paths that need to be
> dropped,
>   or they clash with /sdn/zones/<zone>/<vnet>[/<vlan>]
> 
I have send separated patch to fix permissions
https://lists.proxmox.com/pipermail/pve-devel/2023-June/057374.html

I don't have changed api for now, as I should need to rework the gui a
little bit.
(and I'm not sure about api path like /sdn/zones/zone1/vnet1  ,
/sdn/zones/zone2/vnet1  , as vnet should really be unique)


But for now, acl are working fine with this patch.


> - the GUI seems to be broken when "Advanced" is not ticked
> 

I really don't see where is the problem (don't happen with
bgpcontroller advanced, and it's exactly the same code....)


> 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?
> 
Maybe later ^_^, I need a little bit more time to think about it.
 

> 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 => 'GET',
>      description => "SDN subnets index.",
>      permissions => {
> -       description => "Only list entries where you have 'SDN.Audit'
> or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
> +       description => "Only list entries where you have 'SDN.Audit',
> 'SDN.Use' or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
>         user => 'all',
>      },
>      parameters => {
> @@ -89,7 +89,7 @@ __PACKAGE__->register_method ({
>         my @sids = PVE::Network::SDN::Subnets::sdn_subnets_ids($cfg);
>         my $res = [];
>         foreach my $id (@sids) {
> -           my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> +           my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
>             next if !$rpcenv->check_any($authuser,
> "/sdn/vnets/$vnetid/subnets/$id", $privs, 1);
>  
>             my $scfg = &$api_sdn_subnets_config($cfg, $id);
> diff --git a/src/PVE/API2/Network/SDN/Vnets.pm
> b/src/PVE/API2/Network/SDN/Vnets.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 = sub {
>      }
>  };
>  
> +# checks access, but masks zone to avoid info leak..
> +my $check_vnet_access = sub {
> +    sub ($rpcenv, $authuser, $zone, $vnet, $privs) = @_;
> +    $rpcenv->check_any($authuser, "/sdn/zones/<zone>/$vnet", $privs)
> +       if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet",
> $privs, 1);
> +}
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -57,7 +64,7 @@ __PACKAGE__->register_method ({
>      description => "SDN vnets index.",
>      permissions => {
>         description => "Only list entries where you have 'SDN.Audit'
> or 'SDN.Allocate'"
> -           ." permissions on '/sdn/vnets/<vnet>'",
> +           ." permissions on '/sdn/zones/<zone>/<vnet>'",
>         user => 'all',
>      },
>      parameters => {
> @@ -104,8 +111,10 @@ __PACKAGE__->register_method ({
>         my @sids = PVE::Network::SDN::Vnets::sdn_vnets_ids($cfg);
>         my $res = [];
>         foreach my $id (@sids) {
> -           my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> -           next if !$rpcenv->check_any($authuser, "/sdn/vnets/$id",
> $privs, 1);
> +           my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
> +           my $zone = $cfg->{$id}->{zone};
> +           next if !$zone;
> +           next if !$rpcenv->check_any($authuser,
> "/sdn/zones/$zone/$id", $privs, 1);
>  
>             my $scfg = &$api_sdn_vnets_config($cfg, $id);
>             push @$res, $scfg;
> @@ -120,8 +129,9 @@ __PACKAGE__->register_method ({
>      method => 'GET',
>      description => "Read sdn vnet configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets/{vnet}', ['SDN.Allocate']],
> -   },
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
> +    },
>      parameters => {
>         additionalProperties => 0,
>         properties => {
> @@ -144,6 +154,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $cfg = {};
>         if($param->{pending}) {
>             my $running_cfg = PVE::Network::SDN::running_config();
> @@ -156,6 +169,11 @@ __PACKAGE__->register_method ({
>             $cfg = PVE::Network::SDN::Vnets::config();
>         }
>  
> +       my $zone = $cfg->{$vnet}->{zone};
> +       return if !$zone;
> +
> +       $check_vnet_access($rpcenv, $authuser, $zone, $vnet,
> ['SDN.Allocate']);
> +
>         return $api_sdn_vnets_config->($cfg, $param->{vnet});
>      }});
>  
> @@ -166,7 +184,7 @@ __PACKAGE__->register_method ({
>      method => 'POST',
>      description => "Create a new sdn vnet object.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       check => ['perm', '/sdn/zones/{zone}', ['SDN.Allocate']],
>      },
>      parameters => PVE::Network::SDN::VnetPlugin->createSchema(),
>      returns => { type => 'null' },
> @@ -210,24 +228,36 @@ __PACKAGE__->register_method ({
>      method => 'PUT',
>      description => "Update sdn vnet object configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
>      },
>      parameters => PVE::Network::SDN::VnetPlugin->updateSchema(),
>      returns => { type => 'null' },
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $id = extract_param($param, 'vnet');
>         my $digest = extract_param($param, 'digest');
>  
>         PVE::Network::SDN::lock_sdn_config(sub {
>             my $cfg = PVE::Network::SDN::Vnets::config();
>  
> -           PVE::SectionConfig::assert_if_modified($cfg, $digest);
> +           my $zone = $cfg->{ids}->{$id}->{zone} // $params->{zone};
> +           # TODO can this even happen?
> +           raise_param_exc({ zone => "missing zone" }) if !$zone;
> +
> +           $check_vnet_access($rpcenv, $authuser, $zone, $id,
> ['SDN.Allocate']);
> +
> +           if (my $new_zone = $params->{zone}) {
> +               $rpcenv->check($authuser, "/sdn/zones/$new_zone/$id",
> ['SDN.Allocate']);
> +           }
>  
> +           PVE::SectionConfig::assert_if_modified($cfg, $digest);
>  
>             my $opts = PVE::Network::SDN::VnetPlugin-
> >check_config($id, $param, 0, 1);
> -           raise_param_exc({ zone => "missing zone"}) if !$opts-
> >{zone};
>             my $subnets = PVE::Network::SDN::Vnets::get_subnets($id);
>             raise_param_exc({ zone => "can't change zone if subnets
> exists"}) if($subnets && $opts->{zone} ne $cfg->{ids}->{$id}-
> >{zone});
>  
> @@ -256,7 +286,8 @@ __PACKAGE__->register_method ({
>      method => 'DELETE',
>      description => "Delete sdn vnet object configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
>      },
>      parameters => {
>         additionalProperties => 0,
> @@ -270,10 +301,19 @@ __PACKAGE__->register_method ({
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $id = extract_param($param, 'vnet');
>  
>          PVE::Network::SDN::lock_sdn_config(sub {
>             my $cfg = PVE::Network::SDN::Vnets::config();
> +           my $zone = $cfg->{ids}->{$id}->{zone};
> +           # TODO can this even happen?
> +           raise_param_exc({ zone => "missing zone" }) if !$zone;
> +
> +           $check_vnet_access($rpcenv, $authuser, $zone, $id,
> ['SDN.Allocate']);
> +
>             my $scfg =
> PVE::Network::SDN::Vnets::sdn_vnets_config($cfg, $id); # check if
> exists
>             my $vnet_cfg = PVE::Network::SDN::Vnets::config();
>  
> ```
> 
> On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> > new path is /zones/<zone>/<vnetid>
> > 
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> >  PVE/Network/SDN.pm | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 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 = $vnet->{zone};
> >         my $comments = $vnet->{alias};
> >  
> > -       my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> > +       my $privs = [ 'SDN.Audit', 'SDN.Use' ];
> >  
> >         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);
> >  
> >         my $zone_config =
> > PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid);
> >  
> > -- 
> > 2.30.2
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=MUo0RzFIRTVvbFhYVGloQoloZQj6tvqyhpERsn5z8Z4&r=cFdGNHFjVENnWDEzUVliSYiK92A-8tPjy0OkrQBFKsNtwFQPVFVwvPagaFXOdIvK&f=ODlJNFRJTjZBcWFlaWxQaCCCfKFEiPqnNIdA-OFeRWhRkYGvCokAsY8PdoPF8z-Ieq4V3WNpzo4Gr8nE76YOxQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=b1p5
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=MUo0RzFIRTVvbFhYVGloQoloZQj6tvqyhpERsn5z8Z4&r=cFdGNHFjVENnWDEzUVliSYiK92A-8tPjy0OkrQBFKsNtwFQPVFVwvPagaFXOdIvK&f=ODlJNFRJTjZBcWFlaWxQaCCCfKFEiPqnNIdA-OFeRWhRkYGvCokAsY8PdoPF8z-Ieq4V3WNpzo4Gr8nE76YOxQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=b1p5
>