Am 15.11.24 um 13:09 schrieb Stefan Hanreich:
> This also includes support for parsing rules referencing IPSets in the
> new SDN scope and generating those IPSets in the firewall.
>
> Loading SDN configuration is optional, since loading it requires root
> privileges which we do not have in all call sites. Adding the flag
> allows us to selectively load the SDN configuration only where
> required and at the same time allows us to only elevate privileges in
> the API where absolutely needed.
>
> Signed-off-by: Stefan Hanreich <s.hanre...@proxmox.com>
> Reviewed-by: Wolfgang Bumiller <w.bumil...@proxmox.com>
> Tested-By: Gabriel Goller <g.gol...@proxmox.com>
> Tested-By: Hannes Dürr <h.du...@proxmox.com>
> ---
> src/PVE/Firewall.pm | 62 +++++++++++++++++++++++++++++----
> src/PVE/Service/pve_firewall.pm | 4 +--
> 2 files changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 09544ba..7642bf6 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -25,6 +25,7 @@ use PVE::Tools qw($IPV4RE $IPV6RE);
> use PVE::Tools qw(run_command lock_file dir_glob_foreach);
>
> use PVE::Firewall::Helpers;
> +use PVE::RS::Firewall::SDN;
>
> my $pvefw_conf_dir = "/etc/pve/firewall";
> my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
> @@ -1689,9 +1690,12 @@ sub verify_rule {
>
> if (my $value = $rule->{$name}) {
> if ($value =~ m/^\+/) {
> - if ($value =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) {
> + if ($value =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@)
> {
> &$add_error($name, "no such ipset '$2'")
> - if !($cluster_conf->{ipset}->{$2} || ($fw_conf &&
> $fw_conf->{ipset}->{$2}));
> + if !($cluster_conf->{ipset}->{$2}
> + || ($fw_conf && $fw_conf->{ipset}->{$2})
> + || ($cluster_conf->{sdn} &&
> $cluster_conf->{sdn}->{ipset}->{$2})
> + || ($fw_conf->{sdn} &&
> $fw_conf->{sdn}->{ipset}->{$2}));
Avoid multi-line post-ifs:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If
rather use something like
if (
!$cluster_conf->{ipset}->{$2}
&& !($fw_conf && $fw_conf->{ipset}->{$2})
&& !($cluster_conf->{sdn} && $cluster_conf->{sdn}->{ipset}->{$2})
&& !($fw_conf->{sdn} && $fw_conf->{sdn}->{ipset}->{$2}))
) {
$add_error->($name, "no such ipset '$2'")
}
>
> } else {
> &$add_error($name, "invalid ipset name '$value'");
> @@ -2108,13 +2112,15 @@ sub ipt_gen_src_or_dst_match {
>
> my $match;
> if ($adr =~ m/^\+/) {
> - if ($adr =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) {
> + if ($adr =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@) {
> my $scope = $1 // "";
> my $name = $2;
A small closer could help to reduce bewlow if expressions, e.g. like:
my $is_scope = sub { return !$scope || $scope eq "$_[0]/" };
#...
if ($is_scope->('guest') && $fw_conf && $fw_conf->{ipset}->{$name}) {
#...
> my $ipset_chain;
> - if ($scope ne 'dc/' && $fw_conf && $fw_conf->{ipset}->{$name}) {
> + if ((!$scope || $scope eq 'guest/') && $fw_conf &&
> $fw_conf->{ipset}->{$name}) {
> $ipset_chain = compute_ipset_chain_name($fw_conf->{vmid},
> $name, $ipversion);
> - } elsif ($scope ne 'guest/' && $cluster_conf &&
> $cluster_conf->{ipset}->{$name}) {
> + } elsif ((!$scope || $scope eq 'dc/') && $cluster_conf &&
> $cluster_conf->{ipset}->{$name}) {
> + $ipset_chain = compute_ipset_chain_name(0, $name, $ipversion);
> + } elsif ((!$scope || $scope eq 'sdn/') && $cluster_conf->{sdn} &&
> $cluster_conf->{sdn}->{ipset}->{$name}) {
> $ipset_chain = compute_ipset_chain_name(0, $name, $ipversion);
> } else {
> die "no such ipset '$name'\n";
> @@ -3644,7 +3650,8 @@ sub lock_clusterfw_conf {
> }
>
> sub load_clusterfw_conf {
> - my ($filename) = @_;
> + my ($filename, $options) = @_;
> +
>
> $filename = $clusterfw_conf_filename if !defined($filename);
> my $empty_conf = {
> @@ -3655,6 +3662,7 @@ sub load_clusterfw_conf {
> group_comments => {},
> ipset => {} ,
> ipset_comments => {},
> + sdn => load_sdn_conf(),
it's a bit odd to assign the full SDN related config to a variable named
$empty_config, but assigning it after the parser will cause a semantic
difference
for the case where the firewall config is empty, not sure if that is fine.
> };
>
> my $cluster_conf = generic_fw_config_parser($filename, $empty_conf,
> $empty_conf, 'cluster');
> @@ -3663,6 +3671,45 @@ sub load_clusterfw_conf {
> return $cluster_conf;
> }
>
> +sub load_sdn_conf {
> + my $rpcenv = eval { PVE::RPCEnvironment::get(); };
Missing use statement for that module, also the semicolon inside the eval can
be dropped.
> +
> + 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 = [];
> + foreach my $vmid (sort keys %{$guests->{ids}}) {
> + next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
> + push @$allowed_vms, $vmid;
> + }
Above one could be:
$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 $sdn_config = {
> + ipset => {} ,
> + ipset_comments => {},
> + };
> +
> + eval {
> + $sdn_config = PVE::RS::Firewall::SDN::config($allowed_vnets,
> $allowed_vms);
> + };
> + warn $@ if $@;
above might be more readable by doing:
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;
> +
> + return $sdn_config;
> +}
> +
> sub save_clusterfw_conf {
> my ($cluster_conf) = @_;
>
> @@ -3768,7 +3815,7 @@ sub compile {
>
> $vmfw_configs = read_vm_firewall_configs($cluster_conf, $vmdata,
> $testdir);
> } else { # normal operation
> - $cluster_conf = load_clusterfw_conf(undef) if !$cluster_conf;
> + $cluster_conf = load_clusterfw_conf() if !$cluster_conf;
>
> $hostfw_conf = load_hostfw_conf($cluster_conf, undef) if !$hostfw_conf;
>
> @@ -4043,6 +4090,7 @@ sub compile_ipsets {
> }
>
> generate_ipset_chains($ipset_ruleset, undef, $cluster_conf, undef,
> $cluster_conf->{ipset});
> + generate_ipset_chains($ipset_ruleset, undef, $cluster_conf, undef,
> $cluster_conf->{sdn}->{ipset});
>
> return $ipset_ruleset;
> }
> diff --git a/src/PVE/Service/pve_firewall.pm b/src/PVE/Service/pve_firewall.pm
> index 65cb2b8..02b507a 100755
> --- a/src/PVE/Service/pve_firewall.pm
> +++ b/src/PVE/Service/pve_firewall.pm
> @@ -158,7 +158,7 @@ __PACKAGE__->register_method ({
>
> PVE::Firewall::set_verbose(1); # show syntax errors
>
> - my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef);
> + my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
just to be sure, above and below does not change anything, or?
> $res->{enable} = $cluster_conf->{options}->{enable} ? 1 : 0;
>
> if ($status eq 'running') {
> @@ -202,7 +202,7 @@ __PACKAGE__->register_method ({
>
> PVE::Firewall::set_verbose(1);
>
> - my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef);
> + my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) =
> PVE::Firewall::compile($cluster_conf, undef, undef);
>
> print "ipset cmdlist:\n";
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
