Save issued certificates to database #44
Conversation
WalkthroughThis pull request introduces enhancements to the node certificate management system across multiple files in the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
nodeman/db_models.py (1)
55-64: Consider adding validation and helper methodsA few suggestions to enhance the model:
- Add validation for the certificate field to ensure it's a valid PEM format
- Consider storing
serialas an integer field for consistent handling with X.509 operations- Add a classmethod to create instance from an X.509 certificate object
Example implementation:
- serial = StringField() + serial = IntField() + + @classmethod + def from_x509_cert(cls, name: str, cert: 'x509.Certificate') -> 'TapirCertificate': + return cls( + name=name, + issuer=cert.issuer.rfc4514_string(), + subject=cert.subject.rfc4514_string(), + serial=cert.serial_number, + not_valid_before=cert.not_valid_before_utc, + not_valid_after=cert.not_valid_after_utc, + certificate=cert.public_bytes(serialization.Encoding.PEM).decode(), + ) + + def clean(self): + """Validate certificate field format""" + if self.certificate: + try: + load_pem_x509_certificate(self.certificate.encode()) + except ValueError as e: + raise ValidationError("Invalid certificate PEM format") from enodeman/models.py (1)
75-75: Consider adding validation for certificate expirationThe new field
x509_certificate_not_valid_aftershould have validation to ensure it's:
- Not in the past when creating new certificates
- After
not_valid_beforeif that field is presentExample implementation:
- x509_certificate_not_valid_after: datetime + x509_certificate_not_valid_after: datetime = Field( + title="Certificate expiration date", + description="When the X.509 certificate expires", + ) + + @field_validator("x509_certificate_not_valid_after") + @classmethod + def validate_expiration(cls, v: datetime): + if v < datetime.now(timezone.utc): + raise ValueError("Certificate expiration date cannot be in the past") + return vnodeman/nodes.py (1)
Line range hint
332-333: Consider adding debug logging for activation checkThe validation check for node activation is a good security measure. Consider adding debug logging to help with troubleshooting activation-related issues.
Add logging before raising the exception:
if not node.activated: + logging.debug("Renewal attempt for non-activated node %s", name, extra={"nodename": name}) raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="Node not activated")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nodeman/db_models.py(1 hunks)nodeman/models.py(1 hunks)nodeman/nodes.py(1 hunks)nodeman/x509.py(3 hunks)
🔇 Additional comments (4)
nodeman/db_models.py (1)
46-53: LGTM on model structure and indexes
The model structure with indexes on name and composite [issuer, serial] is well-designed for efficient querying. The unique constraint on [issuer, serial] correctly enforces certificate uniqueness.
nodeman/x509.py (2)
175-175: LGTM on certificate expiration field
The addition of x509_certificate_not_valid_after to the return value is correct and consistent with the model changes.
149-157: Verify certificate uniqueness constraints
Let's verify the effectiveness of the unique constraint on [issuer, serial] and check for potential race conditions.
✅ Verification successful
Certificate uniqueness is properly enforced
The model definition shows that a unique compound index is set on ["issuer", "serial"] fields, which will prevent duplicate certificates at the database level. While there's no explicit transaction handling, MongoDB's atomic operations on single documents provide sufficient protection against race conditions in this case.
- ✓ Unique constraint on
["issuer", "serial"]is properly defined innodeman/db_models.py - ✓ Document-level atomicity in MongoDB handles concurrent save operations safely
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing duplicate certificates
ast-grep --pattern 'TapirCertificate.objects($$$)' | grep -i 'find\|get'
# Check for any concurrent certificate operations
rg -A 5 'save_certificate|TapirCertificate.*save'
Length of output: 126
Script:
#!/bin/bash
# Check model definition for uniqueness constraints
rg -A 10 "class TapirCertificate"
# Check for any existing certificate operations
rg -l "TapirCertificate"
# Check for any transaction or locking mechanisms
rg -A 5 "@transaction.atomic|select_for_update"
Length of output: 624
nodeman/nodes.py (1)
279-279: Verify NodeConfiguration model includes the new field
The addition of x509_certificate_not_valid_after aligns with the PR objective of tracking certificate data. The field is correctly propagated from the certificate processing.
Let's verify the field is properly defined in the NodeConfiguration model:
✅ Verification successful
Field is properly defined in the model hierarchy
The x509_certificate_not_valid_after field is correctly defined in the NodeCertificate base class, which NodeConfiguration inherits from. The field is properly typed as datetime and is a required field (no default value), which aligns with the usage in the code under review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NodeConfiguration model definition
ast-grep --pattern 'class NodeConfiguration' -A 20
Length of output: 52
Script:
#!/bin/bash
# Let's try searching for NodeConfiguration using ripgrep for better coverage
rg "class NodeConfiguration" -A 20
Length of output: 673
Script:
#!/bin/bash
# Let's check the NodeCertificate class since NodeConfiguration inherits from it
rg "class NodeCertificate" -A 20
Length of output: 1335
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
nodeman/x509.py (1)
Line range hint
149-167: Consider using database transactionThe certificate issuance process involves multiple database operations (saving certificate and potentially updating node status). Consider wrapping these operations in a transaction to ensure data consistency.
+from mongoengine import get_db + def process_csr_request(request: Request, csr: x509.CertificateSigningRequest, name: str) -> NodeCertificate: """Verify CSR and issue certificate""" + session = get_db().client.start_session() + with session.start_transaction(): + try: # ... existing code ... - TapirCertificate.from_x509_certificate(name=name, x509_certificate=x509_certificate).save() + TapirCertificate.from_x509_certificate( + name=name, + x509_certificate=x509_certificate + ).save(session=session) # ... rest of the code ... + except Exception as exc: + session.abort_transaction() + raise
🧹 Nitpick comments (3)
nodeman/db_models.py (2)
48-55: Consider index ordering optimizationThe composite index on
["issuer", "serial"]might benefit from reverse order["serial", "issuer"]if queries frequently filter by serial number first. This could improve query performance for certificate lookups by serial number.
80-85: Enhance certificate validationThe current validation only checks if the certificate can be parsed. Consider adding additional validations:
- Certificate chain validation
- Expiration date validation
- Key usage validation
def clean(self): """Validate certificate format""" try: - x509.load_pem_x509_certificate(self.certificate.encode()) + cert = x509.load_pem_x509_certificate(self.certificate.encode()) + # Validate certificate dates + now = datetime.now(timezone.utc) + if now < cert.not_valid_before_utc or now > cert.not_valid_after_utc: + raise ValidationError("Certificate is not valid at the current time") + # Validate key usage + if cert.extensions.get_extension_for_oid(ExtensionOID.KEY_USAGE).value.key_cert_sign: + raise ValidationError("Certificate must not have key_cert_sign usage") except ValueError as exc: raise ValidationError("Invalid certificate PEM format") from excnodeman/nodes.py (1)
300-301: Improve error handling for non-activated nodesConsider providing a more descriptive error message and moving the activation check earlier in the function to fail fast.
- if not node.activated: - logging.debug("Renewal attempt for non-activated node %s", name, extra={"nodename": name}) - raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="Node not activated") + if not node.activated: + msg = f"Cannot renew certificate for node '{name}' as it has not completed the enrollment process" + logging.warning(msg, extra={"nodename": name}) + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=msg + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nodeman/db_models.py(2 hunks)nodeman/nodes.py(2 hunks)nodeman/x509.py(3 hunks)
🔇 Additional comments (2)
nodeman/x509.py (1)
149-150: Add error handling for database operations
The certificate saving operation could fail silently. Consider adding error handling for database operations.
nodeman/nodes.py (1)
279-279: LGTM: Certificate expiration tracking added
The addition of x509_certificate_not_valid_after to the NodeConfiguration response is a good improvement for certificate lifecycle management.
Summary by CodeRabbit
New Features
TapirCertificateclass for storing certificate data.x509_certificate_not_valid_afterfield toNodeCertificatefor expiration dates.enroll_nodefunction to include certificate expiration date in responses.Bug Fixes
renew_nodeto prevent renewal of inactive nodes.Documentation