[pve-devel] [PATCH firewall 3/4] config: make groups, IPSets and aliases case-insensitive

[Leo Nunner <l.nunner@proxmox.com>]

So far, the names of groups and IPSets were automatically converted into
lower-case. Aliases were parsed in a *kind of* case-insensitive way, where
both the original name AND the lower-case one was stored.

This patch removes all instances where names are converted to lower-case.

We need some legacy handling for the aliases here, since rules/… will
contain the lower-case name for them, while the definition itself has the
original name. As such, internally, the names still get lower-cased while
resolving aliases; this has no effect on the user.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 src/PVE/API2/Firewall/Aliases.pm | 28 +++++++++++------
 src/PVE/API2/Firewall/Groups.pm  |  4 +--
 src/PVE/API2/Firewall/IPSet.pm   |  4 +--
 src/PVE/Firewall.pm              | 54 ++++++++++++++++++--------------
 4 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index c1af408..9f8e71e 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -72,7 +72,16 @@ my $aliases_to_list = sub {
 
     my $list = [];
     foreach my $k (sort keys %$aliases) {
-	push @$list, $aliases->{$k};
+	my $alias = $aliases->{$k};
+	my $data = {
+	    name => $k,
+	    cidr => $alias->{cidr},
+	    ipversion => $alias->{ipversion},
+	};
+	if(my $comment = $alias->{comment}) {
+	    $data->{comment} = $comment;
+	}
+	push @$list, $data;
     }
     return $list;
 };
@@ -148,10 +157,10 @@ sub register_create_alias {
 
 		my ($fw_conf, $aliases) = $class->load_config($param);
 
-		my $name = lc($param->{name});
+		my $name = $param->{name};
 
 		raise_param_exc({ name => "alias '$param->{name}' already exists" })
-		    if defined($aliases->{$name});
+		    if grep { lc($_) eq lc($name) } (keys $aliases->%*);
 
 		my $data = { name => $param->{name}, cidr => $param->{cidr} };
 		$data->{comment} = $param->{comment} if $param->{comment};
@@ -188,7 +197,7 @@ sub register_read_alias {
 
 	    my ($fw_conf, $aliases) = $class->load_config($param);
 
-	    my $name = lc($param->{name});
+	    my $name = $param->{name};
 
 	    raise_param_exc({ name => "no such alias" })
 		if !defined($aliases->{$name});
@@ -234,20 +243,19 @@ sub register_update_alias {
 
 		PVE::Tools::assert_if_modified($digest, $param->{digest});
 
-		my $rename_to = lc($param->{rename}) if defined($param->{rename});
-		my $rename_from = lc($param->{name});
+		my $rename_to = $param->{rename} if defined($param->{rename});
+		my $rename_from = $param->{name};
 
 		raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
 
-		my $data = { name => $param->{name}, cidr => $param->{cidr} };
+		my $data = { name => $rename_from, cidr => $param->{cidr} };
 		$data->{comment} = $param->{comment} if $param->{comment};
 
 		$aliases->{$rename_from} = $data;
 
 		if ($rename_to && ($rename_from ne $rename_to)) {
 		    raise_param_exc({ name => "alias '$param->{rename}' already exists" })
-			if defined($aliases->{$rename_to});
-		    $aliases->{$rename_from}->{name} = $param->{rename};
+			if grep { lc($_) eq lc($rename_to) } (keys $aliases->%*);
 		    $aliases->{$rename_to} = $aliases->{$rename_from};
 		    delete $aliases->{$rename_from};
 		}
@@ -291,7 +299,7 @@ sub register_delete_alias {
 		my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
 		PVE::Tools::assert_if_modified($digest, $param->{digest});
 
-		my $name = lc($param->{name});
+		my $name = $param->{name};
 		delete $aliases->{$name};
 
 		$class->save_aliases($param, $fw_conf, $aliases);
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index cf9dc06..6d30c53 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -116,7 +116,7 @@ __PACKAGE__->register_method({
 
 		# prevent overwriting an existing group
 		raise_param_exc({ group => "Security group '$rename_to' does already exist" })
-		    if $cluster_conf->{groups}->{$rename_to} &&
+		    if (grep { lc($_) eq lc($rename_to) } (keys $cluster_conf->{groups}->%*)) &&
 		    $rename_to ne $rename_from;
 
 		if ($rename_from eq $rename_to) {
@@ -179,7 +179,7 @@ __PACKAGE__->register_method({
 	    } else {
 		# In this context, $rename_to is the name for the new group
 		raise_param_exc({ group => "Security group '$rename_to' already exists" })
-		    if $cluster_conf->{groups}->{$rename_to};
+		    if grep { lc($_) eq lc($rename_to) } (keys $cluster_conf->{groups}->%*);
 
 		$cluster_conf->{groups}->{$rename_to} = [];
 		$cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment);
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index fee9046..bb7f098 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -660,7 +660,7 @@ sub register_create {
 
 		    # prevent overwriting existing ipset
 		    raise_param_exc({ name => "IPSet '$rename_to' does already exist"})
-			if $fw_conf->{ipset}->{$rename_to} &&
+			if (grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*)) &&
 			$rename_to ne $rename_from;
 
 		    my $data = delete $fw_conf->{ipset}->{$rename_from};
@@ -672,7 +672,7 @@ sub register_create {
 		} else {
 		    # In this context, $rename_to is the name for the new IPSet
 		    raise_param_exc({ name => "IPSet '$rename_to' already exists" })
-		        if $fw_conf->{ipset}->{$rename_to};
+			if grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*);
 
 		    $fw_conf->{ipset}->{$rename_to} = [];
 		    $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment});
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 4924d51..cbb72f5 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1646,18 +1646,17 @@ sub verify_rule {
 		if ($value =~ m/^\+(${ipset_name_pattern})$/) {
 		    &$add_error($name, "no such ipset '$1'")
 			if !($cluster_conf->{ipset}->{$1} || ($fw_conf && $fw_conf->{ipset}->{$1}));
-
 		} else {
 		    &$add_error($name, "invalid ipset name '$value'");
 		}
 	    } elsif ($value =~ m/^${ip_alias_pattern}$/){
-		my $alias = lc($value);
-		&$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 $ipversion;
+
+		eval { (undef, $ipversion) = resolve_alias($cluster_conf, $fw_conf, $value) };
+		&$add_error($name, "no such alias '$value' $@")
+		    if $@;
 
-		&$set_ip_version($e->{ipversion});
+		&$set_ip_version($ipversion);
 	    }
 	}
     };
@@ -2069,11 +2068,8 @@ sub ipt_gen_src_or_dst_match {
 	    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;
-	die "no such alias '$adr'\n" if !$e;
-	$match = "-${dir} $e->{cidr}";
+	my ($cidr) = resolve_alias($cluster_conf, $fw_conf, $adr);
+	$match = "-${dir} $cidr";
     } elsif ($adr =~ m/\-/){
 	$match = "-m iprange --${srcdst}-range $adr";
     } else {
@@ -2919,11 +2915,22 @@ sub parse_clusterfw_option {
 sub resolve_alias {
     my ($clusterfw_conf, $fw_conf, $cidr) = @_;
 
-    my $alias = lc($cidr);
-    my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
-    $e = $clusterfw_conf->{aliases}->{$alias} if !$e && $clusterfw_conf;
+    my $alias = $cidr;
+    my $e;
+
+    # This is a special legacy case: aliases were the only thing that used to be case-insensitive. Due to this, old 
+    # configs can contain upper-case aliases in the definition, but rules contain the lower-case name.
+    if ($fw_conf) {
+	my ($name) = grep { lc($_) eq lc($alias) } keys $fw_conf->{aliases}->%*;
+	$e = $fw_conf->{aliases}->{$name} if $name;
+    }
+
+    if (!$e && $clusterfw_conf) {
+	my ($name) = grep { lc($_) eq lc($alias) } keys $clusterfw_conf->{aliases}->%*;
+	$e = $clusterfw_conf->{aliases}->{$name} if $name;
+    }
 
-    die "no such alias '$cidr'\n" if !$e;;
+    die "no such alias '$cidr'\n" if !$e;
 
     return wantarray ? ($e->{cidr}, $e->{ipversion}) : $e->{cidr};
 }
@@ -2959,12 +2966,11 @@ sub parse_alias {
 	($cidr, $ipversion) = parse_ip_or_cidr($cidr);
 
 	my $data = {
-	    name => $name,
 	    cidr => $cidr,
 	    ipversion => $ipversion,
 	};
-	$data->{comment} = $comment  if $comment;
-	return $data;
+	$data->{comment} = $comment if $comment;
+	return ($name, $data);
     }
 
     return undef;
@@ -3010,7 +3016,7 @@ sub generic_fw_config_parser {
 
 	if ($empty_conf->{groups} && ($line =~ m/^\[group\s+(\S+)\]\s*(?:#\s*(.*?)\s*)?$/i)) {
 	    $section = 'groups';
-	    $group = lc($1);
+	    $group = $1;
 	    my $comment = $2;
 	    eval {
 		die "security group name too long\n" if length($group) > $max_group_name_length;
@@ -3035,7 +3041,7 @@ sub generic_fw_config_parser {
 
 	if ($empty_conf->{ipset} && ($line =~ m/^\[ipset\s+(\S+)\]\s*(?:#\s*(.*?)\s*)?$/i)) {
 	    $section = 'ipset';
-	    $group = lc($1);
+	    $group = $1;
 	    my $comment = $2;
 	    eval {
 		die "ipset name too long\n" if length($group) > $max_ipset_name_length;
@@ -3075,8 +3081,8 @@ sub generic_fw_config_parser {
 	    warn "$prefix: $@" if $@;
 	} elsif ($section eq 'aliases') {
 	    eval {
-		my $data = parse_alias($line);
-		$res->{aliases}->{lc($data->{name})} = $data;
+		my ($name, $data) = parse_alias($line);
+		$res->{aliases}->{$name} = $data;
 	    };
 	    warn "$prefix: $@" if $@;
 	} elsif ($section eq 'rules') {
@@ -3285,7 +3291,7 @@ my $format_aliases = sub {
     $raw .= "[ALIASES]\n\n";
     foreach my $k (keys %$aliases) {
 	my $e = $aliases->{$k};
-	$raw .= "$e->{name} $e->{cidr}";
+	$raw .= "$k $e->{cidr}";
 	$raw .= " # " . encode('utf8', $e->{comment})
 	    if $e->{comment} && $e->{comment} !~ m/^\s*$/;
 	$raw .= "\n";
-- 
2.30.2