[pve-devel] [PATCH pve-network 1/1] get_local_vnets: fix permission path && perm
DERUMIER, Alexandre
alexandre.derumier at groupe-cyllene.com
Thu Jun 8 03:34:00 CEST 2023
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 at 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 at 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 at 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
>
More information about the pve-devel
mailing list