fix: remove automatic InsecureSkipVerify=true for HTTPS connections (CWE-295) (#359)

* fix: remove automatic InsecureSkipVerify=true for HTTPS connections (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>

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* restore url.Parse and HTTPS scheme auto-detection; read InsecureSkipVerify from config

* reorder nil check before scheme check for consistency

* fix: use exact scheme equality for HTTPS detection, remove unused strings import

* test: add TLS verification regression coverage

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: zouyx <3828072+zouyx@users.noreply.github.com>
Co-authored-by: Joe Zou <yixian.zou@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: 3 people

component/remote/abs.go

@@ -36,11 +36,12 @@ func (a *AbsApolloConfig) SyncWithNamespace(namespace string, appConfigFunc func
 	urlSuffix := a.remoteApollo.GetSyncURI(appConfig, namespace)
 
 	c := &env.ConnectConfig{
-		URI:     urlSuffix,
-		AppID:   appConfig.AppID,
-		Secret:  appConfig.Secret,
-		Timeout: notifyConnectTimeout,
-		IsRetry: true,
+		URI:                urlSuffix,
+		AppID:              appConfig.AppID,
+		Secret:             appConfig.Secret,
+		Timeout:            notifyConnectTimeout,
+		IsRetry:            true,
+		InsecureSkipVerify: appConfig.InsecureSkipVerify,
 	}
 	if appConfig.SyncServerTimeout > 0 {
 		c.Timeout = time.Duration(appConfig.SyncServerTimeout) * time.Second

component/remote/async.go

@@ -108,9 +108,10 @@ func (a *asyncApolloConfig) notifyRemoteConfig(appConfigFunc func() config.AppCo
 	urlSuffix := a.GetNotifyURLSuffix(notificationsMap.GetNotifies(namespace), appConfig)
 
 	connectConfig := &env.ConnectConfig{
-		URI:    urlSuffix,
-		AppID:  appConfig.AppID,
-		Secret: appConfig.Secret,
+		URI:                urlSuffix,
+		AppID:              appConfig.AppID,
+		Secret:             appConfig.Secret,
+		InsecureSkipVerify: appConfig.InsecureSkipVerify,
 	}
 	connectConfig.Timeout = notifyConnectTimeout
 	notifies, err := http.RequestRecovery(appConfig, connectConfig, &http.CallBack{

component/serverlist/sync.go

@@ -108,8 +108,9 @@ func SyncServerIPList(appConfigFunc func() config.AppConfig) (map[string]*config
 
 	appConfig := appConfigFunc()
 	c := &env.ConnectConfig{
-		AppID:  appConfig.AppID,
-		Secret: appConfig.Secret,
+		AppID:              appConfig.AppID,
+		Secret:             appConfig.Secret,
+		InsecureSkipVerify: appConfig.InsecureSkipVerify,
 	}
 	if appConfig.SyncServerTimeout > 0 {
 		c.Timeout = time.Duration(appConfig.SyncServerTimeout) * time.Second

env/config/config.go

@@ -48,8 +48,13 @@ type AppConfig struct {
 	Label             string `json:"label"`
 	SyncServerTimeout int    `json:"syncServerTimeout"`
 	// MustStart 可用于控制第一次同步必须成功
-	MustStart               bool `default:"false"`
-	notificationsMap        *notificationsMap
+	MustStart bool `default:"false"`
+	// InsecureSkipVerify controls whether the client skips TLS certificate
+	// verification. This should only be set to true in test environments or
+	// when connecting to a server with a self-signed certificate. Leaving it
+	// false (the default) keeps the connection secure.
+	InsecureSkipVerify bool `default:"false" json:"insecureSkipVerify"`
+	notificationsMap   *notificationsMap
 	currentConnApolloConfig *CurrentApolloConfig
 }
 

env/request_config.go

@@ -30,4 +30,8 @@ type ConnectConfig struct {
 	AppID string
 	//密钥
 	Secret string
+	// InsecureSkipVerify controls whether the HTTP client skips TLS certificate
+	// verification. Defaults to false (secure). Set to true only in test
+	// environments or when connecting to servers with self-signed certificates.
+	InsecureSkipVerify bool
 }

protocol/http/request.go

@@ -22,7 +22,6 @@ import (
 	"net"
 	"net/http"
 	"net/url"
-	"strings"
 	"sync"
 	"time"
 
@@ -53,24 +52,40 @@ var (
 	once sync.Once
 	// defaultTransport http.Transport
 	defaultTransport *http.Transport
+	// insecureOnce for single insecure http.Transport
+	insecureOnce sync.Once
+	// insecureTransport is an http.Transport with TLS verification disabled.
+	// Use only when InsecureSkipVerify is explicitly requested.
+	insecureTransport *http.Transport
 )
 
+func newTransport(insecureSkipVerify bool) *http.Transport {
+	t := &http.Transport{
+		Proxy:               http.ProxyFromEnvironment,
+		MaxIdleConns:        defaultMaxConnsPerHost,
+		MaxIdleConnsPerHost: defaultMaxConnsPerHost,
+		DialContext: (&net.Dialer{
+			KeepAlive: defaultKeepAliveSecond,
+			Timeout:   defaultTimeoutBySecond,
+		}).DialContext,
+	}
+	if insecureSkipVerify {
+		t.TLSClientConfig = &tls.Config{
+			InsecureSkipVerify: true, //nolint:gosec // explicitly opt-in by caller
+		}
+	}
+	return t
+}
+
 func getDefaultTransport(insecureSkipVerify bool) *http.Transport {
+	if insecureSkipVerify {
+		insecureOnce.Do(func() {
+			insecureTransport = newTransport(true)
+		})
+		return insecureTransport
+	}
 	once.Do(func() {
-		defaultTransport = &http.Transport{
-			Proxy:               http.ProxyFromEnvironment,
-			MaxIdleConns:        defaultMaxConnsPerHost,
-			MaxIdleConnsPerHost: defaultMaxConnsPerHost,
-			DialContext: (&net.Dialer{
-				KeepAlive: defaultKeepAliveSecond,
-				Timeout:   defaultTimeoutBySecond,
-			}).DialContext,
-		}
-		if insecureSkipVerify {
-			defaultTransport.TLSClientConfig = &tls.Config{
-				InsecureSkipVerify: insecureSkipVerify,
-			}
-		}
+		defaultTransport = newTransport(false)
 	})
 	return defaultTransport
 }
@@ -92,15 +107,14 @@ func Request(requestURL string, connectionConfig *env.ConnectConfig, callBack *C
 	} 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 && u.Scheme == "https" {
+		insecureSkipVerify = connectionConfig.InsecureSkipVerify
 	}
 	client.Transport = getDefaultTransport(insecureSkipVerify)
 	retry := 0
@@ -138,11 +152,11 @@ func Request(requestURL string, connectionConfig *env.ConnectConfig, callBack *C
 
 		var res *http.Response
 		res, err = client.Do(req)
-		if res != nil {
-			defer res.Body.Close()
-		}
 
 		if res == nil || err != nil {
+			if res != nil {
+				res.Body.Close()
+			}
 			log.Errorf("Connect Apollo Server Fail, url:%s, error:%v", requestURL, err)
 			// if error then sleep
 			time.Sleep(onErrorRetryInterval)
@@ -154,6 +168,7 @@ func Request(requestURL string, connectionConfig *env.ConnectConfig, callBack *C
 		case http.StatusOK:
 			var responseBody []byte
 			responseBody, err = io.ReadAll(res.Body)
+			res.Body.Close()
 			if err != nil {
 				log.Errorf("Connect Apollo Server Fail, url: %s , error: %v", requestURL, err)
 				// if error then sleep
@@ -166,15 +181,18 @@ func Request(requestURL string, connectionConfig *env.ConnectConfig, callBack *C
 			}
 			return nil, nil
 		case http.StatusNotModified:
+			res.Body.Close()
 			log.Debugf("Config Not Modified, error: %v", err)
 			if callBack != nil && callBack.NotModifyCallBack != nil {
 				return nil, callBack.NotModifyCallBack()
 			}
 			return nil, nil
 		case http.StatusBadRequest, http.StatusUnauthorized, http.StatusNotFound, http.StatusMethodNotAllowed:
+			res.Body.Close()
 			log.Errorf("Connect Apollo Server Fail, url:%s, StatusCode:%d", requestURL, res.StatusCode)
 			return nil, errors.New(fmt.Sprintf("Connect Apollo Server Fail, StatusCode:%d", res.StatusCode))
 		default:
+			res.Body.Close()
 			log.Errorf("Connect Apollo Server Fail, url:%s, StatusCode:%d", requestURL, res.StatusCode)
 			// if error then sleep
 			time.Sleep(onErrorRetryInterval)

protocol/http/request_test.go

@@ -18,6 +18,7 @@ import (
 	json2 "encoding/json"
 	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"net/url"
 	"testing"
 	"time"
@@ -62,15 +63,17 @@ func TestHttpsRequestRecovery(t *testing.T) {
 	server := runNormalBackupConfigResponseWithHTTPS()
 	appConfig := getTestAppConfig()
 	appConfig.IP = server.URL
+	appConfig.InsecureSkipVerify = true
 
 	mockIPList(t, func() config.AppConfig {
 		return *appConfig
 	})
 	urlSuffix := getConfigURLSuffix(appConfig, appConfig.NamespaceName)
 
 	o, err := RequestRecovery(*appConfig, &env.ConnectConfig{
-		URI:     urlSuffix,
-		IsRetry: true,
+		URI:                urlSuffix,
+		IsRetry:            true,
+		InsecureSkipVerify: true,
 	}, &CallBack{
 		SuccessCallBack: nil,
 	})
@@ -79,6 +82,22 @@ func TestHttpsRequestRecovery(t *testing.T) {
 	Assert(t, o, NilVal())
 }
 
+func TestTLSVerificationDefaultsToEnabled(t *testing.T) {
+	server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	_, err := Request(server.URL, &env.ConnectConfig{IsRetry: false}, nil)
+	Assert(t, err, NotNilVal())
+
+	_, err = Request(server.URL, &env.ConnectConfig{
+		IsRetry:            false,
+		InsecureSkipVerify: true,
+	}, nil)
+	Assert(t, err, NilVal())
+}
+
 func TestRequestRecovery(t *testing.T) {
 	time.Sleep(1 * time.Second)
 	server := runNormalBackupConfigResponse()