[pki] Add possibility of renewing certificates #83 #84

NoumbissiValere · 2020-03-18T08:34:14Z

Added a new function in the Ca model which
renews the certificate, private key and
validity end date by calling the generate
function and updating the validity end date.
Also added actions in CA and cert admins to
make use of this newly added function.
Also added a confirmation page to the actions
which Fixes #85

Fixes #83

coveralls · 2020-03-18T08:44:10Z

Coverage increased (+0.1%) to 99.27% when pulling aad30c1 on NoumbissiValere:renew_ca into 056b7b9 on openwisp:master.

NoumbissiValere · 2020-03-18T08:46:32Z

@nemesisdesign Please can you review this and give me feedback?
Thanks

nemesifier

Keep in mind that renewing a certificate means re-issuing it, so also the serial number must change (please put assertions in tests to ensure this).

We need a renew method for the Cert model, Some logic will be shared so it can be put in the base class. The difference is that CA will also renew related certs.

nemesifier · 2020-03-22T01:02:29Z

django_x509/base/admin.py

+        for ca in queryset:
+            ca.renew_ca()
+            renewed_rows += 1
+        message = _('{0} CAs have been successfully renewed'.format(renewed_rows))


please use pluralization to display the correct message depending on the amount of objects on which the operation was performed, see here: https://docs.djangoproject.com/en/3.0/topics/i18n/translation/#pluralization

Follow the exact style used in that example.

nemesifier · 2020-03-22T01:02:35Z

django_x509/base/admin.py

+        for cert in queryset:
+            cert.ca.renew_ca()
+            renewed_rows += 1
+        message = _('{0} Certificates have been successfully renewed'.format(renewed_rows))


please use pluralization to display the correct message depending on the amount of objects on which the operation was performed, see here: https://docs.djangoproject.com/en/3.0/topics/i18n/translation/#pluralization

Follow the exact style used in that example.

nemesifier · 2020-03-22T01:14:43Z

django_x509/base/admin.py

+    def renew_cert(self, request, queryset):
+        renewed_rows = 0
+        for cert in queryset:
+            cert.ca.renew_ca()


why are you renewing the CA? In this case we have to renew only the certificate.

NoumbissiValere · 2020-03-24T18:14:11Z

Hello @nemesisdesign I have updated this PR but travis ci has not gone through it, don't know why. Please can you review it and give me feedback. if there are errors, will update the PR ASAP.
Thanks

nemesifier

Tests locally run fine but the implementation looks still incorrect to me, let me know if I interpreted the code correctly, I also left several other hints for improvement.

nemesifier · 2020-03-27T01:25:07Z

django_x509/base/models.py

+            self.ca.validity_end = default_ca_validity_end()
+            self.ca.serial_number = uuid.uuid4().int
+            self.ca.save()
+            self.validity_end = default_cert_validity_end()


wait a minute. If the model has the ca attribute it means we're dealing with a certificate.

So if the certificate is being renewed, the renewal of the CA is triggered.
If my understanding of this code is correct, the implementation is wrong.

When renewing a certificate, only the single certificate should be renewed.
When renewing a CA, all the certificates related to it should be renewed.

I would keep this private method, but make sure it only acts on self. See below for futher suggestions.

nemesifier · 2020-03-27T01:27:39Z

django_x509/base/models.py

+        Renew the certificate, private key and
+        validity end date of a CA
+        """
+        self._renew()


I would rename this method to just renew, after renewing the CA (calling super()), I would loop over the related certificates and call renew on each of them.

Since the second operation could take potentially very long, I would implement it in a separate method, so that in OpenWISP we can then overwrite the method and move it to a celery task if needed.

nemesifier · 2020-03-27T01:28:41Z

django_x509/base/models.py

@@ -429,6 +429,23 @@ def _add_extensions(self, cert):
            ])
        return cert

+    def _renew(self):


I'd call this just renew

nemesifier · 2020-03-27T01:28:54Z

django_x509/base/models.py

@@ -499,3 +523,10 @@ def revoke(self):
        self.revoked = True
        self.revoked_at = now
        self.save()
+
+    def renew_cert(self):


I'd remove this since it's defined on the base model

nemesifier · 2020-03-27T01:31:38Z

django_x509/base/models.py

+        else:
+            self.validity_end = default_ca_validity_end()
+        self._generate()
+        self.serial_number = uuid.uuid4().int


the instructions to generate a new serial number is already present in the code and is being repeated here, hence it's better to move this to a private method (eg: _generate_serial_number()) to ensure future maintainers won't accidentally forget to upgrade part of the code if needed

nemesifier · 2020-03-27T01:32:22Z

django_x509/tests/test_ca.py

@@ -622,3 +622,15 @@ def test_datetime_to_string(self):
                         generalized_datetime.strftime(generalized_time))
        self.assertEqual(datetime_to_string(utc_datetime),
                         utc_datetime.strftime(utc_time))
+
+    def test_renew_certificate_key(self):
+        ca = self._create_ca()


please generate 2 certs related to the CA and ensure that when the renew is triggered, also the related certs are renewed

nemesifier · 2020-03-27T01:33:12Z

django_x509/tests/test_ca.py

@@ -622,3 +622,15 @@ def test_datetime_to_string(self):
                         generalized_datetime.strftime(generalized_time))
        self.assertEqual(datetime_to_string(utc_datetime),
                         utc_datetime.strftime(utc_time))
+
+    def test_renew_certificate_key(self):


test method name can be just test_renew, keep it simple

nemesifier · 2020-03-27T01:33:25Z

django_x509/tests/test_cert.py

@@ -488,3 +488,23 @@ def test_generate_ca_with_passphrase(self):
        ca.full_clean()
        ca.save()
        self.assertIsInstance(ca.pkey, crypto.PKey)
+
+    def test_renew_certificate_and_key(self):


test method name can be just test_renew, keep it simple

nemesifier · 2020-03-27T01:33:52Z

django_x509/tests/test_cert.py

+        cert = self._create_cert()
+        old_cert = cert.certificate
+        old_key = cert.private_key
+        old_validity_end = cert.validity_end


ca should not be changed.

nemesifier · 2020-03-27T01:44:58Z

django_x509/base/admin.py

+            ca.renew_ca()
+            renewed_rows += 1
+        message = ngettext(
+            '%(renewed_rows)d CA has been successfully renewed',


I think we could make more explicit the fact that also related certificates have been renewed, eg:

CA and its related certificates have been successfully renewed, maybe in this form we don't need to pluralize.

I also thought it would be good to have a confirmation step, but that can be done afterwards, so I created #85

NoumbissiValere · 2020-03-27T13:43:00Z

Thank you for taking out time to review and give me feedback. I think I now see what you have been seeing. I will update the PR ASAP. And also add the confirmation page.

NoumbissiValere · 2020-03-30T20:06:01Z

Good day @nemesisdesign I have updated this PR and also added the confirmation page to handle #85 . while testing the work on the UI with 3 CAs having 3 related certificates, i noticed the admin UI was a little slow and when i think of circumstances where an admin can have 10+ relations, it will really be slow so i was thinking of adding the celery task too to make renewing of related certificates to happen at the background.
Please review the work and tell me if i should add the celery task now or leave it for later.

Thanks

nemesifier

Good progress.

Don't worry about the celery task now.

nemesifier · 2020-03-31T01:53:11Z

requirements.txt

@@ -3,3 +3,4 @@ django-model-utils>=4.0
 jsonfield>=3.1.0,<4.0.0
 cryptography>=2.4.0,<2.9.0
 pyopenssl>=17.5.0,<20.0.0
+openwisp_utils>=0.4.0,<0.5.0


why are you doing this? Please do not add changes that are not requested

I added these because the UI of django_x509 was not like the other openwisp module's. It was still having plain django's UI. I wanted to look like the other modules. But since it was not requested, i will remove it

nemesifier · 2020-03-31T01:53:30Z

tests/settings.py

@@ -21,6 +21,7 @@
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.staticfiles',
+    'openwisp_utils.admin_theme',


nemesifier · 2020-03-31T01:53:51Z

django_x509/templates/admin/django_x509/renew_confirmation.html

+    <div id="content" class="colM">
+        <h1> Are You Sure ? </h1>
+        <p>
+            Are you sure you want to renew the selected Certificates?


all strings need to be translatable.
Also needs a

If renewing a CA we should write "CAs and certificates". Let's use lowercase "certificate" because it's not a person.

nemesifier · 2020-03-31T01:54:07Z

django_x509/templates/admin/django_x509/renew_confirmation.html

+        <h1> Are You Sure ? </h1>
+        <p>
+            Are you sure you want to renew the selected Certificates?
+            All the Certificates listed below will be renewed


final dot is missing

If renewing a CA we should write "CAs and certificates". Let's use lowercase "certificate" because it's not a person.

nemesifier · 2020-03-31T01:56:18Z

django_x509/templates/admin/django_x509/renew_confirmation.html

+
+{% block content %}
+    <div id="content" class="colM">
+        <h1> Are You Sure ? </h1>


should be: <h1>{% trans 'Are You Sure?' %}</h1>

nemesifier · 2020-03-31T02:56:17Z

django_x509/base/admin.py

+            for ca in queryset:
+                ca_count += 1
+                certs = ca.cert_set.all()
+                cert_count += certs.count()


since ca.cert_set.all() will generate 1 query, let's avoid generating another query for counting the objects. do this instead:

cert_count += len(certs)

The query will be evaluated only once (results are cached by django), so it will be used both for counting the objects and displaying them.

nemesifier · 2020-03-31T02:58:56Z

django_x509/base/models.py

+        self.save()
+
+    @classmethod
+    def renew_certs(cls, certs):


this method does not look like its needed just loop over the certs and call renew on them

nemesifier · 2020-03-31T02:59:14Z

django_x509/base/models.py

+        super().renew()
+        certs = self.cert_set.all()
+        self.renew_certs(certs)
+


for cert in self.cert_set.all(): cert.renew()

nemesifier · 2020-03-31T03:00:45Z

django_x509/base/admin.py

+        count = len(queryset)
+        if request.POST.get('post'):
+            self.model.renew_certs(queryset)
+            renewed_rows = count


renewed_rows = 0 for cert in queryset: cert.renew() renewed_rows += 1

Keep consistency with what you did in the cert action.

nemesifier · 2020-03-31T03:00:50Z

django_x509/base/admin.py

@@ -175,6 +207,27 @@ def revoke_action(self, request, queryset):

    revoke_action.short_description = _('Revoke selected certificates')

+    def renew_cert(self, request, queryset):
+        count = len(queryset)


remove this line

NoumbissiValere · 2020-03-31T13:39:14Z

@nemesisdesign I have updated the PR please can you review it and give me feedback?
Thanks

nemesifier

@NoumbissiValere almost there! See my comments.

nemesifier · 2020-04-03T23:05:41Z

django_x509/base/admin.py

+        context = dict()
+        if ca_count:
+            message = _('Are you sure you want to renew the selected CAs and their related certificates? '
+                        'All the CAs listed below with related certificates will be renewed.')


there's no reason to put this here, can you put it back in the template and use blocktranslate if needed please?
use
to separate the two lines.

nemesifier · 2020-04-03T23:07:23Z

django_x509/base/models.py

+        if hasattr(self, 'ca'):
+            self.validity_end = default_cert_validity_end()
+        else:
+            self.validity_end = default_ca_validity_end()


have you tried with:

self.validity_end = self.__class__().validity_end

It would eliminate the need of the if.

nemesifier · 2020-04-03T23:08:19Z

django_x509/templates/admin/django_x509/renew_confirmation.html

+            <input type="hidden" name="post" value="yes">
+            <input type="submit" value="Yes I'm sure">
+            <input type="hidden" name="action" value="{{ action }}">
+            <a href="{% url 'admin:'|add:cancel_url %}" class="button cancel-link"> No, take me back </a>


There's an extra space here before No,

Added a new function in the base class which renews the certificate, private key and validity end date by calling the generate function and updating the validity end date. Also added actions in CA and cert admins to make use of this newly added function. Also, a confirmation page was added to the renew actions which Fixes openwisp#85 Fixes openwisp#83

NoumbissiValere · 2020-04-07T17:35:44Z

@nemesisdesign I have updated this PR as required

nemesifier

Yess 👍

Related to #84 Related to #85

NoumbissiValere force-pushed the renew_ca branch from e74b723 to 0063915 Compare March 18, 2020 08:42

nemesifier requested changes Mar 22, 2020

View reviewed changes

NoumbissiValere force-pushed the renew_ca branch from 0063915 to 703d0d6 Compare March 24, 2020 18:07

NoumbissiValere requested a review from nemesifier March 24, 2020 18:10

nemesifier mentioned this pull request Mar 27, 2020

[admin] Add confirmation step for renew CA action #85

Closed

nemesifier requested changes Mar 27, 2020

View reviewed changes

NoumbissiValere force-pushed the renew_ca branch 2 times, most recently from 60cc9c0 to b3aaefd Compare March 30, 2020 19:49

NoumbissiValere requested a review from nemesifier March 30, 2020 20:06

nemesifier reviewed Mar 31, 2020

View reviewed changes

NoumbissiValere force-pushed the renew_ca branch from b3aaefd to 5a985d9 Compare March 31, 2020 13:36

NoumbissiValere requested a review from nemesifier March 31, 2020 13:39

nemesifier requested changes Apr 3, 2020

View reviewed changes

NoumbissiValere force-pushed the renew_ca branch from 5a985d9 to aad30c1 Compare April 7, 2020 17:30

NoumbissiValere requested a review from nemesifier April 7, 2020 17:35

nemesifier approved these changes Apr 7, 2020

View reviewed changes

nemesifier merged commit 27227b5 into openwisp:master Apr 7, 2020

nemesifier added a commit that referenced this pull request Apr 7, 2020

[admin] Minor improvements to #84

8c79c3e

Related to #84 Related to #85

[pki] Add possibility of renewing certificates #83 #84

[pki] Add possibility of renewing certificates #83 #84

Conversation

NoumbissiValere commented Mar 18, 2020 • edited

coveralls commented Mar 18, 2020 • edited

NoumbissiValere commented Mar 18, 2020

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoumbissiValere commented Mar 24, 2020

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoumbissiValere commented Mar 27, 2020

NoumbissiValere commented Mar 30, 2020

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoumbissiValere commented Mar 31, 2020

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoumbissiValere commented Apr 7, 2020

nemesifier left a comment

Choose a reason for hiding this comment

NoumbissiValere commented Mar 18, 2020 •

edited

coveralls commented Mar 18, 2020 •

edited