openvpn: T3074: fix site-2-site operation mode

When rendering the configs "ifconfig" statement wrong IP addresses have been
used for the "tun" operating mode. This has been corrected.

3 files changed, 46 insertions, 22 deletions

diff --git a/data/templates/openvpn/server.conf.tmpl b/data/templates/openvpn/server.conf.tmpl
index ef1f235b0..a1daaa078 100644
--- a/data/templates/openvpn/server.conf.tmpl
+++ b/data/templates/openvpn/server.conf.tmpl
@@ -52,13 +52,14 @@ push "redirect-gateway def1"
 compress lzo
 {% endif %}
 
-{% if 'client' in mode %}
+{% if mode == 'client' %}
 #
 # OpenVPN Client mode
 #
 client
 nobind
-{% elif 'server' in mode %}
+
+{% elif mode == 'server' %}
 #
 # OpenVPN Server mode
 #
@@ -129,6 +130,7 @@ push "route-ipv6 {{ route6 }}"
 push "dhcp-option DNS6 {{ ns6 }}"
 {%     endfor %}
 {%   endif %}
+
 {% else %}
 #
 # OpenVPN site-2-site mode
@@ -136,19 +138,24 @@ push "dhcp-option DNS6 {{ ns6 }}"
 ping {{ keep_alive.interval }}
 ping-restart {{ keep_alive.failure_count }}
 
-{%   for laddr, laddr_conf in local_address.items() if laddr | is_ipv4 %}
-{%     if laddr_conf is defined and laddr_conf.subnet_mask is defined and laddr_conf.subnet_mask is not none %}
+{%   if device_type == 'tap' %}
+{%     for laddr, laddr_conf in local_address.items() if laddr | is_ipv4 %}
+{%       if laddr_conf is defined and laddr_conf.subnet_mask is defined and laddr_conf.subnet_mask is not none %}
 ifconfig {{ laddr }} {{ laddr_conf.subnet_mask }}
-{%     else %}
-{%       for raddr in remote_address %}
-{%         if raddr | is_ipv4 %}
+{%       endif %}
+{%     endfor %}
+{%   else %}
+{%     for laddr in local_address if laddr | is_ipv4 %}
+{%       for raddr in remote_address if raddr | is_ipv4 %}
 ifconfig {{ laddr }} {{ raddr }}
-{%         else %}
+{%       endfor %}
+{%     endfor %}
+{%     for laddr in local_address if laddr | is_ipv6 %}
+{%       for raddr in remote_address if raddr | is_ipv6 %}
 ifconfig-ipv6 {{ laddr }} {{ raddr }}
-{%         endif %}
 {%       endfor %}
-{%     endif %}
-{%   endfor %}
+{%     endfor %}
+{%   endif %}
 {% endif %}
 
 {% if tls is defined and tls is not none %}
diff --git a/smoketest/scripts/cli/test_interfaces_openvpn.py b/smoketest/scripts/cli/test_interfaces_openvpn.py
index 637b42fa0..e636e107d 100755
--- a/smoketest/scripts/cli/test_interfaces_openvpn.py
+++ b/smoketest/scripts/cli/test_interfaces_openvpn.py
@@ -554,7 +554,7 @@ class TestInterfacesOpenVPN(unittest.TestCase):
 
         self.session.commit()
 
-    def test_site2site_interfaces(self):
+    def test_site2site_interfaces_tun(self):
         """
         Create two OpenVPN site-to-site interfaces
         """
@@ -566,13 +566,22 @@ class TestInterfacesOpenVPN(unittest.TestCase):
         for ii in num_range:
             interface = f'vtun{ii}'
             local_address = f'192.0.{ii}.1'
+            local_address_subnet = '255.255.255.252'
             remote_address = f'172.16.{ii}.1'
             path = base_path + [interface]
             port = str(3000 + ii)
 
+            self.session.set(path + ['local-address', local_address])
+
+            # even numbers use tun type, odd numbers use tap type
+            if ii % 2 == 0:
+                self.session.set(path + ['device-type', 'tun'])
+            else:
+                self.session.set(path + ['device-type', 'tap'])
+                self.session.set(path + ['local-address', local_address, 'subnet-mask', local_address_subnet])
+
             self.session.set(path + ['mode', 'site-to-site'])
             self.session.set(path + ['local-port', port])
-            self.session.set(path + ['local-address', local_address])
             self.session.set(path + ['remote-port', port])
             self.session.set(path + ['shared-secret-key-file', s2s_key])
             self.session.set(path + ['remote-address', remote_address])
@@ -589,12 +598,19 @@ class TestInterfacesOpenVPN(unittest.TestCase):
             config_file = f'/run/openvpn/{interface}.conf'
             config = read_file(config_file)
 
+            # even numbers use tun type, odd numbers use tap type
+            if ii % 2 == 0:
+                self.assertIn(f'dev-type tun', config)
+                self.assertIn(f'ifconfig {local_address} {remote_address}', config)
+            else:
+                self.assertIn(f'dev-type tap', config)
+                self.assertIn(f'ifconfig {local_address} {local_address_subnet}', config)
+
             self.assertIn(f'dev {interface}', config)
-            self.assertIn(f'dev-type tun', config)
             self.assertIn(f'secret {s2s_key}', config)
             self.assertIn(f'lport {port}', config)
             self.assertIn(f'rport {port}', config)
-            self.assertIn(f'ifconfig {local_address} {remote_address}', config)
+
 
             self.assertTrue(process_named_running(PROCESS_NAME))
             self.assertEqual(get_vrf(interface), vrf_name)
@@ -644,4 +660,4 @@ if __name__ == '__main__':
     for file in [ca_cert, ssl_cert, ssl_key, dh_pem, s2s_key, auth_key]:
         cmd(f'sudo chown openvpn:openvpn {file}')
 
-    unittest.main(failfast=True)
+    unittest.main()
diff --git a/src/conf_mode/interfaces-openvpn.py b/src/conf_mode/interfaces-openvpn.py
index a4524a59e..0e661c84b 100755
--- a/src/conf_mode/interfaces-openvpn.py
+++ b/src/conf_mode/interfaces-openvpn.py
@@ -120,7 +120,7 @@ def verify(openvpn):
     # OpenVPN site-to-site - VERIFY
     #
     elif openvpn['mode'] == 'site-to-site':
-        if not 'local_address' in openvpn:
+        if 'local_address' not in openvpn and 'is_bridge_member' not in openvpn:
             raise ConfigError('Must specify "local-address" or add interface to bridge')
 
         if len([addr for addr in openvpn['local_address'] if is_ipv4(addr)]) > 1:
@@ -166,15 +166,16 @@ def verify(openvpn):
             if dict_search('remote_host', openvpn) in dict_search('remote_address', openvpn):
                 raise ConfigError('"remote-address" and "remote-host" can not be the same')
 
-
-        if 'local_address' in openvpn:
+        if openvpn['device_type'] == 'tap':
             # we can only have one local_address, this is ensured above
             v4addr = None
             for laddr in openvpn['local_address']:
-                if is_ipv4(laddr): v4addr = laddr
+                if is_ipv4(laddr):
+                    v4addr = laddr
+                    break
 
-            if 'remote_address' not in openvpn and (v4addr not in openvpn['local_address'] or 'subnet_mask' not in openvpn['local_address'][v4addr]):
-                raise ConfigError('IPv4 "local-address" requires IPv4 "remote-address" or IPv4 "local-address subnet"')
+            if v4addr in openvpn['local_address'] and 'subnet_mask' not in openvpn['local_address'][v4addr]:
+                raise ConfigError('Must specify IPv4 "subnet-mask" for local-address')
 
         if dict_search('encryption.ncp_ciphers', openvpn):
             raise ConfigError('NCP ciphers can only be used in client or server mode')