[change] Removing VPN template will not delete related Cert #827

Removing VPN template from a device will not delete the related
Cert object. Instead, it will mark the certificate as revoked.

Closes #827

openwisp_controller/config/base/config.py

@@ -356,7 +356,7 @@ def enforce_required_templates(
 
     @classmethod
     def certificate_updated(cls, instance, created, **kwargs):
-        if created:
+        if created or instance.revoked:
             return
         try:
             config = instance.vpnclient.config

openwisp_controller/config/base/vpn.py

@@ -921,10 +921,14 @@ class method for ``post_delete`` signal
         if instance.vpn._is_backend_type('zerotier'):
             instance.vpn._remove_zt_network_member(instance.zerotier_member_id)
         try:
-            # only deletes related certificates
-            # if auto_cert field is set to True
+            # For OpenVPN, the related certificates are revoked, not deleted.
+            # This is because if the device retains a copy of the certificate,
+            # it could continue using it against the OpenVPN CA.
+            # By revoking the certificate, it gets added to the
+            # Certificate Revocation List (CRL). OpenVPN can then use this
+            # CRL to reject the certificate, thereby ensuring its invalidation.
             if instance.cert and instance.auto_cert:
-                instance.cert.delete()
+                instance.cert.revoke()
         except ObjectDoesNotExist:
             pass
         try:

openwisp_controller/config/tests/test_admin.py

@@ -1399,6 +1399,8 @@ def _update_template(templates):
             auto_cert=True,
         )
         cert_query = Cert.objects.exclude(pk=vpn.cert_id)
+        valid_cert_query = cert_query.filter(revoked=False)
+        revoked_cert_query = cert_query.filter(revoked=True)
 
         # Add a new device
         path = reverse(f'admin:{self.app_label}_device_add')
@@ -1427,28 +1429,32 @@ def _update_template(templates):
             self.assertEqual(config.templates.count(), 1)
             self.assertEqual(config.vpnclient_set.count(), 1)
             self.assertEqual(cert_query.count(), 1)
+            self.assertEqual(valid_cert_query.count(), 1)
 
             # Remove VpnClient template from the device
             _update_template(templates=[])
 
             self.assertEqual(config.templates.count(), 0)
             self.assertEqual(config.vpnclient_set.count(), 0)
-            self.assertEqual(cert_query.count(), 0)
+            # Removing VPN template marks the related certificate as revoked
+            self.assertEqual(revoked_cert_query.count(), 1)
+            self.assertEqual(valid_cert_query.count(), 0)
 
         with self.subTest('Add VpnClient template along with another template'):
             # Adding templates to the device
             _update_template(templates=[template, vpn_template])
 
             self.assertEqual(config.templates.count(), 2)
             self.assertEqual(config.vpnclient_set.count(), 1)
-            self.assertEqual(cert_query.count(), 1)
+            self.assertEqual(valid_cert_query.count(), 1)
 
             # Remove VpnClient template from the device
             _update_template(templates=[template])
 
             self.assertEqual(config.templates.count(), 1)
             self.assertEqual(config.vpnclient_set.count(), 0)
-            self.assertEqual(cert_query.count(), 0)
+            self.assertEqual(valid_cert_query.count(), 0)
+            self.assertEqual(revoked_cert_query.count(), 2)
 
     def test_ip_not_in_add_device(self):
         path = reverse(f'admin:{self.app_label}_device_add')

openwisp_controller/config/tests/test_config.py

@@ -468,7 +468,7 @@ def test_automatically_created_cert_not_deleted_post_clear(self):
         self.assertNotEqual(c.vpnclient_set.count(), 0)
         self.assertNotEqual(cert_model.objects.filter(pk=cert.pk).count(), 0)
 
-    def test_automatically_created_cert_deleted_post_remove(self):
+    def test_automatically_created_cert_revoked_post_remove(self):
         self.test_create_cert()
         c = Config.objects.get(device__name='test-create-cert')
         t = Template.objects.get(name='test-create-cert')
@@ -477,7 +477,7 @@ def test_automatically_created_cert_deleted_post_remove(self):
         cert_model = cert.__class__
         c.templates.remove(t)
         self.assertEqual(c.vpnclient_set.count(), 0)
-        self.assertEqual(cert_model.objects.filter(pk=cert.pk).count(), 0)
+        self.assertEqual(cert_model.objects.filter(pk=cert.pk, revoked=True).count(), 1)
 
     def test_create_cert_false(self):
         vpn = self._create_vpn()

openwisp_controller/config/tests/test_vpn.py

@@ -132,7 +132,7 @@ def test_vpn_client_unique_together(self):
         else:
             self.fail('unique_together clause not triggered')
 
-    def test_vpn_client_auto_cert_deletes_cert(self):
+    def test_vpn_client_auto_cert_revokes_cert(self):
         org = self._get_org()
         vpn = self._create_vpn()
         t = self._create_template(name='vpn-test', type='vpn', vpn=vpn, auto_cert=True)
@@ -143,7 +143,7 @@ def test_vpn_client_auto_cert_deletes_cert(self):
         self.assertEqual(Cert.objects.filter(pk=cert_pk).count(), 1)
         c.delete()
         self.assertEqual(VpnClient.objects.filter(pk=vpnclient.pk).count(), 0)
-        self.assertEqual(Cert.objects.filter(pk=cert_pk).count(), 0)
+        self.assertEqual(Cert.objects.filter(pk=cert_pk, revoked=True).count(), 1)
 
     def test_vpn_client_cert_post_deletes_cert(self):
         org = self._get_org()
@@ -251,7 +251,9 @@ def _assert_vpn_client_cert(cert, vpn_client, cert_ct, vpn_client_ct):
             self.assertEqual(Cert.objects.filter(pk=cert.pk).count(), 1)
             self.assertEqual(VpnClient.objects.filter(pk=vpn_client.pk).count(), 1)
             vpnclient.delete()
-            self.assertEqual(Cert.objects.filter(pk=cert.pk).count(), cert_ct)
+            self.assertEqual(
+                Cert.objects.filter(pk=cert.pk, revoked=False).count(), cert_ct
+            )
             self.assertEqual(
                 VpnClient.objects.filter(pk=vpn_client.pk).count(), vpn_client_ct
             )

openwisp_controller/pki/tests/test_api.py

@@ -287,7 +287,7 @@ def test_post_cert_revoke_api(self):
         cert1 = self._create_cert(name='cert1')
         self.assertFalse(cert1.revoked)
         path = reverse('pki_api:cert_revoke', args=[cert1.pk])
-        with self.assertNumQueries(6):
+        with self.assertNumQueries(5):
             r = self.client.post(path)
         cert1.refresh_from_db()
         self.assertEqual(r.status_code, 200)