Skip to content

Conversation

arturfromtabnine
Copy link
Contributor

@arturfromtabnine arturfromtabnine commented Jul 31, 2025

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:

import Portkey from "portkey-ai";

const client = new Portkey({
      ...config,
      tlsOptions: {
        rejectUnathorized: false
      },
    });
curl http://localhost:8787/v1/chat/completions \
  -H "Content-Type: application/json" \
  -H "x-portkey-tls-options: {\"rejectUnathorized\": \"false\"}" \
  -d '{
    "model": "gpt-4o",
    "messages": [{"role": "user", "content": "Hello!"}]
  }'

Motivation

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

Copy link
Contributor

matter-code-review bot commented Jul 31, 2025

Code Quality new feature maintainability

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

This PR refines the TLS options support by removing cert and key parameters from the options object in the getCustomHttpsAgent function within src/utils/fetch.ts. This change streamlines the TLS configuration, focusing the utility on rejectUnauthorized and ca (Certificate Authority) settings. The overall feature adds support for TLS options in outgoing requests, allowing users to customize TLS settings such as providing custom CA certificates or disabling certificate validation. These options can be sent via the x-portkey-tls-options header, which is parsed and used to configure a custom HTTPS agent for fetch requests.

🔍 Impact of the Change

This refinement simplifies the API for configuring custom HTTPS agents, making the TLS options more focused on common enterprise requirements for custom CAs and certificate validation. It continues to support the overall goal of providing flexibility for enterprise network requirements by allowing custom CA certificates and ignoring certificate issues via x-portkey-tls-options header. This is particularly useful in enterprise environments with private PKI infrastructure or when connecting to development/staging environments with non-standard certificates.

📁 Total Files Changed

  • src/utils/fetch.ts: Removed cert and key options from getCustomHttpsAgent function signature.

🧪 Test Added

N/A. This specific PR only removes parameters; the comprehensive unit tests for the overall TLS feature, as mentioned in the previous summary, would cover the remaining rejectUnauthorized and ca options.

🔒Security Vulnerabilities

The underlying TLS feature still allows disabling certificate validation (rejectUnauthorized = false), which remains a security consideration. The default behavior is secure (rejectUnauthorized = true), and this is clearly documented as a security consideration.

Motivation

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 required flexibility for enterprise network requirements where we do need to support per-model certificate configuration while maintaining deployment flexibility.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

N/A

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

N/A

Tanka Poem ♫

TLS options refined,
Cert and key now depart,
CA stands alone, strong.
Simpler path, secure flow,
Enterprise needs now met. 🚀

Sequence Diagram

sequenceDiagram
    participant Client as Client Application
    participant API as API Gateway/Service
    participant Handler as Webhook Handler
    participant FetchUtil as src/utils/fetch.ts

    Client->>+API: HTTP Request (x-portkey-tls-options header)
    Note over API: Request with TLS options
    API->>+Handler: Forward Request
    Note over Handler: Parse x-portkey-tls-options
    Handler->>Handler: Extract {rejectUnauthorized, ca} from header
    Handler->>+FetchUtil: getCustomHttpsAgent({rejectUnauthorized, ca})
    Note over FetchUtil: Removed 'cert' and 'key' options
    FetchUtil-->>-Handler: httpsAgent
    Handler->>API: Use custom httpsAgent for outgoing call
    API-->>-Client: Response

Loading

Copy link
Contributor

@matter-code-review matter-code-review bot left a 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

Copy link
Contributor

Important

PR Review Skipped

PR review skipped as per the configuration setting. Run a manually review by commenting /matter review

💡Tips to use Matter AI

Command List

  • /matter summary: Generate AI Summary for the PR
  • /matter review: Generate AI Reviews for the latest commit in the PR
  • /matter review-full: Generate AI Reviews for the complete PR
  • /matter release-notes: Generate AI release-notes for the PR
  • /matter : Chat with your PR with Matter AI Agent
  • /matter remember : Generate AI memories for the PR
  • /matter explain: Get an explanation of the PR
  • /matter help: Show the list of available commands and documentation
  • Need help? Join our Discord server: https://discord.gg/fJU5DvanU3

Copy link
Collaborator

@narengogi narengogi left a 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

@arturfromtabnine
Copy link
Contributor Author

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.
ie. in our case one particular model endpoint need to have tls check disabled or different CA used for different models sitting behind different proxies with in same installation/setup.
envs do not give us that flexibility
let me know if you see it other way

@narengogi
Copy link
Collaborator

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. ie. in our case one particular model endpoint need to have tls check disabled or different CA used for different models sitting behind different proxies with in same installation/setup. envs do not give us that flexibility let me know if you see it other way

Makes sense

Copy link
Collaborator

@narengogi narengogi left a 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!

Copy link
Contributor

Code change removes unused cert and key parameters from TLS options interface, simplifying the API surface.

@arturfromtabnine
Copy link
Contributor Author

bump for a merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants