[pve-devel] [PATCH firewall/manager] firewall: introduce scoping for ipsets/aliases

[pve-devel] [PATCH firewall/manager] firewall: introduce scoping for ipsets/aliases

[Leo Nunner]

This patch introduces scoping mechanisms for IPsets and aliases, since
its possible to have two of them with the same name on different layers
(i.e. one on the cluster layer, and one on the VM layer). Datacenter
entries are now prefixed with "dc/", and VM entries are prefixed with
"ct/".

The first two patches:

  fix #4556: introduce 'dc' and 'vm' prefix for IPSets
  fix #4556: introduce 'dc' and 'vm' prefix for aliases

should retain backwards compatibility with existing setups. All older
configs will continue to work as previously, since unscoped values
retain the previous behaviour.

  fix #4556: api: return scoped IPSets and aliases
  firewall: add scope field to IPRefSelector

introduce new return values to the API endpoints for /refs, leading to
all new entries that are being added via the GUI being scoped by
default. This will break compatibility with older systems, since the
scoped values cannot be parsed.

firewall:

Leo Nunner (3):
  fix #4556: introduce 'dc' and 'vm' prefix for IPSets
  fix #4556: introduce 'dc' and 'vm' prefix for aliases
  fix #4556: api: return scoped IPSets and aliases

 src/PVE/API2/Firewall/Cluster.pm | 34 ++-------------
 src/PVE/API2/Firewall/IPSet.pm   |  9 ++--
 src/PVE/API2/Firewall/VM.pm      | 47 +++++----------------
 src/PVE/Firewall.pm              | 71 +++++++++++++++++++++-----------
 src/PVE/Firewall/Helpers.pm      | 43 +++++++++++++++++++
 5 files changed, 110 insertions(+), 94 deletions(-)

manager:

Leo Nunner (1):
  firewall: add scope field to IPRefSelector

 www/manager6/form/IPRefSelector.js | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH firewall 1/3] fix #4556: introduce 'dc' and 'vm' prefix for IPSets

[Leo Nunner]

to differentiate whether they should be taken from the datacenter config
or from the local config. The parser now accepts IPSets in the following
format:

    +dc/ipset
	Looks for the IPSet on the Datacenter level.
    +vm/ipset
	Looks for the IPSet on the VM level.
    +ipset
	Uses the previous method of scoping, where it first looks at the
	VM level and then at the Datacenter level.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 src/PVE/Firewall.pm | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 8e40872..ff18de0 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1683,9 +1683,9 @@ sub verify_rule {
 
 	if (my $value = $rule->{$name}) {
 	    if ($value =~ m/^\+/) {
-		if ($value =~ m/^\+(${ipset_name_pattern})$/) {
-		    &$add_error($name, "no such ipset '$1'")
-			if !($cluster_conf->{ipset}->{$1} || ($fw_conf && $fw_conf->{ipset}->{$1}));
+		if ($value =~ m@^\+(vm/|dc/)?(${ipset_name_pattern})$@) {
+		    &$add_error($name, "no such ipset '$2'")
+			if !($cluster_conf->{ipset}->{$2} || ($fw_conf && $fw_conf->{ipset}->{$2}));
 
 		} else {
 		    &$add_error($name, "invalid ipset name '$value'");
@@ -2095,12 +2095,13 @@ sub ipt_gen_src_or_dst_match {
 
     my $match;
     if ($adr =~ m/^\+/) {
-	if ($adr =~ m/^\+(${ipset_name_pattern})$/) {
-	    my $name = $1;
+	if ($adr =~ m@^\+(vm/|dc/)?(${ipset_name_pattern})$@) {
+	    my $scope = $1;
+	    my $name = $2;
 	    my $ipset_chain;
-	    if ($fw_conf && $fw_conf->{ipset}->{$name}) {
+	    if ($scope ne 'dc/' && $fw_conf && $fw_conf->{ipset}->{$name}) {
 		$ipset_chain = compute_ipset_chain_name($fw_conf->{vmid}, $name, $ipversion);
-	    } elsif ($cluster_conf && $cluster_conf->{ipset}->{$name}) {
+	    } elsif ($scope ne 'vm/' && $cluster_conf && $cluster_conf->{ipset}->{$name}) {
 		$ipset_chain = compute_ipset_chain_name(0, $name, $ipversion);
 	    } else {
 		die "no such ipset '$name'\n";
-- 
2.30.2

[pve-devel] [PATCH firewall 2/3] fix #4556: introduce 'dc' and 'vm' prefix for aliases

[Leo Nunner]

since they had the same issue as IPSets, detailed in #4556. The format
works the same as for IPSets:

    dc/alias
	Looks for the alias on the Datacenter level.
    vm/alias
	Looks for the alias on the VM level.
    alias
	Uses the previous method of scoping, where it first looks at the
	VM level and then at the Datacenter level.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 src/PVE/API2/Firewall/IPSet.pm |  9 ++++--
 src/PVE/Firewall.pm            | 58 +++++++++++++++++++++++-----------
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 14bcfcb..515f2d3 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -199,11 +199,14 @@ sub register_create_ip {
 
 		my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
 
-		my $cidr = PVE::Firewall::clean_cidr($param->{cidr});
-		if ($cidr =~ m/^${PVE::Firewall::ip_alias_pattern}$/) {
+		my $cidr = $param->{cidr};
+		if ($cidr =~ m@^(dc/|vm/)?(${PVE::Firewall::ip_alias_pattern})$@) {
+		    my $scope = $1 // "";
+		    my $alias = $2;
 		    # make sure alias exists (if $cidr is an alias)
-		    PVE::Firewall::resolve_alias($cluster_conf, $fw_conf, $cidr);
+		    PVE::Firewall::resolve_alias($cluster_conf, $fw_conf, $alias, $scope);
 		} else {
+		    $cidr = PVE::Firewall::clean_cidr($cidr);
 		    # normalize like config parser, otherwise duplicates might slip through
 		    $cidr = PVE::Firewall::parse_ip_or_cidr($cidr);
 		}
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index ff18de0..ecde176 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -85,7 +85,7 @@ PVE::JSONSchema::register_format('IPorCIDRorAlias', \&pve_verify_ip_or_cidr_or_a
 sub pve_verify_ip_or_cidr_or_alias {
     my ($cidr, $noerr) = @_;
 
-    return if $cidr =~ m/^(?:$ip_alias_pattern)$/;
+    return if $cidr =~ m@^(dc/|vm/)?(?:$ip_alias_pattern)$@;
 
     return pve_verify_ip_or_cidr($cidr, $noerr);
 }
@@ -1067,7 +1067,7 @@ sub parse_address_list {
 	return;
     }
 
-    if ($str =~ m/^${ip_alias_pattern}$/) {
+    if ($str =~ m@^(dc/|vm/)?${ip_alias_pattern}$@) {
 	die "alias name too long\n" if length($str) > $max_alias_name_length;
 	return;
     }
@@ -1690,12 +1690,19 @@ sub verify_rule {
 		} else {
 		    &$add_error($name, "invalid ipset name '$value'");
 		}
-	    } elsif ($value =~ m/^${ip_alias_pattern}$/){
-		my $alias = lc($value);
+	    } elsif ($value =~ m@^(vm/|dc/)?(${ip_alias_pattern})$@){
+		my $scope = $1 // "";
+		my $alias = lc($2);
 		&$add_error($name, "no such alias '$value'")
 		    if !($cluster_conf->{aliases}->{$alias} || ($fw_conf && $fw_conf->{aliases}->{$alias}));
-		my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
-		$e = $cluster_conf->{aliases}->{$alias} if !$e && $cluster_conf;
+
+		my $e;
+		if ($scope ne 'dc/' && $fw_conf) {
+		    $e = $fw_conf->{aliases}->{$alias};
+		}
+		if ($scope ne 'vm/' && !$e && $cluster_conf) {
+		    $e = $cluster_conf->{aliases}->{$alias};
+		}
 
 		&$set_ip_version($e->{ipversion});
 	    }
@@ -2096,7 +2103,7 @@ sub ipt_gen_src_or_dst_match {
     my $match;
     if ($adr =~ m/^\+/) {
 	if ($adr =~ m@^\+(vm/|dc/)?(${ipset_name_pattern})$@) {
-	    my $scope = $1;
+	    my $scope = $1 // "";
 	    my $name = $2;
 	    my $ipset_chain;
 	    if ($scope ne 'dc/' && $fw_conf && $fw_conf->{ipset}->{$name}) {
@@ -2110,10 +2117,16 @@ sub ipt_gen_src_or_dst_match {
 	} else {
 	    die "invalid security group name '$adr'\n";
 	}
-    } elsif ($adr =~ m/^${ip_alias_pattern}$/){
-	my $alias = lc($adr);
-	my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
-	$e = $cluster_conf->{aliases}->{$alias} if !$e && $cluster_conf;
+    } elsif ($adr =~ m@^(dc/|vm/)?(${ip_alias_pattern})$@){
+	my $scope = $1 // "";
+	my $alias = lc($2);
+	my $e;
+	if ($scope ne 'dc/' && $fw_conf) {
+	    $e = $fw_conf->{aliases}->{$alias};
+	}
+	if ($scope ne 'vm/' && !$e && $cluster_conf) {
+	    $e = $cluster_conf->{aliases}->{$alias};
+	}
 	die "no such alias '$adr'\n" if !$e;
 	$match = "-${dir} $e->{cidr}";
     } elsif ($adr =~ m/\-/){
@@ -2964,11 +2977,16 @@ sub parse_clusterfw_option {
 }
 
 sub resolve_alias {
-    my ($clusterfw_conf, $fw_conf, $cidr) = @_;
+    my ($clusterfw_conf, $fw_conf, $cidr, $scope) = @_;
 
     my $alias = lc($cidr);
-    my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
-    $e = $clusterfw_conf->{aliases}->{$alias} if !$e && $clusterfw_conf;
+    my $e;
+    if ($scope ne 'dc/' && $fw_conf) {
+	$e = $fw_conf->{aliases}->{$alias};
+    }
+    if ($scope ne 'vm/' && !$e && $clusterfw_conf) {
+	$e = $clusterfw_conf->{aliases}->{$alias};
+    }
 
     die "no such alias '$cidr'\n" if !$e;;
 
@@ -3156,8 +3174,10 @@ sub generic_fw_config_parser {
 	    }
 
 	    eval {
-		if ($cidr =~ m/^${ip_alias_pattern}$/) {
-		    resolve_alias($cluster_conf, $res, $cidr); # make sure alias exists
+		if ($cidr =~ m@^(dc/|vm/)?(${ip_alias_pattern}$)@) {
+		    my $scope = $1 // "";
+		    my $alias = $2;
+		    resolve_alias($cluster_conf, $res, $alias, $scope); # make sure alias exists
 		} else {
 		    $cidr = parse_ip_or_cidr($cidr);
 		}
@@ -3508,8 +3528,10 @@ sub generate_ipset_chains {
 	    next if $entry->{errors}; # skip entries with errors
 	    eval {
 		my ($cidr, $ver);
-		if ($entry->{cidr} =~ m/^${ip_alias_pattern}$/) {
-		    ($cidr, $ver) = resolve_alias($clusterfw_conf, $fw_conf, $entry->{cidr});
+		if ($entry->{cidr} =~ m@^(dc/|vm/)?(${ip_alias_pattern})$@) {
+		    my $scope = $1 // "";
+		    my $alias = $2;
+		    ($cidr, $ver) = resolve_alias($clusterfw_conf, $fw_conf, $alias, $scope);
 		} else {
 		    ($cidr, $ver) = parse_ip_or_cidr($entry->{cidr});
 		}
-- 
2.30.2

[pve-devel] [PATCH firewall 3/3] fix #4556: api: return scoped IPSets and aliases

[Leo Nunner]

Introduce a new 'scope' field in the return values for the /ref
endpoints. Also add the 'ref' field in the VM endpoint, since it has
been missing up until now.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 src/PVE/API2/Firewall/Cluster.pm | 34 +++--------------------
 src/PVE/API2/Firewall/VM.pm      | 47 +++++++-------------------------
 src/PVE/Firewall/Helpers.pm      | 43 +++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 67 deletions(-)

diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm
index c9c3e67..48ad90d 100644
--- a/src/PVE/API2/Firewall/Cluster.pm
+++ b/src/PVE/API2/Firewall/Cluster.pm
@@ -240,6 +240,9 @@ __PACKAGE__->register_method({
 		ref => {
 		    type => 'string',
 		},
+		scope => {
+		    type => 'string',
+		},
 		comment => {
 		    type => 'string',
 		    optional => 1,
@@ -252,36 +255,7 @@ __PACKAGE__->register_method({
 
 	my $conf = PVE::Firewall::load_clusterfw_conf();
 
-	my $res = [];
-
-	if (!$param->{type} || $param->{type} eq 'ipset') {
-	    foreach my $name (keys %{$conf->{ipset}}) {
-		my $data = {
-		    type => 'ipset',
-		    name => $name,
-		    ref => "+$name",
-		};
-		if (my $comment = $conf->{ipset_comments}->{$name}) {
-		    $data->{comment} = $comment;
-		}
-		push @$res, $data;
-	    }
-	}
-
-	if (!$param->{type} || $param->{type} eq 'alias') {
-	    foreach my $name (keys %{$conf->{aliases}}) {
-		my $e = $conf->{aliases}->{$name};
-		my $data = {
-		    type => 'alias',
-		    name => $name,
-		    ref => $name,
-		};
-		$data->{comment} = $e->{comment} if $e->{comment};
-		push @$res, $data;
-	    }
-	}
-
-	return $res;
+	return PVE::Firewall::Helpers::collect_refs($conf, $param->{type}, "dc");
     }});
 
 1;
diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm
index fb255e0..69cdf54 100644
--- a/src/PVE/API2/Firewall/VM.pm
+++ b/src/PVE/API2/Firewall/VM.pm
@@ -262,6 +262,12 @@ sub register_handlers {
 		    name => {
 			type => 'string',
 		    },
+		    ref => {
+			type => 'string',
+		    },
+		    scope => {
+			type => 'string',
+		    },
 		    comment => {
 			type => 'string',
 			optional => 1,
@@ -275,44 +281,11 @@ sub register_handlers {
 	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 	    my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, $rule_env, $param->{vmid});
 
-	    my $ipsets = {};
-	    my $aliases = {};
-
-	    foreach my $conf (($cluster_conf, $fw_conf)) {
-		next if !$conf;
-		if (!$param->{type} || $param->{type} eq 'ipset') {
-		    foreach my $name (keys %{$conf->{ipset}}) {
-			my $data = {
-			    type => 'ipset',
-			    name => $name,
-			    ref => "+$name",
-			};
-			if (my $comment = $conf->{ipset_comments}->{$name}) {
-			    $data->{comment} = $comment;
-			}
-			$ipsets->{$name} = $data;
-		    }
-		}
-
-		if (!$param->{type} || $param->{type} eq 'alias') {
-		    foreach my $name (keys %{$conf->{aliases}}) {
-			my $e = $conf->{aliases}->{$name};
-			my $data = {
-			    type => 'alias',
-			    name => $name,
-			    ref => $name,
-			};
-			$data->{comment} = $e->{comment} if $e->{comment};
-			$aliases->{$name} = $data;
-		    }
-		}
-	    }
-
-	    my $res = [];
-	    foreach my $e (values %$ipsets) { push @$res, $e; };
-	    foreach my $e (values %$aliases) { push @$res, $e; };
+	    my $dc_refs = PVE::Firewall::Helpers::collect_refs($cluster_conf, $param->{type}, 'dc');
+	    my $vm_refs = PVE::Firewall::Helpers::collect_refs($fw_conf, $param->{type}, 'vm');
 
-	    return $res;
+	    my @ret = (@$dc_refs, @$vm_refs);
+	    return \@ret;
 	}});
 }
 
diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm
index a8e18e2..ca7d26f 100644
--- a/src/PVE/Firewall/Helpers.pm
+++ b/src/PVE/Firewall/Helpers.pm
@@ -15,6 +15,7 @@ our @EXPORT_OK = qw(
 lock_vmfw_conf
 remove_vmfw_conf
 clone_vmfw_conf
+collect_refs
 );
 
 my $pvefw_conf_dir = "/etc/pve/firewall";
@@ -130,4 +131,46 @@ sub dump_fw_logfile {
     return ($state{'count'}, $state{'lines'});
 }
 
+sub collect_refs {
+    my ($conf, $type, $scope) = @_;
+
+    my $ipsets = {};
+    my $aliases = {};
+
+    if (!$type || $type eq 'ipset') {
+	foreach my $name (keys %{$conf->{ipset}}) {
+	    my $data = {
+		type => 'ipset',
+		name => $name,
+		ref => "+$name",
+		scope => "+$scope/$name",
+	    };
+	    if (my $comment = $conf->{ipset_comments}->{$name}) {
+		$data->{comment} = $comment;
+	    }
+	    $ipsets->{$name} = $data;
+	}
+    }
+
+    if (!$type || $type eq 'alias') {
+	foreach my $name (keys %{$conf->{aliases}}) {
+	    my $e = $conf->{aliases}->{$name};
+	    my $data = {
+		type => 'alias',
+		name => $name,
+		ref => $name,
+		scope => "$scope/$name",
+	    };
+	    $data->{comment} = $e->{comment} if $e->{comment};
+	    $aliases->{$name} = $data;
+	}
+    }
+
+    my $res = [];
+    foreach my $e (values %$ipsets) { push @$res, $e; };
+    foreach my $e (values %$aliases) { push @$res, $e; };
+
+    return $res;
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH manager] firewall: add scope field to IPRefSelector

[Leo Nunner]

and send the scoped value to the firewall when choosing new values.
This happens for both IPSets and aliases.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 www/manager6/form/IPRefSelector.js | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
index 9ccc2fe10..69bb139d6 100644
--- a/www/manager6/form/IPRefSelector.js
+++ b/www/manager6/form/IPRefSelector.js
@@ -8,7 +8,7 @@ Ext.define('PVE.form.IPRefSelector', {
 
     ref_type: undefined, // undefined = any [undefined, 'ipset' or 'alias']
 
-    valueField: 'ref',
+    valueField: 'scope',
     displayField: 'ref',
     notFoundIsValid: true,
 
@@ -65,17 +65,27 @@ Ext.define('PVE.form.IPRefSelector', {
 		hideable: false,
 		width: 140,
 	    },
+	    {
+		header: gettext('Scope'),
+		dataIndex: 'scope',
+		hideable: false,
+		width: 140,
+	    },
 	    {
 		header: gettext('Comment'),
 		dataIndex: 'comment',
 		renderer: Ext.String.htmlEncode,
+		minWidth: 60,
 		flex: 1,
 	    },
 	);
 
 	Ext.apply(me, {
 	    store: store,
-            listConfig: { columns: columns },
+            listConfig: {
+		columns: columns,
+		width: 500,
+	    },
 	});
 
 	me.on('change', disable_query_for_ips);
-- 
2.30.2

Re: [pve-devel] [PATCH firewall 2/3] fix #4556: introduce 'dc' and 'vm' prefix for aliases

[Wolfgang Bumiller]

applied 1 & 2 to master + stable-7