Add internal CA #30
Add internal CA #30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a comprehensive certificate authority management system with the new Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/test_internal_ca.py (1)
36-36: Remove unnecessary print statementPrinting the certificate PEM in the test is unnecessary and can clutter the test output. It's best practice to remove such statements unless needed for debugging.
Apply this diff to remove the print statement:
- print(x509_certificate_pem)tests/utils.py (1)
27-29: Consider making certificate validity configurableThe certificate validity period is hardcoded to 1 day. Consider making this configurable through a parameter to improve flexibility for different test scenarios.
-def generate_ca_certificate(ca_name: x509.Name | str, ca_private_key: PrivateKey) -> x509.Certificate: +def generate_ca_certificate( + ca_name: x509.Name | str, + ca_private_key: PrivateKey, + validity: timedelta = timedelta(days=1) +) -> x509.Certificate: """Generate CA Certificate""" now = datetime.now(tz=timezone.utc) - validity = timedelta(days=1)tests/test_api.py (1)
37-41: Consider parameterizing CA key typeThe
get_ca_clientfunction always uses SECP256R1 for the CA key. Consider parameterizing the key type to allow testing with different CA key algorithms, especially since the test suite covers various client key types.-def get_ca_client(ca_name: str) -> CertificateAuthorityClient: +def get_ca_client( + ca_name: str, + key_type: str = "EC", + curve: str = "SECP256R1" +) -> CertificateAuthorityClient: - ca_private_key = ec.generate_private_key(ec.SECP256R1()) + ca_private_key = ( + ec.generate_private_key(getattr(ec, curve)) + if key_type == "EC" + else Ed25519PrivateKey.generate() + if key_type == "Ed25519" + else Ed448PrivateKey.generate() + if key_type == "Ed448" + else rsa.generate_private_key( + public_exponent=65537, + key_size=2048 + ) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
nodeman/internal_ca.py(1 hunks)tests/test_api.py(3 hunks)tests/test_dummy_ca.py(0 hunks)tests/test_internal_ca.py(1 hunks)tests/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/test_dummy_ca.py
🔇 Additional comments (5)
tests/utils.py (2)
24-26: LGTM: Function signature and docstring
The function signature with type hints and docstring is clear and well-defined.
39-56: LGTM: CA certificate configuration
The certificate builder configuration is correct with appropriate extensions:
- BasicConstraints with CA=True for CA certificate
- KeyUsage with correct flags for certificate signing
tests/test_api.py (3)
43-47: LGTM: Test client setup
The test client setup with CA configuration is clean and well-structured.
Line range hint 55-196: LGTM: Comprehensive enrollment test implementation
The _test_enroll function provides thorough testing of the enrollment flow:
- Creates and validates node
- Tests enrollment with proper signatures
- Verifies certificate renewal
- Handles cleanup properly
Line range hint 198-238: LGTM: Excellent key type coverage
The test suite comprehensively covers various key types:
- SECP256R1/P-256
- SECP384R1/P-384
- Ed25519
- Ed448
- RSA
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
nodeman/server.py (1)
68-74: Add logging to indicate which CA client is being usedTo improve traceability and debugging, consider adding logging statements to indicate whether the
internal_caorstepcertificate authority is being used. This will help in understanding the server's configuration at startup.tests/test_internal_ca.py (3)
59-63: Remove print statements from test functionsPrinting certificate PEM data in tests can clutter the output. Consider removing these
Apply this diff to remove print statements:
x509_certificate_pem = "".join( [certificate.public_bytes(serialization.Encoding.PEM).decode() for certificate in res.cert_chain] ) -print(x509_certificate_pem) x509_ca_certificate_pem = res.ca_cert.public_bytes(serialization.Encoding.PEM).decode() -print(x509_ca_certificate_pem)
144-146: Document why verification is disabled for Ed25519 keysIn
test_internal_ca_ed25519, verification is disabled by settingverify=False. Adding a comment explaining why verification is skipped will improve code clarity.Apply this diff to add a comment:
def test_internal_ca_ed25519() -> None: ca_private_key = Ed25519PrivateKey.generate() + # Verification is disabled due to lack of support for Ed25519 in the verification utility. return _test_internal_ca(ca_private_key, verify=False)
148-150: Document why verification is disabled for Ed448 keysSimilarly, in
test_internal_ca_ed448, explain why verification is set toFalseto aid future maintenance.Apply this diff to add a comment:
def test_internal_ca_ed448() -> None: ca_private_key = Ed448PrivateKey.generate() + # Verification is disabled due to lack of support for Ed448 in the verification utility. return _test_internal_ca(ca_private_key, verify=False)nodeman/settings.py (1)
35-39: Consider improving readability of the validity period defaultThe validity period is currently specified in seconds which reduces readability. Consider using a more explicit format.
- validity: int = Field(default=24 * 60 * 60 * 90) + validity: int = Field( + default=24 * 60 * 60 * 90, # 90 days + description="Validity period in seconds for issued certificates" + )nodeman/internal_ca.py (2)
15-25: Consider more restrictive key usage flagsThe current key usage flags might be too permissive. For client certificates, consider enabling only digital_signature since other operations aren't typically needed.
106-111: Consider documenting certificate chain behaviorThe certificate chain building logic could benefit from a docstring explaining when and why the issuer certificate is included in the chain.
if self.root_ca_certificate != self.issuer_ca_certificate: + # Include intermediate CA certificate in chain when using separate root and issuer CAs cert_chain = [certificate, self.issuer_ca_certificate] else: + # Use single certificate when root and issuer are the same cert_chain = [certificate]tests/test_api.py (2)
38-48: Consider adding test cases for CA chain validationThe get_ca_client function creates a single CA certificate. Consider adding test cases with separate root and issuer CAs to validate certificate chain building.
def get_ca_client_with_chain() -> CertificateAuthorityClient: """Create CA client with separate root and issuer certificates for testing""" root_name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "Internal Root CA")]) issuer_name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "Internal Issuing CA")]) root_key = ec.generate_private_key(ec.SECP256R1()) issuer_key = ec.generate_private_key(ec.SECP256R1()) root_cert = generate_ca_certificate(root_name, root_key) issuer_cert = generate_ca_certificate(issuer_name, issuer_key, root_cert, root_key) return InternalCertificateAuthority( issuer_ca_certificate=issuer_cert, issuer_ca_private_key=issuer_key, root_ca_certificate=root_cert, validity=timedelta(minutes=10), )
41-42: Make test CA validity period more explicitThe test CA uses a 10-minute validity period which differs from the default in settings. Consider making this more explicit or aligning it with the default.
- validity = timedelta(minutes=10) + # Use short validity for tests to avoid long-lived certificates + test_validity = timedelta(minutes=10)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
nodeman/internal_ca.py(1 hunks)nodeman/server.py(3 hunks)nodeman/settings.py(2 hunks)pyproject.toml(2 hunks)tests/test_api.py(3 hunks)tests/test_internal_ca.py(1 hunks)tests/utils.py(2 hunks)
🔇 Additional comments (4)
tests/test_internal_ca.py (1)
48-55: Good job adding assertions to verify the signed certificate
The added assertions enhance the test by verifying that the certificate chain is not empty and that the common name matches the expected value.
pyproject.toml (2)
26-26: Confirm necessity and usage of redis dependency
The redis library has been added. Verify that it is required and that any new code utilizing redis is correctly implemented and tested.
Run the following script to check for redis usage:
14-14: Verify compatibility with updated dnstapir version
The dnstapir dependency has been updated to v1.1.0. Ensure that the project is compatible with this version and that any deprecated features are addressed.
Run the following script to check for deprecated dnstapir usage:
✅ Verification successful
No compatibility issues found with dnstapir v1.1.0
The codebase uses only stable, well-documented features from dnstapir:
- OpenTelemetry integration (
configure_opentelemetry,OtlpSettings) - JSON logging configuration (
configure_json_logging)
These are core features that are actively maintained in the v1.1.0 release, and no deprecated functions or patterns are being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for deprecated functions from previous `dnstapir` versions.
# Test: Find usage of deprecated `dnstapir` functions.
rg --json --type py 'dnstapir\.\w+' | jq -r '.data.lines.text' | while read -r line; do
if [[ "$line" =~ \.deprecated_function ]]; then
echo "Deprecated function found: $line"
fi
done
Length of output: 798
Script:
#!/bin/bash
# Let's check the actual usage of dnstapir in the codebase
rg -A 5 -B 5 'dnstapir' --type py
# Also check for any imports and their usage
rg -l 'from dnstapir|import dnstapir' --type py
# Look for any configuration or settings related to dnstapir
rg -A 3 'configure_opentelemetry|configure_json_logging|OtlpSettings' --type py
Length of output: 3103
nodeman/settings.py (1)
70-71: LGTM: Settings class updates
The addition of optional internal_ca and step fields provides good flexibility for different CA configurations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/utils.py (1)
23-28: Consider parameterizing the validity period.The certificate validity period is hardcoded to 1 day. While this might be sufficient for tests, consider making it configurable through a parameter for better flexibility.
def generate_ca_certificate( issuer_ca_name: x509.Name, issuer_ca_private_key: PrivateKey, root_ca_name: x509.Name | None = None, root_ca_private_key: PrivateKey | None = None, + validity_days: int = 1, ) -> x509.Certificate: """Generate CA Certificate""" now = datetime.now(tz=timezone.utc) - validity = timedelta(days=1) + validity = timedelta(days=validity_days)Also applies to: 31-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/utils.py(2 hunks)
🔇 Additional comments (3)
tests/utils.py (3)
7-7: LGTM! Import changes align with the new functionality.
The imports are correctly organized and necessary for the certificate generation functionality.
Line range hint 10-21: Skipping review of unchanged code.
This existing function remains unchanged in this PR.
34-65: LGTM! Robust CA certificate generation with proper extensions.
The implementation correctly:
- Sets all required certificate fields
- Adds proper CA extensions with appropriate criticality
- Uses secure defaults (random serial number)
- Fixed the previous issue with hash algorithm selection
Let's verify the certificate generation with different key types:
✅ Verification successful
Let me check what key types are actually being used in the tests.
The CA certificate generation is thoroughly tested with diverse key types
The implementation is verified to work correctly with multiple key types through comprehensive test coverage:
- RSA (2048-bit)
- ECDSA with P-256 (SECP256R1)
- ECDSA with P-384 (SECP384R1)
- Ed25519
- Ed448
The test suite in test_internal_ca.py explicitly verifies the CA certificate generation with each key type, and the get_hash_algorithm_from_key function (referenced in the code) is used to select the appropriate signing algorithm for each key type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the function is used with different key types
# Expected: Find test cases using different key algorithms
rg -A 5 "generate_ca_certificate" tests/
Length of output: 3031
Script:
#!/bin/bash
# Search for key generation and usage patterns
rg -B 2 -A 2 "generate_private_key|private_key =" tests/
Length of output: 6812
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nodeman/server.py (2)
67-73:⚠️ Potential issueAdd validation for mutually exclusive CA settings.
When both
internal_caandstepsettings are provided, the code silently prefersinternal_ca. This should be explicitly validated to prevent confusion.+ if self.settings.internal_ca and self.settings.step: + raise ValueError("Cannot specify both internal_ca and step settings") + if self.settings.internal_ca: self.ca_client = self.get_internal_ca_client(self.settings.internal_ca) elif self.settings.step: self.ca_client = self.get_step_client(self.settings.step)
74-81:⚠️ Potential issueAdd error handling for certificate loading operations.
The
get_internal_ca_clientmethod should handle potential file operation errors and provide meaningful error messages.@staticmethod def get_internal_ca_client(settings: InternalCaSettings) -> InternalCertificateAuthority: - return InternalCertificateAuthority.load( - issuer_ca_certificate_file=settings.issuer_ca_certificate, - issuer_ca_private_key_file=settings.issuer_ca_private_key, - root_ca_certificate_file=settings.root_ca_certificate, - validity_days=settings.validity_days, - ) + try: + return InternalCertificateAuthority.load( + issuer_ca_certificate_file=settings.issuer_ca_certificate, + issuer_ca_private_key_file=settings.issuer_ca_private_key, + root_ca_certificate_file=settings.root_ca_certificate, + validity_days=settings.validity_days, + ) + except OSError as e: + logger.error("Failed to load CA certificates: %s", e) + raise + except ValueError as e: + logger.error("Invalid CA certificate or private key: %s", e) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nodeman/internal_ca.py(1 hunks)nodeman/server.py(2 hunks)nodeman/settings.py(2 hunks)tests/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nodeman/settings.py
🔇 Additional comments (4)
tests/utils.py (1)
23-66: LGTM! The certificate generation logic is well-implemented.
The function correctly:
- Handles both issuer and root CA scenarios
- Sets appropriate validity periods
- Configures proper CA extensions
- Uses the correct private key and hash algorithm for signing
nodeman/internal_ca.py (3)
34-49: LGTM! Constructor is well-implemented with proper defaults.
The initialization logic correctly:
- Handles optional parameters with sensible defaults
- Sets up the signature hash algorithm based on the private key
- Properly manages the root CA certificate fallback
50-78: LGTM! The load method handles file operations safely.
The implementation:
- Uses proper type hints with Path objects
- Includes password=None as required
- Handles root CA certificate loading conditionally
15-25: Consider enabling key_encipherment for broader compatibility.
The KEY_USAGE extension has key_encipherment=False, which might limit compatibility with some clients that expect to use the certificate for TLS connections where key encipherment is required.
KEY_USAGE = x509.KeyUsage(
digital_signature=True,
content_commitment=False,
- key_encipherment=False,
+ key_encipherment=True,
data_encipherment=False,
key_agreement=False,
key_cert_sign=False,
crl_sign=False,
encipher_only=False,
decipher_only=False,
)✅ Verification successful
The current key usage settings are appropriate for client authentication
The code appears to be generating client certificates for TLS client authentication, as evidenced by the client.py configuration which specifically handles TLS client certificates. For client authentication certificates, digital_signature capability alone is sufficient and standard practice, while key_encipherment is typically only required for server certificates. The current KEY_USAGE settings align with the intended use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any existing certificates in the codebase use key_encipherment
rg -l "key_encipherment.*=.*True" --type python
Length of output: 80
Script:
#!/bin/bash
# Check if any existing certificates in the codebase use key_encipherment
rg -l "key_encipherment.*=.*True"
# Also search for any TLS-related code or configurations
rg -l "TLS|SSL"
# Look for any certificate-related test files or configurations
fd -e py -e yaml -e json "cert|certificate" --type f
Length of output: 126
Script:
#!/bin/bash
# Let's check the client.py file to understand the TLS/SSL usage context
rg -A 5 -B 5 "TLS|SSL" nodeman/client.py
# Let's also check if there are any certificate validation or usage patterns
rg "verify.*certificate|check.*certificate" nodeman/client.py
# Check for any certificate-related configurations or settings
rg "cert.*config|certificate.*config" nodeman/client.py
Length of output: 768
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nodeman/internal_ca.py (1)
56-56: 🛠️ Refactor suggestionDefault validity period should align with settings
The default validity period in
InternalCertificateAuthorityis set to 1 day (validity_days=1), which may be inconsistent with the project's default validity period defined in settings (e.g.,validity = 90 days). Consider updating the default or passing the settings' value to ensure consistency.
🧹 Nitpick comments (2)
tests/test_api.py (2)
38-47: Consider making the CA certificate validity period configurableThe current implementation uses a fixed 10-minute validity period. Consider making this configurable to support different test scenarios, such as testing certificate expiration or long-lived certificates.
-def get_ca_client() -> CertificateAuthorityClient: +def get_ca_client(validity: timedelta = timedelta(minutes=10)) -> CertificateAuthorityClient: ca_name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "Internal Test CA")]) ca_private_key = ec.generate_private_key(ec.SECP256R1()) ca_certificate = generate_ca_certificate(ca_name, ca_private_key) - validity = timedelta(minutes=10) return InternalCertificateAuthority( issuer_ca_certificate=ca_certificate, issuer_ca_private_key=ca_private_key, validity=validity, )
38-47: Consider adding edge case tests for CA operationsWhile the test coverage is comprehensive for normal operations, consider adding tests for the following scenarios:
- CA certificate expiration
- Invalid CA private key
- CA certificate chain validation
- Different key sizes for CA keys
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nodeman/internal_ca.py(1 hunks)nodeman/x509.py(4 hunks)tests/test_api.py(4 hunks)tests/test_internal_ca.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_internal_ca.py
🔇 Additional comments (5)
nodeman/x509.py (1)
18-18: Ensure compatibility with Python versions below 3.10
The use of the | operator for type unions in type PrivateKey = ... requires Python 3.10 or newer. If your project needs to support earlier Python versions, consider using typing.Union instead.
Run the following script to verify the Python version specified in your project configurations:
nodeman/internal_ca.py (1)
4-4: Ensure compatibility of typing.Self usage with Python versions below 3.11
The Self type annotation was introduced in Python 3.11. If your project needs to support earlier Python versions, consider replacing -> Self with -> InternalCertificateAuthority or using from __future__ import annotations to postpone evaluation of annotations.
Run the following script to check the Python version specified in your project configurations:
Also applies to: 62-67
✅ Verification successful
Let me search for any other files that might specify Python version requirements, such as requirements.txt, tox.ini, or any other configuration files.
Let me check the project's dependency management files more thoroughly, particularly the poetry.lock and pyproject.toml files, as we've seen poetry.lock references.
Python version requirement is compatible with typing.Self usage
The project explicitly requires Python 3.12 (python = "^3.12" in pyproject.toml), which is newer than Python 3.11 where typing.Self was introduced. Therefore, there are no compatibility concerns with using this feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the project specifies Python 3.11 or newer.
# Search for Python version requirements in setup.py or pyproject.toml
rg "python_requires.*" setup.py pyproject.toml || echo "Python version requirement not specified."
Length of output: 190
Script:
#!/bin/bash
# Search for Python version specifications in various configuration files
echo "=== Checking requirements.txt ==="
rg "^python[>=<]" requirements*.txt || echo "No Python version in requirements.txt"
echo -e "\n=== Checking tox.ini ==="
rg "^python.*=" tox.ini || echo "No tox.ini found or no Python version specified"
echo -e "\n=== Checking any .python-version files ==="
fd -g ".python-version" -x cat {}
echo -e "\n=== Checking for runtime version checks ==="
rg "sys\.version_info|python_version" -g "!*.pyc"
echo -e "\n=== Checking CI configuration files ==="
fd -e yml -e yaml | xargs rg "python.*version|python:.*3\." || echo "No Python version in CI configs"
Length of output: 1429
Script:
#!/bin/bash
# Check poetry configuration and dependencies
echo "=== Checking pyproject.toml for Python version constraints ==="
cat pyproject.toml || echo "No pyproject.toml found"
echo -e "\n=== Checking actual usage of Self type ==="
rg "Self" -A 5 -B 5 nodeman/internal_ca.py
echo -e "\n=== Checking typing_extensions dependency ==="
rg "typing-extensions" pyproject.toml poetry.lock
Length of output: 2931
tests/test_api.py (3)
4-4: LGTM: Import changes align with new CA implementation
The new imports support the transition to using InternalCertificateAuthority and provide necessary utilities for certificate generation and validation.
Also applies to: 14-14, 21-21, 26-26
52-52: LGTM: Clean integration of new CA client
The test client setup correctly integrates the new CA client implementation.
246-246: Verify RSA_EXPONENT value for security considerations
The code now uses the centralized RSA_EXPONENT constant. Let's verify its value meets security standards.
✅ Verification successful
RSA exponent value meets security standards
The RSA_EXPONENT is set to 65537 (0x10001) which is the recommended value for RSA key generation. This is a Fermat prime commonly used in practice as it provides a good balance between security and performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check RSA_EXPONENT value in x509.py
# Common secure values are typically 65537 (0x10001)
rg -A 1 "RSA_EXPONENT\s*="
Length of output: 177
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Makefile (1)
53-55: Consider adding clean target for internal CA filesThe internal CA files should be included in the clean target to ensure proper cleanup.
Add the following to the clean target:
clean: - rm -f $(STEP_CA_FILES) $(CLIENT_FILES) + rm -f $(STEP_CA_FILES) $(CLIENT_FILES) internal_ca_*.pem rm -f $(BUILDINFO) $(OPENAPI)nodeman/internal_ca.py (2)
44-59: Consider using settings.validity instead of validity_daysThe initialization accepts both validity_days and validity parameters, which could lead to confusion. Consider standardizing on settings.validity (in seconds) to align with the rest of the codebase.
95-132: Consider adding certificate policy and CRL distribution points extensionsThe sign_csr method could be enhanced with optional certificate policy and CRL distribution points extensions for better PKI management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(1 hunks)nodeman/client.py(1 hunks)nodeman/internal_ca.py(1 hunks)nodeman/server.py(2 hunks)nodeman/x509.py(4 hunks)tests/test_internal_ca.py(1 hunks)tests/test_step_ca.py(2 hunks)tests/test_x509.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_internal_ca.py
🔇 Additional comments (9)
tests/test_x509.py (2)
11-11: LGTM: Function import renamed consistently
The import statement has been updated to use the new function name verify_x509_csr_data, maintaining consistency with the function rename in the codebase.
36-36: LGTM: Test cases updated consistently
All test cases have been properly updated to use the renamed function verify_x509_csr_data while maintaining their original test logic and error handling.
Let's verify that all occurrences of the old function name have been updated:
Also applies to: 42-42, 48-48, 54-54, 60-60
✅ Verification successful
Function renaming verification complete - all instances properly updated
The codebase search shows consistent usage of two distinct functions:
verify_x509_csr_data: Used in test files and main code for data validationverify_x509_csr_signature: Used for signature verification
All test cases are correctly using verify_x509_csr_data, and there are no remaining instances of any old function name that needs updating. The changes are consistent across the entire codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old function name
rg "verify_x509_csr[^_]"
Length of output: 26
Script:
#!/bin/bash
# Let's first check for both old and new function names to understand the changes
rg "verify_x509_csr" -A 2
Length of output: 1980
Script:
#!/bin/bash
# Let's also check the function definition to understand the change
ast-grep --pattern 'def verify_x509_csr$$$'
Length of output: 45
Script:
#!/bin/bash
# Let's see the test file content to verify the changes
cat tests/test_x509.py
Length of output: 2088
tests/test_step_ca.py (1)
10-10: LGTM: StepCA test updated consistently
The import statement and function call have been properly updated to use verify_x509_csr_data, maintaining consistency with the function rename while preserving the test's integration with the StepCA client.
Also applies to: 49-49
Makefile (1)
53-55:
Security: Enhance protection for CA private key and certificate
The current implementation has several security concerns:
- The private key is created without password protection (--no-password)
- The --insecure flag bypasses security checks
- No specific file permissions are set for sensitive files
Consider the following improvements:
internal_ca:
- step certificate create root-ca internal_ca_certificate.pem internal_ca_private_key.pem --profile root-ca --insecure --no-password
+ step certificate create root-ca internal_ca_certificate.pem internal_ca_private_key.pem --profile root-ca
+ chmod 600 internal_ca_private_key.pem
+ chmod 644 internal_ca_certificate.pemLet's verify the current file permissions of any existing CA files:
nodeman/server.py (2)
67-73:
Add validation for mutually exclusive CA settings
The code silently prefers internal_ca over step when both are configured. This should be explicitly validated.
+ if self.settings.internal_ca and self.settings.step:
+ raise ValueError("Cannot configure both internal_ca and step settings")
if self.settings.internal_ca:
self.ca_client = self.get_internal_ca_client(self.settings.internal_ca)
elif self.settings.step:
self.ca_client = self.get_step_client(self.settings.step)
else:
self.ca_client = NoneLikely invalid or redundant comment.
74-83: 🛠️ Refactor suggestion
Add error handling for file operations in get_internal_ca_client
The method should handle file operation errors gracefully with proper logging.
@staticmethod
def get_internal_ca_client(settings: InternalCaSettings) -> InternalCertificateAuthority:
- res = InternalCertificateAuthority.load(
- issuer_ca_certificate_file=settings.issuer_ca_certificate,
- issuer_ca_private_key_file=settings.issuer_ca_private_key,
- root_ca_certificate_file=settings.root_ca_certificate,
- validity_days=settings.validity_days,
- )
- logger.info("Configured Internal CA (%s)", res.ca_fingerprint)
- return res
+ try:
+ res = InternalCertificateAuthority.load(
+ issuer_ca_certificate_file=settings.issuer_ca_certificate,
+ issuer_ca_private_key_file=settings.issuer_ca_private_key,
+ root_ca_certificate_file=settings.root_ca_certificate,
+ validity_days=settings.validity_days,
+ )
+ logger.info("Configured Internal CA (%s)", res.ca_fingerprint)
+ return res
+ except OSError as exc:
+ logger.error("Failed to load Internal CA: %s", exc)
+ raiseLikely invalid or redundant comment.
nodeman/x509.py (1)
126-130:
Fix incorrect signature verification parameters
The current implementation incorrectly uses signature_algorithm_parameters for padding and signature algorithm.
if isinstance(public_key, RSAPublicKey):
- verify_kwargs = {"algorithm": csr.signature_hash_algorithm, "padding": csr.signature_algorithm_parameters}
+ verify_kwargs = {
+ "padding": padding.PKCS1v15(),
+ "algorithm": csr.signature_hash_algorithm
+ }
elif isinstance(public_key, EllipticCurvePublicKey):
- verify_kwargs = {"signature_algorithm": csr.signature_algorithm_parameters}
+ verify_kwargs = {
+ "signature_algorithm": ec.ECDSA(csr.signature_hash_algorithm)
+ }Likely invalid or redundant comment.
nodeman/client.py (2)
240-241: LGTM! Good security improvement.
Using the key type and curve from the existing data key ensures cryptographic consistency during certificate renewal, preventing potential key type mismatches.
240-241: Verify error handling for all possible key types.
Let's ensure proper error handling for all possible key types that could be present in the data key.
✅ Verification successful
Based on the results, I can now generate the final response as we have comprehensive information about the error handling for all key types.
Error handling for key types is properly implemented
The generate_x509_key function has exhaustive pattern matching that handles all key types used in the codebase:
- Explicitly rejects RSA keys with a clear error message
- Supports EC keys with P-256 and P-384 curves
- Supports OKP keys with Ed25519 and Ed448 curves
The implementation aligns perfectly with all key types used across the codebase, as evidenced by the JWK key generation patterns in tests and client code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check supported key types in generate_x509_key function and data key usage
# Find all key type handling in generate_x509_key
echo "Checking key type handling in generate_x509_key:"
rg -A 1 "case \(" nodeman/client.py
# Find all places where JWK keys are created to identify possible key types
echo "Checking JWK key creation:"
rg "JWK\.generate|JWK\.from_"
Length of output: 1986
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
nodeman/internal_ca.py (4)
50-52: Simplify validity period configurationThe constructor accepts both
validity_daysandvalidityparameters, which could lead to confusion. Consider:
- Using only
validity: timedelta- Adding validation to ensure the validity period is reasonable
- validity_days: int = 1, - validity: timedelta | None = None, + validity: timedelta = timedelta(days=1),
57-58: Document the rationale for time_skew and validity defaultsThe default values for
time_skew(10 minutes) and validity period (1 day) should be documented to explain their purpose and why these specific values were chosen.
70-75: Enhance error handling for file operationsThe file operations should be wrapped in try-except blocks to provide more informative error messages when files are missing or corrupted.
- with open(issuer_ca_certificate_file, "rb") as fp: - issuer_ca_certificate = x509.load_pem_x509_certificate(fp.read()) + try: + with open(issuer_ca_certificate_file, "rb") as fp: + issuer_ca_certificate = x509.load_pem_x509_certificate(fp.read()) + except FileNotFoundError as e: + raise ValueError(f"CA certificate file not found: {e.filename}") + except Exception as e: + raise ValueError(f"Failed to load CA certificate: {e}")
97-99: Consider adding more detailed loggingThe debug log could include more information about the CSR, such as the public key algorithm and requested extensions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nodeman/internal_ca.py(1 hunks)tests/test_internal_ca.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_internal_ca.py
🔇 Additional comments (5)
nodeman/internal_ca.py (5)
1-21: LGTM! Well-organized imports and proper logger setup.
The imports are comprehensive and logically grouped, covering all necessary cryptographic functionality. The logger setup follows best practices.
26-36: Consider adding key_encipherment to KEY_USAGE for TLS certificates
The current KEY_USAGE flags might be too restrictive for TLS certificates. For TLS client and server authentication, key_encipherment is typically required, especially when using RSA keys.
73-74: Add type checking for private key in load method
The load method should verify that issuer_ca_private_key is an instance of PrivateKey before returning.
90-93: LGTM! Secure implementation of certificate fingerprint.
The implementation correctly uses SHA256 for fingerprinting and properly formats the output.
126-130: LGTM! Proper certificate chain construction.
The logic for building the certificate chain correctly handles both root and intermediate CA scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
nodeman/x509.py (2)
57-70:⚠️ Potential issueAdd key size and curve validations for security.
While the function correctly handles all supported key types, it should include validations for key sizes and curves.
Apply this diff to add the necessary validations:
def generate_similar_key(key: PrivateKey) -> PrivateKey: """Generate similar new private key""" if isinstance(key, rsa.RSAPrivateKey): + if key.key_size < 2048: + raise ValueError("RSA key size must be at least 2048 bits") return rsa.generate_private_key(public_exponent=RSA_EXPONENT, key_size=key.key_size) elif isinstance(key, ec.EllipticCurvePrivateKey): + if not isinstance(key.curve, (ec.SECP256R1, ec.SECP384R1, ec.SECP521R1)): + raise ValueError("Unsupported curve, must be NIST P-256, P-384, or P-521") return ec.generate_private_key(curve=key.curve)
120-147:⚠️ Potential issueFix incorrect verification parameters in CSR signature verification.
The current implementation may not correctly verify the CSR signature due to improper use of
csr.signature_algorithm_parameters.Apply this diff to correct the verification parameters:
+from cryptography.hazmat.primitives.asymmetric import padding, ec if isinstance(public_key, RSAPublicKey): - verify_kwargs = { - "algorithm": csr.signature_hash_algorithm, - "padding": csr.signature_algorithm_parameters, - } + verify_kwargs = { + "padding": padding.PKCS1v15(), + "algorithm": csr.signature_hash_algorithm + } elif isinstance(public_key, EllipticCurvePublicKey): - verify_kwargs = { - "signature_algorithm": ec.ECDSA(csr.signature_hash_algorithm), - } + verify_kwargs = { + "signature_algorithm": ec.ECDSA(csr.signature_hash_algorithm) + }
🧹 Nitpick comments (1)
nodeman/x509.py (1)
152-183: LGTM! Consider enhancing error logging.The changes improve error handling and logging with good use of structured logging fields. However, consider including more details in the error log.
Apply this diff to enhance error logging:
try: ca_response = request.app.ca_client.sign_csr(csr, name) except Exception as exc: - logger.error("Failed to process CSR for %s", name) + logger.error("Failed to process CSR for %s: %s", name, str(exc), exc_info=exc) raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Error issuing certificate") from exc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nodeman/internal_ca.py(1 hunks)nodeman/x509.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nodeman/internal_ca.py
🔇 Additional comments (2)
nodeman/x509.py (2)
4-18: LGTM! Well-structured imports and type definitions.
The imports are properly organized, and the RSA_EXPONENT value of 65537 (Fermat F4 number) is the standard choice for RSA key generation. The PrivateKey type alias correctly encompasses all supported key types.
Line range hint 88-118: LGTM! Comprehensive CSR data validation.
The function performs thorough validation of the CSR data with proper error handling and logging. The specific exception types help in precise error identification.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores