From 3433c2b25ddec763f25bf94c3780c6865feaa79e Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 9 Feb 2024 19:04:20 +0530 Subject: [PATCH] [feature] 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 | 2 +- openwisp_controller/config/base/vpn.py | 10 +++++++--- openwisp_controller/config/tests/test_admin.py | 12 +++++++++--- openwisp_controller/config/tests/test_config.py | 4 ++-- openwisp_controller/config/tests/test_vpn.py | 8 +++++--- openwisp_controller/pki/tests/test_api.py | 2 +- 6 files changed, 25 insertions(+), 13 deletions(-) diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 2c9fda910..6e19c6424 100644 --- a/openwisp_controller/config/base/config.py +++ b/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 diff --git a/openwisp_controller/config/base/vpn.py b/openwisp_controller/config/base/vpn.py index ddfec7fea..7a6c8cd53 100644 --- a/openwisp_controller/config/base/vpn.py +++ b/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: diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index f56d973d7..4727b55c7 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/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,13 +1429,16 @@ 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 @@ -1441,14 +1446,15 @@ def _update_template(templates): 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') diff --git a/openwisp_controller/config/tests/test_config.py b/openwisp_controller/config/tests/test_config.py index 3d25f1c6e..203da5166 100644 --- a/openwisp_controller/config/tests/test_config.py +++ b/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() diff --git a/openwisp_controller/config/tests/test_vpn.py b/openwisp_controller/config/tests/test_vpn.py index 4b69d1dc1..e321e6db4 100644 --- a/openwisp_controller/config/tests/test_vpn.py +++ b/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 ) diff --git a/openwisp_controller/pki/tests/test_api.py b/openwisp_controller/pki/tests/test_api.py index e94623aca..668988655 100644 --- a/openwisp_controller/pki/tests/test_api.py +++ b/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)