Use JWKS for list of trusted keys #37
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 20 minutes and 4 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 (4)
WalkthroughThe pull request introduces a comprehensive renaming of Changes
Sequence DiagramsequenceDiagram
participant Client
participant NodemanServer
participant NodeConfiguration
Client->>NodemanServer: Enrollment Request
NodemanServer->>NodemanServer: Validate Trusted JWKs
NodemanServer->>NodeConfiguration: Create Configuration
NodeConfiguration-->>NodemanServer: Return Config with Trusted JWKs
NodemanServer->>Client: Send Enrollment Response
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: 6
🧹 Nitpick comments (3)
README.md (1)
24-24: Consider adding JWKSet format detailsThe documentation now correctly refers to
trusted_jwksas a JWKSet, but it would be helpful to include:
- Expected JWKSet format and required fields
- Supported key types and algorithms
- Example JWKSet structure
nodeman/server.py (1)
45-53: Consider adding JWKS validation middlewareGiven the security-critical nature of trusted keys, consider adding a validation middleware that:
- Validates JWKS format and individual JWK entries
- Verifies key algorithms and key usage
- Implements key rotation policies
- Monitors for expired or soon-to-expire keys
This would provide an additional security layer and better key management.
Would you like me to help design and implement this validation middleware?
nodeman/nodes.py (1)
Line range hint
149-149: Improve error message clarity for security failures.The current error message "Node enrollment failed" is generic and could be more specific while still maintaining security. Consider a more descriptive message that doesn't leak sensitive information.
- raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="Node enrollment failed") + raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="Node enrollment failed: Invalid or expired enrollment token")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)nodeman.toml(1 hunks)nodeman/models.py(1 hunks)nodeman/nodes.py(1 hunks)nodeman/server.py(1 hunks)nodeman/settings.py(1 hunks)tests/test.toml(1 hunks)
🔇 Additional comments (3)
nodeman/models.py (1)
91-91:
Significant data structure change requires careful migration
The change from list[PublicJwk] to dict[str, list[PublicJwk]] is more than just a rename - it's a breaking change that restructures how trusted keys are stored. This better aligns with standard JWKS format but requires careful migration of existing data.
Let's verify the impact on existing code:
nodeman/settings.py (1)
45-45: LGTM: Consistent rename of configuration field
The field rename from trusted_keys to trusted_jwks is straightforward and maintains the same type and behavior.
nodeman/nodes.py (1)
271-271: LGTM! Verify client compatibility with the renamed field.
The change from trusted_keys to trusted_jwks aligns with the standardization of JWK terminology. The implementation correctly uses the new field name while maintaining the same data source.
Let's verify the integration points:
✅ Verification successful
The change is consistent with the codebase schema and no legacy references remain
The verification confirms:
- No remaining references to
trusted_keysexist in Python files - The
NodeConfigurationmodel innodeman/models.pycorrectly defines the field astrusted_jwks: dict[str, list[PublicJwk]]
Let's verify the client-side handling:
Let's verify the client-side handling and tests specifically for the enrollment flow:
Let's check the test helper function _test_enroll since it's used by all enrollment tests:
Let's check the test configuration and app setup to see how the trusted_jwks is initialized:
The change is correctly implemented and consistent with the codebase
The verification confirms:
- The field is properly defined in the
NodesSettingsmodel astrusted_jwks: FilePath - The field is correctly used in the node enrollment endpoint
- No legacy references to
trusted_keysremain in the codebase - The change is consistent with the application's configuration structure
The change from trusted_keys to trusted_jwks is a safe terminology standardization that:
- Maintains the same security model
- Is properly reflected in the configuration schema
- Has no remaining legacy references that could cause compatibility issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to trusted_keys and verify NodeConfiguration schema updates
# Check for any remaining references to trusted_keys
echo "Checking for remaining trusted_keys references..."
rg "trusted_keys" --type py
# Look for NodeConfiguration schema/model definitions
echo "Checking NodeConfiguration definitions..."
rg "class NodeConfiguration" --type py -A 10
Length of output: 929
Script:
#!/bin/bash
# Check for client-side code that might be consuming NodeConfiguration
rg "NodeConfiguration" --type py -B 2 -A 2
# Look for any test files that might verify this behavior
fd "test.*\.py$" --exec rg "trusted_j[w]?k" {}
Length of output: 2133
Script:
#!/bin/bash
# Check enrollment tests
fd "test.*\.py$" --exec rg "def test.*enroll" -B 2 -A 10 {}
# Check the actual enrollment response handling in tests
fd "test.*\.py$" --exec rg "NodeConfiguration\(" -B 2 -A 5 {}
# Look for any mock data or fixtures that might be using the old field name
fd "test.*\.py$" --exec rg "trusted_(keys|jwks)" -B 2 {}
Length of output: 2557
Script:
#!/bin/bash
# Look for the test helper function implementation
rg "_test_enroll" -B 5 -A 20
# Check the test fixtures and configuration
fd "conftest\.py$|fixtures\.py$" --exec rg "trusted_(keys|jwks)" -B 2 -A 2 {}
# Look for any test data setup in the test files
fd "test.*\.py$" --exec rg "_test_enroll.*trusted" -B 10 -A 10 {}
Length of output: 5004
Script:
#!/bin/bash
# Check test configuration and app setup
rg "app\.trusted_jwks" -B 5 -A 2
# Look for app configuration in test files
fd "conftest\.py$" --exec rg "def.*app" -B 5 -A 15 {}
# Check settings initialization
rg "class.*Settings" -A 20 --type py
Length of output: 3699
| if filename := self.settings.nodes.trusted_jwks: | ||
| try: | ||
| with open(filename) as fp: | ||
| self.trusted_keys = JWKSet.from_json(fp.read()) | ||
| self.trusted_jwks = JWKSet.from_json(fp.read()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JWKS format validation before parsing
The code should validate the JWKS format before parsing to provide better error messages and prevent potential security issues.
if filename := self.settings.nodes.trusted_jwks:
try:
with open(filename) as fp:
- self.trusted_jwks = JWKSet.from_json(fp.read())
+ content = fp.read()
+ # Basic JWKS format validation
+ if '"keys"' not in content:
+ raise ValueError("Invalid JWKS format: missing 'keys' array")
+ self.trusted_jwks = JWKSet.from_json(content)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if filename := self.settings.nodes.trusted_jwks: | |
| try: | |
| with open(filename) as fp: | |
| self.trusted_keys = JWKSet.from_json(fp.read()) | |
| self.trusted_jwks = JWKSet.from_json(fp.read()) | |
| if filename := self.settings.nodes.trusted_jwks: | |
| try: | |
| with open(filename) as fp: | |
| content = fp.read() | |
| # Basic JWKS format validation | |
| if '"keys"' not in content: | |
| raise ValueError("Invalid JWKS format: missing 'keys' array") | |
| self.trusted_jwks = JWKSet.from_json(content) |
| self.trusted_jwks = JWKSet.from_json(fp.read()) | ||
| except OSError as exc: | ||
| logger.error("Failed to read trusted keys from %s", filename) | ||
| raise exc |
There was a problem hiding this comment.
Improve error handling for JSON parsing
The current error handling only catches OSError (file operations) but should also handle JSON parsing errors explicitly.
try:
with open(filename) as fp:
self.trusted_jwks = JWKSet.from_json(fp.read())
- except OSError as exc:
- logger.error("Failed to read trusted keys from %s", filename)
+ except OSError as exc:
+ logger.error("Failed to read trusted keys file: %s", filename, exc_info=True)
+ raise
+ except ValueError as exc:
+ logger.error("Invalid JWKS format in %s: %s", filename, exc, exc_info=True)
raise exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.trusted_jwks = JWKSet.from_json(fp.read()) | |
| except OSError as exc: | |
| logger.error("Failed to read trusted keys from %s", filename) | |
| raise exc | |
| self.trusted_jwks = JWKSet.from_json(fp.read()) | |
| except OSError as exc: | |
| logger.error("Failed to read trusted keys file: %s", filename, exc_info=True) | |
| raise | |
| except ValueError as exc: | |
| logger.error("Invalid JWKS format in %s: %s", filename, exc, exc_info=True) | |
| raise exc |
| @@ -42,15 +42,15 @@ | |||
| else: | |||
| self.logger.info("Configured without OpenTelemetry") | |||
|
|
|||
| self.trusted_keys = [] | |||
| if filename := self.settings.nodes.trusted_keys: | |||
| self.trusted_jwks = [] | |||
There was a problem hiding this comment.
Initialize with empty JWKSet instead of empty list
The initial value should be a JWKSet object to match the type after loading from file.
- self.trusted_jwks = []
+ self.trusted_jwks = JWKSet()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.trusted_jwks = [] | |
| self.trusted_jwks = JWKSet() |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Jakob Schlyter <[email protected]>
Summary by CodeRabbit
Documentation
trusted_keystotrusted_jwks.Configuration Changes
trusted_keystotrusted_jwksinnodeman.tomlandtests/test.toml.nodeman.toml.Improvements
enroll_nodefunction innodeman/nodes.py.Refactor