Skip to content

fix(openai_compat): load system CA certs from Termux paths #1397

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.

Sign up for GitHub

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

Closed
badgerbees wants to merge 1 commit into from
Closed

fix(openai_compat): load system CA certs from Termux paths #1397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions pkg/providers/openai_compat/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import (
"bufio"
"bytes"
"context"
"crypto/tls"
"crypto/x509"
"encoding/json"
"fmt"
"io"
"log"
"net/http"
"net/url"
"os"
"strings"
"time"

Expand Down Expand Up @@ -54,22 +57,48 @@ func WithRequestTimeout(timeout time.Duration) Option {
}
}

func NewProvider(apiKey, apiBase, proxy string, opts ...Option) *Provider {
client := &http.Client{
Timeout: defaultRequestTimeout,
// buildCertPool returns the system cert pool supplemented with CA bundles from
// well-known Termux paths. On Android/Termux, Go's x509.SystemCertPool returns
// an empty pool because it does not probe Termux-specific locations, causing
// TLS handshakes to fail with "certificate signed by unknown authority".
// InsecureSkipVerify is never set.
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)
}
}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
}
}
// 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
}

Copilot uses AI. Check for mistakes.
return pool
}
Comment on lines +65 to +80
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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()
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
transport.TLSClientConfig = &tls.Config{RootCAs: buildCertPool()}

if proxy != "" {
parsed, err := url.Parse(proxy)
if err == nil {
client.Transport = &http.Transport{
Proxy: http.ProxyURL(parsed),
}
transport.Proxy = http.ProxyURL(parsed)
} else {
log.Printf("openai_compat: invalid proxy URL %q: %v", proxy, err)
}
}

client := &http.Client{
Timeout: defaultRequestTimeout,
Transport: transport,
}

p := &Provider{
apiKey: apiKey,
apiBase: strings.TrimRight(apiBase, "/"),
Expand Down
28 changes: 28 additions & 0 deletions pkg/providers/openai_compat/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,3 +841,31 @@ func TestSerializeMessages_StripsSystemParts(t *testing.T) {
t.Fatal("system_parts should not appear in serialized output")
}
}

// TestNewProvider_TLSTransportNeverInsecure ensures every provider — whether
// configured with a proxy or not — has an explicit TLS transport that never
// sets InsecureSkipVerify. This is the security guard for issue #1375.
func TestNewProvider_TLSTransportNeverInsecure(t *testing.T) {
tests := []struct {
name string
proxy string
}{
{name: "no proxy", proxy: ""},
{name: "with proxy", proxy: "http://127.0.0.1:8080"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
p := NewProvider("key", "https://example.com", tt.proxy)
tr, ok := p.httpClient.Transport.(*http.Transport)
if !ok || tr == nil {
t.Fatalf("Transport = %T, want *http.Transport", p.httpClient.Transport)
}
if tr.TLSClientConfig == nil {
t.Fatal("TLSClientConfig is nil")
}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
}
}
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")
}

Copilot uses AI. Check for mistakes.
if tr.TLSClientConfig.InsecureSkipVerify {
t.Fatal("InsecureSkipVerify must never be true")
}
})
}
}
Loading