Generic enrollment keys #36
Conversation
|
Warning Rate limit exceeded@jschlyter has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces significant changes to the node management system, focusing on enhancing the configuration and key management for node enrollment. The modifications span multiple files, including Changes
Sequence DiagramsequenceDiagram
participant Client
participant NodeManager
participant InternalCA
participant Database
Client->>NodeManager: Enroll Node
NodeManager->>InternalCA: Generate Enrollment Key
InternalCA-->>NodeManager: Return Enrollment Key
NodeManager->>Database: Save TapirNodeEnrollment
Database-->>NodeManager: Confirm Save
NodeManager->>Client: Return Enrollment Response
Possibly Related PRs
Suggested Reviewers
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: 2
🧹 Nitpick comments (5)
nodeman/nodes.py (3)
77-80: Potential concurrency race when creating TapirNodeEnrollment.When generating the enrollment key and immediately storing it, consider wrapping the creation of both TapirNode and TapirNodeEnrollment in a transaction (if your DB and ODM support it) to ensure consistency in high-concurrency scenarios.
222-224: Potentially handle multiple node enrollments.Currently, only one enrollment entry is expected per node. If in the future multiple enrollment processes are introduced (or re-enrollment entries are needed), consider whether this logic should handle multiple matches instead of just the first.
236-236: Clear inline comment on signature verification.Minor note: good to see a clarifying comment reminding future maintainers of the purpose of signature verification by the enrollment key.
nodeman.toml (1)
15-16: Commented-out [step] lines.Retaining them is helpful for historical context or fallback. However, if the old approach is truly deprecated, consider fully removing or referencing them in a readme to avoid confusion.
tests/test_api.py (1)
325-328: Consider extracting key generation parametersThe key generation parameters (kty, crv, alg) are duplicated across test cases. Consider extracting these into constants or test parameters.
+# At the top of the file +TEST_KEY_PARAMS = { + "ed25519": {"kty": "OKP", "crv": "Ed25519", "alg": "EdDSA"}, + "p256": {"kty": "EC", "crv": "P-256"}, + # ... other key types +} - data_key = JWK.generate(kty=kty, crv=crv, alg="EdDSA") - bad_data_key = JWK.generate(kty=kty, crv=crv, alg="EdDSA") + params = TEST_KEY_PARAMS["ed25519"] + data_key = JWK.generate(**params) + bad_data_key = JWK.generate(**params)Also applies to: 340-341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
nodeman.toml(1 hunks)nodeman/db_models.py(1 hunks)nodeman/jose.py(4 hunks)nodeman/models.py(2 hunks)nodeman/nodes.py(5 hunks)tests/test_api.py(6 hunks)
🔇 Additional comments (17)
nodeman/nodes.py (7)
14-14: Good move changing to TapirNodeEnrollment.
You switched from TapirNodeSecret to TapirNodeEnrollment. Ensure that all references to TapirNodeSecret across the codebase are removed or updated to prevent confusion.
191-192: Ensure consistency and maintain auditing logic for node enrollment deletion.
On node deletion, you also remove its enrollment record. This is good for ensuring no stale secrets remain; however, some systems prefer to keep a historical record for auditing or forensic analysis.
227-227: Empty line note.
No direct issue, but be mindful if you intend to insert logic or comments here.
228-228: JWK import is correct.
The usage of JWK(**node_enrollment.key) aligns with the changes to store the key as a DictField. Ensure error handling is robust if the stored data is malformed.
238-238: Signature verification is correct.
The logic for verifying JWS by enrollment key is appropriate.
241-242: Fine-grained error response.
Raising 401 Unauthorized for invalid enrollment signature is an appropriate usage of HTTP status codes.
267-267: Final enrollment deletion could omit re-enrollment.
After successful enrollment, the record is deleted. If this node needs to re-enroll, it might need a new record. Validate that it is the desired flow for your business logic.
nodeman.toml (1)
4-8: Good introduction of [internal_ca] section.
This new section clarifies where the internal CA details reside. The inline comments referencing the prior [step] section are helpful but ensure that future maintainers understand which config is active.
nodeman/db_models.py (1)
46-50: New TapirNodeEnrollment model looks good.
By storing both name and key in a dedicated collection, you now have a more granular enrollment record for each node. Be sure to add any indexes you might need for lookup or expiry.
nodeman/jose.py (4)
14-14: Optional alg field is beneficial.
Allowing an optional algorithm field in BaseJWK is a good enhancement for flexibility across different cryptographic operations.
40-42: EC private key representation is correct.
The addition of a private scalar (d) for curves P-256/P-384 aligns with typical Elliptic Curve standard usage.
52-54: OKP private key representation is correct.
Similar to RSA/EC changes, properly storing the private field for Ed/X curves. Same caution applies about logging.
64-64: PrivateJwk union ensures flexible usage.
Defining PrivateJwk as a union of RSA, EC, and OKP private keys is a clear approach to handle multiple key types.
nodeman/models.py (2)
10-10: LGTM: Import changes align with new key type support
The addition of PrivateJwk import supports the enhanced key type flexibility for node enrollment.
68-68: Verify backward compatibility with existing nodes
The field type change from PrivateSymmetric to PrivateSymmetric | PrivateJwk enhances flexibility but requires verification of existing node compatibility.
✅ Verification successful
Let me gather more information about the key usage and type handling in the codebase.
Based on the gathered information, I can now provide the final response:
The type change to PrivateSymmetric | PrivateJwk is backward compatible
The code analysis shows that:
- The key handling in
enroll_nodeuses JWK operations (JWK(**node_enrollment.key)) which works with both types - The client code in
command_enrollalready handles JWK format for enrollment - No direct dependencies on
PrivateSymmetrictype are found in the codebase - The key is only used during the enrollment process and not stored long-term
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing nodes that might be affected by the key type change
# Search for node creation/enrollment patterns
rg -A 5 "NodeBootstrapInformation|create_node|enroll_node"
# Look for any existing key type references
ast-grep --pattern 'key: PrivateSymmetric = $_'
Length of output: 2670
Script:
#!/bin/bash
# Check how the key is used in node enrollment and validation
rg -A 5 "key.*PrivateSymmetric|key.*PrivateJwk"
# Look for key validation or processing logic
ast-grep --pattern 'def $_($$$) -> NodeConfiguration:
$$$
'
# Check for any key-related migrations or database schemas
fd -e sql -e py -e json "migration|schema" --exec rg -l "key|PrivateSymmetric|PrivateJwk" {}
Length of output: 8515
tests/test_api.py (2)
103-103: LGTM: Improved algorithm detection logic
The new algorithm detection with fallback mechanism is more robust:
- First tries to get the algorithm directly from the key
- Falls back to jwk_to_alg if not present
Also applies to: 283-283
Line range hint 101-114: Verify JWS signature order requirements
The code adds signatures in a specific order (enrollment key first, data key second). Verify if this order is required by the API implementation.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests