feat: add tls support for streamable-http #568
Conversation
WalkthroughAdds optional TLS support to StreamableHTTPServer via a new WithTLSCert option, two new tls file fields, runtime validation of cert/key paths, conditional ListenAndServeTLS use in Start, a unit test asserting TLS fields, and docs showing the new option. (≤50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/streamable_http.go (2)
676-697: canUseTLS comment overpromises; either validate keypair or soften wordingThe code only checks file existence; it doesn’t validate certificate/key contents. Either update the comment or load/parse the keypair.
Minimal doc fix:
-// canUseTLS checks if TLS is properly configured and files are valid +// canUseTLS reports whether TLS was configured and both files exist on disk. +// Note: It does not validate certificate/key contents.Optionally, actually validate:
func (s *StreamableHTTPServer) canUseTLS() bool { @@ - // Check certificate file - if _, err := os.Stat(s.tlsCertFile); err != nil { + // Check certificate file + if _, err := os.Stat(s.tlsCertFile); err != nil { s.logger.Errorf("TLS certificate file error: %v", err) return false } @@ - // Check key file - if _, err := os.Stat(s.tlsKeyFile); err != nil { + // Check key file + if _, err := os.Stat(s.tlsKeyFile); err != nil { s.logger.Errorf("TLS key file error: %v", err) return false } - return true + // Ensure the pair is loadable + if _, err := tls.LoadX509KeyPair(s.tlsCertFile, s.tlsKeyFile); err != nil { + s.logger.Errorf("TLS keypair load error: %v", err) + return false + } + return true }Add import:
import ( // ... "crypto/tls" )
97-105: Clarify option semantics and scope in docstringMake it explicit this affects Start only and that invalid paths will cause Start to error (with the above change).
-// WithTLSCert sets the TLS certificate and key files for HTTPS support. -// Both certFile and keyFile must be provided to enable TLS. +// WithTLSCert sets the TLS certificate and key files for HTTPS support. +// Only used by Start(); it has no effect when using this server as an http.Handler behind another server. +// If either parameter is set, TLS is considered requested. Start will return an error if the files are not usable.server/streamable_http_test.go (1)
962-987: Rename test to reflect existence-only check (not validity)The files are empty; canUseTLS doesn’t validate contents. Rename for clarity.
- t.Run("canUseTLS returns true with valid temp cert and key files", func(t *testing.T) { + t.Run("canUseTLS returns true when cert and key files exist (content not validated)", func(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/streamable_http.go(10 hunks)server/streamable_http_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/streamable_http.go (1)
mcp/types.go (1)
JSONRPCNotification(333-336)
server/streamable_http_test.go (2)
server/server.go (1)
NewMCPServer(299-325)server/streamable_http.go (2)
NewStreamableHTTPServer(150-165)WithTLSCert(99-104)
🔇 Additional comments (7)
server/streamable_http.go (4)
145-146: LGTM: private TLS fields addedKeeps surface minimal; combined with options pattern this is fine.
257-259: LGTM: clearer sampling-response detection conditionThe boolean is precise and readable.
411-414: LGTM: active session tracking with cleanupStore/Delete via defer avoids leaks for sampling responses.
785-797: LGTM: sampling request lifecycle and backpressureBuffered chans, per-request response channel, Store/Delete, and context handling look correct for avoiding deadlocks.
Also applies to: 852-886
server/streamable_http_test.go (3)
898-915: LGTM: option wiring testAsserts fields are set via WithTLSCert.
917-960: LGTM: negative cases for canUseTLSCovers unconfigured and missing-file scenarios well.
989-1025: LGTM: asymmetric existence cases coveredGood negative coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
www/docs/pages/servers/basics.mdx (2)
181-187: Add TLS option example: good; fix option name and clarify PEM paths
- The example looks right, but the option is spelled WithStateLess in code (capital L). Docs show WithStateless. Align the snippet.
- Consider showing PEM extensions to reduce confusion.
Apply:
httpServer := server.NewStreamableHTTPServer(s, server.WithEndpointPath("/mcp"), - server.WithStateless(true), - server.WithTLSCert("/path/to/cert", "/path/to/key") + server.WithStateLess(true), + server.WithTLSCert("/path/to/cert.pem", "/path/to/key.pem") )
228-231: Stray closing brace in example blockThere appears to be one extra “}” at the end of the Environment-Based Configuration example. Trim it to avoid copy/paste errors.
Apply:
-} -} +}server/streamable_http.go (4)
204-216: Fail-fast TLS startup logic: good; add minor hardening and polish
- Keep fail-fast behavior (great).
- Improve error style (lowercase, Go convention).
- Guard against directories being passed.
- Optionally enforce TLS 1.2+ via http.Server.TLSConfig.
Apply:
if s.tlsCertFile != "" || s.tlsKeyFile != "" { - if s.tlsCertFile == "" || s.tlsKeyFile == "" { - return fmt.Errorf("both TLS cert and key must be provided") - } - if _, err := os.Stat(s.tlsCertFile); err != nil { - return fmt.Errorf("Failed to find TLS certificate file: %w", err) - } - if _, err := os.Stat(s.tlsKeyFile); err != nil { - return fmt.Errorf("Failed to find TLS key file: %w", err) - } + if s.tlsCertFile == "" || s.tlsKeyFile == "" { + return fmt.Errorf("both tls cert and key must be provided") + } + if fi, err := os.Stat(s.tlsCertFile); err != nil { + return fmt.Errorf("failed to find tls certificate file: %w", err) + } else if !fi.Mode().IsRegular() { + return fmt.Errorf("tls certificate path is not a regular file: %q", s.tlsCertFile) + } + if fi, err := os.Stat(s.tlsKeyFile); err != nil { + return fmt.Errorf("failed to find tls key file: %w", err) + } else if !fi.Mode().IsRegular() { + return fmt.Errorf("tls key path is not a regular file: %q", s.tlsKeyFile) + } + // Enforce modern TLS by default; honor existing TLSConfig if set + if srv.TLSConfig == nil { + srv.TLSConfig = &tls.Config{MinVersion: tls.VersionTLS12} + } else if srv.TLSConfig.MinVersion == 0 { + srv.TLSConfig.MinVersion = tls.VersionTLS12 + } return srv.ListenAndServeTLS(s.tlsCertFile, s.tlsKeyFile) }Add import:
import "crypto/tls"
772-775: Sampling flow: consider configurability and timeout ergonomics
- Channel sizes (notification 100, sampling 10) are hard-coded. Consider a StreamableHTTPOption to tune these for high-load deployments.
- RequestSampling returns “server overloaded” when the queue is full; offering a context-aware wait with a short timeout could be nicer DX.
Also applies to: 779-784, 839-874
185-201: Minor: log when starting with TLSOptional: emit an Info-level log when TLS is enabled (address + endpoint), which helps ops verify mode.
Example:
s.mu.Unlock() if s.tlsCertFile != "" || s.tlsKeyFile != "" { + s.logger.Infof("Starting HTTPS server on %s%s", srv.Addr, s.endpointPath) ... }
81-88: Custom http.Server note is clear, but TLSConfig interplay is implicitIf callers pass a custom http.Server, clarifying that Start honors srv.TLSConfig (and won’t override it) would help. No code change required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
server/streamable_http.go(9 hunks)server/streamable_http_test.go(1 hunks)www/docs/pages/servers/basics.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/streamable_http_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
server/streamable_http.go (1)
mcp/types.go (1)
JSONRPCNotification(333-336)
🔇 Additional comments (5)
server/streamable_http.go (5)
11-11: Import os: OKNeeded for cert/key existence checks. No issues.
97-104: Public TLS option API: clear and minimally scopedNaming and doc comment are clear; storing paths on the server is appropriate.
145-147: New TLS fields: appropriate encapsulationUnexported fields on the server struct are correct.
266-268: Sampling-response detection readability: OKRefactor improves clarity without changing behavior.
420-423: Active session registration for sampling: OKRegistering and deferring delete looks correct and prevents leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
www/docs/pages/servers/basics.mdx (1)
181-186: Fix Go snippet: add trailing comma after WithTLSCert in multiline callMissing trailing comma will make this example not compile when gofmt’d.
httpServer := server.NewStreamableHTTPServer(s, server.WithEndpointPath("/mcp"), server.WithStateless(true), - server.WithTLSCert("/path/to/cert.pem", "/path/to/key.pem") + server.WithTLSCert("/path/to/cert.pem", "/path/to/key.pem"), )
🧹 Nitpick comments (1)
www/docs/pages/servers/basics.mdx (1)
181-186: Optional doc note: clarify TLS expectationsConsider a one-liner below the snippet: “WithTLSCert enables HTTPS; expects PEM-encoded cert/key files readable by the process. Start returns an error if either file is missing/invalid.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/streamable_http.go(9 hunks)www/docs/pages/servers/basics.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/streamable_http.go
🔇 Additional comments (1)
www/docs/pages/servers/basics.mdx (1)
181-186: No action needed:WithTLSCertsignature and behavior match the documentation
Verified that in server/streamable_http.go (around line 99) the exported functionfunc WithTLSCert(certFile, keyFile string) StreamableHTTPOptionexactly matches the docs’ use of
server.WithTLSCert("/path/to/cert.pem", "/path/to/key.pem")– the parameter order (cert then key) and return type (
StreamableHTTPOption) are correct. The TLS fields onStreamableHTTPServerare properly applied andListenAndServeTLSis invoked when both files are set.
Description
Fixes #352
Adds an option to configure hosting the streamble-http server with TLS
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests
Documentation