Send enrollment key as JWK #34
Conversation
WalkthroughThe pull request introduces significant changes to the node enrollment process across multiple files in the nodeman project. The primary focus is on refactoring how node enrollment secrets are handled, moving from a raw secret to a more structured JSON Web Key (JWK) approach. The changes involve modifying the Changes
Possibly related issues
Sequence DiagramsequenceDiagram
participant Client
participant NodeManager
participant Database
Client->>NodeManager: Request Node Creation
NodeManager->>NodeManager: Generate HS256 Enrollment Key
NodeManager->>Database: Store Node Information
NodeManager-->>Client: Return NodeBootstrapInformation with Key
Client->>NodeManager: Enroll Node using Key
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 (2)
nodeman/nodes.py (1)
77-77: Consider storing the complete JWKCurrently only storing the key component (
k) might bypass algorithm enforcement during verification. Consider storing the complete JWK:- TapirNodeSecret(name=node.name, secret=node_enrollment_key.k).save() + TapirNodeSecret(name=node.name, secret=node_enrollment_key.export(private_key=True)).save()Then update the verification code to reconstruct the complete JWK with the correct algorithm:
- hmac_key = JWK(kty="oct", k=node_secret.secret) + hmac_key = JWK.from_json(node_secret.secret)tests/test_api.py (1)
Line range hint
278-330: Consider adding test for malformed JWK handlingWhile the current tests cover valid JWK and incorrect signatures, consider adding a test case for malformed JWK responses to ensure robust error handling.
Example test case:
def test_enroll_malformed_jwk() -> None: client = get_test_client() admin_client = get_test_client() admin_client.auth = BACKEND_CREDENTIALS # Mock response to return malformed JWK response = admin_client.post("/api/v1/node") create_response = response.json() create_response["key"]["kty"] = "invalid" with pytest.raises(ValueError): hmac_key = JWK(**create_response["key"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
nodeman/client.py(2 hunks)nodeman/jose.py(1 hunks)nodeman/models.py(2 hunks)nodeman/nodes.py(2 hunks)tests/test_api.py(4 hunks)
🔇 Additional comments (7)
nodeman/models.py (1)
68-68: Verify API compatibility for existing clients
The change from secret: str to key: PrivateSymmetric is a breaking change in the API response structure. Ensure that:
- All clients are updated to handle JWK format
- API version is bumped if backward compatibility isn't maintained
✅ Verification successful
API change is safe and properly handled
The codebase analysis shows that the NodeBootstrapInformation model is only used internally within the application:
- In
nodes.py, the API endpoint creates the key usingJWK.generate()and exports it as a dictionary before returning - In
client.py, the client code already expects and handles the key in the new format - No external clients or other usages of the old
secretfield were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find potential client implementations that might break
rg -A 3 "NodeBootstrapInformation.*secret" --type py
Length of output: 54
Script:
#!/bin/bash
# Let's try a broader search to find usages of NodeBootstrapInformation
rg -A 5 "NodeBootstrapInformation" --type py
# Also search for any API endpoints or handlers that might use this model
rg -A 5 "def.*bootstrap" --type py
# Look for test files that might show usage patterns
fd "test.*\.py$" --exec rg -l "NodeBootstrapInformation"
Length of output: 2352
nodeman/nodes.py (1)
82-83: LGTM: Response handling is correct
The response correctly includes the complete JWK with algorithm metadata.
nodeman/client.py (2)
156-159: LGTM! Improved security by moving sensitive data to debug level.
The change appropriately moves the full response logging to debug level, reducing the risk of accidentally exposing sensitive data in production logs.
213-213: Verify key format validation
While accessing the JWK's 'k' parameter is correct for symmetric keys, we should ensure that the key format is validated before access.
✅ Verification successful
Based on the search results, I can now provide a final response as there's sufficient evidence to verify the key validation concern:
Key validation is properly implemented in the codebase
The key validation is handled through multiple layers:
- The
PrivateSymmetricmodel injose.pyenforces JWK structure validation with strict type constraints forktyandkfields - The key is validated during JWS verification in
nodes.pywherejws.verify(key=hmac_key)is called - The
jwcryptolibrary's JWK implementation provides additional validation when constructing the key object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for key format validation before accessing 'k' parameter
# Look for validation code in the codebase
# Search for key validation patterns
rg -A 5 "def.*validate.*key"
# Search for JWK validation patterns
ast-grep --pattern 'if $key["kty"] != "oct": $_'
Length of output: 81
Script:
#!/bin/bash
# Let's try a broader search for key validation patterns and JWK handling
# Search for JWK-related code and validation
rg -B 2 -A 5 "jwk|JWK"
# Search for key validation or verification patterns
rg -B 2 -A 5 "(verify|validate|check).*key" -g "*.py"
# Look at the context around where the key is used
rg -B 10 -A 5 "node_bootstrap_information.key" -g "*.py"
Length of output: 28822
tests/test_api.py (3)
83-83: LGTM! Improved key handling by using complete JWK object.
The change properly uses the complete JWK object from the response instead of reconstructing it, which is more secure and maintains all key metadata.
Also applies to: 102-103
279-282: LGTM! Added explicit algorithm verification.
The addition of the algorithm assertion ensures the test uses the correct HMAC algorithm (HS256) and helps catch any unintended algorithm changes.
327-330: LGTM! Consistent algorithm verification in error test case.
The changes maintain consistency with the successful enrollment test cases and properly verify the HMAC algorithm in the error test scenario.
Summary by CodeRabbit
Release Notes
New Features
PrivateSymmetricclass for handling JSON Web Keys.create_nodeendpoint to return a structuredNodeBootstrapInformationobject with a key attribute.Bug Fixes
Chores