feat:cipher negotiation #40
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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 (8)
hivemind_core/protocol.py (8)
88-90: Consider configurable defaults forcipherandencoding
Hard-coding these fields is fine for now, but you may want to load or override them from a config file or environment variables to avoid frequent code changes if you later decide to update defaults.
129-129: Add error handling around binary encryption
Ifencrypt_binfails (e.g., invalid key or cipher parameters), the exception will bubble up. Recommend adding a try-except block to log or gracefully handle encryption failures.try: - payload = encrypt_bin(key=self.crypto_key, plaintext=payload, cipher=self.cipher) + payload = encrypt_bin(key=self.crypto_key, + plaintext=payload, + cipher=self.cipher) except Exception as e: LOG.error(f"Encryption failed: {e}") return
134-135: Add consistent exception handling for JSON encryption
Similar to the binary encryption above, consider protectingencrypt_as_jsonwith a try-except clause to handle any unexpected errors during encryption.try: - payload = encrypt_as_json( - key=self.crypto_key, plaintext=message.serialize(), - cipher=self.cipher, encoding=self.encoding - ) + payload = encrypt_as_json( + key=self.crypto_key, + plaintext=message.serialize(), + cipher=self.cipher, + encoding=self.encoding + ) except Exception as e: LOG.error(f"JSON encryption failed: {e}") return
148-149: Handle errors from binary decryption
Decryption errors can crash the server under certain conditions (e.g., malformed ciphertext). Consider wrappingdecrypt_binin a try-except.
152-153: Handle errors from JSON decryption
Similar to binary decryption, add a try-except fordecrypt_from_jsonto log or reject malformed input.
274-275: Consider limiting advertised ciphers and encodings
Here you list all possibleSupportedEncodingsandSupportedCiphersto the client. In a more restrictive environment, you may want to omit or filter ciphers you don’t truly support to prevent misconfigurations.
476-479: Robust cipher selection
Automatically selecting the first cipher and encoding in the client’s list is simplistic. Consider implementing a more robust negotiation strategy (e.g., matching the highest-security cipher that both client and server support).
482-482: Investigate Tornado events
This comment suggests Tornado never emits certain events. Confirm if additional debugging or event handling is needed, or remove the comment if it’s no longer relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hivemind_core/protocol.py(9 hunks)
🔇 Additional comments (5)
hivemind_core/protocol.py (5)
18-20: Import statements look good
The encryption-related imports are consistent and appear correctly structured. No issues here.
173-175: No actionable changes in this comment block
These lines only contain a comment and do not affect code behavior. No issues to address.
460-463: Graceful fallback for missing handshake data
This logic correctly defaults to JSON_HEX or AES_GCM if the client doesn’t provide any encodings/ciphers. That is appropriate for backward compatibility.
512-515: Handshake message assembly looks correct
The handshake response includesencodingandcipherfor proper synchronization. No issues here.
6-6: Ensure Python 3.8+ environment or backportLiteralfor broader compatibility
UsingLiteralis fine, but note that it was introduced in Python 3.8’stypingmodule. If older Python versions must be supported, consider a backport or alternative type annotations.To confirm your environment compatibility, run:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
hivemind_core/config.py (1)
41-43: Ensure concurrency-safety when updating defaults
When iterating over_DEFAULT.items()and updatingdb, consider potential concurrency issues if multiple threads callget_server_config()simultaneously. You might wrap this logic in a proper lock or transaction if the storage backend supports concurrent writes.hivemind_core/protocol.py (9)
18-21: Usage of private encryption methods
The imports_norm_encodingand_norm_cipherare non-public (underscored) methods. Consider making them part of the public API if widely used, or encapsulate them in a dedicated helper to avoid relying on private functions that may change unexpectedly.
130-130: Check for potential size overhead in binary encryption
Register how large the output might grow. Some ciphers add an authentication tag, which increases payload size. Large sizes could degrade performance in networks with limited bandwidth.
135-136: Add error handling for encryption failures
Ifencrypt_as_jsonfails for some reason (e.g., invalid key), consider catching and logging or raising a specific exception to help with troubleshooting.
149-150: Decrypting binary data
Check if there's any need to verify the integrity of the decrypted payload (e.g., checking signatures or HMAC). Without it, there might be a risk of undetected data corruption.
153-154: Graceful fallback on decryption errors
Whendecrypt_from_jsonfails, consider whether we should explicitly handle the error to avoid cryptic stack traces. This might safeguard the entire handshake/communication mechanism.
174-176: Expand the TODO with a plan
This TODO around intent/skill blocking is crucial for security. Provide a more detailed plan or timeline to ensure it’s not left indefinitely.
465-470: Prefer single use of “preferences”
The code reads the requested encodings and ciphers from the client and normalizes them. If we expect a wide range of possible preferences, validate their correctness earlier to avoid normalizing invalid or malicious values.
471-475: Reference the server config only once
We read the server config again here. If possible, store its results in a local variable or pass them to reduce repeated I/O or function calls. This can enhance efficiency.
484-487: Honor client preference order
Grabbing the first option from the client preference list is fine, but consider including a server “priority order” for more robust negotiation if compatibility issues arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hivemind_core/config.py(2 hunks)hivemind_core/protocol.py(9 hunks)
🔇 Additional comments (10)
hivemind_core/config.py (1)
8-10: Validate new default values with protocol usage
The newly added"allowed_encodings"and"allowed_ciphers"default values look good. Please ensure they remain in sync with the supported ones used inprotocol.py, especially if we add or remove encodings/ciphers there.hivemind_core/protocol.py (9)
6-6: Check Python version compatibility forLiteral
UsingLiteralrequires Python ≥ 3.8. Confirm that the environment meets this requirement, or otherwise include a compatibility library such astyping_extensionsif needed.
14-14: Prevent potential circular imports
Ensure that importingget_server_configfromhivemind_core.configdoes not create circular dependencies, especially now thathivemind_core/config.pyreferences_DEFAULTused for protocol defaults.
89-91: Flexible default cipher and encoding
The addition ofcipherandencodingwith defaults (AES_GCMandJSON_HEX) is a good enhancement. Just ensure this is documented so future maintainers know the default security posture.
264-267: Synchronized server config usage
When pulling server config values (allowed_ciphers,allowed_encodings), confirm whether multiple calls toget_server_configremain consistent across threads or processes. If these values change at runtime, consider a caching strategy.
279-280: Transparent handshake message
These lines expose allowed encodings and ciphers to the client. Confirm that revealing these defaults doesn’t pose a security risk. Generally, listing available ciphers is acceptable, but be mindful of revealing too much internal knowledge.
476-478: Reject or degrade gracefully if no valid cipher/encoding remains
The filtering approach is good, but if no valid options remain, the code breaks the connection. Check whether a fallback or negotiation is needed for partial support.
479-483: Close handshake on invalid request
Disconnecting is appropriate for security. Ensure thorough logs are captured to facilitate debugging if a client is unexpectedly disconnected.
490-490: Clarify password handshake
Quick mention that the password handshake ensures a derived key is never transmitted. Good approach. Document any fallback or re-handshake logic if partial or multiple attempts are needed.
520-523: Explicit handshake confirmation
Returning thecipherandencodingis a solid finalization step. Confirm that clients subsequently adhere to these settings; if they deviate, consider forcibly dropping the connection.
companion to JarbasHiveMind/hivemind-websocket-client#50
message size reduction showcase:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores
hivemind_bus_client.