PLT-5705 Created a single source of http.Client creation logic with internet proxy support, reasonable timeouts and optional insecure connections (#6503)

app/command.go

@@ -4,7 +4,6 @@
 package app
 
 import (
-	"crypto/tls"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -204,18 +203,13 @@ func ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, *model.App
 						method = "GET"
 					}
 
-					tr := &http.Transport{
-						TLSClientConfig: &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
-					}
-					client := &http.Client{Transport: tr}
-
 					req, _ := http.NewRequest(method, cmd.URL, strings.NewReader(p.Encode()))
 					req.Header.Set("Accept", "application/json")
 					if cmd.Method == model.COMMAND_METHOD_POST {
 						req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 					}
 
-					if resp, err := client.Do(req); err != nil {
+					if resp, err := utils.HttpClient().Do(req); err != nil {
 						return nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": trigger}, err.Error(), http.StatusInternalServerError)
 					} else {
 						if resp.StatusCode == http.StatusOK {

app/notification.go

@@ -4,7 +4,6 @@
 package app
 
 import (
-	"crypto/tls"
 	"fmt"
 	"html"
 	"html/template"
@@ -557,14 +556,9 @@ func ClearPushNotification(userId string, channelId string) *model.AppError {
 func sendToPushProxy(msg model.PushNotification, session *model.Session) {
 	msg.ServerId = utils.CfgDiagnosticId
 
-	tr := &http.Transport{
-		TLSClientConfig:   &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
-		DisableKeepAlives: true,
-	}
-	httpClient := &http.Client{Transport: tr}
 	request, _ := http.NewRequest("POST", *utils.Cfg.EmailSettings.PushNotificationServer+model.API_URL_SUFFIX_V1+"/send_push", strings.NewReader(msg.ToJson()))
 
-	if resp, err := httpClient.Do(request); err != nil {
+	if resp, err := utils.HttpClient().Do(request); err != nil {
 		l4g.Error("Device push reported as error for UserId=%v SessionId=%v message=%v", session.UserId, session.Id, err.Error())
 	} else {
 		pushResponse := model.PushResponseFromJson(resp.Body)

app/oauth.go

@@ -5,7 +5,6 @@ package app
 
 import (
 	"bytes"
-	"crypto/tls"
 	b64 "encoding/base64"
 	"fmt"
 	"io"
@@ -576,10 +575,6 @@ func AuthorizeOAuthUser(service, code, state, redirectUri string) (io.ReadCloser
 	p.Set("grant_type", model.ACCESS_TOKEN_GRANT_TYPE)
 	p.Set("redirect_uri", redirectUri)
 
-	tr := &http.Transport{
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
-	}
-	client := &http.Client{Transport: tr}
 	req, _ := http.NewRequest("POST", sso.TokenEndpoint, strings.NewReader(p.Encode()))
 
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
@@ -587,14 +582,11 @@ func AuthorizeOAuthUser(service, code, state, redirectUri string) (io.ReadCloser
 
 	var ar *model.AccessResponse
 	var respBody []byte
-	if resp, err := client.Do(req); err != nil {
+	if resp, err := utils.HttpClient().Do(req); err != nil {
 		return nil, "", nil, model.NewLocAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.token_failed.app_error", nil, err.Error())
 	} else {
 		ar = model.AccessResponseFromJson(resp.Body)
-		defer func() {
+		defer CloseBody(resp)
-			ioutil.ReadAll(resp.Body)
-			resp.Body.Close()
-		}()
 		if ar == nil {
 			return nil, "", nil, model.NewLocAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.bad_response.app_error", nil, "")
 		}
@@ -616,7 +608,7 @@ func AuthorizeOAuthUser(service, code, state, redirectUri string) (io.ReadCloser
 	req.Header.Set("Accept", "application/json")
 	req.Header.Set("Authorization", "Bearer "+ar.AccessToken)
 
-	if resp, err := client.Do(req); err != nil {
+	if resp, err := utils.HttpClient().Do(req); err != nil {
 		return nil, "", nil, model.NewLocAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.service.app_error",
 			map[string]interface{}{"Service": service}, err.Error())
 	} else {

app/post.go

@@ -4,12 +4,8 @@
 package app
 
 import (
-	"net"
 	"net/http"
-	"net/url"
-	"os"
 	"regexp"
-	"time"
 
 	l4g "github.com/alecthomas/log4go"
 	"github.com/dyatlov/go-opengraph/opengraph"
@@ -19,35 +15,7 @@ import (
 	"github.com/mattermost/platform/utils"
 )
 
-var (
+var linkWithTextRegex = regexp.MustCompile(`<([^<\|]+)\|([^>]+)>`)
-	httpClient *http.Client
-
-	httpTimeout       = time.Duration(5 * time.Second)
-	linkWithTextRegex = regexp.MustCompile(`<([^<\|]+)\|([^>]+)>`)
-)
-
-func dialTimeout(network, addr string) (net.Conn, error) {
-	return net.DialTimeout(network, addr, httpTimeout)
-}
-
-func init() {
-	p, ok := os.LookupEnv("HTTP_PROXY")
-	if ok {
-		if u, err := url.Parse(p); err == nil {
-			httpClient = &http.Client{
-				Transport: &http.Transport{
-					Proxy: http.ProxyURL(u),
-					Dial:  dialTimeout,
-				},
-			}
-			return
-		}
-	}
-
-	httpClient = &http.Client{
-		Timeout: httpTimeout,
-	}
-}
 
 func CreatePostAsUser(post *model.Post) (*model.Post, *model.AppError) {
 	// Check that channel has not been deleted
@@ -126,7 +94,7 @@ func CreatePost(post *model.Post, teamId string, triggerWebhooks bool) (*model.P
 	}
 
 	esInterface := einterfaces.GetElasticSearchInterface()
-	if (esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableIndexing) {
+	if esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableIndexing {
 		go esInterface.IndexPost(rpost, teamId)
 	}
 
@@ -314,7 +282,7 @@ func UpdatePost(post *model.Post, safeUpdate bool) (*model.Post, *model.AppError
 		rpost := result.Data.(*model.Post)
 
 		esInterface := einterfaces.GetElasticSearchInterface()
-		if (esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableIndexing) {
+		if esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableIndexing {
 			go func() {
 				if rchannel := <-Srv.Store.Channel().GetForPost(rpost.Id); rchannel.Err != nil {
 					l4g.Error("Couldn't get channel %v for post %v for ElasticSearch indexing.", rpost.ChannelId, rpost.Id)
@@ -501,7 +469,7 @@ func DeletePost(postId string) (*model.Post, *model.AppError) {
 		go DeleteFlaggedPosts(post.Id)
 
 		esInterface := einterfaces.GetElasticSearchInterface()
-		if (esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableIndexing) {
+		if esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableIndexing {
 			go esInterface.DeletePost(post.Id)
 		}
 
@@ -532,7 +500,7 @@ func SearchPostsInTeam(terms string, userId string, teamId string, isOrSearch bo
 	paramsList := model.ParseSearchParams(terms)
 
 	esInterface := einterfaces.GetElasticSearchInterface()
-	if (esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableSearching && utils.IsLicensed && *utils.License.Features.ElasticSearch) {
+	if esInterface != nil && *utils.Cfg.ElasticSearchSettings.EnableSearching && utils.IsLicensed && *utils.License.Features.ElasticSearch {
 		finalParamsList := []*model.SearchParams{}
 
 		for _, params := range paramsList {
@@ -643,7 +611,7 @@ func GetFileInfosForPost(postId string, readFromMaster bool) ([]*model.FileInfo,
 func GetOpenGraphMetadata(url string) *opengraph.OpenGraph {
 	og := opengraph.NewOpenGraph()
 
-	res, err := httpClient.Get(url)
+	res, err := utils.HttpClient().Get(url)
 	if err != nil {
 		l4g.Error("GetOpenGraphMetadata request failed for url=%v with err=%v", url, err.Error())
 		return og

app/webhook.go

@@ -4,9 +4,7 @@
 package app
 
 import (
-	"crypto/tls"
 	"io"
-	"io/ioutil"
 	"net/http"
 	"regexp"
 	"strings"
@@ -87,23 +85,16 @@ func handleWebhookEvents(post *model.Post, team *model.Team, channel *model.Chan
 				body = strings.NewReader(payload.ToFormValues())
 				contentType = "application/x-www-form-urlencoded"
 			}
-			tr := &http.Transport{
-				TLSClientConfig: &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
-			}
-			client := &http.Client{Transport: tr}
 
 			for _, url := range hook.CallbackURLs {
 				go func(url string) {
 					req, _ := http.NewRequest("POST", url, body)
 					req.Header.Set("Content-Type", contentType)
 					req.Header.Set("Accept", "application/json")
-					if resp, err := client.Do(req); err != nil {
+					if resp, err := utils.HttpClient().Do(req); err != nil {
 						l4g.Error(utils.T("api.post.handle_webhook_events_and_forget.event_post.error"), err.Error())
 					} else {
-						defer func() {
+						defer CloseBody(resp)
-							ioutil.ReadAll(resp.Body)
-							resp.Body.Close()
-						}()
 						respProps := model.MapFromJson(resp.Body)
 
 						if text, ok := respProps["text"]; ok {

app/webrtc.go

@@ -6,7 +6,6 @@ package app
 import (
 	"crypto/hmac"
 	"crypto/sha1"
-	"crypto/tls"
 	"encoding/base64"
 	"net/http"
 	"strconv"
@@ -60,11 +59,7 @@ func GetWebrtcToken(sessionId string) (string, *model.AppError) {
 	rq, _ := http.NewRequest("POST", *utils.Cfg.WebrtcSettings.GatewayAdminUrl, strings.NewReader(model.MapToJson(data)))
 	rq.Header.Set("Content-Type", "application/json")
 
-	tr := &http.Transport{
+	if rp, err := utils.HttpClient().Do(rq); err != nil {
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
-	}
-	httpClient := &http.Client{Transport: tr}
-	if rp, err := httpClient.Do(rq); err != nil {
 		return "", model.NewAppError("WebRTC.Token", "model.client.connecting.app_error", nil, err.Error(), http.StatusInternalServerError)
 	} else if rp.StatusCode >= 300 {
 		defer CloseBody(rp)

app/webtrc.go

@@ -4,7 +4,6 @@
 package app
 
 import (
-	"crypto/tls"
 	"encoding/base64"
 	"net/http"
 	"strings"
@@ -25,9 +24,5 @@ func RevokeWebrtcToken(sessionId string) {
 	rq.Header.Set("Content-Type", "application/json")
 
 	// we do not care about the response
-	tr := &http.Transport{
+	utils.HttpClient().Do(rq)
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: *utils.Cfg.ServiceSettings.EnableInsecureOutgoingConnections},
-	}
-	httpClient := &http.Client{Transport: tr}
-	httpClient.Do(rq)
 }

utils/httpclient.go

@@ -0,0 +1,60 @@
+// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package utils
+
+import (
+	"crypto/tls"
+	"net"
+	"net/http"
+	"time"
+)
+
+const (
+	connectTimeout = 3 * time.Second
+	requestTimeout = 5 * time.Second
+)
+
+// HttpClient returns a variation the default implementation of Client.
+// It uses a Transport with the same settings as the default Transport
+// but with the following modifications:
+// - shorter timeout for dial and TLS handshake (defined as constant
+//   "connectTimeout")
+// - timeout for the end-to-end request (defined as constant
+//   "requestTimeout")
+// - skipping server certificate check if specified in "config.json"
+//   via "ServiceSettings.EnableInsecureOutgoingConnections"
+func HttpClient() *http.Client {
+	if Cfg.ServiceSettings.EnableInsecureOutgoingConnections != nil && *Cfg.ServiceSettings.EnableInsecureOutgoingConnections {
+		return insecureHttpClient
+	}
+	return secureHttpClient
+}
+
+var (
+	secureHttpClient   = createHttpClient(false)
+	insecureHttpClient = createHttpClient(true)
+)
+
+func createHttpClient(enableInsecureConnections bool) *http.Client {
+	client := &http.Client{
+		Transport: &http.Transport{
+			Proxy: http.ProxyFromEnvironment,
+			DialContext: (&net.Dialer{
+				Timeout:   connectTimeout,
+				KeepAlive: 30 * time.Second,
+				DualStack: true,
+			}).DialContext,
+			MaxIdleConns:          100,
+			IdleConnTimeout:       90 * time.Second,
+			TLSHandshakeTimeout:   connectTimeout,
+			ExpectContinueTimeout: 1 * time.Second,
+			TLSClientConfig: &tls.Config{
+				InsecureSkipVerify: enableInsecureConnections,
+			},
+		},
+		Timeout: requestTimeout,
+	}
+
+	return client
+}

utils/httpclient_test.go

@@ -0,0 +1,42 @@
+// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package utils
+
+import (
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+)
+
+func TestHttpClientWithProxy(t *testing.T) {
+	proxy := createProxyServer()
+	defer proxy.Close()
+	os.Setenv("HTTP_PROXY", proxy.URL)
+
+	client := HttpClient()
+	resp, err := client.Get("http://acme.com")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer resp.Body.Close()
+
+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(body) != "proxy" {
+		t.FailNow()
+	}
+}
+
+func createProxyServer() *httptest.Server {
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		w.Header().Set("Content-Type", "text/plain; charset=us-ascii")
+		fmt.Fprint(w, "proxy")
+	}))
+}