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 796BA1FF163 for ; Thu, 26 Sep 2024 08:37:35 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 459283A806; Thu, 26 Sep 2024 08:37:49 +0200 (CEST) Message-ID: Date: Thu, 26 Sep 2024 08:37:43 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Stefan Hanreich References: <20240911093116.112960-1-s.hanreich@proxmox.com> <20240911093116.112960-11-s.hanreich@proxmox.com> Content-Language: en-GB, de-AT From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20240911093116.112960-11-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.051 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 10/15] api: add vnet endpoints 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 11/09/2024 um 11:31 schrieb Stefan Hanreich: > Signed-off-by: Stefan Hanreich > --- > src/PVE/API2/Firewall/Makefile | 1 + > src/PVE/API2/Firewall/Rules.pm | 71 ++++++++++++++ > src/PVE/API2/Firewall/Vnet.pm | 166 +++++++++++++++++++++++++++++++++ > src/PVE/Firewall.pm | 10 ++ > 4 files changed, 248 insertions(+) > create mode 100644 src/PVE/API2/Firewall/Vnet.pm > > diff --git a/src/PVE/API2/Firewall/Makefile b/src/PVE/API2/Firewall/Makefile > index e916755..325c4d3 100644 > --- a/src/PVE/API2/Firewall/Makefile > +++ b/src/PVE/API2/Firewall/Makefile > @@ -9,6 +9,7 @@ LIB_SOURCES= \ > Cluster.pm \ > Host.pm \ > VM.pm \ > + Vnet.pm \ > Groups.pm > > all: > diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm > index ebb51af..906c6d7 100644 > --- a/src/PVE/API2/Firewall/Rules.pm > +++ b/src/PVE/API2/Firewall/Rules.pm > @@ -18,6 +18,10 @@ my $api_properties = { > }, > }; > > +sub before_method { > + my ($class, $method_name, $param) = @_; a short comment here stating explicitly that/why this is a no-op implementation by design might be good. > +} > + > sub lock_config { > my ($class, $param, $code) = @_; > > @@ -93,6 +97,7 @@ sub register_get_rules { > }, > code => sub { > my ($param) = @_; please keep the empty line after above's method param extraction one. > + $class->before_method('get_rules', $param); > > my ($cluster_conf, $fw_conf, $rules) = $class->load_config($param); > > @@ -191,6 +196,7 @@ sub register_get_rule { > }, > code => sub { > my ($param) = @_; same as above > + $class->before_method('get_rule', $param); > > my ($cluster_conf, $fw_conf, $rules) = $class->load_config($param); > > @@ -231,6 +237,7 @@ sub register_create_rule { > returns => { type => "null" }, > code => sub { > my ($param) = @_; > + $class->before_method('create_rule', $param); same as above > > $class->lock_config($param, sub { > my ($param) = @_; > @@ -292,6 +299,7 @@ sub register_update_rule { > returns => { type => "null" }, > code => sub { > my ($param) = @_; same as above > + $class->before_method('update_rule', $param); > > $class->lock_config($param, sub { > my ($param) = @_; > @@ -358,6 +366,7 @@ sub register_delete_rule { > returns => { type => "null" }, > code => sub { > my ($param) = @_; same as above > + $class->before_method('delete_rule', $param); > > $class->lock_config($param, sub { > my ($param) = @_; > @@ -636,4 +645,66 @@ sub save_rules { > > __PACKAGE__->register_handlers(); > > +package PVE::API2::Firewall::VnetRules; > + > +use strict; > +use warnings; > +use PVE::JSONSchema qw(get_standard_option); > + > +use base qw(PVE::API2::Firewall::RulesBase); > + > +__PACKAGE__->additional_parameters({ > + vnet => get_standard_option('pve-sdn-vnet-id'), > +}); > + > +sub before_method { I'd prefer a _hook suffix for such a method for slightly added clarity. And FWIW, if all you do now is check privileges, and there's nothing you already know that's gonna get added here soon, you could just name it after what it does and avoid being all to generic, i.e. something like sub assert_privs_for_method > + my ($class, $method_name, $param) = @_; > + > + my $privs; this is a "one of those privs", not "all of those privs" thing FWICT, maybe encode that also in the name to slightly reduce potential for introducing errors (as in: unlikely, but too cheap to not do it), like e.g.: my $requires_one_of_privs: Alternatively, avoid the variable and call check_vnet_access directly, but no hard feelings. > + > + if ($method_name eq 'get_rule' || $method_name eq 'get_rules') { > + $privs = ['SDN.Audit', 'SDN.Allocate']; > + } elsif ($method_name eq 'update_rule' > + || $method_name eq 'create_rule' > + || $method_name eq 'delete_rule' It's certainly not always better but IMO a regex match would improve readability slightly, e.g.: } elsif ($method_name =~ /^(create|delete|update)_rule$/) { > + ) { > + $privs = ['SDN.Allocate']; > + } else { > + die "unknown method: $method_name"; > + } > + > + PVE::API2::Firewall::Vnet::check_vnet_access($param->{vnet}, $privs); > +} > + > +sub rule_env { > + my ($class, $param) = @_; > + > + return 'vnet'; > +} > + > +sub lock_config { > + my ($class, $param, $code) = @_; > + > + PVE::Firewall::lock_vnetfw_conf($param->{vnet}, 10, $code, $param); > +} > + > +sub load_config { > + my ($class, $param) = @_; > + > + my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1); > + my $fw_conf = PVE::Firewall::load_vnetfw_conf($cluster_conf, 'vnet', $param->{vnet}); > + my $rules = $fw_conf->{rules}; > + > + return ($cluster_conf, $fw_conf, $rules); > +} > + > +sub save_rules { > + my ($class, $param, $fw_conf, $rules) = @_; > + > + $fw_conf->{rules} = $rules; > + PVE::Firewall::save_vnetfw_conf($param->{vnet}, $fw_conf); > +} > + > +__PACKAGE__->register_handlers(); > + > 1; > +sub check_vnet_access { > + my ($vnetid, $privileges) = @_; > + > + my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, 1); > + die "invalid vnet specified" if !$vnet; nit: could be combined my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, 1) or die "invalid vnet '$vnetid'\n"; for sake of completeness: what's not ok is a conditional definition like e.g: my $foo = 'bar' if baz(); As that keeps the value of the last call if the if evaluates to false. But, as long as the definition itself is not conditional it's fine. > + > + my $zoneid = $vnet->{zone}; > + > + my $rpcenv = PVE::RPCEnvironment::get(); > + my $authuser = $rpcenv->get_user(); > + > + $rpcenv->check_any($authuser, "/sdn/zones/$zoneid/$vnetid", $privileges); > +}; > + }}); > + > +my $option_properties = $PVE::Firewall::vnet_option_properties; might need a clone to avoid modifying the original reference I think > + > +my $add_option_properties = sub { > + my ($properties) = @_; > + > + foreach my $k (keys %$option_properties) { > + $properties->{$k} = $option_properties->{$k}; > + } > + > + return $properties; > +}; > + > + > +__PACKAGE__->register_method({ > + name => 'set_options', > + path => 'options', > + method => 'PUT', > + description => "Set Firewall options.", > + protected => 1, > + permissions => { > + description => "Needs SDN.Allocate permissions on '/sdn/zones//'", Hmm, I'm wondering might it make sense to add a SDN.Firewall privilege to separate allowing VNet allocation and allowing to configure a VNet's firewall? While adding privs is a bit tricky, this one might be dooable later one too though. But whatever gets chosen should then be also addressed in a commit message with some background reasoning (if it's already then I might have overlooked, I did not checked every all too closely yet). > + user => 'all', > + }, > + parameters => { > + additionalProperties => 0, > + properties => &$add_option_properties({ nit: probably stems from copying this from existing code, but please prefer modern $code_ref->() calling style, i.e.: properties => $add_option_properties->({ alternatively the $add_option_properties could be replaced with a locally scoped sub: my sub add_option_properties { ... > + vnet => get_standard_option('pve-sdn-vnet-id'), > + delete => { > + type => 'string', format => 'pve-configid-list', > + description => "A list of settings you want to delete.", > + optional => 1, > + }, > + digest => get_standard_option('pve-config-digest'), > + }), > + }, > + returns => { type => "null" }, > + code => sub { > + my ($param) = @_; > + > + check_vnet_access($param->{vnet}, ['SDN.Allocate']); > + > + PVE::Firewall::lock_vnetfw_conf($param->{vnet}, 10, sub { > + my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); > + my $vnetfw_conf = PVE::Firewall::load_vnetfw_conf($cluster_conf, 'vnet', $param->{vnet}); > + > + my (undef, $digest) = PVE::Firewall::copy_opject_with_digest($vnetfw_conf->{options}); > + PVE::Tools::assert_if_modified($digest, $param->{digest}); > + > + if ($param->{delete}) { > + foreach my $opt (PVE::Tools::split_list($param->{delete})) { nit: we prefer for over foreach for new code: https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices > + raise_param_exc({ delete => "no such option '$opt'" }) > + if !$option_properties->{$opt}; > + delete $vnetfw_conf->{options}->{$opt}; > + } > + } > + > + if (defined($param->{enable})) { > + $param->{enable} = $param->{enable} ? 1 : 0; > + } > + > + foreach my $k (keys %$option_properties) { same nit w.r.t. foreach as above > + next if !defined($param->{$k}); > + $vnetfw_conf->{options}->{$k} = $param->{$k}; > + } > + > + PVE::Firewall::save_vnetfw_conf($param->{vnet}, $vnetfw_conf); > + }); > + > + return undef; > + }}); > + > +1; _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel