1 files changed, 121 insertions, 85 deletions

diff --git a/src/conf_mode/snmp.py b/src/conf_mode/snmp.py
index b98741913..69952e5e2 100755
--- a/src/conf_mode/snmp.py
+++ b/src/conf_mode/snmp.py
@@ -21,7 +21,6 @@ import os
 import shutil
 import stat
 import pwd
-import time
 
 import jinja2
 import random
@@ -38,6 +37,7 @@ config_file_client = r'/etc/snmp/snmp.conf'
 config_file_daemon = r'/etc/snmp/snmpd.conf'
 config_file_access = r'/usr/share/snmp/snmpd.conf'
 config_file_user   = r'/var/lib/snmp/snmpd.conf'
+config_file_init   = r'/etc/default/snmpd'
 
 # SNMP OIDs used to mark auth/priv type
 OIDs = {
@@ -59,34 +59,32 @@ clientaddr {{ trap_source }}
 # SNMPS template - be careful if you edit the template.
 access_config_tmpl = """
 ### Autogenerated by snmp.py ###
-{% if v3_users %}
-{% for u in v3_users %}
+{%- for u in v3_users %}
 {{ u.mode }}user {{ u.name }}
-{% endfor %}
-{% endif -%}
+{%- endfor %}
+
 rwuser {{ vyos_user }}
+
 """
 
 # SNMPS template - be careful if you edit the template.
 user_config_tmpl = """
 ### Autogenerated by snmp.py ###
 # user
-{% if v3_users %}
-{% for u in v3_users %}
-{% if u.authOID == 'none' %}
+{%- for u in v3_users %}
+{%- if u.authOID == 'none' %}
 createUser {{ u.name }}
-{% elif u.authPassword %}
+{%- elif u.authPassword %}
 createUser {{ u.name }} {{ u.authProtocol | upper }} "{{ u.authPassword }}" {{ u.privProtocol | upper }} {{ u.privPassword }}
-{% else %}
+{%- else %}
 usmUser 1 3 {{ u.engineID }} "{{ u.name }}" "{{ u.name }}" NULL {{ u.authOID }} {{ u.authMasterKey }} {{ u.privOID }} {{ u.privMasterKey }} 0x
-{% endif %}
-{% endfor %}
-{% endif %}
+{%- endif %}
+{%- endfor %}
 
 createUser {{ vyos_user }} MD5 "{{ vyos_user_pass }}" DES
-{% if v3_engineid %}
+{%- if v3_engineid %}
 oldEngineID {{ v3_engineid }}
-{%- endif -%}
+{%- endif %}
 """
 
 # SNMPS template - be careful if you edit the template.
@@ -123,112 +121,108 @@ monitor  -r 10 -e linkDownTrap "Generate linkDown" ifOperStatus == 2
 ########################
 # configurable section #
 ########################
-
 {% if v3_tsm_key %}
 [snmp] localCert {{ v3_tsm_key }}
-{% endif %}
+{%- endif %}
 
 # Default system description is VyOS version
 sysDescr VyOS {{ version }}
 
-{% if description -%}
+{% if description %}
 # Description
 SysDescr {{ description }}
-{% endif %}
+{%- endif %}
 
 # Listen
 agentaddress unix:/run/snmpd.socket{% if listen_on %}{% for li in listen_on %},{{ li }}{% endfor %}{% else %},udp:161,udp6:161{% endif %}{% if v3_tsm_key %},tlstcp:{{ v3_tsm_port }},dtlsudp::{{ v3_tsm_port }}{% endif %}
 
 # SNMP communities
-{% if communities -%}
-{% for c in communities %}
-{% if c.network -%}
-{% for network in c.network_v4 %}
+{%- for c in communities %}
+{%- if c.network_v4 %}
+{%- for network in c.network_v4 %}
 {{ c.authorization }}community {{ c.name }} {{ network }}
-{% endfor %}
-{% for network in c.network_v6 %}
-{{ c.authorization }}community6 {{ c.name }} {{ network }}
-{% endfor %}
-{% else %}
+{%- endfor %}
+{%- else %}
 {{ c.authorization }}community {{ c.name }}
+{%- endif %}
+{%- if c.network_v6 %}
+{%- for network in c.network_v6 %}
+{{ c.authorization }}community6 {{ c.name }} {{ network }}
+{%- endfor %}
+{%- else %}
 {{ c.authorization }}community6 {{ c.name }}
-{% endif %}
-{% endfor %}
-{% endif %}
+{%- endif %}
+{%- endfor %}
 
-{% if contact -%}
+{% if contact %}
 # system contact information
 SysContact {{ contact }}
-{% endif %}
+{%- endif %}
 
-{% if location -%}
+{% if location %}
 # system location information
 SysLocation {{ location }}
-{% endif %}
+{%- endif %}
 
 {% if smux_peers -%}
 # additional smux peers
-{% for sp in smux_peers %}
+{%- for sp in smux_peers %}
 smuxpeer {{ sp }}
-{% endfor %}
-{% endif %}
+{%- endfor %}
+{%- endif %}
 
 {% if trap_targets -%}
 # if there is a problem - tell someone!
-{% for t in trap_targets %}
+{%- for t in trap_targets %}
 trap2sink {{ t.target }}{% if t.port -%}:{{ t.port }}{% endif %} {{ t.community }}
-{% endfor %}
-{% endif %}
+{%- endfor %}
+{%- endif %}
 
+{%- if v3_enabled %}
 #
 # SNMPv3 stuff goes here
 #
-{% if v3_enabled %}
-
 # views
-{% if v3_views -%}
-{% for v in v3_views %}
-{% for oid in v.oids %}
+{%- for v in v3_views %}
+{%- for oid in v.oids %}
 view {{ v.name }} included .{{ oid.oid }}
-{% endfor %}
-{% endfor %}
-{% endif %}
+{%- endfor %}
+{%- endfor %}
 
 # access
 #             context sec.model sec.level match  read    write  notif
-{% if v3_groups -%}
-{% for g in v3_groups %}
-{% if g.mode == 'ro' %}
-access {{ g.name }} "" usm {{ g.seclevel }} exact {{ g.view }} none none
-access {{ g.name }} "" tsm {{ g.seclevel }} exact {{ g.view }} none none
-{% elif g.mode == 'rw' %}
-access {{ g.name }} "" usm {{ g.seclevel }} exact {{ g.view }} {{ g.view }} none
-access {{ g.name }} "" tsm {{ g.seclevel }} exact {{ g.view }} {{ g.view }} none
-{% endif %}
-{% endfor -%}
-{% endif %}
+{%- for g in v3_groups %}
+access {{ g.name }} "" usm {{ g.seclevel }} exact {{ g.view }} {% if g.mode == 'ro' %}none{% else %}{{ g.view }}{% endif %} none
+access {{ g.name }} "" tsm {{ g.seclevel }} exact {{ g.view }} {% if g.mode == 'ro' %}none{% else %}{{ g.view }}{% endif %} none
+{%- endfor %}
 
 # trap-target
-{% if v3_traps -%}
-{% for t in v3_traps %}
+{%- for t in v3_traps %}
 trapsess -v 3 {{ '-Ci' if t.type == 'inform' }} -e {{ t.engineID }} -u {{ t.secName }} -l {{ t.secLevel }} -a {{ t.authProtocol }} {% if t.authPassword %}-A {{ t.authPassword }}{% elif t.authMasterKey %}-3m {{ t.authMasterKey }}{% endif %} -x {{ t.privProtocol }} {% if t.privPassword %}-X {{ t.privPassword }}{% elif t.privMasterKey %}-3M {{ t.privMasterKey }}{% endif %} {{ t.ipProto }}:{{ t.ipAddr }}:{{ t.ipPort }}
-{% endfor -%}
-{% endif %}
+{%- endfor %}
 
 # group
-{% if v3_users -%}
-{% for u in v3_users %}
+{%- for u in v3_users %}
 group {{ u.group }} usm {{ u.name }}
 group {{ u.group }} tsm {{ u.name }}
 {% endfor %}
-{% endif %}
+{%- endif %}
+"""
 
-{% endif %}
+init_config_tmpl = """
+### Autogenerated by snmp.py ###
+# This file controls the activity of snmpd
 
+# snmpd control (yes means start daemon).
+SNMPDRUN=yes
+
+# snmpd options (use syslog, close stdin/out/err).
+SNMPDOPTS='-LSed -u snmp -g snmp -p /run/snmpd.pid'
 """
 
 default_config_data = {
     'listen_on': [],
+    'listen_address': [],
     'communities': [],
     'smux_peers': [],
     'location' : '',
@@ -281,12 +275,21 @@ def get_config():
             if conf.exists('community {0} authorization'.format(name)):
                 community['authorization'] = conf.return_value('community {0} authorization'.format(name))
 
+            # Subnet of SNMP client(s) allowed to contact system
             if conf.exists('community {0} network'.format(name)):
                 for addr in conf.return_values('community {0} network'.format(name)):
                     if vyos.validate.is_ipv4(addr):
-                        community['network_v4'] = addr
+                        community['network_v4'].append(addr)
                     else:
-                        community['network_v6'] = addr
+                        community['network_v6'].append(addr)
+
+            # IP address of SNMP client allowed to contact system
+            if conf.exists('community {0} client'.format(name)):
+                for addr in conf.return_values('community {0} client'.format(name)):
+                    if vyos.validate.is_ipv4(addr):
+                        community['network_v4'].append(addr)
+                    else:
+                        community['network_v6'].append(addr)
 
             snmp['communities'].append(community)
 
@@ -298,19 +301,20 @@ def get_config():
 
     if conf.exists('listen-address'):
         for addr in conf.list_nodes('listen-address'):
-            listen = ''
             port = '161'
             if conf.exists('listen-address {0} port'.format(addr)):
                 port = conf.return_value('listen-address {0} port'.format(addr))
 
-            if vyos.validate.is_ipv4(addr):
-                # udp:127.0.0.1:161
-                listen = 'udp:' + addr + ':' + port
-            else:
-                # udp6:[::1]:161
-                listen = 'udp6:' + '[' + addr + ']' + ':' + port
+            snmp['listen_address'].append((addr, port))
 
-            snmp['listen_on'].append(listen)
+        # Always listen on localhost if an explicit address has been configured
+        # This is a safety measure to not end up with invalid listen addresses
+        # that are not configured on this system. See https://phabricator.vyos.net/T850
+        if not '127.0.0.1' in conf.list_nodes('listen-address'):
+            snmp['listen_address'].append(('127.0.0.1', '161'))
+
+        if not '::1' in conf.list_nodes('listen-address'):
+            snmp['listen_address'].append(('::1', '161'))
 
     if conf.exists('location'):
         snmp['location'] = conf.return_value('location')
@@ -585,6 +589,24 @@ def verify(snmp):
                 if not os.path.isfile('/config/snmp/tls/certs/' + snmp['v3_tsm_key']):
                     raise ConfigError('TSM key must be fingerprint or filename in "/config/snmp/tls/certs/" folder')
 
+    for listen in snmp['listen_address']:
+        addr = listen[0]
+        port = listen[1]
+
+        if vyos.validate.is_ipv4(addr):
+            # example: udp:127.0.0.1:161
+            listen = 'udp:' + addr + ':' + port
+        else:
+            # example: udp6:[::1]:161
+            listen = 'udp6:' + '[' + addr + ']' + ':' + port
+
+        # We only wan't to configure addresses that exist on the system.
+        # Hint the user if they don't exist
+        if vyos.validate.is_addr_assigned(addr):
+            snmp['listen_on'].append(listen)
+        else:
+            print('WARNING: SNMP listen address {0} not configured!'.format(addr))
+
     if 'v3_groups' in snmp.keys():
         for group in snmp['v3_groups']:
             #
@@ -711,29 +733,35 @@ def generate(snmp):
         return None
 
     # Write client config file
-    tmpl = jinja2.Template(client_config_tmpl, trim_blocks=True)
+    tmpl = jinja2.Template(client_config_tmpl)
     config_text = tmpl.render(snmp)
     with open(config_file_client, 'w') as f:
         f.write(config_text)
 
     # Write server config file
-    tmpl = jinja2.Template(daemon_config_tmpl, trim_blocks=True)
+    tmpl = jinja2.Template(daemon_config_tmpl)
     config_text = tmpl.render(snmp)
     with open(config_file_daemon, 'w') as f:
         f.write(config_text)
 
     # Write access rights config file
-    tmpl = jinja2.Template(access_config_tmpl, trim_blocks=True)
+    tmpl = jinja2.Template(access_config_tmpl)
     config_text = tmpl.render(snmp)
     with open(config_file_access, 'w') as f:
         f.write(config_text)
 
     # Write access rights config file
-    tmpl = jinja2.Template(user_config_tmpl, trim_blocks=True)
+    tmpl = jinja2.Template(user_config_tmpl)
     config_text = tmpl.render(snmp)
     with open(config_file_user, 'w') as f:
         f.write(config_text)
 
+    # Write init config file
+    tmpl = jinja2.Template(init_config_tmpl)
+    config_text = tmpl.render(snmp)
+    with open(config_file_init, 'w') as f:
+        f.write(config_text)
+
     return None
 
 def apply(snmp):
@@ -767,9 +795,17 @@ def apply(snmp):
         # start SNMP daemon
         os.system("sudo systemctl restart snmpd.service")
 
-        # the passwords are not available immediately so this is a workaround
-        # and should be changed to polling
-        time.sleep(2)
+        # Passwords are not available immediately in the configuration file,
+        # after daemon startup - we wait until they have been processed by
+        # snmpd, which we see when a magic line appears in this file.
+        snmpReady = False
+        while not snmpReady:
+            with open(config_file_user, 'r') as f:
+                for line in f:
+                    # Search for our magic string inside the file
+                    if '**** DO NOT EDIT THIS FILE ****' in line:
+                        snmpReady = True
+                        break
 
         # Back in the Perl days the configuration was re-read and any
         # plaintext password inside the configuration was replaced by