[change] Migrate from PyOpenSSL to cryptography #131 #194

stktyagi · 2025-12-27T09:34:02Z

Replaced PyOpenSSL with cryptography with retained functionalities.

Fixes #131

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Closes #131.

Description of Changes

Migrated the cryptographic engine from PyOpenSSL to the cryptography library and resolved the renewal bug by fixing the serial number update order.

Summary by CodeRabbit

Refactor
- Migrated all certificate operations from OpenSSL to the cryptography library, including parsing, generation, signing, validation, renewal, CRL building, extension handling, and improved human-readable certificate rendering.
New Features
- Certificate text output improved and additional extension types supported; CA renewal now propagates to issued certificates with serial synchronization.
Chores
- Removed pyOpenSSL dependency in favor of cryptography.
Tests
- Updated tests for cryptography objects and OID-based assertions; added UTC datetime helper and expanded renewal, serial-sync, and extension coverage.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Replaced PyOpenSSL with cryptography with retained functionalities. Fixes #131

stktyagi · 2025-12-27T09:36:42Z

Closes #131 #193

coveralls · 2025-12-27T10:08:32Z

coverage: 95.411% (-3.9%) from 99.359%
when pulling ea11aed on issues/131-drop-pyopenssl
into 2066447 on master.

nemesifier

Quick review

PEM validation now catches generic Exception, which can hide the real cause of failures and make debugging harder.

The use of default_backend() is deprecated in recent cryptography versions and may cause warnings or issues in the future.

The issuer check (cert.issuer == ca_cert.subject) may be too strict and could reject otherwise valid certificates due to encoding or ordering differences.

Extension parsing is somewhat permissive and may silently ignore unsupported or malformed values instead of failing fast.

What do you think?

stktyagi · 2025-12-27T11:37:03Z

@nemesifier
thanks for the review, before I add a commit in references to these changes, this is how I am thinkning about those concerns. Might save some time for you to check than checking the commit directly.

Instead of a generic except Exception, I can update the validation to catch only ValueError, TypeError, and UnsupportedAlgorithm. In that case unrelated logic or system errors would bubble up with a proper traceback. I also rechecked that i had a redundant except Exception block in the private key condition that was masking similar issues, so I'll fix that too.

Yeah, I have removed all references to default_backend() to not cause any issues.

This was a bit tricky, you are totally right here. I initially tried rfc4514_string(), but shell testing confirmed it is still sequence-dependent and doesn't solve the ordering issue. Comparing canonical sets of attributes regardless position would help here, like:

issuer_identity = set((attr.oid, str(attr.value).strip().lower()) for attr in cert.issuer)
ca_identity = set((attr.oid, str(attr.value).strip().lower()) for attr in ca_cert.subject)

We can make it failfast by checking immediately for required structure and supported values, like:

 for ext in self.extensions:
    if not all(k in ext for k in ("name", "critical", "value")):
        raise ValidationError("Each extension must have 'name', 'critical', and 'value'.")

    if ext["name"] == "extendedKeyUsage":
        allowed = ["serverAuth", "clientAuth"]
        provided = [v.strip() for v in ext["value"].split(",")]
        for p in provided:
            if p not in allowed:
                raise ValidationError(f"Unsupported EKU: {p}")

nemesifier

Not exhaustive but I was able to scan at least 50% of the code.

django_x509/tests/test_cert.py

django_x509/base/models.py

nemesifier · 2025-12-30T23:44:36Z

@coderabbitai review

coderabbitai · 2025-12-30T23:44:41Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-12-30T23:44:44Z

📝 Walkthrough

Walkthrough

Replaces pyOpenSSL usage with the cryptography library across models, tests, CRL generation, PEM validation, certificate building/signing, verification, extension handling, and renewal propagation; requirements drop pyOpenSSL.

Changes

Cohort / File(s)	Summary
Core model migration `django_x509/base/models.py`	Reimplemented certificate/key handling with `cryptography`: `x509` returns `cryptography.x509.Certificate`; `pkey` loads `serialization.load_pem_private_key`; `_generate()` uses `CertificateBuilder` + `rsa.generate_private_key`; `_get_subject()` replaces `_fill_subject()` using `NameOID`; `_add_extensions()` now accepts/returns a `CertificateBuilder` and attaches BasicConstraints, KeyUsage, SubjectKeyIdentifier, AuthorityKeyIdentifier, ExtendedKeyUsage, nsComment, nsCertType; `_validate_pem()`, `_import()`, `_verify_ca()` and CRL building/signing migrated to cryptography; renewal propagation updated to prefer `issued_certificates` then `cert_set`.
Tests updated to cryptography `django_x509/tests/test_ca.py`, `django_x509/tests/test_cert.py`, `django_x509/tests/__init__.py`	Tests converted from pyOpenSSL to cryptography APIs: use `.subject`/`.issuer` and `NameOID` attribute access, extension retrieval via `get_extension_for_class`/`get_extension_for_oid`, serial/validity via cryptography properties; added `UTC_TIME`/`GENERALIZED_TIME` constants and `datetime_to_string` helper; updated renewal and serial-sync expectations and PEM/key loading assertions.
Dependency removal `requirements.txt`	Removed `pyOpenSSL~=25.3.0` from requirements.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Model as Model method
  participant Builder as CertificateBuilder
  participant Crypto as Cryptography primitives
  participant CAKey as CA Private Key
  participant DB as Database

  rect rgba(0,128,96,0.08)
  Note over Model,Builder: Certificate generation / renewal (cryptography)
  Model->>Builder: call _get_subject(), set subject, issuer, serial, validity
  Model->>Crypto: generate or load private/public key
  Model->>Builder: add public_key
  Model->>Builder: call _add_extensions(builder, public_key)
  Builder->>CAKey: sign(builder, chosen hash algorithm)
  CAKey-->>Builder: cryptography.x509.Certificate (signed)
  Builder->>DB: persist PEM (certificate ± private key)
  DB-->>Model: persisted certificate record
  end

  alt Renewal with issued certs
    Note over Model: propagate renew to children
    Model->>Model: iterate issued_certificates / cert_set
    Model->>Builder: regenerate child certs signed by renewed CA
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I hopped from pyOpenSSL to cryptography's glen,
NameOIDs in my paw and builders in my den,
I signed little keys, chased expiries anew,
Renewed tiny certs — a bright carrot-peek view,
PEMs snug like clover, every fingerprint true.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.66% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: migrating from PyOpenSSL to cryptography as referenced in issue #131.
Description check	✅ Passed	The description follows the template and provides key information: migration scope, referenced issue, completed checklist items, and acknowledgment of outstanding documentation updates.
Linked Issues check	✅ Passed	The PR successfully addresses issue #131's coding requirements: migrates from pyOpenSSL to cryptography, preserves existing functionality, and fixes the renewal serial number bug.
Out of Scope Changes check	✅ Passed	All changes are directly related to the migration objectives: replacement of OpenSSL operations with cryptography equivalents, test updates, dependency changes, and helper functions for the new implementation.

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a8ecfc3 and ea11aed.

📒 Files selected for processing (1)

django_x509/base/models.py

🔇 Additional comments (10)

django_x509/base/models.py (10)

6-10: LGTM! Cryptography imports are well-organized.

The migration to the cryptography library is properly reflected in the imports, with all necessary modules, exceptions, and OIDs included.

190-218: LGTM! Certificate property methods correctly use cryptography APIs.

The x509 and x509_text properties have been properly migrated to use cryptography's certificate loading and attribute access patterns.

220-230: LGTM! Private key loading correctly migrated to cryptography.

The pkey property properly uses serialization.load_pem_private_key with appropriate password handling.

232-261: LGTM! PEM validation uses specific exception handling.

The validation correctly catches specific exceptions (ValueError, TypeError, UnsupportedAlgorithm) and provides appropriate error messages. The error message parsing to distinguish passphrase from key errors is a reasonable approach.

275-347: LGTM! Certificate generation correctly uses cryptography's CertificateBuilder.

The _generate method properly:

Generates RSA keys with appropriate parameters

Constructs the certificate builder with all required fields

Maps digest algorithms correctly

Signs the certificate with the appropriate key and algorithm

Serializes both certificate and private key with proper encryption

The _get_subject method correctly maps model attributes to x509.NameAttribute objects.

349-445: LGTM! Certificate import and CA verification properly migrated.

The _import method correctly extracts certificate attributes using cryptography's API, including proper handling of email addresses from either SAN or subject.

The _verify_ca method has been properly fixed per previous review feedback:

Order-independent issuer/CA comparison using set-based attribute matching

Proper exception chaining with from None

Signature verification correctly handles RSA and EC keys using appropriate padding/algorithms

461-597: LGTM! Extension handling implements fail-fast validation as intended.

The _add_extensions method has been properly updated per previous review feedback:

extendedKeyUsage: Validates all values and raises ValidationError for unknown entries

nsComment: Validates length (max 255 bytes) and requires a value

nsCertType: Properly maps values to bit representations and validates them

Fallback: Raises ValidationError for any unsupported extension types

The fail-fast approach ensures that misconfigurations are caught early rather than silently ignored.

599-606: LGTM! Certificate renewal logic is correct.

The renew method properly:

Generates a new serial number

Updates validity end date based on whether it's a CA or certificate

Regenerates the certificate using the new cryptography-based flow

632-642: LGTM! CA renewal propagation handles different relationship names.

The CA renewal method correctly:

Calls the parent renew() to renew the CA itself

Uses getattr with fallback to handle both issued_certificates and cert_set relationships

Propagates renewal to all child certificates

644-679: LGTM! CRL generation correctly uses cryptography APIs.

The crl property properly:

Loads the CA certificate and private key with password handling

Constructs the CRL using CertificateRevocationListBuilder

Adds revoked certificates with appropriate extensions

Signs the CRL with the CA's private key

Returns the CRL in PEM format

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 4

♻️ Duplicate comments (3)

django_x509/tests/test_cert.py (1)

16-24: DRY violation: Same duplication as in test_ca.py.

As noted in past review comments, these time constants and helper function should be moved to a shared module.
django_x509/base/models.py (1)
444-449: Replace broad except Exception with specific cryptography exceptions.

This duplicates the concern from past review comments. The broad exception catch can mask unrelated errors. Use specific exceptions like InvalidSignature from cryptography.exceptions.
🔎 Proposed fix
+from cryptography.exceptions import InvalidSignature, UnsupportedAlgorithm

# ... in _verify_ca method:
         try:
             if isinstance(ca_pubkey, rsa.RSAPublicKey):
                 ca_pubkey.verify(
                     cert.signature,
                     cert.tbs_certificate_bytes,
                     padding.PKCS1v15(),
                     cert.signature_hash_algorithm,
                 )
             elif isinstance(ca_pubkey, ec.EllipticCurvePublicKey):
                 ca_pubkey.verify(
                     cert.signature,
                     cert.tbs_certificate_bytes,
                     ec.ECDSA(cert.signature_hash_algorithm),
                 )
             else:
                 ca_pubkey.verify(cert.signature, cert.tbs_certificate_bytes)
-        except Exception:
+        except InvalidSignature:
             raise ValidationError(
                 _("Cryptographic signature verification failed: CA does not match.")
-            )
+            ) from None
django_x509/tests/test_ca.py (1)
20-33: DRY violation: datetime_to_string and time constants are duplicated.

Per the past review comment, these constants (generalized_time, utc_time) and the datetime_to_string function are duplicated between test_ca.py and test_cert.py. They should be moved to a shared location like __init__.py.
🔎 Proposed refactor - move to django_x509/tests/__init__.py

In django_x509/tests/__init__.py:
generalized_time = "%Y%m%d%H%M%SZ"
utc_time = "%y%m%d%H%M%SZ"

def datetime_to_string(datetime_):
    if datetime_.year < 2050:
        return datetime_.strftime(utc_time)
    return datetime_.strftime(generalized_time)
Then import in both test files:
from . import TestX509Mixin, datetime_to_string, generalized_time, utc_time

🧹 Nitpick comments (3)

django_x509/base/models.py (2)

618-650: Remove default_backend() usage in CRL generation for consistency.

Similar to the earlier comment, default_backend() is deprecated and should be removed for consistency with modern cryptography library usage.

🔎 Proposed fix

     def crl(self):
         """
         Returns up to date CRL of this CA
         """
-        backend = default_backend()
         ca_cert = x509.load_pem_x509_certificate(self.certificate.encode())
-        # import ipdb; ipdb.set_trace()
-        pkey_kwargs = {"backend": backend, "password": None}
+        pkey_kwargs = {"password": None}
         if self.passphrase:
             pkey_kwargs["password"] = self.passphrase.encode()
         private_key = serialization.load_pem_private_key(
             self.private_key.encode(), **pkey_kwargs
         )
         now = timezone.now()
         builder = (
             x509.CertificateRevocationListBuilder()
             .issuer_name(ca_cert.subject)
             .last_update(now)
             .next_update(now + timedelta(days=1))
         )
         for cert in self.get_revoked_certs():
             revoked = (
                 x509.RevokedCertificateBuilder()
                 .serial_number(int(cert.serial_number))
                 .revocation_date(now)
                 .add_extension(
                     x509.CRLReason(x509.ReasonFlags.unspecified),
                     critical=False,
                 )
-                .build(backend)
+                .build()
             )
             builder = builder.add_revoked_certificate(revoked)
         crl = builder.sign(
             private_key=private_key,
             algorithm=hashes.SHA256(),
-            backend=backend,
         )
         return crl.public_bytes(encoding=serialization.Encoding.PEM)

Also, there's a leftover debug comment # import ipdb; ipdb.set_trace() at line 620 that should be removed.

197-199: Remove default_backend() call to align with cryptography library best practices.

In cryptography ≥3.1, default_backend() is no longer required—the backend is automatically provided. The project uses cryptography ~46.0.3, which supports calling the API without the backend parameter. The import of default_backend can also be removed.

Proposed change

         if self.certificate:
             return x509.load_pem_x509_certificate(
-                self.certificate.encode("utf-8"), default_backend()
+                self.certificate.encode("utf-8")
             )

Also remove the import:

-from cryptography.hazmat.backends import default_backend

django_x509/tests/test_ca.py (1)

211-253: Consider using direct attribute assignment instead of setattr.

Per static analysis (B010), using setattr with a constant attribute is no safer than direct assignment. While this pattern is common in tests for temporarily modifying settings, direct assignment is more readable.
🔎 Example refactor
     def test_basic_constraints_not_critical(self):
-        setattr(app_settings, "CA_BASIC_CONSTRAINTS_CRITICAL", False)
+        app_settings.CA_BASIC_CONSTRAINTS_CRITICAL = False
         ca = self._create_ca()
         ext = ca.x509.extensions.get_extension_for_class(x509.BasicConstraints)
         self.assertFalse(ext.critical)
-        setattr(app_settings, "CA_BASIC_CONSTRAINTS_CRITICAL", True)
+        app_settings.CA_BASIC_CONSTRAINTS_CRITICAL = True

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5888b00 and 399e0cf.

📒 Files selected for processing (4)

django_x509/base/models.py
django_x509/tests/test_ca.py
django_x509/tests/test_cert.py
requirements.txt

💤 Files with no reviewable changes (1)

requirements.txt

🧰 Additional context used

🧬 Code graph analysis (1)

django_x509/tests/test_cert.py (4)

django_x509/base/models.py (8)

x509 (192-199)

_verify_ca (401-449)

pkey (224-233)

x509_text (202-221)

_get_subject (330-352)

save (183-189)

renew (568-575)

renew (601-611)

django_x509/tests/__init__.py (3)

TestX509Mixin (23-66)

_create_cert (43-66)

_create_ca (24-41)

django_x509/models.py (1)

Cert (16-23)

tests/openwisp2/sample_x509/models.py (1)

Cert (22-28)

🪛 Gitleaks (8.30.0)

django_x509/tests/test_ca.py

[high] 65-74: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.

(private-key)

🪛 Ruff (0.14.10)

django_x509/tests/test_ca.py

215-215: Do not call setattr with a constant attribute value. It is not any safer than normal property access.