From 77a1dc1f2f12198881c00356a04dddef126b985d Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 22 Nov 2017 09:15:03 -0600 Subject: HTTP client refactor (#7884) * http client refactor * simplification --- api/post.go | 3 +- api4/openGraph.go | 3 +- app/app.go | 39 ++++++++++++++++++++++++++ app/command.go | 2 +- app/notification.go | 2 +- app/oauth.go | 4 +-- app/post.go | 6 ++-- app/webhook.go | 2 +- app/webrtc.go | 4 +-- utils/httpclient.go | 73 ++++++------------------------------------------ utils/httpclient_test.go | 10 +++---- 11 files changed, 65 insertions(+), 83 deletions(-) diff --git a/api/post.go b/api/post.go index 46c3b5439..1e09971b6 100644 --- a/api/post.go +++ b/api/post.go @@ -11,7 +11,6 @@ import ( l4g "github.com/alecthomas/log4go" "github.com/gorilla/mux" - "github.com/mattermost/mattermost-server/app" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" ) @@ -554,7 +553,7 @@ func getOpenGraphMetadata(c *Context, w http.ResponseWriter, r *http.Request) { return } - og := app.GetOpenGraphMetadata(url) + og := c.App.GetOpenGraphMetadata(url) ogJSON, err := og.ToJSON() openGraphDataCache.AddWithExpiresInSecs(props["url"], ogJSON, 3600) // Cache would expire after 1 hour diff --git a/api4/openGraph.go b/api4/openGraph.go index 6ddd0d79c..c552b54d9 100644 --- a/api4/openGraph.go +++ b/api4/openGraph.go @@ -7,7 +7,6 @@ import ( "net/http" l4g "github.com/alecthomas/log4go" - "github.com/mattermost/mattermost-server/app" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" ) @@ -43,7 +42,7 @@ func getOpenGraphMetadata(c *Context, w http.ResponseWriter, r *http.Request) { return } - og := app.GetOpenGraphMetadata(url) + og := c.App.GetOpenGraphMetadata(url) ogJSON, err := og.ToJSON() openGraphDataCache.AddWithExpiresInSecs(props["url"], ogJSON, 3600) // Cache would expire after 1 hour diff --git a/app/app.go b/app/app.go index ea79d8e81..7bd4c561b 100644 --- a/app/app.go +++ b/app/app.go @@ -5,8 +5,10 @@ package app import ( "html/template" + "net" "net/http" "runtime/debug" + "strings" "sync/atomic" l4g "github.com/alecthomas/log4go" @@ -351,6 +353,43 @@ func (a *App) HTMLTemplates() *template.Template { return a.htmlTemplateWatcher.Templates() } +func (a *App) HTTPClient(trustURLs bool) *http.Client { + insecure := a.Config().ServiceSettings.EnableInsecureOutgoingConnections != nil && *a.Config().ServiceSettings.EnableInsecureOutgoingConnections + + if trustURLs { + return utils.NewHTTPClient(insecure, nil, nil) + } + + allowHost := func(host string) bool { + if a.Config().ServiceSettings.AllowedUntrustedInternalConnections == nil { + return false + } + for _, allowed := range strings.Fields(*a.Config().ServiceSettings.AllowedUntrustedInternalConnections) { + if host == allowed { + return true + } + } + return false + } + + allowIP := func(ip net.IP) bool { + if !utils.IsReservedIP(ip) { + return true + } + if a.Config().ServiceSettings.AllowedUntrustedInternalConnections == nil { + return false + } + for _, allowed := range strings.Fields(*a.Config().ServiceSettings.AllowedUntrustedInternalConnections) { + if _, ipRange, err := net.ParseCIDR(allowed); err == nil && ipRange.Contains(ip) { + return true + } + } + return false + } + + return utils.NewHTTPClient(insecure, allowHost, allowIP) +} + func (a *App) Handle404(w http.ResponseWriter, r *http.Request) { err := model.NewAppError("Handle404", "api.context.404.app_error", nil, "", http.StatusNotFound) err.Translate(utils.T) diff --git a/app/command.go b/app/command.go index 8bd025c8e..aefdf6a73 100644 --- a/app/command.go +++ b/app/command.go @@ -221,7 +221,7 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, * req.Header.Set("Content-Type", "application/x-www-form-urlencoded") } - if resp, err := utils.HttpClient(false).Do(req); err != nil { + if resp, err := a.HTTPClient(false).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 { diff --git a/app/notification.go b/app/notification.go index 7708270a4..36934dc9d 100644 --- a/app/notification.go +++ b/app/notification.go @@ -695,7 +695,7 @@ func (a *App) sendToPushProxy(msg model.PushNotification, session *model.Session request, _ := http.NewRequest("POST", *a.Config().EmailSettings.PushNotificationServer+model.API_URL_SUFFIX_V1+"/send_push", strings.NewReader(msg.ToJson())) - if resp, err := utils.HttpClient(true).Do(request); err != nil { + if resp, err := a.HTTPClient(true).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) diff --git a/app/oauth.go b/app/oauth.go index 0aba154fa..f27facbec 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -681,7 +681,7 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service var ar *model.AccessResponse var bodyBytes []byte - if resp, err := utils.HttpClient(true).Do(req); err != nil { + if resp, err := a.HTTPClient(true).Do(req); err != nil { return nil, "", stateProps, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.token_failed.app_error", nil, err.Error(), http.StatusInternalServerError) } else { ar = model.AccessResponseFromJson(resp.Body) @@ -708,7 +708,7 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service req.Header.Set("Accept", "application/json") req.Header.Set("Authorization", "Bearer "+ar.AccessToken) - if resp, err := utils.HttpClient(true).Do(req); err != nil { + if resp, err := a.HTTPClient(true).Do(req); err != nil { return nil, "", stateProps, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.service.app_error", map[string]interface{}{"Service": service}, err.Error(), http.StatusInternalServerError) } else { return resp.Body, teamId, stateProps, nil diff --git a/app/post.go b/app/post.go index 1bada0095..00944ee3b 100644 --- a/app/post.go +++ b/app/post.go @@ -681,10 +681,10 @@ func (a *App) GetFileInfosForPost(postId string, readFromMaster bool) ([]*model. return infos, nil } -func GetOpenGraphMetadata(url string) *opengraph.OpenGraph { +func (a *App) GetOpenGraphMetadata(url string) *opengraph.OpenGraph { og := opengraph.NewOpenGraph() - res, err := utils.HttpClient(false).Get(url) + res, err := a.HTTPClient(false).Get(url) if err != nil { l4g.Error("GetOpenGraphMetadata request failed for url=%v with err=%v", url, err.Error()) return og @@ -721,7 +721,7 @@ func (a *App) DoPostAction(postId string, actionId string, userId string) *model req, _ := http.NewRequest("POST", action.Integration.URL, strings.NewReader(request.ToJson())) req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json") - resp, err := utils.HttpClient(false).Do(req) + resp, err := a.HTTPClient(false).Do(req) if err != nil { return model.NewAppError("DoPostAction", "api.post.do_action.action_integration.app_error", nil, "err="+err.Error(), http.StatusBadRequest) } diff --git a/app/webhook.go b/app/webhook.go index 41a789ead..3f8f035f7 100644 --- a/app/webhook.go +++ b/app/webhook.go @@ -106,7 +106,7 @@ func (a *App) TriggerWebhook(payload *model.OutgoingWebhookPayload, hook *model. req, _ := http.NewRequest("POST", url, body) req.Header.Set("Content-Type", contentType) req.Header.Set("Accept", "application/json") - if resp, err := utils.HttpClient(false).Do(req); err != nil { + if resp, err := a.HTTPClient(false).Do(req); err != nil { l4g.Error(utils.T("api.post.handle_webhook_events_and_forget.event_post.error"), err.Error()) } else { defer consumeAndClose(resp) diff --git a/app/webrtc.go b/app/webrtc.go index b8b0a2827..b10450cab 100644 --- a/app/webrtc.go +++ b/app/webrtc.go @@ -59,7 +59,7 @@ func (a *App) GetWebrtcToken(sessionId string) (string, *model.AppError) { rq, _ := http.NewRequest("POST", *a.Config().WebrtcSettings.GatewayAdminUrl, strings.NewReader(model.MapToJson(data))) rq.Header.Set("Content-Type", "application/json") - if rp, err := utils.HttpClient(true).Do(rq); err != nil { + if rp, err := a.HTTPClient(true).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 consumeAndClose(rp) @@ -93,5 +93,5 @@ func (a *App) RevokeWebrtcToken(sessionId string) { rq.Header.Set("Content-Type", "application/json") // we do not care about the response - utils.HttpClient(true).Do(rq) + a.HTTPClient(true).Do(rq) } diff --git a/utils/httpclient.go b/utils/httpclient.go index afa717637..a21be7aa8 100644 --- a/utils/httpclient.go +++ b/utils/httpclient.go @@ -9,7 +9,6 @@ import ( "errors" "net" "net/http" - "strings" "time" ) @@ -18,37 +17,9 @@ const ( requestTimeout = 30 * time.Second ) -var secureHttpClient *http.Client -var secureUntrustedHttpClient *http.Client -var insecureHttpClient *http.Client -var insecureUntrustedHttpClient *http.Client - -// 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(trustURLs bool) *http.Client { - insecure := Cfg.ServiceSettings.EnableInsecureOutgoingConnections != nil && *Cfg.ServiceSettings.EnableInsecureOutgoingConnections - switch { - case insecure && trustURLs: - return insecureHttpClient - case insecure: - return insecureUntrustedHttpClient - case trustURLs: - return secureHttpClient - default: - return secureUntrustedHttpClient - } -} - var reservedIPRanges []*net.IPNet -func isReserved(ip net.IP) bool { +func IsReservedIP(ip net.IP) bool { for _, ipRange := range reservedIPRanges { if ipRange.Contains(ip) { return true @@ -77,39 +48,6 @@ func init() { } reservedIPRanges = append(reservedIPRanges, parsed) } - - allowHost := func(host string) bool { - if Cfg.ServiceSettings.AllowedUntrustedInternalConnections == nil { - return false - } - for _, allowed := range strings.Fields(*Cfg.ServiceSettings.AllowedUntrustedInternalConnections) { - if host == allowed { - return true - } - } - return false - } - - allowIP := func(ip net.IP) bool { - if !isReserved(ip) { - return true - } - if Cfg.ServiceSettings.AllowedUntrustedInternalConnections == nil { - return false - } - for _, allowed := range strings.Fields(*Cfg.ServiceSettings.AllowedUntrustedInternalConnections) { - if _, ipRange, err := net.ParseCIDR(allowed); err == nil && ipRange.Contains(ip) { - return true - } - } - return false - } - - secureHttpClient = createHttpClient(false, nil, nil) - insecureHttpClient = createHttpClient(true, nil, nil) - - secureUntrustedHttpClient = createHttpClient(false, allowHost, allowIP) - insecureUntrustedHttpClient = createHttpClient(true, allowHost, allowIP) } type DialContextFunction func(ctx context.Context, network, addr string) (net.Conn, error) @@ -159,7 +97,14 @@ func dialContextFilter(dial DialContextFunction, allowHost func(host string) boo } } -func createHttpClient(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) *http.Client { +// NewHTTPClient 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") +func NewHTTPClient(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) *http.Client { dialContext := (&net.Dialer{ Timeout: connectTimeout, KeepAlive: 30 * time.Second, diff --git a/utils/httpclient_test.go b/utils/httpclient_test.go index 1878b58b4..e07c54d08 100644 --- a/utils/httpclient_test.go +++ b/utils/httpclient_test.go @@ -14,9 +14,9 @@ import ( "testing" ) -func TestHttpClient(t *testing.T) { +func TestHTTPClient(t *testing.T) { for _, allowInternal := range []bool{true, false} { - c := HttpClient(allowInternal) + c := NewHTTPClient(false, func(_ string) bool { return false }, func(ip net.IP) bool { return allowInternal || !IsReservedIP(ip) }) for _, tc := range []struct { URL string IsInternal bool @@ -52,11 +52,11 @@ func TestHttpClient(t *testing.T) { } } -func TestHttpClientWithProxy(t *testing.T) { +func TestHTTPClientWithProxy(t *testing.T) { proxy := createProxyServer() defer proxy.Close() - c := createHttpClient(true, nil, nil) + c := NewHTTPClient(true, nil, nil) purl, _ := url.Parse(proxy.URL) c.Transport.(*http.Transport).Proxy = http.ProxyURL(purl) @@ -108,7 +108,7 @@ func TestDialContextFilter(t *testing.T) { filter := dialContextFilter(func(ctx context.Context, network, addr string) (net.Conn, error) { didDial = true return nil, nil - }, func(host string) bool { return host == "10.0.0.1" }, func(ip net.IP) bool { return !isReserved(ip) }) + }, func(host string) bool { return host == "10.0.0.1" }, func(ip net.IP) bool { return !IsReservedIP(ip) }) _, err := filter(context.Background(), "", tc.Addr) switch { case tc.IsValid == (err == AddressForbidden) || (err != nil && err != AddressForbidden): -- cgit v1.2.3-1-g7c22