Merge pull request #1275 from sarthurdev/firewall_limit

firewall: T4345: Fix incorrect firewall rule limit rate format

5 files changed, 18 insertions, 4 deletions

diff --git a/interface-definitions/include/firewall/common-rule.xml.i b/interface-definitions/include/firewall/common-rule.xml.i
index 353804990..cd80b7e28 100644
--- a/interface-definitions/include/firewall/common-rule.xml.i
+++ b/interface-definitions/include/firewall/common-rule.xml.i
@@ -66,11 +66,11 @@
       <properties>
         <help>Maximum average matching rate</help>
         <valueHelp>
-          <format>u32:0-4294967295</format>
-          <description>Maximum average matching rate</description>
+          <format>txt</format>
+          <description>integer/unit (Example: 5/minute)</description>
         </valueHelp>
         <constraint>
-          <validator name="numeric" argument="--range 0-4294967295"/>
+          <regex>^\d+/(second|minute|hour|day)$</regex>
         </constraint>
       </properties>
     </leafNode>
diff --git a/python/vyos/firewall.py b/python/vyos/firewall.py
index 55ce318e7..ff8623592 100644
--- a/python/vyos/firewall.py
+++ b/python/vyos/firewall.py
@@ -174,7 +174,7 @@ def parse_rule(rule_conf, fw_name, rule_id, ip_name):
 
     if 'limit' in rule_conf:
         if 'rate' in rule_conf['limit']:
-            output.append(f'limit rate {rule_conf["limit"]["rate"]}/second')
+            output.append(f'limit rate {rule_conf["limit"]["rate"]}')
             if 'burst' in rule_conf['limit']:
                 output.append(f'burst {rule_conf["limit"]["burst"]} packets')
 
diff --git a/smoketest/configs/dialup-router-complex b/smoketest/configs/dialup-router-complex
index 1b62deb5c..ac5ff5e99 100644
--- a/smoketest/configs/dialup-router-complex
+++ b/smoketest/configs/dialup-router-complex
@@ -498,6 +498,9 @@ firewall {
             destination {
                 port 110,995
             }
+            limit {
+                rate "10/minute"
+            }
             protocol tcp
         }
         rule 123 {
diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
index ecc0c29a0..16b020e07 100755
--- a/smoketest/scripts/cli/test_firewall.py
+++ b/smoketest/scripts/cli/test_firewall.py
@@ -88,6 +88,10 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'destination', 'port', '8888'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'tcp', 'flags', 'syn'])
         self.cli_set(['firewall', 'name', 'smoketest', 'rule', '2', 'tcp', 'flags', 'not', 'ack'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'action', 'accept'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'protocol', 'tcp'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'destination', 'port', '22'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '3', 'limit', 'rate', '5/minute'])
 
         self.cli_set(['interfaces', 'ethernet', 'eth0', 'firewall', 'in', 'name', 'smoketest'])
 
@@ -97,6 +101,7 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
             ['iifname "eth0"', 'jump NAME_smoketest'],
             ['saddr 172.16.20.10', 'daddr 172.16.10.10', 'return'],
             ['tcp flags & (syn | ack) == syn', 'tcp dport { 8888 }', 'reject'],
+            ['tcp dport { 22 }', 'limit rate 5/minute', 'return'],
             ['smoketest default-action', 'drop']
         ]
 
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index 41df1b84a..f33198a49 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -171,6 +171,12 @@ def verify_rule(firewall, rule_conf, ipv6):
         if {'match_frag', 'match_non_frag'} <= set(rule_conf['fragment']):
             raise ConfigError('Cannot specify both "match-frag" and "match-non-frag"')
 
+    if 'limit' in rule_conf:
+        if 'rate' in rule_conf['limit']:
+            rate_int = re.sub(r'\D', '', rule_conf['limit']['rate'])
+            if int(rate_int) < 1:
+                raise ConfigError('Limit rate integer cannot be less than 1')
+
     if 'ipsec' in rule_conf:
         if {'match_ipsec', 'match_non_ipsec'} <= set(rule_conf['ipsec']):
             raise ConfigError('Cannot specify both "match-ipsec" and "match-non-ipsec"')