fix: remove automatic InsecureSkipVerify=true for HTTPS connections (CWE-295) - #359
Conversation
…CWE-295) - Remove URL-scheme-based logic that unconditionally disabled TLS certificate verification for every HTTPS URL - Add InsecureSkipVerify bool to AppConfig and ConnectConfig as an explicit opt-in (defaults to false / secure) - Propagate the flag through ConnectConfig in abs.go, async.go, sync.go - Add a second singleton transport for the insecure case so secure and insecure connections are isolated - Fix defer res.Body.Close() inside retry loop (resource leak) - Update HTTPS unit test to explicitly set InsecureSkipVerify: true Agent-Logs-Url: https://github.com/apolloconfig/agollo/sessions/64e40f20-b4d3-4bd9-bca1-229ede403e51 Co-authored-by: zouyx <3828072+zouyx@users.noreply.github.com>
|
感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。 |
Coverage Report for CI Build 29652155219Warning No base build found for commit Coverage: 75.996%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical TLS security issue (CWE-295) by removing the previous behavior that effectively disabled certificate verification for all HTTPS requests, and replacing it with an explicit opt-in flag (InsecureSkipVerify) passed through configuration.
Changes:
- Remove scheme-based
InsecureSkipVerify=truebehavior; instead, readInsecureSkipVerifyfromenv.ConnectConfigand provide a dedicated singleton Transport for insecure mode. - Fix response body handling in the retry loop to avoid
defer res.Body.Close()accumulating across retries. - Add
InsecureSkipVerifytoConnectConfigandAppConfig, thread it through remote/serverlist callers, and update HTTPS tests to opt in explicitly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| protocol/http/request.go | Uses config-driven TLS verification behavior; introduces secure/insecure singleton transports; fixes response body close behavior in retries |
| protocol/http/request_test.go | Updates HTTPS test to explicitly opt into insecure TLS for self-signed test server |
| env/request_config.go | Adds ConnectConfig.InsecureSkipVerify flag (default false) |
| env/config/config.go | Adds AppConfig.InsecureSkipVerify with JSON tag for configuration |
| component/remote/abs.go | Passes AppConfig.InsecureSkipVerify into ConnectConfig |
| component/remote/async.go | Passes AppConfig.InsecureSkipVerify into ConnectConfig |
| component/serverlist/sync.go | Passes AppConfig.InsecureSkipVerify into ConnectConfig |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestHttpsRequestRecovery(t *testing.T) { | ||
| time.Sleep(1 * time.Second) | ||
| server := runNormalBackupConfigResponseWithHTTPS() | ||
| appConfig := getTestAppConfig() | ||
| appConfig.IP = server.URL | ||
| appConfig.InsecureSkipVerify = true | ||
|
|
There was a problem hiding this comment.
This test updates the HTTPS path to explicitly opt into InsecureSkipVerify, but there is no complementary assertion that the default (no opt-in) fails against the self-signed HTTPS server. Adding a negative test case here (same HTTPS server, but without setting InsecureSkipVerify) would lock in the intended security behavior and prevent regressions back to skipping verification by default.
| } else { | ||
| client.Timeout = connectTimeout | ||
| } | ||
| var err error | ||
| u, err := url.Parse(requestURL) | ||
| if err != nil { | ||
| log.Errorf("request Apollo Server url: %q is invalid: %v", requestURL, err) | ||
| return nil, err | ||
| } | ||
| var insecureSkipVerify bool | ||
| if strings.HasPrefix(u.Scheme, "https") { | ||
| insecureSkipVerify = true | ||
| if connectionConfig != nil { | ||
| insecureSkipVerify = connectionConfig.InsecureSkipVerify | ||
| } | ||
| client.Transport = getDefaultTransport(insecureSkipVerify) | ||
| retry := 0 |
There was a problem hiding this comment.
Removing the explicit url.Parse validation means invalid requestURL errors are no longer returned to callers as the original parse error; they now fall through to the generic "generate connect Apollo request fail" path from http.NewRequest. Consider either reintroducing URL validation (returning the parse error) or returning/wrapping the http.NewRequest error so callers can diagnose misconfigured URLs.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@copilot review |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| var err error | ||
| u, err := url.Parse(requestURL) | ||
| if err != nil { | ||
| log.Errorf("request Apollo Server url: %q is invalid: %v", requestURL, err) | ||
| return nil, err | ||
| } | ||
| var insecureSkipVerify bool | ||
| if strings.HasPrefix(u.Scheme, "https") { | ||
| insecureSkipVerify = true |
There was a problem hiding this comment.
这个理解是需要兼容，而不是让用户传入。
这个方法是兼容用户不传入的场景，用于自动识别。
@copilot
There was a problem hiding this comment.
Restored the url.Parse + HTTPS scheme auto-detection in this commit. The code now keeps the original structure for backward compatibility — it auto-identifies HTTPS from the URL scheme — but instead of unconditionally setting InsecureSkipVerify=true, it reads the value from connectionConfig.InsecureSkipVerify (defaults to false, secure):
u, err := url.Parse(requestURL)
if err != nil {
log.Errorf("request Apollo Server url: %q is invalid: %v", requestURL, err)
return nil, err
}
var insecureSkipVerify bool
if connectionConfig != nil && strings.HasPrefix(u.Scheme, "https") {
insecureSkipVerify = connectionConfig.InsecureSkipVerify
}
client.Transport = getDefaultTransport(insecureSkipVerify)This also restores the URL validation that was previously lost.
There was a problem hiding this comment.
已恢复 url.Parse 和 URL scheme 自动识别逻辑，以保持向后兼容性。当 scheme 为 https 时，InsecureSkipVerify 的值从 connectionConfig 读取（默认为 false，即默认开启证书验证），而不是硬编码为 true。同时将 strings.HasPrefix 替换为精确匹配 u.Scheme == "https"，避免误匹配其他 scheme。
Restored url.Parse and URL scheme auto-detection for backward compatibility. When the scheme is https, InsecureSkipVerify is read from connectionConfig (defaults to false / secure) instead of being hardcoded to true. Also changed from strings.HasPrefix to exact equality u.Scheme == "https" for correctness. Commit: fix: use exact scheme equality for HTTPS detection, remove unused strings import
安全漏洞修复 / Security Fix
问题描述 / Problem
在
protocol/http/request.go中存在一个严重的安全漏洞：代码通过判断 URL scheme 是否为https来决定是否跳过 TLS 证书验证，并无条件地将InsecureSkipVerify设为true。这意味着所有 HTTPS 连接都完全禁用了证书验证，使客户端极易遭受中间人攻击（MITM）。In
protocol/http/request.gothere was a critical security vulnerability: whenever the request URL used thehttpsscheme, the code automatically setInsecureSkipVerify: trueon the TLS config. This completely disabled TLS certificate verification for every HTTPS connection, exposing all communication to man-in-the-middle (MITM) attacks (CWE-295 – Improper Certificate Validation).修复内容 / Changes
protocol/http/request.gourl.ParseURL 校验及 HTTPS scheme 自动识别逻辑（向后兼容）；当 scheme 为https时，从ConnectConfig.InsecureSkipVerify读取配置（默认false，即开启证书验证）而非硬编码为true；使用精确匹配u.Scheme == "https"替代strings.HasPrefix；修复 retry 循环内defer res.Body.Close()资源泄漏；为不安全模式添加独立的单例 Transportenv/request_config.goConnectConfig中新增InsecureSkipVerify bool字段（默认false）env/config/config.goAppConfig中新增InsecureSkipVerify bool字段（默认false，JSON taginsecureSkipVerify）component/remote/abs.goConnectConfig时传递AppConfig.InsecureSkipVerifycomponent/remote/async.goConnectConfig时传递AppConfig.InsecureSkipVerifycomponent/serverlist/sync.goConnectConfig时传递AppConfig.InsecureSkipVerifyprotocol/http/request_test.goInsecureSkipVerify: true（自签名测试证书场景）行为变更 / Behavior Change
net/http客户端一致。HTTPS scheme 自动识别保留，无需用户额外配置即可连接有效证书的 HTTPS 服务器。app.properties中设置"insecureSkipVerify": true，或在代码中设置AppConfig.InsecureSkipVerify = true。测试 / Testing
所有现有测试均通过，CodeQL 扫描无告警。