[feature] Added support for importing ECDSA certificates #118 #201
Conversation
Updated KEY_LENGTH_CHOICES to include 224, 256, and 384 bits to support Elliptic Curve algorithms. Fixes #118
📝 WalkthroughWalkthroughAdds explicit ECDSA (EC) support alongside RSA in certificate handling. Introduces public constants Sequence Diagram(s)sequenceDiagram
participant User
participant Model as Certificate Model
participant Validator as Validation Logic
participant Crypto as Cryptography Lib
User->>Model: request _generate(params)
Model->>Validator: validate key_length ∈ EC_KEY_LENGTHS ∪ RSA_KEY_LENGTHS
Validator-->>Model: valid / error
alt EC length
Model->>Crypto: generate EC key (curve ← key_length)
Crypto-->>Model: EC PrivateKey
Model->>Crypto: serialize private key (PKCS8)
else RSA length
Model->>Crypto: generate RSA key (bits = key_length)
Crypto-->>Model: RSA PrivateKey
Model->>Crypto: serialize private key (TraditionalOpenSSL)
end
Model->>Crypto: normalize digest & sign certificate
Crypto-->>Model: Signed certificate PEM
Model-->>User: certificate PEM + private key PEM
sequenceDiagram
participant User
participant Model as Certificate Model
participant Parser as Cert Parser
participant Validator as Validation Logic
User->>Model: request _import(certificate_bytes)
Model->>Parser: parse certificate -> public key + sig algorithm
Parser-->>Model: PublicKey obj, signature algorithm
Model->>Validator: determine key_type and actual_length from PublicKey
Validator-->>Model: actual_length / unsupported error
Model->>Validator: ensure selected key_length (if set) matches detected type/length
Validator-->>Model: valid / ValidationError
Model->>Model: set self.key_length = actual_length
Model-->>User: import success or ValidationError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-01-16T21:54:25.644ZApplied to files:
🧬 Code graph analysis (3)tests/openwisp2/sample_x509/migrations/0004_alter_ca_key_length_alter_cert_key_length_and_more.py (2)
django_x509/migrations/0011_alter_ca_key_length_alter_cert_key_length.py (2)
django_x509/tests/test_ca.py (3)
🪛 Gitleaks (8.30.0)django_x509/tests/test_cert.py[high] 44-71: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption. (private-key) [high] 100-127: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption. (private-key) 🪛 Ruff (0.14.13)tests/openwisp2/sample_x509/migrations/0004_alter_ca_key_length_alter_cert_key_length_and_more.py9-11: Mutable class attributes should be annotated with (RUF012) 13-68: Mutable class attributes should be annotated with (RUF012) django_x509/migrations/0011_alter_ca_key_length_alter_cert_key_length.py9-11: Mutable class attributes should be annotated with (RUF012) 13-50: Mutable class attributes should be annotated with (RUF012) 🔇 Additional comments (15)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @django_x509/base/models.py:
- Around line 21-37: The EC P-521 curve is missing from the choices causing
validation and unreachable generation code; update KEY_LENGTH_CHOICES to include
("521","521") and add "521" to EC_KEY_LENGTHS so the field validation matches
the curve mapping used in the _generate() method (which references "521":
ec.SECP521R1()), ensuring imports/validation and generation paths are
consistent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
django_x509/base/models.pydjango_x509/migrations/0010_ca_key_type_cert_key_type_alter_ca_key_length_and_more.pytests/openwisp2/sample_x509/migrations/0003_ca_key_type_cert_key_type_customcert_key_type_and_more.py
🧰 Additional context used
🧬 Code graph analysis (2)
django_x509/migrations/0010_ca_key_type_cert_key_type_alter_ca_key_length_and_more.py (2)
tests/openwisp2/sample_x509/migrations/0003_ca_key_type_cert_key_type_customcert_key_type_and_more.py (1)
Migration(7-101)django_x509/base/models.py (1)
default_key_length(80-85)
tests/openwisp2/sample_x509/migrations/0003_ca_key_type_cert_key_type_customcert_key_type_and_more.py (2)
django_x509/migrations/0010_ca_key_type_cert_key_type_alter_ca_key_length_and_more.py (1)
Migration(7-72)django_x509/base/models.py (1)
default_key_length(80-85)
🪛 Ruff (0.14.10)
django_x509/migrations/0010_ca_key_type_cert_key_type_alter_ca_key_length_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-72: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_x509/migrations/0003_ca_key_type_cert_key_type_customcert_key_type_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-101: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (7)
django_x509/migrations/0010_ca_key_type_cert_key_type_alter_ca_key_length_and_more.py (1)
1-72: Migration structure looks correct.The migration properly adds the
key_typefield and updateskey_lengthchoices to support both RSA and EC key types. The field definitions are consistent with the base model constants.The RUF012 static analysis warnings about
ClassVarcan be safely ignored—Django migrations conventionally use mutable class attributes fordependenciesandoperations.django_x509/base/models.py (5)
162-164: Key type field and cross-field validation look correct.The new
key_typefield is properly defined with appropriate choices and default. The validation logic correctly prevents mismatched combinations—RSA keys can't use EC lengths (224/256/384) and EC keys can't use RSA lengths (512/1024/2048/4096).Also applies to: 195-203
306-325: EC key generation logic is well-implemented.The implementation correctly branches based on
key_type, using standard NIST curves (SECP224R1, SECP256R1, SECP384R1, SECP521R1) for EC key generation. Error handling for unsupported key types and lengths is appropriate.
349-354: Digest algorithm normalization correctly handles ECDSA signatures.This change resolves the KeyError from issue #118 by stripping the
"ecdsa-with-"prefix from digest algorithm names during certificate import, allowing proper mapping to the hash algorithm.
362-370: Private key format selection is appropriate.Using PKCS8 format for EC keys (modern standard, better interoperability) while retaining TraditionalOpenSSL for RSA keys (backward compatibility) is a sensible approach.
402-408: Import logic correctly detects key type from certificate.The implementation properly identifies RSA vs EC keys using
isinstancechecks and extracts the appropriate key size. For EC keys,curve.key_sizecorrectly returns the curve's bit length.tests/openwisp2/sample_x509/migrations/0003_ca_key_type_cert_key_type_customcert_key_type_and_more.py (1)
1-101: Test migration is consistent with main migration.The sample app migration correctly mirrors the main django_x509 migration, adding
key_typeto all three models (CA, Cert, CustomCert) and updatingkey_lengthchoices. Field definitions are consistent.The RUF012 static analysis warnings can be ignored—this is standard Django migration structure.
Added P-521 curve in KEY_LENGTH_CHOICES and EC_KEY_LENGTHS Fixes #118
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @django_x509/base/models.py:
- Around line 196-199: The ValidationError raised when self.key_type == "rsa"
and self.key_length in EC_KEY_LENGTHS is misleading; update the error message to
clearly state that the selected key length is an EC-specific value and
incompatible with RSA (include the offending value via self.key_length), and
guide the user to choose a valid RSA key length or switch to an EC key type;
update the ValidationError in this branch (the check referencing EC_KEY_LENGTHS)
to include that clarification and optionally list acceptable RSA lengths or
reference the EC lengths constant for clarity.
- Around line 403-409: When cert.public_key() returns a type other than
rsa.RSAPublicKey or ec.EllipticCurvePublicKey, self.key_type and self.key_length
are left unset; add an else branch after the EC check that sets safe defaults
and records the actual key class: set self.key_type =
cert.public_key().__class__.__name__.lower() (or "unknown" if unavailable), set
self.key_length = "" (or None) to avoid validation errors, and emit a warning
via the module/class logger so imports of DSA/Ed25519/etc. are recorded rather
than leaving fields empty.
🧹 Nitpick comments (2)
django_x509/migrations/0011_alter_ca_key_length_alter_cert_key_length.py (1)
18-27: Consider reordering key length choices for better UX.The key_length choices mix EC lengths (224, 256, 384, 521) with RSA lengths (512, 1024, 2048, 4096) in a grouped-by-type order rather than numerical order. While functionally correct, this may confuse users selecting values in UI dropdowns, especially since the key_type context isn't visible in the choice value itself.
Consider either:
- Sorting numerically: 224, 256, 384, 512, 521, 1024, 2048, 4096
- Adding prefixes to clarify type: ("224", "224 (EC)"), ("512", "512 (RSA)"), etc.
Also applies to: 38-47
tests/openwisp2/sample_x509/migrations/0004_alter_ca_key_length_alter_cert_key_length_and_more.py (1)
18-27: Consider reordering key length choices for better UX.The key_length choices are grouped by type (EC: 224, 256, 384, 521; RSA: 512, 1024, 2048, 4096) rather than sorted numerically. This non-intuitive ordering may confuse users in UI dropdowns where the key_type context isn't visible.
Consider numerical sorting or type-prefixed labels as suggested in the main migration (0011).
Also applies to: 38-47, 58-67
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
django_x509/base/models.pydjango_x509/migrations/0011_alter_ca_key_length_alter_cert_key_length.pytests/openwisp2/sample_x509/migrations/0004_alter_ca_key_length_alter_cert_key_length_and_more.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/openwisp2/sample_x509/migrations/0004_alter_ca_key_length_alter_cert_key_length_and_more.py (2)
django_x509/migrations/0011_alter_ca_key_length_alter_cert_key_length.py (1)
Migration(7-54)django_x509/base/models.py (1)
default_key_length(81-86)
🪛 Ruff (0.14.10)
django_x509/migrations/0011_alter_ca_key_length_alter_cert_key_length.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-54: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_x509/migrations/0004_alter_ca_key_length_alter_cert_key_length_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-74: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (5)
django_x509/base/models.py (5)
21-30: LGTM! EC key lengths added correctly.The added EC lengths (224, 256, 384, 521) correspond to standard NIST curves (SECP224R1, SECP256R1, SECP384R1, SECP521R1) and are correctly integrated with existing RSA lengths.
32-38: LGTM! Key type support well-structured.The new constants (KEY_TYPE_CHOICES, RSA_KEY_LENGTHS, EC_KEY_LENGTHS) and key_type field are well-designed. The default value "rsa" ensures backward compatibility with existing data.
Also applies to: 163-165
350-354: LGTM! Digest normalization addresses issue #118.The digest name normalization correctly strips EC-specific prefixes like "ecdsa-with-" and RSA-specific prefixes like "withrsaencryption", directly resolving the KeyError: 'ecdsa-with-SHA384' reported in issue #118.
365-369: LGTM! Appropriate serialization formats chosen.Using PKCS8 for EC keys (modern standard) and TraditionalOpenSSL for RSA keys (backward compatible) is the right approach for maintaining compatibility while supporting new key types.
307-326: EC curve mappings are correct.The key length to curve mappings align with the cryptography library's standard NIST/SECG curves:
- 224 → ec.SECP224R1 (P-224)
- 256 → ec.SECP256R1 (P-256)
- 384 → ec.SECP384R1 (P-384)
- 521 → ec.SECP521R1 (P-521)
The implementation correctly instantiates and passes these curves to
ec.generate_private_key(), following the cryptography library's standard usage pattern.
Improved error message clarity and added validation of unsupported keys. Fixes #118
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @django_x509/base/models.py:
- Around line 317-329: Remove support for the 224-bit EC curve by deleting the
"224": ec.SECP224R1() entry from the curves mapping in the EC key generation
branch (the dict used in the elif self.key_type == "ec" block) and update the
EC_KEY_LENGTHS constant to remove "224" so it lists only ("256", "384", "521");
ensure no other references expect "224" in the same module (e.g., validation
logic that uses EC_KEY_LENGTHS) are left unchanged except for that removal.
🧹 Nitpick comments (5)
django_x509/base/models.py (5)
21-30: Consider grouping or labeling key length choices by type.The combined
KEY_LENGTH_CHOICESnow mixes EC (224, 256, 384, 521) and RSA (512, 1024, 2048, 4096) lengths without visual separation. Users selecting a length in the admin or forms may not immediately know which lengths apply to their chosen key type until validation fails.Potential UX improvement
Consider one of these approaches:
- Add visual grouping in forms using
optgroup(requires custom widget)- Add descriptive labels like
("224", "224 (EC)")and("2048", "2048 (RSA)")- Dynamically filter choices based on
key_typein formsExample for approach 2:
KEY_LENGTH_CHOICES = ( - ("224", "224"), - ("256", "256"), - ("384", "384"), - ("521", "521"), - ("512", "512"), - ("1024", "1024"), - ("2048", "2048"), - ("4096", "4096"), + ("224", "224 (EC)"), + ("256", "256 (EC)"), + ("384", "384 (EC)"), + ("521", "521 (EC)"), + ("512", "512 (RSA)"), + ("1024", "1024 (RSA)"), + ("2048", "2048 (RSA)"), + ("4096", "4096 (RSA)"), )
196-209: Consider validating that key_length is in the appropriate set.The current validation correctly prevents mismatched key_type/key_length combinations (e.g., RSA with EC lengths). However, it doesn't validate that the key_length is actually valid for ANY key type.
More comprehensive validation
+ # Validate key_length is in the valid set for the chosen key_type + if self.key_type == "rsa": + if self.key_length not in RSA_KEY_LENGTHS: + raise ValidationError( + { + "key_length": _( + "Invalid key length for RSA. " + "Valid lengths are: %s" + ) % ", ".join(RSA_KEY_LENGTHS) + } + ) + elif self.key_type == "ec": + if self.key_length not in EC_KEY_LENGTHS: + raise ValidationError( + { + "key_length": _( + "Invalid key length for Elliptic Curve. " + "Valid lengths are: %s" + ) % ", ".join(EC_KEY_LENGTHS) + } + ) - if self.key_type == "rsa" and self.key_length in EC_KEY_LENGTHS: - raise ValidationError( - { - "key_length": _( - "Selected length is only valid for Elliptic Curve keys. " - "RSA keys must use 512, 1024, 2048, or 4096 bits." - ) - } - ) - - if self.key_type == "ec" and self.key_length in RSA_KEY_LENGTHS: - raise ValidationError( - {"key_length": _("Selected length is not valid for Elliptic Curve.")} - )This approach validates positively (is the value in the valid set?) rather than negatively (is the value in the wrong set?), which is more robust.
318-323: Consider extracting curve mapping to a module-level constant.The
curvesdictionary is recreated on every certificate generation. While the performance impact is negligible (certificate generation is infrequent), extracting it improves maintainability and keeps related constants together.Refactor suggestion
At module level, after the existing constants:
EC_CURVES = { "224": ec.SECP224R1, "256": ec.SECP256R1, "384": ec.SECP384R1, "521": ec.SECP521R1, }Then in
_generate:elif self.key_type == "ec": - curves = { - "224": ec.SECP224R1(), - "256": ec.SECP256R1(), - "384": ec.SECP384R1(), - "521": ec.SECP521R1(), - } - curve = curves.get(self.key_length) + curve_class = EC_CURVES.get(self.key_length) - if not curve: + if not curve_class: raise ValidationError( _("Unsupported EC key length: %s") % self.key_length ) - key = ec.generate_private_key(curve) + key = ec.generate_private_key(curve_class())Note: Store the class, not the instance, to avoid unnecessary instantiation at module import.
408-421: Import may accept certificates with unsupported EC curves.The import logic correctly detects RSA vs EC keys and extracts the key size. However, when importing a certificate with an EC curve not in the supported set (e.g., SECP192R1, SECP256K1), the validation error from
clean()will say "Selected length is not valid for Elliptic Curve," which is misleading—the issue is the curve, not the length value.Improve error messaging for unsupported curves
Add curve validation immediately after detection:
elif isinstance(public_key, ec.EllipticCurvePublicKey): self.key_type = "ec" self.key_length = str(public_key.curve.key_size) + if self.key_length not in EC_KEY_LENGTHS: + raise ValidationError( + _( + "Unsupported EC curve in certificate. " + "Supported curves: SECP224R1 (224-bit), SECP256R1 (256-bit), " + "SECP384R1 (384-bit), SECP521R1 (521-bit)." + ) + ) else:This provides immediate, clear feedback during import rather than a confusing validation error later.
37-37: 512-bit RSA support is intentional and used for certificate compatibility.While 512-bit RSA is indeed cryptographically weak by modern standards, its inclusion in
RSA_KEY_LENGTHSappears deliberate. The model's validation logic explicitly lists it as a valid option, and the test suite actively verifies that 512-bit certificates can be imported and parsed. This suggests the library is designed to handle legacy X.509 certificates rather than restrict key generation.If the project's scope includes supporting certificate parsing and import of existing infrastructure, retaining 512-bit support may be necessary. However, if the intention is to restrict new certificate generation, consider:
- Adding a separate constant for SUPPORTED_RSA_KEY_LENGTHS (for parsing) vs GENERATED_RSA_KEY_LENGTHS (for new keys)
- Documenting the rationale for 512-bit support in a code comment
- Adding a deprecation warning when 512-bit certificates are used
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
django_x509/base/models.py
🔇 Additional comments (4)
django_x509/base/models.py (4)
163-165: LGTM!The
key_typefield is well-defined with an appropriate default of"rsa"for backward compatibility.
370-374: LGTM! Private key format selection is appropriate.Using PKCS8 for EC keys is correct and necessary, as the TraditionalOpenSSL format only supports RSA. Maintaining TraditionalOpenSSL for RSA keys preserves backward compatibility with existing certificates.
436-436: No changes needed. The digest field is consistently stored as a simple hash algorithm name (e.g., "sha256", "sha1") during both import and generation. Thesignature_hash_algorithm.nameproperty from the cryptography library returns only the hash name without signature scheme prefixes, so the import at line 436 already stores normalized values that match theDIGEST_CHOICESfield options. The normalization code in_generate()(lines 355-359) is defensive but unnecessary since those prefixes are never present in practice.Likely an incorrect or invalid review comment.
355-359: The digest normalization may be unnecessary — verify it actually solves the reported issue.The
signature_hash_algorithm.nameproperty from Python's cryptography library returns only the hash algorithm name (e.g.,"sha256","sha1"), not the full signature algorithm identifier. The code attempts to strip RSA and EC prefixes ("withrsaencryption","ecdsa-with-"), but these should not appear in the digest value based on how the certificate is parsed. If issue #118 involved a KeyError with the digest, the root cause may be elsewhere. Please confirm this normalization is actually needed by verifying the exact error from that issue.
Removed 224-bit curve support as modern standards recommend 256-bit curves (SECP256R1/P-256) as the minimum. Fixes #118
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
django_x509/base/models.pydjango_x509/migrations/0012_alter_ca_key_length_alter_cert_key_length.pytests/openwisp2/sample_x509/migrations/0005_alter_ca_key_length_alter_cert_key_length_and_more.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/openwisp2/sample_x509/migrations/0005_alter_ca_key_length_alter_cert_key_length_and_more.py (2)
django_x509/migrations/0012_alter_ca_key_length_alter_cert_key_length.py (1)
Migration(7-52)django_x509/base/models.py (1)
default_key_length(80-85)
django_x509/migrations/0012_alter_ca_key_length_alter_cert_key_length.py (2)
tests/openwisp2/sample_x509/migrations/0005_alter_ca_key_length_alter_cert_key_length_and_more.py (1)
Migration(7-71)django_x509/base/models.py (1)
default_key_length(80-85)
🪛 Ruff (0.14.10)
tests/openwisp2/sample_x509/migrations/0005_alter_ca_key_length_alter_cert_key_length_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-71: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
django_x509/migrations/0012_alter_ca_key_length_alter_cert_key_length.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (8)
django_x509/migrations/0012_alter_ca_key_length_alter_cert_key_length.py (1)
1-52: LGTM!The migration correctly adds EC key length choices (256, 384, 521) alongside existing RSA key lengths. The structure is consistent with the sample_x509 migration and follows Django migration conventions.
Note: The Ruff RUF012 warnings about
ClassVarannotations are false positives—Django migrations intentionally use mutable class attributes as part of their design pattern.tests/openwisp2/sample_x509/migrations/0005_alter_ca_key_length_alter_cert_key_length_and_more.py (1)
1-71: LGTM!The test migration is consistent with the main
django_x509migration (0012), applying the same key_length choices across all three models (CA, Cert, CustomCert). The Ruff warnings are false positives for Django migrations.django_x509/base/models.py (6)
21-37: LGTM!The key length choices and type constants are well-structured. The EC key lengths (256, 384, 521) correctly correspond to SECP256R1 (P-256), SECP384R1 (P-384), and SECP521R1 (P-521) curves.
162-164: LGTM!The
key_typefield is correctly defined with appropriatemax_length=3(sufficient for "rsa" and "ec"), and the default of"rsa"maintains backward compatibility with existing certificates.
195-208: LGTM!The cross-field validation correctly prevents mismatched key type and key length combinations, with clear error messages that guide users to valid choices.
311-329: LGTM!The key generation logic correctly handles both RSA and EC key types with proper curve mapping. The defensive error handling for unsupported key lengths and types provides good resilience even though clean() validation should catch these cases earlier.
353-358: LGTM!The digest name normalization correctly handles both RSA signature algorithm names (e.g.,
sha256WithRSAEncryption→sha256) and ECDSA signature algorithm names (e.g.,ecdsa-with-sha384→sha384), ensuring the hash algorithm lookup succeeds.
366-374: LGTM!Using PKCS8 format for EC keys and TraditionalOpenSSL for RSA keys is a sensible choice—PKCS8 is the recommended format for EC keys while TraditionalOpenSSL maintains backward compatibility for RSA keys.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
django_x509/base/models.py (2)
369-373: Consider using PKCS8 format for both RSA and EC keys.The current implementation uses TraditionalOpenSSL for RSA (preserving backward compatibility) and PKCS8 for EC. While this works, using PKCS8 for both key types would simplify the codebase and align with modern standards.
If backward compatibility with existing private keys is not a concern, consider this simplification:
♻️ Unified PKCS8 format
self.private_key = key.private_bytes( encoding=serialization.Encoding.PEM, - format=( - serialization.PrivateFormat.PKCS8 - if self.key_type == "ec" - else serialization.PrivateFormat.TraditionalOpenSSL - ), + format=serialization.PrivateFormat.PKCS8, encryption_algorithm=encryption, ).decode("utf-8")
353-358: The digest normalization is unnecessary for current cryptography usage.The
signature_hash_algorithm.namefrom the cryptography library already returns lowercase hash names (e.g., "sha256", "sha384") in the format expected by HASH_MAP. The.replace()calls for "withrsaencryption", "ecdsa-with-", and "withsha384" handle formats that cryptography never produces. This defensive code is harmless but can be simplified by directly using the digest value from imported certificates, removing the unnecessary string replacements.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
django_x509/base/models.py
🔇 Additional comments (7)
django_x509/base/models.py (7)
21-29: LGTM! EC key lengths are correct.The key lengths for EC curves are accurate. Note that "521" (not "512") is correct for the SECP521R1 curve—this is a well-known naming quirk in NIST elliptic curves.
31-37: LGTM! Key type choices and constants are well-defined.The separation of RSA and EC key lengths into distinct constants enables clean validation logic throughout the codebase.
162-164: LGTM! Default ensures backward compatibility.The
key_typefield defaults to "rsa", preserving existing behavior for records created before this feature.
195-208: LGTM! Validation prevents key type/length mismatches.The symmetric validation ensures users cannot select incompatible key_type and key_length combinations, addressing a key objective from the PR.
407-423: LGTM! Import logic correctly detects and validates key types.The import path now properly handles both RSA and EC certificates, resolving the KeyError from issue #118. The validation ensures only supported EC curves (P-256, P-384, P-521) can be imported, providing clear error messages for unsupported curves.
495-507: LGTM! CA verification supports both RSA and EC signatures.The
_verify_ca()method correctly handles signature verification for both RSA (using PKCS1v15 padding) and EC (using ECDSA) public keys, ensuring imported EC certificates can be properly validated against their CA.
311-329: EC curve support is confirmed in cryptography 46.0.3.The EC key generation logic correctly maps key lengths to NIST curves (SECP256R1, SECP384R1, SECP521R1) with appropriate error handling for unsupported lengths and key types. The implementation is correct.
Added a test to verify full edcsa lifecycle with respect to the changes. Fixes #118
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @django_x509/tests/test_ca.py:
- Around line 725-732: Add a call to full_clean() on the Ca instance before
save() in the test: when creating gen_ca via Ca(name=f"Generated-EC-{length}",
key_type="ec", key_length=length) call gen_ca.full_clean() prior to
gen_ca.save() so the test mirrors the import test’s validation pattern; keep the
existing assertions on gen_ca.pkey, original_cert/ gen_ca.certificate after
gen_ca.renew(), and key_type intact.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
django_x509/tests/test_ca.py
🔇 Additional comments (1)
django_x509/tests/test_ca.py (1)
4-7: Imports look appropriate for the new EC lifecycle test.
No concerns with addinghashes,serialization, andechere.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
django_x509/tests/test_ca.py (1)
689-737: Excellent EC lifecycle test with good coverage!The test effectively covers EC CA import, generation, and renewal flows with appropriate curve/digest pairings. The use of
subTestfor parameterization is a best practice.Optional: Consider adding a few more assertions for completeness
While the test provides solid coverage, you could enhance it with:
- Verify imported CA key type (after line 726):
self.assertIsInstance(ca.pkey, ec.EllipticCurvePrivateKey)
- Verify renewal preserves key metadata (after line 737):
gen_ca.refresh_from_db() self.assertEqual(gen_ca.key_type, "ec") self.assertEqual(gen_ca.key_length, length) self.assertNotEqual(gen_ca.private_key, original_cert) # Verify key changedThese additions would align with the patterns in
test_renew()(lines 624-656) andtest_pkey_property()(lines 107-109).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
django_x509/tests/test_ca.py
🔇 Additional comments (1)
django_x509/tests/test_ca.py (1)
5-6: LGTM! Necessary imports for EC key operations.The added imports (
hashes,serialization,ec) are appropriately used in the newtest_ecdsa_full_lifecycletest method for EC key generation, certificate creation, and serialization.
Added assertions to verify imported CA key type and verify renewal preserves key metadata. Fixes #118
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great work @stktyagi, I did a quick scan, see a few comments below.
I'll take some time in the next few days to perform manual testing.
Verifies Cert ECDSA import, generation, and renewal issued by an ECDSA CA. Fixes #118
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@django_x509/tests/test_ca.py`:
- Around line 736-742: The test saves the original certificate to original_cert
but then incorrectly compares it to gen_ca.private_key; instead capture the
original private key before calling gen_ca.renew() (e.g., store
gen_ca.private_key into a variable like original_key), then after
gen_ca.refresh_from_db() assert that gen_ca.private_key != original_key to
verify the key changed, and keep the existing assertion that gen_ca.certificate
!= original_cert to verify the certificate changed.
🧹 Nitpick comments (1)
django_x509/base/models.py (1)
352-357: Incomplete digest normalization pattern.The normalization handles
withsha384specifically but misses other potential patterns likewithsha256orwithsha512. Consider a more general approach.♻️ Suggested fix for more robust normalization
digest_name = ( self.digest.lower() .replace("withrsaencryption", "") .replace("ecdsa-with-", "") - .replace("withsha384", "sha384") + .replace("withsha", "sha") )This handles
withsha256,withsha384,withsha512, etc. uniformly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
django_x509/base/models.pydjango_x509/tests/test_ca.pydjango_x509/tests/test_cert.py
🧰 Additional context used
🧬 Code graph analysis (2)
django_x509/tests/test_cert.py (3)
django_x509/models.py (2)
Ca(6-13)Cert(16-23)tests/openwisp2/sample_x509/models.py (2)
Ca(13-19)Cert(22-28)django_x509/base/models.py (5)
save(209-215)x509(218-223)pkey(248-257)renew(666-673)renew(699-709)
django_x509/tests/test_ca.py (2)
django_x509/base/models.py (5)
x509(218-223)save(209-215)pkey(248-257)renew(666-673)renew(699-709)tests/openwisp2/sample_x509/models.py (1)
Ca(13-19)
🔇 Additional comments (10)
django_x509/base/models.py (6)
21-37: LGTM! Well-structured key type and length constants.The separation of RSA and EC key lengths into distinct tuples enables clear validation. The removal of 224-bit and inclusion of 521-bit aligns with modern security recommendations.
162-164: LGTM! Field definition is appropriate.The
key_typefield with max_length=3, choices constraint, and "rsa" default maintains backward compatibility with existing certificates.
195-207: LGTM! Comprehensive cross-field validation.The bidirectional validation between
key_typeandkey_lengthprevents invalid combinations with clear, actionable error messages.
310-328: LGTM! Correct EC key generation implementation.The curve mapping matches
EC_KEY_LENGTHSand uses the appropriate NIST curves (SECP256R1, SECP384R1, SECP521R1). Error handling for unsupported lengths and key types is properly included.
368-372: LGTM! Appropriate private key format selection.Using PKCS8 for EC keys is the standard choice, while TraditionalOpenSSL for RSA maintains backward compatibility with existing systems and certificates.
406-422: LGTM! Robust import handling for different key types.The implementation correctly:
- Detects and sets appropriate
key_typeandkey_lengthfor RSA and EC keys- Validates EC curve sizes against supported lengths
- Rejects unsupported key types (DSA, Ed25519, etc.) with a clear error message
django_x509/tests/test_ca.py (2)
5-6: LGTM!Necessary imports for ECDSA certificate operations in the new test.
689-735: LGTM! Comprehensive ECDSA CA lifecycle test.The test properly covers:
- All three supported EC curves with curve-appropriate hash algorithms
- Import flow with externally generated EC certificates
- Generation flow with model-based EC CA creation
- Proper use of
subTestfor clear test isolation and reportingdjango_x509/tests/test_cert.py (2)
5-6: LGTM!Necessary imports for ECDSA certificate operations in the new test.
495-550: LGTM! Comprehensive ECDSA Cert lifecycle test.The test properly covers:
- All three supported EC curves with curve-appropriate hash algorithms
- Creation of EC CAs for issuing certificates
- Import flow with externally generated EC certificates signed by the CA
- Generation flow with model-based EC Cert creation
- Renewal verification ensuring
key_typeis preserved- Proper use of
subTestfor clear test isolationThis addresses the reviewer request from past comments to add a corresponding test in
test_cert.py.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi what do you think about this?
🧹 Nitpick comments (1)
django_x509/base/models.py (1)>
352-357: Incomplete digest normalization pattern.The normalization handles
withsha384specifically but misses other potential patterns likewithsha256orwithsha512. Consider a more general approach.♻️ Suggested fix for more robust normalization
digest_name = ( self.digest.lower() .replace("withrsaencryption", "") .replace("ecdsa-with-", "") - .replace("withsha384", "sha384") + .replace("withsha", "sha") )This handles
withsha256,withsha384,withsha512, etc. uniformly.
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
Totally valid, I might have missed this. I'll update this right away. |
Updated digest pattern to more generalized approach to handle every pattern. Fixes #118
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi have you tested this in the admin at all? I don't think so. The feature is not available at all from the admin.
The README also will need to be updated to reflect these changes.
See also my comments below.
@nemesifier The original error was about not being able to import ecdsa certs and I was able to manually test it in the earlier commits but as the commits went by and we went to full fledged ecdsa support I didnt keep up with the testing. I'll make sure going ahead. |
@stktyagi sounds good! We'll need to test how OpenWISP Controller interacts with this version and ensure its admin and REST API continue working fine. |
Implemented ECDSA support using internal logic to automatically infer the algorithm from the key_length choice, ensuring full backward compatibility without adding new database fields or migrations. Fixes #118
Add migrations files related to changes made to the KEY_LENGTH_CHOICES Fixes #118
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@django_x509/migrations/0010_alter_ca_key_length_alter_cert_key_length.py`:
- Around line 18-26: The migration's choices arrays in
django_x509.migrations.0010_alter_ca_key_length_alter_cert_key_length.py are
missing the "224 (ECDSA)" option; update both the CA key length choices and the
cert key length choices (the two choices= [...] lists shown) to include ("224",
"224 (ECDSA)") in the appropriate position so the migration matches the model's
allowed EC key sizes and prevents drift.
In
`@tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py`:
- Around line 18-26: The three choices arrays for key length in the migration
(for the fields ca_key_length, cert_key_length, and customcert_key_length) are
missing the 224 ECDSA option; add the tuple ("224", "224 (ECDSA)") into each
choices list (preferably placed before ("256", "256 (ECDSA)") to keep numeric
order) so the AlterField choice definitions in
0003_alter_ca_key_length_alter_cert_key_length_and_more.py match the newly
supported EC key sizes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
django_x509/migrations/0010_alter_ca_key_length_alter_cert_key_length.pytests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py
🧰 Additional context used
🧬 Code graph analysis (2)
django_x509/migrations/0010_alter_ca_key_length_alter_cert_key_length.py (2)
tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py (1)
Migration(7-71)django_x509/base/models.py (1)
default_key_length(75-80)
tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py (2)
django_x509/migrations/0010_alter_ca_key_length_alter_cert_key_length.py (1)
Migration(7-52)django_x509/base/models.py (1)
default_key_length(75-80)
🪛 Ruff (0.14.13)
django_x509/migrations/0010_alter_ca_key_length_alter_cert_key_length.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-71: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
django_x509/migrations/0010_alter_ca_key_length_alter_cert_key_length.py
Show resolved
Hide resolved
.../openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py
Show resolved
Hide resolved
Modified clean() to check for unsupported key lengths and removed previous complex mismatch checks Fixes #118
Remove 512-bit RSA support from models and refactor test data by replacing 512-bit certificates with 2048-bit RSA Fixes #118
Updated KEY_LENGTH_CHOICES to include 256, 384 and 521 bits to support Elliptic Curve algorithms.
Fixes #118
Checklist
Reference to Existing Issue
Closes #118.
Description of Changes
Added support for importing and generating ECDSA certificates, resolving the KeyError when importing certificates with Elliptic Curve algorithms, while preventing confusion between RSA and EC key lengths.