Rules can't be deleted from firewall rule sets used in zone policies
Open, NormalPublicBUG

Description

If a single rule is deleted from a firewall rule set used in a zone policy, a superfluous error is produced --

admin@vyos# delete firewall name wan-to-local-ipv4 rule 9
[edit]
admin@vyos# compare
[edit firewall name wan-to-local-ipv4]
-rule 9 {
-    action accept
-    protocol icmp
-}
[edit]
admin@vyos# commit
[ firewall name wan-to-local-ipv4 ]
Firewall configuration error: Cannot delete rule set "wan-to-local-ipv4" (still in use)
 
 
 
[[firewall name wan-to-local-ipv4]] failed
Commit failed

If there are other rules in the rule set then the chain will still exist and can be modified without consequence.

Please see the following patch which resolves the issue by considering it an error condition only to delete *every* rule in a rule set.

# diff vyatta-firewall.pl vyatta-firewall.pl.bak
529,530d528
<     my $all_rules_deleted = 1;
< 
532,560c530,560
<         if ("$test_rule_hash{$test_rule}" ne 'deleted') {
<             $all_rules_deleted = 0;       
< 
<             if ("$test_rule_hash{$test_rule}" eq 'static') {
<                 next;
<             } elsif ("$test_rule_hash{$test_rule}" eq 'added') {
<                 my $test_node = new Vyatta::IpTables::Rule;
<                 $test_node->setup("$tree $name rule $test_rule");
<                 $test_node->set_ip_version($ip_version_hash{$tree});
<                 my ($err_str, @rule_strs) = $test_node->rule();
<                 if (defined($err_str)) {
<                     Vyatta::Config::outputError([$tree,$name],"Firewall configuration error: $err_str\n");
<                     exit 1;
<                 }
<                 my $test_chain = chain_configured(2, $name, $tree);
<                 if (defined($test_chain)) {
<                     # Chain name must be unique in both trees
<                     Vyatta::Config::outputError([$tree,$name], "Firewall configuration error: Rule set name \"$name\" already used in \"$test_chain\"\n");
<                     exit 1;
<                 }
<             } elsif ("$test_rule_hash{$test_rule}" eq 'changed') {
<                 my $test_node = new Vyatta::IpTables::Rule;
<                 $test_node->setup("$tree $name rule $test_rule");
<                 $test_node->set_ip_version($ip_version_hash{$tree});
<                 my ($err_str, @rule_strs) = $test_node->rule();
<                 if (defined($err_str)) {
<                     Vyatta::Config::outputError([$tree,$name],"Firewall configuration error: $err_str\n");
<                     exit 1;
<                 }
---
>         if ("$test_rule_hash{$test_rule}" eq 'static') {
>             next;
>         } elsif ("$test_rule_hash{$test_rule}" eq 'added') {
>             my $test_node = new Vyatta::IpTables::Rule;
>             $test_node->setup("$tree $name rule $test_rule");
>             $test_node->set_ip_version($ip_version_hash{$tree});
>             my ($err_str, @rule_strs) = $test_node->rule();
>             if (defined($err_str)) {
>                 Vyatta::Config::outputError([$tree,$name],"Firewall configuration error: $err_str\n");
>                 exit 1;
>             }
>             my $test_chain = chain_configured(2, $name, $tree);
>             if (defined($test_chain)) {
>                 # Chain name must be unique in both trees
>                 Vyatta::Config::outputError([$tree,$name], "Firewall configuration error: Rule set name \"$name\" already used in \"$test_chain\"\n");
>                 exit 1;
>             }
>         } elsif ("$test_rule_hash{$test_rule}" eq 'changed') {
>             my $test_node = new Vyatta::IpTables::Rule;
>             $test_node->setup("$tree $name rule $test_rule");
>             $test_node->set_ip_version($ip_version_hash{$tree});
>             my ($err_str, @rule_strs) = $test_node->rule();
>             if (defined($err_str)) {
>                 Vyatta::Config::outputError([$tree,$name],"Firewall configuration error: $err_str\n");
>                 exit 1;
>             }
>         } elsif ("$test_rule_hash{$test_rule}" eq 'deleted') {
>             if (Vyatta::IpTables::Mgr::chain_referenced($table, $name, $iptables_cmd)) {
>                 # Disallow deleting a chain if it's still referenced
>                 Vyatta::Config::outputError([$tree,$name],"Firewall configuration error: Cannot delete rule set \"$name\" (still in use)\n");
>                 exit 1;
564,571d563
< 
< 
<     if ($all_rules_deleted and Vyatta::IpTables::Mgr::chain_referenced($table, $name, $iptables_cmd)) {
<         # Disallow deleting a chain if it's still referenced
<         Vyatta::Config::outputError([$tree,$name],"Firewall configuration error: Cannot delete rule set \"$name\" (still in use)\n");
<         exit 1;
<     }
<

Details

Difficulty level
Easy (less than an hour)
Version
999.201711232137
Why the issue appeared?
Will be filled on close
Tiberius created this task.Dec 3 2017, 12:14 AM

At a glance, a lot more looks wrong here than just this. Why is it checking for every rule in the rule set if the rule set is uniquely named?

Hi @Tiberius,

I appreciate your work! Could you make the patch easier for us to merge and then to track for release and changelog?
Here's the proper procedure for making patches: https://wiki.vyos.net/wiki/Submit_a_patch

If the bug only appears in 1.2.0 nightly builds, the git branch should be "current" and you do not need to worry about "helium" (1.1.x). If it's not unique to 1.2.0, however, we'll also need to cherry-pick it into helium, so it may be better to start with helium and import it into current instead.

Tiberius changed Version from 1.2 to 999.201711232137.Dec 4 2017, 1:37 AM

Here it is

syncer assigned this task to dmbaturin.Dec 21 2017, 9:10 PM
syncer triaged this task as Normal priority.
syncer added a subscriber: syncer.

@dmbaturin can you merge it in