-
Notifications
You must be signed in to change notification settings - Fork 701
feat: add tls options support #1257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add tls options support #1257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds TLS options support for outgoing requests, which is a valuable feature for enterprise environments. The implementation is well-structured with good test coverage. I have a few suggestions to improve security awareness and error handling.
Skipped files
package-lock.json
: Skipped file pattern
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is not the best approach for configuring tls, we could allow configuring environment variables for tls cert and certificate authority paths instead of passing it in request
proposed approach is more flexible than setting NODE_EXTRA_CA_CERTS/NODE_TLS_REJECT_UNAUTHORIZED which will affect all the requests. |
Makes sense |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
Description
Add tls options support. Supporting custom Certificate Authority + Ignore certificate issues.
Options can be send via x-portkey-tls-options header which is parsed later on and used for custom https agent.
This change adds flexibility for enterprise network environments where we do need to support per-model certificate configuration while maintaining deployment flexibility.
Examples:
Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues