fix(openai_compat): load system CA certs from Termux paths#1397
fix(openai_compat): load system CA certs from Termux paths#1397badgerbees wants to merge 1 commit intosipeed:mainfrom
Conversation
On Android/Termux, Go's x509.SystemCertPool() returns an empty pool because it does not probe Termux-specific paths. This causes TLS handshakes to fail with 'certificate signed by unknown authority' for all LLM providers, including volcengine. Changes: - Add buildCertPool() helper that supplements the system cert pool with CA bundles from Termux-specific paths (/data/data/com.termux/files/usr/etc/tls/cert.pem, etc.) - Update NewProvider() to always use an explicit TLS-configured transport that clones http.DefaultTransport and sets RootCAs - Preserve proxy support and all http.DefaultTransport defaults - Add TestNewProvider_TLSTransportNeverInsecure() to ensure InsecureSkipVerify is never set The fix is secure: certificate verification remains enforced, no weak ciphers or version downgrades, InsecureSkipVerify is never set. Fixes sipeed#1375
|
|
There was a problem hiding this comment.
Pull request overview
Fixes TLS certificate verification failures on Android/Termux by ensuring the provider’s HTTP transport uses a cert pool that includes Termux CA bundle locations.
Changes:
- Added
buildCertPool()to supplement the system cert pool with Termux CA bundle paths. - Switched
NewProviderto clonehttp.DefaultTransportand set aTLSClientConfigwithRootCAs. - Added a regression test ensuring
InsecureSkipVerifyis never enabled (with/without proxy).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/providers/openai_compat/provider.go | Adds Termux CA loading and updates transport initialization to use explicit RootCAs. |
| pkg/providers/openai_compat/provider_test.go | Adds TLS transport security regression test for InsecureSkipVerify. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func buildCertPool() *x509.CertPool { | ||
| pool, err := x509.SystemCertPool() | ||
| if err != nil || pool == nil { | ||
| pool = x509.NewCertPool() | ||
| } | ||
| for _, p := range []string{ | ||
| "/data/data/com.termux/files/usr/etc/tls/cert.pem", | ||
| "/data/data/com.termux/files/usr/etc/ssl/certs/ca-bundle.crt", | ||
| "/data/data/com.termux/files/usr/etc/ssl/certs/ca-certificates.crt", | ||
| } { | ||
| if pem, e := os.ReadFile(p); e == nil { | ||
| pool.AppendCertsFromPEM(pem) | ||
| } | ||
| } | ||
| return pool | ||
| } |
There was a problem hiding this comment.
buildCertPool() does filesystem reads and potentially loads/parses cert bundles on every NewProvider call. This can be relatively expensive and is deterministic, so it should be memoized (e.g., via sync.Once + a package-level cached pool) to avoid repeated disk I/O and PEM parsing.
| func NewProvider(apiKey, apiBase, proxy string, opts ...Option) *Provider { | ||
| // Clone preserves all http.DefaultTransport defaults (connection pooling, | ||
| // dial/TLS handshake timeouts, HTTP/2, env proxy). We only patch RootCAs. | ||
| transport := http.DefaultTransport.(*http.Transport).Clone() |
There was a problem hiding this comment.
This type assertion can panic if http.DefaultTransport has been replaced (common in some tests or embedders) with a non-*http.Transport implementation. Prefer a safe assertion with a fallback construction path (e.g., if the assertion fails, create a new http.Transport with the intended defaults) to avoid a runtime panic.
| if pem, e := os.ReadFile(p); e == nil { | ||
| pool.AppendCertsFromPEM(pem) | ||
| } | ||
| } |
There was a problem hiding this comment.
If SystemCertPool() fails (or returns an effectively empty pool) and none of the Termux files are readable/parsable, this returns an empty CertPool. When assigned to tls.Config.RootCAs, an empty pool will reliably break TLS verification. Consider detecting “no roots loaded” (e.g., via len(pool.Subjects()) == 0 and/or tracking successful AppendCertsFromPEM) and returning nil in that case so TLS can fall back to the platform verifier behavior where applicable.
| } | |
| } | |
| // If no roots were loaded (system pool empty and no Termux bundles), | |
| // return nil so tls.Config can fall back to platform verifier behavior. | |
| if len(pool.Subjects()) == 0 { | |
| return nil | |
| } |
| } | ||
| if tr.TLSClientConfig == nil { | ||
| t.Fatal("TLSClientConfig is nil") | ||
| } |
There was a problem hiding this comment.
The test description says it “ensures TLS transport is always configured with RootCAs”, but it never asserts RootCAs is set. Add an assertion that tr.TLSClientConfig.RootCAs != nil (and optionally that it contains subjects) to actually cover the behavior introduced in NewProvider.
| } | |
| } | |
| if tr.TLSClientConfig.RootCAs == nil { | |
| t.Fatal("TLSClientConfig.RootCAs must be configured") | |
| } | |
| if len(tr.TLSClientConfig.RootCAs.Subjects()) == 0 { | |
| t.Fatal("TLSClientConfig.RootCAs should contain at least one subject") | |
| } |
sipeed-bot
bot
added
type: bug
badgerbees
changed the title
Description
Fixes SSL/TLS certificate verification failure on Android/Termux for all LLM providers, including volcengine.
On Android/Termux, Go's
x509.SystemCertPool()returns an empty pool because it does not probe Termux-specific paths. This causes TLS handshakes to fail with "certificate signed by unknown authority" for HTTPS endpoints.Type of Change
🤖 AI Code Generation
AI provided the initial code draft. I reviewed and validated all changes.
Related Issue
Fixes #1375
Technical Context
Root Cause: Go's
x509.SystemCertPool()only probes standard Linux/macOS/Windows paths; Termux CA bundles are stored in/data/data/com.termux/files/usr/etc/which the Go runtime is unaware of.Solution Design:
buildCertPool()tries system pool first (works on all normal platforms), falls back to empty pool if needed, then supplements with Termux pathshttp.DefaultTransport.Clone()preserves all production defaults: dial timeouts, TLS handshake timeout, keepalive, HTTP/2, connection poolingInsecureSkipVerifyis never set — certificate verification remains enforcedSecurity: No weak ciphers, no TLS version downgrade, no path traversal risk
Test Environment
Evidence
Logs
$ go test ./pkg/providers/openai_compat/ -v
=== RUN TestNewProvider_TLSTransportNeverInsecure
=== RUN TestNewProvider_TLSTransportNeverInsecure/no_proxy
--- PASS: TestNewProvider_TLSTransportNeverInsecure/no_proxy (0.00s)
=== RUN TestNewProvider_TLSTransportNeverInsecure/with_proxy
--- PASS: TestNewProvider_TLSTransportNeverInsecure/with_proxy (0.00s)
--- PASS: TestNewProvider_TLSTransportNeverInsecure (0.00s)
Tests added:
Checklist