-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add certificate trust configuration for MCP servers #8357
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
Conversation
- Add certificateTrust configuration options for SSE and StreamableHTTP transports - Support allowSelfSigned, caCertPath, and rejectUnauthorized options - Implement HTTPS agent configuration for Node.js fetch operations - Add comprehensive tests for certificate trust configuration - Add documentation explaining usage and security considerations Fixes #8355
- Use spread operator with type assertion for Node.js-specific agent property - Ensures compatibility with standard RequestInit type while allowing HTTPS agent
| headers: configInjected.headers, | ||
| } | ||
|
|
||
| // Apply certificate trust settings if configured |
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.
The certificate trust configuration logic for both the 'streamable-http' and 'sse' branches is nearly identical. Consider extracting this duplicated code into a shared helper function to improve maintainability.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
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.
Self-reviewing my own code like a linter staring into a mirror; flagging issues without mercy.
| @@ -0,0 +1 @@ | |||
| Subproject commit e46929b8d8add0cd3c412d69f8ac882c405a4ba9 | |||
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.
[P0] Accidental submodule/gitlink committed. Please remove this file from the PR and add the .review directory to .gitignore to prevent future commits.
| @@ -0,0 +1 @@ | |||
| Subproject commit 88a473b017af37091c85ce3056e444e856f80d6e | |||
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.
[P0] Accidental submodule/gitlink committed. Please remove this file from the PR and add the tmp directory to .gitignore to prevent future commits.
|
|
||
| // Create HTTPS agent with certificate trust settings | ||
| const agent = new https.Agent(agentOptions) | ||
| ;(requestInit as any).agent = agent |
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.
[P1] Using https.Agent and assigning requestInit.agent will be ignored by Node’s built-in fetch (Undici). Use undici.Agent (or Pool) and pass it via the RequestInit "dispatcher" option so TLS settings (ca/rejectUnauthorized) actually apply for streamable-http.
| return fetch(url, { | ||
| ...init, | ||
| headers, | ||
| ...({ agent } as any), |
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.
[P1] The custom fetch for SSE sets an "agent" option, which Undici-based fetch ignores. Build an Undici Agent/Pool and pass it via the "dispatcher" option here to ensure certificateTrust takes effect during reconnects.
| const agentOptions: any = {} | ||
|
|
||
| // Handle certificate rejection | ||
| if (rejectUnauthorized === false || allowSelfSigned === true) { |
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.
[P2] Precedence between allowSelfSigned and rejectUnauthorized is implicit (this forces rejectUnauthorized=false if allowSelfSigned=true). Consider validating mutually exclusive/precedence in schema and documenting it to avoid confusing combos.
| // Load CA certificate if provided | ||
| if (caCertPath) { | ||
| try { | ||
| const caCert = await fs.readFile(caCertPath, "utf-8") |
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.
[P2] caCertPath: consider supporting multiple CA files (array) and validating absolute vs relative paths with clearer error messages. Node agents accept an array for "ca".
| headers: configInjected.headers, | ||
| } | ||
|
|
||
| // Apply certificate trust settings if configured |
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.
[P3] The certificate-trust wiring is duplicated across streamable-http and sse branches. Consider factoring a helper that returns the proper fetch options (Undici dispatcher) to reduce drift.
| const https = await import("https") | ||
| const fs = await import("fs/promises") | ||
|
|
||
| const agentOptions: any = {} |
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.
[P3] agentOptions is typed as any. Replace with a typed shape (and avoid casting requestInit as any) to improve maintainability.
| }) | ||
|
|
||
| describe("Certificate trust configuration", () => { | ||
| it("should accept SSE server with certificate trust configuration", () => { |
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.
[P3] These tests cover schema parsing only. Add runtime tests that assert a dispatcher/agent is passed to fetch and that TLS options (ca/rejectUnauthorized) are honored (can be done with mocked Undici fetch).
|
Messy PR |
Summary
This PR addresses Issue #8355 by adding certificate trust configuration support for MCP servers using HTTPS connections (SSE and StreamableHTTP transports).
Problem
MCP servers using self-signed or internal CA-signed certificates cannot be used because there's no way to specify trust information for these certificates. This prevents users from connecting to MCP servers in development environments or corporate networks with internal certificate authorities.
Solution
Added a
certificateTrustconfiguration option for SSE and StreamableHTTP server types with three settings:allowSelfSigned: Allow self-signed certificates (for development)caCertPath: Path to custom CA certificate file (for internal CAs)rejectUnauthorized: Control certificate validation (defaults to true for security)Changes
CertificateTrustSchemato validate certificate trust configurationTesting
Security Considerations
rejectUnauthorized: true)Documentation
Added comprehensive documentation in
docs/mcp-certificate-trust.mdincluding:Fixes #8355
Important
Adds certificate trust configuration for MCP servers using HTTPS, with support for self-signed and custom CA certificates.
certificateTrustconfiguration for SSE and StreamableHTTP transports inMcpHub.ts.allowSelfSigned,caCertPath, andrejectUnauthorizedoptions.rejectUnauthorizedto true for security.CertificateTrustSchemafor validating certificate trust settings.connectToServer()forstreamable-httpandssetypes.McpHub.spec.tsfor various certificate trust configurations.mcp-certificate-trust.mdwith configuration examples and security notes.This description was created by
for b73bd7c. You can customize this summary. It will automatically update as commits are pushed.