feat(#92): add SSL/TLS configuration support for HTTP requests#282
feat(#92): add SSL/TLS configuration support for HTTP requests#282Oscaner wants to merge 1 commit intolanggenius:mainfrom
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.
|
/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.
|
/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.
|
/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.
|
/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.
|
/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.
python/dify_plugin/core/utils/ssl.py
Outdated
| 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 |
|
Hi @Yeuoly， httpx 不遵守 SSL_CERT_FILE、REQUESTS_CA_BUNDLE、CURL_CA_BUNDLE 这一类的 CA 证书环境参数， 麻烦审核一下这个 PR，不然在企业内部环境，很多跟文件相关的插件都会因为自签名证书问题无法使用。 |
Add environment-based SSL configuration for httpx clients with automatic patching on module import. This enables plugins to handle enterprise scenarios requiring custom certificates or mTLS without code changes. - Add SSL configuration environment variables to DifyPluginEnv: - HTTP_REQUEST_NODE_SSL_VERIFY: toggle SSL certificate verification - HTTP_REQUEST_NODE_SSL_CERT_DATA: custom CA certificate (base64) - HTTP_REQUEST_NODE_SSL_CLIENT_CERT_DATA: client cert for mTLS (base64) - HTTP_REQUEST_NODE_SSL_CLIENT_KEY_DATA: client key for mTLS (base64) - Implement automatic SSL context patching for httpx.Client/AsyncClient - Monkey-patch client initialization to inject SSL configuration - Support custom CA certificates with secure temporary file handling - Support mutual TLS (mTLS) authentication - Auto-apply patches on dify_plugin module import for zero-config usage - Add comprehensive test suite covering SSL configuration and patching This allows plugins to inherit SSL settings from environment variables transparently, supporting enterprise deployments with custom certificate authorities and mutual authentication requirements.
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