feat: TLS_INSECURE_SKIP_VERIFY env - #2668
Conversation
WalkthroughThis PR introduces a configurable TLS certificate verification skipping feature via a new TLS_INSECURE_SKIP_VERIFY environment variable. It adds global TLS configuration constants and integrates insecure TLS handling across HTTP clients used in model synchronization, ratio synchronization, and proxy connections. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![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
🤖 Fix all issues with AI agents
In `@service/http_client.go`:
- Around line 43-45: Add a warning log when the TLS verification bypass is
enabled: where TLSInsecureSkipVerify is evaluated during initialization (same
place/session-secret check in common/init.go that emits the SESSION_SECRET
warning), detect if common.TLSInsecureSkipVerify is true and emit a warning via
the same logger used for SESSION_SECRET (e.g., processLogger.Warn / Warnf)
stating that TLS insecure skip verify is enabled and is a security risk; this
mirrors the existing SESSION_SECRET warning pattern so operators are explicitly
notified.
- Around line 147-159: The transport's DialContext wrapper ignores the provided
context; update the code that builds the http.Transport (variable transport) to
detect if the dialer implements proxy.ContextDialer and, if so, assign
transport.DialContext to that dialer’s DialContext method, otherwise fall back
to the existing func(ctx, network, addr) calling dialer.Dial(network, addr).
Ensure you still set transport.TLSClientConfig when common.TLSInsecureSkipVerify
and preserve CheckRedirect on the created http.Client.
🧹 Nitpick comments (3)
common/constants.go (1)
77-78: Consider addingMinVersionfor defense in depth.The static analysis tool flagged that
MinVersionis missing from the TLS configuration. While this config is explicitly for insecure scenarios (skipping certificate verification), adding a minimum TLS version still provides protection against protocol downgrade attacks.Suggested improvement
-var InsecureTLSConfig = &tls.Config{InsecureSkipVerify: true} +var InsecureTLSConfig = &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, +}controller/ratio_sync.go (1)
112-131: Consider reusing HTTP client likemodel_sync.gofor connection pooling.Unlike
controller/model_sync.gowhich uses a lazy singleton HTTP client pattern (sync.Once+getHTTPClient()), this function creates a new transport and client per request. This loses connection pooling benefits whenFetchUpstreamRatiosis called frequently.Consider extracting the client initialization to a package-level singleton similar to
model_sync.gofor consistency and performance.service/http_client.go (1)
108-116: Consider extracting common transport configuration.The transport configuration (MaxIdleConns, MaxIdleConnsPerHost, ForceAttemptHTTP2, TLSClientConfig) is duplicated across
InitHttpClient, the HTTP/HTTPS proxy branch, and the SOCKS5 branch. Consider extracting a helper function to reduce duplication.♻️ Optional: Extract helper for transport configuration
// Helper to apply common transport settings func applyCommonTransportConfig(transport *http.Transport) { transport.MaxIdleConns = common.RelayMaxIdleConns transport.MaxIdleConnsPerHost = common.RelayMaxIdleConnsPerHost transport.ForceAttemptHTTP2 = true if common.TLSInsecureSkipVerify { transport.TLSClientConfig = common.InsecureTLSConfig } }Then use it in each branch:
- transport := &http.Transport{ - MaxIdleConns: common.RelayMaxIdleConns, - MaxIdleConnsPerHost: common.RelayMaxIdleConnsPerHost, - ForceAttemptHTTP2: true, - Proxy: http.ProxyURL(parsedURL), - } - if common.TLSInsecureSkipVerify { - transport.TLSClientConfig = common.InsecureTLSConfig - } + transport := &http.Transport{ + Proxy: http.ProxyURL(parsedURL), + } + applyCommonTransportConfig(transport)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.examplecommon/constants.gocommon/init.gocontroller/model_sync.gocontroller/ratio_sync.goservice/http_client.go
🧰 Additional context used
🧬 Code graph analysis (4)
controller/ratio_sync.go (1)
common/constants.go (2)
TLSInsecureSkipVerify(77-77)InsecureTLSConfig(78-78)
common/init.go (1)
common/constants.go (2)
TLSInsecureSkipVerify(77-77)InsecureTLSConfig(78-78)
service/http_client.go (2)
common/constants.go (4)
TLSInsecureSkipVerify(77-77)InsecureTLSConfig(78-78)RelayMaxIdleConns(128-128)RelayMaxIdleConnsPerHost(129-129)pkg/ionet/types.go (1)
Client(8-12)
controller/model_sync.go (2)
common/constants.go (2)
TLSInsecureSkipVerify(77-77)InsecureTLSConfig(78-78)pkg/ionet/types.go (1)
Client(8-12)
🪛 ast-grep (0.40.5)
common/constants.go
[warning] 77-77: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (5)
.env.example (1)
60-62: LGTM! Documentation is clear.The new TLS configuration section is well-documented. The default value of
falseensures secure behavior out of the box, with the option available for environments that require it (e.g., self-signed certificates in development).common/init.go (1)
86-94: Consider the scope of modifyinghttp.DefaultTransport.Modifying
http.DefaultTransportaffects all HTTP clients in the application that use the default transport, including third-party libraries. While this may be intentional for broad coverage, it could have unintended side effects on dependencies that rely on proper TLS verification.The implementation is correct, but consider documenting this global behavior in the
.env.exampleor README to alert operators.Also, the nil check on line 87 (
tr != nil) is redundant after a successful type assertion—ifokis true,tris guaranteed to be non-nil.Minor cleanup (optional)
if TLSInsecureSkipVerify { - if tr, ok := http.DefaultTransport.(*http.Transport); ok && tr != nil { + if tr, ok := http.DefaultTransport.(*http.Transport); ok { if tr.TLSClientConfig != nil { tr.TLSClientConfig.InsecureSkipVerify = true } else { tr.TLSClientConfig = InsecureTLSConfig } } }controller/model_sync.go (2)
102-104: LGTM! TLS configuration is correctly applied.The insecure TLS config is properly applied to the transport before the client is initialized, ensuring all requests through this client respect the configuration.
121-131: LGTM! Thread-safe lazy initialization.The
sync.Oncepattern ensures the HTTP client is initialized exactly once in a thread-safe manner, with proper connection pooling for reuse across requests.controller/ratio_sync.go (1)
114-116: LGTM! TLS configuration correctly applied.The insecure TLS config is properly applied to the transport when the environment variable is enabled.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if common.TLSInsecureSkipVerify { | ||
| transport.TLSClientConfig = common.InsecureTLSConfig | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for TLS insecure skip verify references and any associated logging
rg -n -C3 'TLSInsecureSkipVerify|TLS_INSECURE_SKIP_VERIFY' --type=goRepository: QuantumNous/new-api
Length of output: 3240
🏁 Script executed:
#!/bin/bash
# View the full context of TLSInsecureSkipVerify initialization in common/init.go
rg -n -B5 -A15 'if TLSInsecureSkipVerify' common/init.goRepository: QuantumNous/new-api
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Check for any logging related to TLS or insecure in common/init.go
# Also check imports to see what logging is available
head -50 common/init.goRepository: QuantumNous/new-api
Length of output: 1245
🏁 Script executed:
#!/bin/bash
# Search for any logging statements in common/init.go
rg -n 'log\.|fmt\.Print|slog\.' common/init.goRepository: QuantumNous/new-api
Length of output: 825
Add a warning log when TLS insecure skip verification is enabled.
The code correctly applies insecure TLS configuration across all transport instances. However, there is currently no warning logged in common/init.go when TLSInsecureSkipVerify is enabled via the environment variable. Since TLS verification bypass is a significant security setting, operators should be explicitly notified. Add a warning log similar to the SESSION_SECRET warning pattern already in the codebase (lines 52-54 of common/init.go).
🤖 Prompt for AI Agents
In `@service/http_client.go` around lines 43 - 45, Add a warning log when the TLS
verification bypass is enabled: where TLSInsecureSkipVerify is evaluated during
initialization (same place/session-secret check in common/init.go that emits the
SESSION_SECRET warning), detect if common.TLSInsecureSkipVerify is true and emit
a warning via the same logger used for SESSION_SECRET (e.g., processLogger.Warn
/ Warnf) stating that TLS insecure skip verify is enabled and is a security
risk; this mirrors the existing SESSION_SECRET warning pattern so operators are
explicitly notified.
| transport := &http.Transport{ | ||
| MaxIdleConns: common.RelayMaxIdleConns, | ||
| MaxIdleConnsPerHost: common.RelayMaxIdleConnsPerHost, | ||
| ForceAttemptHTTP2: true, | ||
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| return dialer.Dial(network, addr) | ||
| }, | ||
| CheckRedirect: checkRedirect, | ||
| } | ||
| if common.TLSInsecureSkipVerify { | ||
| transport.TLSClientConfig = common.InsecureTLSConfig | ||
| } | ||
|
|
||
| client := &http.Client{Transport: transport, CheckRedirect: checkRedirect} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golang.org/x/net/proxy ContextDialer interface
💡 Result:
The ContextDialer interface (package golang.org/x/net/proxy) is:
type ContextDialer interface {
DialContext(ctx context.Context, network, address string) (net.Conn, error)
}
Purpose: a Dialer that supports context-aware dialing (cancellation/deadlines). It complements the older Dialer interface (Dial(network, addr string) (net.Conn, error)); custom Dialers should implement ContextDialer when possible. The package’s Dial function will use DialContext when the configured Dialer implements ContextDialer. Examples of implementers include net.Dialer and the package’s Direct dialer. [1][2]
Sources:
- pkg.go.dev documentation for golang.org/x/net/proxy. [1]
- proxy package source (dial.go / direct.go / socks5.go). [2]
🏁 Script executed:
# Find the file to understand context around the code
fd -n "http_client.go" -t f
# Search for imports related to proxy in the file
rg "import|proxy" service/http_client.go -A 2 -B 2Repository: QuantumNous/new-api
Length of output: 182
🌐 Web query:
golang.org/x/net/proxy SOCKS5 ContextDialer implementation
💡 Result:
Short answer: golang.org/x/net/proxy.SOCKS5 creates and returns an internal socks.Dialer (from golang.org/x/net/internal/socks). If you pass a non-nil forward Dialer and it implements proxy.ContextDialer, SOCKS5 sets the internal Dialer.ProxyDial to call forward.DialContext(ctx,...). If the forward Dialer does not implement ContextDialer, SOCKS5 sets ProxyDial to a wrapper that calls dialContext(ctx, forward, ...). The code is in proxy/socks5.go. [1][2]
Sources:
- proxy/socks5.go source (golang.org/x/net): lines showing ProxyDial assignment and ContextDialer check. [1]
- pkg.go.dev documentation for golang.org/x/net/proxy (SOCKS5, ContextDialer, Dialer types). [2]
🌐 Web query:
golang.org/x/net/proxy.SOCKS5 source code DialContext
💡 Result:
The SOCKS5 function (golang.org/x/net/proxy.SOCKS5) sets the internal socks.Dialer.ProxyDial to call the forward Dialer’s DialContext if the provided forward implements proxy.ContextDialer; otherwise it wraps the non-context Dialer with the package helper dialContext. See the source:
- SOCKS5 selects DialContext from the forward Dialer when available (calls f.DialContext). [1]
- If forward does not implement ContextDialer, SOCKS5 uses dialContext(ctx, forward, ...). [1][2]
- The internal socks.Dialer used by SOCKS5 exposes DialContext. [3]
Sources:
[1] golang.org/x/net/proxy/socks5.go (SOCKS5 implementation).
[2] golang.org/x/net/proxy/dial.go (dialContext / ContextDialer definitions).
[3] golang.org/x/net/internal/socks (Dialer has DialContext).
Use the dialer's ContextDialer interface if available to properly handle context cancellation and deadlines.
The DialContext function currently ignores the context parameter and only calls dialer.Dial(network, addr), bypassing any context support. When using golang.org/x/net/proxy.SOCKS5, the returned dialer implements the proxy.ContextDialer interface and exposes a DialContext method that properly respects context cancellation and timeouts. Check if the dialer implements ContextDialer and use that directly instead of wrapping it with a context-ignoring function.
🔧 Proposed fix
transport := &http.Transport{
MaxIdleConns: common.RelayMaxIdleConns,
MaxIdleConnsPerHost: common.RelayMaxIdleConnsPerHost,
ForceAttemptHTTP2: true,
- DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
- return dialer.Dial(network, addr)
- },
+ }
+ if contextDialer, ok := dialer.(proxy.ContextDialer); ok {
+ transport.DialContext = contextDialer.DialContext
+ } else {
+ transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+ return dialer.Dial(network, addr)
+ }
}
if common.TLSInsecureSkipVerify {
transport.TLSClientConfig = common.InsecureTLSConfig
}🤖 Prompt for AI Agents
In `@service/http_client.go` around lines 147 - 159, The transport's DialContext
wrapper ignores the provided context; update the code that builds the
http.Transport (variable transport) to detect if the dialer implements
proxy.ContextDialer and, if so, assign transport.DialContext to that dialer’s
DialContext method, otherwise fall back to the existing func(ctx, network, addr)
calling dialer.Dial(network, addr). Ensure you still set
transport.TLSClientConfig when common.TLSInsecureSkipVerify and preserve
CheckRedirect on the created http.Client.
…config feat: TLS_INSECURE_SKIP_VERIFY env
2 participants
#1069
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.