Page MenuHomeVyOS Platform

Rules can't be deleted from firewall rule sets used in zone policies
Closed, ResolvedPublicBUG

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
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

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 triaged this task as Normal priority.
syncer added a subscriber: syncer.

@dmbaturin can you merge it in

@c-po can you probably merge and close this one

Patch does not apply cleanly, need to backport it but will do

Please retest with a new rolling release tomorrow

syncer moved this task from Need Triage to Finished on the VyOS 1.3 Equuleus board.

@c-po need to backport it into the crux branch

c-po moved this task from Needs Triage to Finished on the VyOS 1.2 Crux (VyOS 1.2.1) board.
dmbaturin set Is it a breaking change? to Unspecified (possibly destroys the router).Sep 3 2021, 7:30 AM
dmbaturin set Issue type to Bug (incorrect behavior).