Merge pull request #1697 from nicolas-fort/snmp_rework

T4857: SNMP: Implement FRR SNMP Recomendations

7 files changed, 129 insertions, 15 deletions

diff --git a/data/templates/snmp/etc.snmpd.conf.j2 b/data/templates/snmp/etc.snmpd.conf.j2
index a9bbf68ce..793facc3f 100644
--- a/data/templates/snmp/etc.snmpd.conf.j2
+++ b/data/templates/snmp/etc.snmpd.conf.j2
@@ -62,28 +62,47 @@ agentaddress unix:/run/snmpd.socket{{ ',' ~ options | join(',') if options is vy
 {%         if comm_config.client is vyos_defined %}
 {%             for client in comm_config.client %}
 {%                 if client | is_ipv4 %}
-{{ comm_config.authorization }}community {{ comm }} {{ client }}
+{{ comm_config.authorization }}community {{ comm }} {{ client }} -V RESTRICTED
 {%                 elif client | is_ipv6 %}
-{{ comm_config.authorization }}community6 {{ comm }} {{ client }}
+{{ comm_config.authorization }}community6 {{ comm }} {{ client }} -V RESTRICTED
 {%                 endif %}
 {%             endfor %}
 {%         endif %}
 {%         if comm_config.network is vyos_defined %}
 {%             for network in comm_config.network %}
 {%                 if network | is_ipv4 %}
-{{ comm_config.authorization }}community {{ comm }} {{ network }}
+{{ comm_config.authorization }}community {{ comm }} {{ network }} -V RESTRICTED
 {%                 elif network | is_ipv6 %}
-{{ comm_config.authorization }}community6 {{ comm }} {{ network }}
+{{ comm_config.authorization }}community6 {{ comm }} {{ network }} -V RESTRICTED
 {%                 endif %}
 {%             endfor %}
 {%         endif %}
 {%         if comm_config.client is not vyos_defined and comm_config.network is not vyos_defined %}
-{{ comm_config.authorization }}community {{ comm }}
-{{ comm_config.authorization }}community6 {{ comm }}
+{{ comm_config.authorization }}community {{ comm }} -V RESTRICTED
+{{ comm_config.authorization }}community6 {{ comm }} -V RESTRICTED
 {%         endif %}
 {%     endfor %}
 {% endif %}
 
+# Default RESTRICTED view
+view RESTRICTED    included .1 80
+{% if 'ip-route-table' not in oid_enable %}
+# ipRouteTable oid: excluded
+view RESTRICTED    excluded  .1.3.6.1.2.1.4.21
+{% endif %}
+{% if 'ip-net-to-media-table' not in oid_enable %}
+# ipNetToMediaTable oid: excluded
+view RESTRICTED    excluded  .1.3.6.1.2.1.4.22
+{% endif %}
+{% if 'ip-net-to-physical-phys-address' not in oid_enable %}
+# ipNetToPhysicalPhysAddress oid: excluded
+view RESTRICTED    excluded  .1.3.6.1.2.1.4.35
+{% endif %}
+{% if 'ip-forward' not in oid_enable %}
+# ipForward oid: excluded
+view RESTRICTED    excluded  .1.3.6.1.2.1.4.24
+{% endif %}
+
 {% if contact is vyos_defined %}
 # system contact information
 SysContact {{ contact }}
diff --git a/data/templates/snmp/override.conf.j2 b/data/templates/snmp/override.conf.j2
index 5d787de86..443ee64db 100644
--- a/data/templates/snmp/override.conf.j2
+++ b/data/templates/snmp/override.conf.j2
@@ -1,5 +1,4 @@
 {% set vrf_command = 'ip vrf exec ' ~ vrf ~ ' ' if vrf is vyos_defined else '' %}
-{% set oid_route_table = ' ' if oid_enable is vyos_defined('route-table') else '-I -ipCidrRouteTable,inetCidrRouteTable' %}
 [Unit]
 StartLimitIntervalSec=0
 After=vyos-router.service
@@ -8,7 +7,7 @@ After=vyos-router.service
 Environment=
 Environment="MIBDIRS=/usr/share/snmp/mibs:/usr/share/snmp/mibs/iana:/usr/share/snmp/mibs/ietf:/usr/share/vyos/mibs"
 ExecStart=
-ExecStart={{ vrf_command }}/usr/sbin/snmpd -LS0-5d -Lf /dev/null -u Debian-snmp -g Debian-snmp {{ oid_route_table }} -f -p /run/snmpd.pid
+ExecStart={{ vrf_command }}/usr/sbin/snmpd -LS0-5d -Lf /dev/null -u Debian-snmp -g Debian-snmp -f -p /run/snmpd.pid
 Restart=always
 RestartSec=10
 
diff --git a/interface-definitions/include/version/snmp-version.xml.i b/interface-definitions/include/version/snmp-version.xml.i
index 0416288f0..fa58672a5 100644
--- a/interface-definitions/include/version/snmp-version.xml.i
+++ b/interface-definitions/include/version/snmp-version.xml.i
@@ -1,3 +1,3 @@
 <!-- include start from include/version/snmp-version.xml.i -->
-<syntaxVersion component='snmp' version='2'></syntaxVersion>
+<syntaxVersion component='snmp' version='3'></syntaxVersion>
 <!-- include end -->
diff --git a/interface-definitions/snmp.xml.in b/interface-definitions/snmp.xml.in
index 7ec60b2e7..10dd828a5 100644
--- a/interface-definitions/snmp.xml.in
+++ b/interface-definitions/snmp.xml.in
@@ -123,18 +123,31 @@
           </leafNode>
           <leafNode name="oid-enable">
             <properties>
-              <help>Enable specific OIDs</help>
+              <help>Enable specific OIDs that by default are disable</help>
               <completionHelp>
-                <list>route-table</list>
+                <list>ip-forward ip-route-table ip-net-to-media-table ip-net-to-physical-phys-address</list>
               </completionHelp>
               <valueHelp>
-                <format>route-table</format>
-                <description>Enable routing table OIDs (ipCidrRouteTable inetCidrRouteTable)</description>
+                <format>ip-forward</format>
+                <description>Enable ipForward: .1.3.6.1.2.1.4.24</description>
+              </valueHelp>
+              <valueHelp>
+                <format>ip-route-table</format>
+                <description>Enable ipRouteTable: .1.3.6.1.2.1.4.21</description>
+              </valueHelp>
+              <valueHelp>
+                <format>ip-net-to-media-table</format>
+                <description>Enable ipNetToMediaTable: .1.3.6.1.2.1.4.22</description>
+              </valueHelp>
+              <valueHelp>
+                <format>ip-net-to-physical-phys-address</format>
+                <description>Enable ipNetToPhysicalPhysAddress: .1.3.6.1.2.1.4.35</description>
               </valueHelp>
               <constraint>
-                <regex>(route-table)</regex>
+                <regex>(ip-forward|ip-route-table|ip-net-to-media-table|ip-net-to-physical-phys-address)</regex>
               </constraint>
-              <constraintErrorMessage>OID must be 'route-table'</constraintErrorMessage>
+              <constraintErrorMessage>OID must be one of the liste options</constraintErrorMessage>
+              <multi/>
             </properties>
           </leafNode>
           #include <include/snmp/protocol.xml.i>
diff --git a/smoketest/scripts/cli/test_service_snmp.py b/smoketest/scripts/cli/test_service_snmp.py
index e80c689cc..b18b9e7a1 100755
--- a/smoketest/scripts/cli/test_service_snmp.py
+++ b/smoketest/scripts/cli/test_service_snmp.py
@@ -123,6 +123,28 @@ class TestSNMPService(VyOSUnitTestSHIM.TestCase):
         self.assertTrue(process_named_running(PROCESS_NAME))
         self.cli_delete(['interfaces', 'dummy', dummy_if])
 
+        ## Check communities and default view RESTRICTED
+        for auth in ['ro', 'rw']:
+            community = 'VyOS' + auth
+            for addr in clients:
+                if is_ipv4(addr):
+                    entry = auth + 'community ' + community + ' ' + addr + ' -V'
+                else:
+                    entry = auth + 'community6 ' + community + ' ' + addr + ' -V'
+                config = get_config_value(entry)
+                expected = 'RESTRICTED'
+                self.assertIn(expected, config)
+            for addr in networks:
+                if is_ipv4(addr):
+                    entry = auth + 'community ' + community + ' ' + addr + ' -V'
+                else:
+                    entry = auth + 'community6 ' + community + ' ' + addr + ' -V'
+                config = get_config_value(entry)
+                expected = 'RESTRICTED'
+                self.assertIn(expected, config)
+        # And finally check global entry for RESTRICTED view
+        config = get_config_value('view RESTRICTED    included .1')
+        self.assertIn('80', config)
 
     def test_snmpv3_sha(self):
         # Check if SNMPv3 can be configured with SHA authentication
diff --git a/src/conf_mode/snmp.py b/src/conf_mode/snmp.py
index 914ec245c..ab2ccf99e 100755
--- a/src/conf_mode/snmp.py
+++ b/src/conf_mode/snmp.py
@@ -166,6 +166,10 @@ def verify(snmp):
             if 'community' not in trap_config:
                 raise ConfigError(f'Trap target "{trap}" requires a community to be set!')
 
+    if 'oid_enable' in snmp:
+        Warning(f'Custom OIDs are enabled and may lead to system instability and high resource consumption')
+
+
     verify_vrf(snmp)
 
     # bail out early if SNMP v3 is not configured
diff --git a/src/migration-scripts/snmp/2-to-3 b/src/migration-scripts/snmp/2-to-3
new file mode 100755
index 000000000..5f8d9c88d
--- /dev/null
+++ b/src/migration-scripts/snmp/2-to-3
@@ -0,0 +1,57 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2022 VyOS maintainers and contributors
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 or later as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# T4857: Implement FRR SNMP recomendations
+#  cli changes from:
+#  set service snmp oid-enable route-table
+#  To
+#  set service snmp oid-enable ip-forward
+
+import re
+
+from sys import argv
+from sys import exit
+
+from vyos.configtree import ConfigTree
+from vyos.ifconfig import Section
+
+if (len(argv) < 1):
+    print("Must specify file name!")
+    exit(1)
+
+file_name = argv[1]
+
+with open(file_name, 'r') as f:
+    config_file = f.read()
+
+base = ['service snmp']
+config = ConfigTree(config_file)
+
+if not config.exists(base):
+    # Nothing to do
+    exit(0)
+
+if config.exists(base + ['oid-enable']):
+    config.delete(base + ['oid-enable'])
+    config.set(base + ['oid-enable'], 'ip-forward')
+
+
+try:
+    with open(file_name, 'w') as f:
+        f.write(config.to_string())
+except OSError as e:
+    print("Failed to save the modified config: {}".format(e))
+    exit(1)