[change] OpenVPN: just use common_name as ID #94

Allow duplicate common names with duplicate_cn.

Closes #94

README.rst

@@ -389,6 +389,27 @@ TopologyRetrievalError
 Raised when it is not possible to retrieve the topology data
 (eg: the URL might be temporary unreachable).
 
+Specialized features
+--------------------
+
+OpenVPN
+~~~~~~~
+
+By default, the OpenVPN parser uses the common name to identify a client,
+this was chosen because if the public IP address is used, the same client
+will not be recognized if it connects with a different IP address
+(very probable since many ISPs use dynamic public IP addresses).
+
+This does not work when the vpn server configuration allows different clients
+to use the same common name (which is generally not recommended anyway).
+
+If you need to support legacy systems which are configured with the OpenVPN
+``duplicate-cn`` feature enabled, you can pass ``duplicate_cn=True`` during
+the initialization of ``OpenvpnParser``.
+This will change the behavior of the parser so that each client is identified
+by their common name and IP address (and additionally the port used if there
+are multiple clients with same common name and IP).
+
 Known Issues
 ------------
 

netdiff/parsers/openvpn.py

@@ -11,9 +11,14 @@ class OpenvpnParser(BaseParser):
     protocol = 'OpenVPN Status Log'
     version = '1'
     metric = 'static'
+    duplicate_cn = False
     # for internal use only
     _server_common_name = 'openvpn-server'
 
+    def __init__(self, *args, **kwargs):
+        self.duplicate_cn = kwargs.pop('duplicate_cn', OpenvpnParser.duplicate_cn)
+        super().__init__(*args, **kwargs)
+
     def to_python(self, data):
         if not data:
             return None
@@ -40,14 +45,7 @@ def parse(self, data):
         else:
             clients = data.client_list.values()
             links = data.routing_table.values()
-        id_list = []
-        special_cases = []
-        for client in clients:
-            id_ = f'{client.common_name},{client.real_address.host}'
-            if id_ in id_list:
-                special_cases.append(id_)
-                continue
-            id_list.append(id_)
+        special_cases = self._find_special_cases(clients)
         # add clients in graph as nodes
         for client in clients:
             if client.common_name == 'UNDEF':
@@ -62,6 +60,7 @@ def parse(self, data):
                 ),
                 'bytes_received': int(client.bytes_received),
                 'bytes_sent': int(client.bytes_sent),
+                'common_name': client.common_name,
             }
             local_addresses = [
                 str(route.virtual_address)
@@ -70,19 +69,57 @@ def parse(self, data):
             ]
             if local_addresses:
                 client_properties['local_addresses'] = local_addresses
-            # if there are multiple nodes with the same common name
-            # and host address, add the port to the node ID
-            node_id = f'{client.common_name},{address.host}'
-            if node_id in special_cases:
-                node_id = f'{node_id}:{address.port}'
+            node_id = self.get_node_id(client, special_cases)
             graph.add_node(node_id, **client_properties)
         # add links in routing table to graph
         for link in links:
             if link.common_name == 'UNDEF':
                 continue
-            address = link.real_address
-            target_id = f'{link.common_name},{address.host}'
-            if target_id in special_cases:
-                target_id = f'{target_id}:{address.port}'
+            target_id = self.get_target_id(link, special_cases)
             graph.add_edge(server, str(target_id), weight=1)
         return graph
+
+    def get_node_id(self, client, special_cases):
+        """
+        when duplicate_cn is True
+            if there are multiple nodes with the same common name
+            and host address, add the port to the node ID
+        when self.duplicate_cn is False:
+            just use the common_name as node ID
+        """
+        if not self.duplicate_cn:
+            return client.common_name
+        address = client.real_address
+        node_id = f'{client.common_name},{address.host}'
+        if node_id in special_cases:
+            node_id = f'{node_id}:{address.port}'
+        return node_id
+
+    def get_target_id(self, link, special_cases):
+        """
+        when duplicate_cn is True
+            if there are multiple nodes with the same common name
+            and host address, add the port to the target ID
+        when self.duplicate_cn is False:
+            just use the common_name as target ID
+        """
+        if not self.duplicate_cn:
+            return link.common_name
+        address = link.real_address
+        target_id = f'{link.common_name},{address.host}'
+        if target_id in special_cases:
+            target_id = f'{target_id}:{address.port}'
+        return target_id
+
+    def _find_special_cases(self, clients):
+        if not self.duplicate_cn:
+            return []
+        id_list = []
+        special_cases = []
+        for client in clients:
+            id_ = f'{client.common_name},{client.real_address.host}'
+            if id_ in id_list:
+                special_cases.append(id_)
+                continue
+            id_list.append(id_)
+        return special_cases

tests/test_openvpn.py

@@ -107,7 +107,7 @@ def test_label_diff_added(self):
         self.assertIn('nodeE', labels)
 
     def test_parse_bug(self):
-        p = OpenvpnParser(bug)
+        p = OpenvpnParser(bug, duplicate_cn=True)
         data = p.json(dict=True)
         self.assertIsInstance(p.graph, networkx.Graph)
 
@@ -143,14 +143,51 @@ def test_parse_bug(self):
         }
         self.assertEqual(expected, set(targets))
 
-    def test_parse_special_case(self):
+    def test_parse_bug_duplicate_cn(self):
+        p = OpenvpnParser(bug, duplicate_cn=True)
+        data = p.json(dict=True)
+        self.assertIsInstance(p.graph, networkx.Graph)
+
+        with self.subTest('Count nodes and links'):
+            self.assertEqual(len(data['nodes']), 7)
+            self.assertEqual(len(data['links']), 6)
+
+        labels = []
+        for node in data['nodes']:
+            labels.append(node['label'])
+        expected = {
+            '60c5a8fffe77607a',
+            '60c5a8fffe77606b',
+            '60C5A8FFFE74CB6D',
+            '60c5a8fffe77607a',
+            '58a0cbeffe0176d4',
+            '58a0cbeffe0156b0',
+            '',
+        }
+        with self.subTest('Check contents of nodes'):
+            self.assertEqual(expected, set(labels))
+
+        targets = []
+        for link in data['links']:
+            targets.append(link['target'])
+        expected = {
+            '60c5a8fffe77607a,185.211.160.5',
+            '60c5a8fffe77606b,185.211.160.87',
+            '60C5A8FFFE74CB6D,194.183.10.51',
+            '60c5a8fffe77607a,194.183.10.51',
+            '58a0cbeffe0176d4,195.94.160.52',
+            '58a0cbeffe0156b0,217.72.97.67',
+        }
+        self.assertEqual(expected, set(targets))
+
+    def test_parse_special_case_duplicate_cn(self):
         """
         Tests behavior when the topology contains
         nodes that have the same common name and same address
         (it can happen when allowing reusing the same certificate
          and multiple clients are connected behind the same public IP)
         """
-        p = OpenvpnParser(special_case)
+        p = OpenvpnParser(special_case, duplicate_cn=True)
         data = p.json(dict=True)
         self.assertIsInstance(p.graph, networkx.Graph)
         with self.subTest('Count nodes and links'):
@@ -188,9 +225,10 @@ def test_common_name_as_id(self):
         id_list = []
         for node in result['added']['nodes']:
             id_list.append(node['id'])
+            self.assertEqual(node['id'], node['properties']['common_name'])
         self.assertEqual(len(id_list), 5)
-        self.assertIn('nodeA,2.226.154.66', id_list)
-        self.assertIn('nodeB,93.40.230.50', id_list)
-        self.assertIn('nodeC,95.250.161.57', id_list)
-        self.assertIn('nodeD,79.18.21.144', id_list)
-        self.assertIn('nodeE,87.3.36.166', id_list)
+        self.assertIn('nodeA', id_list)
+        self.assertIn('nodeB', id_list)
+        self.assertIn('nodeC', id_list)
+        self.assertIn('nodeD', id_list)
+        self.assertIn('nodeE', id_list)