From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C902F1FF15F for ; Mon, 18 Nov 2024 15:24:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F3F9011A84; Mon, 18 Nov 2024 15:24:35 +0100 (CET) Message-ID: <2bf7ebfb-f046-4138-916c-2a333ddc993f@proxmox.com> Date: Mon, 18 Nov 2024 15:24:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: pve-devel@lists.proxmox.com References: <20241118114134.83882-1-s.hanreich@proxmox.com> <20241118114134.83882-3-s.hanreich@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20241118114134.83882-3-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.657 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 Subject: Re: [pve-devel] [PATCH pve-firewall v5 2/5] add support for loading sdn firewall configuration 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: , Reply-To: Proxmox VE development discussion Cc: Wolfgang Bumiller Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 11/18/24 12:41, Stefan Hanreich wrote: > +sub load_sdn_conf { > + my $rpcenv = eval { PVE::RPCEnvironment::get() }; After some additional consideration and testing, I think it is a bad idea to have the permission filtering in the core firewall code. Particularly because loading and validating is a single step in pve-firewall and you cannot opt out of it. As soon as we limit the amount of IPSets returned here, we run into the possibility of validation errors. Also, it's generally bad w.r.t. separation of concerns. I think we should *always* load the whole configuration here and filter which IPSets we output in the API methods instead (by invoking load_sdn_conf there explicitly and updating the cluster_conf hash). Consider the following situation: A user has permissions for one (out of many) SDN subnet and one (out of many) VM and wants to edit the VM firewall config from the Web UI. When editing the VM Firewall configuration, as soon as we load the cluster config in the code of that endpoint and the cluster configuration references an IPSet the user cannot see, there are validation errors in the syslog (although they do not pose any functional issue - the firewall works correctly). The only way to avoid this I see is: * always load the *full* SDN IPsets initially, so cfg validation always sees the big pictrue * filter the output of IPSets in the API endpoints returning valid IPsets for rule creation * use the filtered IPSet list when validating rules on creation / update When returning a firewall configuration from the API, and it contains IPSets that actually exist, but cannot be seen by the user, we're in a bit of a pickle. Currently we show them as errors in the UI (since the referenced IPSet is not loaded because of the filtering we do currently load_sdn_conf). With the proposed changes, there would be no errors displayed in the frontend unless the user tries to edit the rule (since then we're validating against the filtered IPSet list). I'm not sure what would be the best behavior here. I don't think filtering the rules is a good option, since it can lead to unexpected behavior of the firewall rules and is generally just intransparent. I think just showing them as valid but not letting the user actually use them in rules is fine. Currently we show errors in the syslog in this situation (could not find ipset ...), but the functionality of the firewall is not affected in any way! So we can also decide that this is okay for now and fix this with a subsequent patch. > + > + if ($@) { > + warn "could not load SDN configuration because RPCEnvironment is not initialized."; > + return {}; > + } > + > + my $authuser = $rpcenv->get_user(); > + > + my $guests = PVE::Cluster::get_vmlist(); > + my $allowed_vms = [ > + grep { $rpcenv->check($authuser, "/vms/$_", [ 'VM.Audit' ], 1) } sort keys $guests->{ids}->%* > + ]; > + > + my $vnets = PVE::Network::SDN::Vnets::config(1); > + my $privs = [ 'SDN.Audit', 'SDN.Allocate' ]; > + my $allowed_vnets = []; > + foreach my $vnet (sort keys %{$vnets->{ids}}) { > + my $zone = $vnets->{ids}->{$vnet}->{zone}; > + next if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet", $privs, 1); > + push @$allowed_vnets, $vnet; > + } > + > + my $empty_sdn_config = { ipset => {} , ipset_comments => {} }; > + > + my $sdn_config = eval { > + PVE::RS::Firewall::SDN::config($allowed_vnets, $allowed_vms) > + }; > + warn $@ if $@; > + > + return $sdn_config // $empty_sdn_config; > +} > + _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel