feat(#92): add SSL/TLS configuration support for HTTP requests #282
Conversation
Summary of ChangesHello @Oscaner, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture and flexibility of the plugin system by integrating comprehensive SSL/TLS configuration capabilities for outgoing HTTP requests. It allows users to configure certificate verification, custom CA certificates, and mutual TLS authentication through environment variables, which are then automatically applied to all Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive SSL/TLS configuration for HTTP requests made via httpx, by monkey-patching httpx.Client. The changes allow configuring SSL verification, custom CA certificates, and mTLS through environment variables.
My review focuses on the security and implementation of the SSL context creation in python/dify_plugin/core/utils/ssl.py. I've identified a few areas for improvement:
- A critical security enhancement to avoid writing CA certificates to disk by using the
cadataparameter. - A high-severity security concern regarding the temporary storage of client private keys on the filesystem.
- A recommendation to use more specific exception handling.
The new configuration options in DifyPluginEnv and the accompanying tests are well-implemented. Overall, this is a valuable feature for enhancing security.
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive SSL/TLS configuration for HTTP requests made via httpx, which is a great feature for enhancing security and connectivity in various environments. The implementation uses monkey-patching to apply settings globally, which is a clever approach to avoid changing all call sites.
My review focuses on improving the security and robustness of the new SSL utility module. I've identified a critical security vulnerability related to the handling of temporary files for certificates and private keys, and I've provided suggestions to mitigate this. I also have a few recommendations for improving exception handling and performance.
The addition of tests and example configuration is well done and makes the new feature easy to understand and verify.
Add comprehensive SSL/TLS configuration capabilities to the plugin system: - Add environment variables for SSL verification control, custom CA certificates, and mTLS client authentication - Implement httpx.Client monkey patch to automatically apply SSL settings from environment configuration - Support base64-encoded certificate data (CA cert, client cert, and client key) to avoid file system dependencies - Enable SSL verification bypass for development/testing environments - Apply SSL patches at module import time to ensure all HTTP requests use configured settings This allows plugins to communicate securely with services using custom certificates or mutual TLS authentication without code changes in individual HTTP client implementations.
… of temp file Refactor SSL context creation to use in-memory certificate loading via cadata parameter instead of writing to a temporary file. This simplifies the code by removing file I/O operations and temporary file cleanup, improving performance and reducing potential issues with file system access.
…decoding Import binascii module and catch specific binascii.Error instead of broad Exception when decoding base64 certificate data. This improves error handling specificity and follows best practices for exception handling by catching only the expected error type from base64.b64decode().
Enhance security of temporary SSL certificate and key file handling by: - Setting restrictive file permissions (0o600) immediately after creation - Using auto-delete temporary files to ensure cleanup - Adding explicit flush() calls to guarantee data is written to disk - Eliminating manual file cleanup code in favor of context manager - Adding comprehensive security tests for file permissions and cleanup This prevents potential security vulnerabilities where sensitive SSL keys could be exposed with overly permissive file permissions or left on disk after use. The ssl.SSLContext.load_cert_chain() reads file contents into memory, allowing immediate deletion after loading.
Move DifyPluginEnv instantiation from function scope to module level to avoid repeated object creation on every httpx.Client initialization. This improves performance since environment-based SSL settings are static and do not change during runtime.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable SSL/TLS configuration capabilities for HTTP requests by monkey-patching httpx. The approach is solid, particularly the secure handling of temporary files for mTLS. However, I've identified a critical issue and a high-severity issue that need to be addressed:
- Critical: The monkey patch is not applied to
httpx.AsyncClient, which means asynchronous requests will not use the new SSL configuration. This could lead to inconsistent behavior and security risks. - High: The integration tests for the patching logic are flawed and do not correctly verify that the configuration is being applied, reducing confidence in the implementation.
I have provided detailed comments and suggestions to resolve these issues. The other changes, including the new configuration options and the security-focused tests for temporary files, are well-implemented.
Extended SSL configuration patching to include httpx.AsyncClient in addition to the existing httpx.Client support. This ensures that both synchronous and asynchronous HTTP requests automatically apply SSL settings from environment variables (verification control, custom CA certificates, and mTLS). Changes: - Patched httpx.AsyncClient.__init__ to inject SSL configuration - Added comprehensive tests for AsyncClient SSL behavior - Updated module documentation to reflect async client support - Stored original AsyncClient.__init__ to enable proper patching This fix is necessary because the project uses gevent and async operations, which would otherwise bypass SSL configuration when using AsyncClient.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for configuring SSL/TLS for HTTP requests via environment variables, including support for custom CA and mTLS. The implementation via monkey-patching httpx is clever and minimizes impact on existing code. The addition of comprehensive tests, especially for security aspects like temporary file handling, is excellent.
My review includes a couple of points for improvement. I've identified a high-severity security and portability issue related to file permissions on non-POSIX systems. I've also suggested a medium-severity refactoring to improve the readability of the SSL context creation logic.
Overall, this is a well-executed feature that significantly enhances the plugin's security capabilities.
…patibility Refactor SSL context creation by decoding all certificate data upfront instead of checking boolean flags first. This reduces redundant decoding operations and simplifies the logic flow. Add Windows compatibility by conditionally applying chmod only on POSIX systems, as Windows handles file permissions differently. This prevents potential errors when running on Windows platforms.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for configuring SSL/TLS for all httpx requests by monkey-patching the client's __init__ methods. The changes are well-implemented, allowing for SSL verification control, custom CA certificates, and mTLS via environment variables. The use of base64-encoded data avoids filesystem dependencies, and the security of temporary files for mTLS keys is handled carefully. The accompanying tests are comprehensive, covering unit, integration, and security aspects. I have a couple of suggestions to improve code clarity and robustness.
…ate data Add try-except block to catch UnicodeDecodeError when decoding CA certificate data and raise a more descriptive ValueError with context about the HTTP_REQUEST_NODE_SSL_CERT_DATA requirement. This provides clearer feedback when non-UTF-8 data is provided instead of allowing the error to propagate uncaught. Also refactor client certificate/key file writing to use a loop for reduced code duplication, and add test case to verify proper error handling for invalid UTF-8 encoded certificate data.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed and thoroughly tested feature for SSL/TLS configuration via environment variables. The approach of monkey-patching httpx at import time is clever and provides a seamless experience for plugin developers. The code quality is high, with excellent test coverage that includes functionality, edge cases, and security considerations. I've included a couple of minor suggestions to further improve code clarity in a security-sensitive area and enhance test suite portability.
…IX test guard - Extract file path into separate variable in SSL context creation loop - Use explicit path variable for os.chmod instead of file.name - Add pytest skipif decorator to skip permissions test on non-POSIX systems - Improve code clarity and maintain consistent permission handling across platforms This change makes the temporary file permission setting more explicit and ensures tests only run on systems where chmod permissions work as expected.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive SSL/TLS configuration for HTTP requests by monkey-patching httpx. The implementation is robust, covering custom CA certificates and mTLS, and is backed by an excellent set of tests that validate functionality, edge cases, and security aspects like temporary file handling. The changes are well-structured and the new configuration options are clearly documented. I have a couple of minor suggestions to improve code clarity and test precision, but overall this is a high-quality contribution.
| for file, data, path in [ | ||
| (cert_file, client_cert_data, cert_file.name), | ||
| (key_file, client_key_data, key_file.name), | ||
| ]: | ||
| if os.name == "posix": | ||
| os.chmod(path, 0o600) | ||
| file.write(data) | ||
| file.flush() # Ensure data is written to disk |
There was a problem hiding this comment.
For improved readability and conciseness, you can simplify this loop by removing the redundant path variable. The file path can be accessed directly from the file object using file.name.
for file, data in [(cert_file, client_cert_data), (key_file, client_key_data)]:
if os.name == "posix":
os.chmod(file.name, 0o600)
file.write(data)
file.flush() # Ensure data is written to disk| pass | ||
|
|
||
| # Verify that chmod was called with 0o600 for both files | ||
| assert len(permissions_set) >= 2 |
There was a problem hiding this comment.
For a more precise test assertion, it's better to check for an exact number of calls. Since os.chmod is expected to be called exactly twice (once for the certificate and once for the key), using == 2 makes the test stricter and more accurate.
| assert len(permissions_set) >= 2 | |
| assert len(permissions_set) == 2 |
|
@Mairuis Please review |
1 participant
Add comprehensive SSL/TLS configuration capabilities to the plugin system:
This allows plugins to communicate securely with services using custom certificates or mutual TLS authentication without code changes in individual HTTP client implementations.
Pull Request Checklist
Thank you for your contribution! Before submitting your PR, please make sure you have completed the following checks:
Compatibility Check
README.mdREADME.mdREADME.mdREADME.mdAvailable Checks