diff --git a/.golangci.yml b/.golangci.yml index ffee7806ae..bfad5df0cb 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -3,6 +3,7 @@ linters: enable: - deadcode - depguard + - gocritic - gofmt - govet - ineffassign @@ -22,6 +23,13 @@ linters-settings: - github.com/rotisserie/eris: "use ctxerr.New or ctxerr.Wrap[f] instead" - github.com/pkg/errors: "use ctxerr if a context.Context is available or stdlib errors.New / fmt.Errorf with the %w verb" + gocritic: + enabled-checks: + - ruleguard + settings: + ruleguard: + rules: "tools/ci/rules.go" + gofmt: # simplify code: gofmt with `-s` option, true by default simplify: false diff --git a/changes/issue-2613-uniform-http-client b/changes/issue-2613-uniform-http-client new file mode 100644 index 0000000000..718d9ef95b --- /dev/null +++ b/changes/issue-2613-uniform-http-client @@ -0,0 +1 @@ +* Ensure uniformity of http clients across the codebase, so that all use sane defaults and are proxy-aware. diff --git a/cmd/fleetctl/api.go b/cmd/fleetctl/api.go index 07d8d186c7..882fb95887 100644 --- a/cmd/fleetctl/api.go +++ b/cmd/fleetctl/api.go @@ -13,6 +13,7 @@ import ( "os" "runtime" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/service" "github.com/urfave/cli/v2" ) @@ -139,14 +140,10 @@ func rawHTTPClientFromConfig(cc Context) (*http.Client, *url.URL, error) { } } - cli := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: cc.TLSSkipVerify, - RootCAs: rootCA, - }, - }, - } + cli := fleethttp.NewClient(fleethttp.WithTLSClientConfig(&tls.Config{ + InsecureSkipVerify: cc.TLSSkipVerify, + RootCAs: rootCA, + })) return cli, baseURL, nil } diff --git a/cmd/fleetctl/preview.go b/cmd/fleetctl/preview.go index 81b6683b72..1bd76d88b7 100644 --- a/cmd/fleetctl/preview.go +++ b/cmd/fleetctl/preview.go @@ -21,6 +21,7 @@ import ( "github.com/cenkalti/backoff/v4" "github.com/fleetdm/fleet/v4/orbit/pkg/packaging" "github.com/fleetdm/fleet/v4/orbit/pkg/update" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/service" "github.com/mitchellh/go-ps" @@ -396,11 +397,7 @@ func waitStartup() error { retryStrategy := backoff.NewExponentialBackOff() retryStrategy.MaxInterval = 1 * time.Second - client := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - }, - } + client := fleethttp.NewClient(fleethttp.WithTLSClientConfig(&tls.Config{InsecureSkipVerify: true})) if err := backoff.Retry( func() error { diff --git a/cmd/fleetctl/query.go b/cmd/fleetctl/query.go index ab82dd45f3..cb539cf054 100644 --- a/cmd/fleetctl/query.go +++ b/cmd/fleetctl/query.go @@ -90,7 +90,7 @@ func queryCommand() *cli.Command { } if flQuery != "" && flQueryName != "" { - return fmt.Errorf("--query and --query-name must not be provided together") + return errors.New("--query and --query-name must not be provided together") } if flQueryName != "" { @@ -102,7 +102,7 @@ func queryCommand() *cli.Command { } if flQuery == "" { - return fmt.Errorf("Query must be specified with --query or --query-name") + return errors.New("Query must be specified with --query or --query-name") } var output outputWriter diff --git a/cmd/fleetctl/user.go b/cmd/fleetctl/user.go index 3aa407ddfd..763c9dff7c 100644 --- a/cmd/fleetctl/user.go +++ b/cmd/fleetctl/user.go @@ -123,7 +123,7 @@ func createUserCommand() *cli.Command { } if sso && len(password) > 0 { - return fmt.Errorf("Password may not be provided for SSO users.") + return errors.New("Password may not be provided for SSO users.") } if !sso && len(password) == 0 { fmt.Print("Enter password for user: ") @@ -133,7 +133,7 @@ func createUserCommand() *cli.Command { return fmt.Errorf("Failed to read password: %w", err) } if len(passBytes) == 0 { - return fmt.Errorf("Password may not be empty.") + return errors.New("Password may not be empty.") } fmt.Print("Enter password for user (confirm): ") @@ -144,7 +144,7 @@ func createUserCommand() *cli.Command { } if !bytes.Equal(passBytes, confBytes) { - return fmt.Errorf("Confirmation does not match") + return errors.New("Confirmation does not match") } password = string(passBytes) diff --git a/cmd/fleetctl/vulnerability_data_stream.go b/cmd/fleetctl/vulnerability_data_stream.go index 99503b2fcd..b350855712 100644 --- a/cmd/fleetctl/vulnerability_data_stream.go +++ b/cmd/fleetctl/vulnerability_data_stream.go @@ -2,10 +2,10 @@ package main import ( "errors" - "net/http" "os" "path" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/config" "github.com/fleetdm/fleet/v4/server/vulnerabilities" "github.com/urfave/cli/v2" @@ -46,7 +46,7 @@ Downloads (if needed) the data streams that can be used by the Fleet server to p log(c, "[-] Downloading CPE database...") dbPath := path.Join(dir, "cpe.sqlite") - client := &http.Client{} + client := fleethttp.NewClient() err = vulnerabilities.SyncCPEDatabase(client, dbPath, config.FleetConfig{}) if err != nil { return err diff --git a/cmd/osquery-perf/agent.go b/cmd/osquery-perf/agent.go index fb41e54d16..966251a7c8 100644 --- a/cmd/osquery-perf/agent.go +++ b/cmd/osquery-perf/agent.go @@ -5,6 +5,7 @@ import ( "crypto/tls" "embed" "encoding/json" + "errors" "flag" "fmt" "log" @@ -217,7 +218,7 @@ func (a *agent) enroll(i int, onlyAlreadyEnrolled bool) error { } if onlyAlreadyEnrolled { - return fmt.Errorf("not enrolled") + return errors.New("not enrolled") } var body bytes.Buffer diff --git a/go.mod b/go.mod index 6e7f10cce5..5a2beebc4c 100644 --- a/go.mod +++ b/go.mod @@ -60,6 +60,7 @@ require ( github.com/prometheus/client_model v0.2.0 // indirect github.com/prometheus/common v0.4.1 // indirect github.com/prometheus/procfs v0.2.0 // indirect + github.com/quasilyte/go-ruleguard/dsl v0.3.10 // indirect github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0 // indirect github.com/rotisserie/eris v0.5.1 github.com/rs/zerolog v1.20.0 diff --git a/go.sum b/go.sum index 9975e7dc32..a0e207aaff 100644 --- a/go.sum +++ b/go.sum @@ -714,6 +714,8 @@ github.com/quasilyte/go-consistent v0.0.0-20190521200055-c6f3937de18c/go.mod h1: github.com/quasilyte/go-ruleguard v0.2.0/go.mod h1:2RT/tf0Ce0UDj5y243iWKosQogJd8+1G3Rs2fxmlYnw= github.com/quasilyte/go-ruleguard v0.2.1 h1:56eRm0daAyny9UhJnmtJW/UyLZQusukBAB8oT8AHKHo= github.com/quasilyte/go-ruleguard v0.2.1/go.mod h1:hN2rVc/uS4bQhQKTio2XaSJSafJwqBUWWwtssT3cQmc= +github.com/quasilyte/go-ruleguard/dsl v0.3.10 h1:4tVlVVcBT+nNWoF+t/zrAMO13sHAqYotX1K12Gc8f8A= +github.com/quasilyte/go-ruleguard/dsl v0.3.10/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU= github.com/quasilyte/regex/syntax v0.0.0-20200407221936-30656e2c4a95/go.mod h1:rlzQ04UMyJXu/aOvhd8qT+hvDrFpiwqp8MRXDY9szc0= github.com/quasilyte/regex/syntax v0.0.0-20200805063351-8f842688393c h1:+gtJ/Pwj2dgUGlZgTrNFqajGYKZQc7Piqus/S6DK9CE= github.com/quasilyte/regex/syntax v0.0.0-20200805063351-8f842688393c/go.mod h1:rlzQ04UMyJXu/aOvhd8qT+hvDrFpiwqp8MRXDY9szc0= diff --git a/orbit/pkg/insecure/proxy.go b/orbit/pkg/insecure/proxy.go index 24fea500b4..23373af11d 100644 --- a/orbit/pkg/insecure/proxy.go +++ b/orbit/pkg/insecure/proxy.go @@ -16,6 +16,8 @@ import ( "net/url" "strings" "time" + + "github.com/fleetdm/fleet/v4/pkg/fleethttp" ) const ( @@ -150,22 +152,9 @@ func newProxyHandler(targetURL string) (*httputil.ReverseProxy, error) { }, } // Adapted from http.DefaultTransport - reverseProxy.Transport = &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, - DualStack: true, - }).DialContext, - ForceAttemptHTTP2: true, - MaxIdleConns: 100, - IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 1 * time.Second, - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, - }, - } + reverseProxy.Transport = fleethttp.NewTransport(fleethttp.WithTLSConfig( + &tls.Config{InsecureSkipVerify: true}, + )) return reverseProxy, nil } diff --git a/orbit/pkg/process/process.go b/orbit/pkg/process/process.go index 0cd1e33785..02e24f688c 100644 --- a/orbit/pkg/process/process.go +++ b/orbit/pkg/process/process.go @@ -2,7 +2,7 @@ package process import ( "context" - "fmt" + "errors" "os" "os/exec" "runtime" @@ -50,7 +50,7 @@ func newWithMock(cmd ExecCmd) *Process { // https://github.com/golang/go/blob/8981092d71aee273d27b0e11cf932a34d4d365c1/src/cmd/go/script_test.go#L1131-L1190 func (p *Process) WaitOrKill(ctx context.Context, killDelay time.Duration) error { if p.OsProcess() == nil { - return fmt.Errorf("WaitOrKill requires a non-nil OsProcess - missing Start call?") + return errors.New("WaitOrKill requires a non-nil OsProcess - missing Start call?") } errc := make(chan error) diff --git a/orbit/pkg/process/process_test.go b/orbit/pkg/process/process_test.go index 6028445022..8378391dd5 100644 --- a/orbit/pkg/process/process_test.go +++ b/orbit/pkg/process/process_test.go @@ -2,7 +2,7 @@ package process import ( "context" - "fmt" + "errors" "testing" "time" @@ -42,7 +42,7 @@ func TestWaitOrKillProcessCompletedError(t *testing.T) { mockProcess := &mockOsProcess{} defer mock.AssertExpectationsForObjects(t, mockCmd, mockProcess) mockCmd.On("OsProcess").Return(mockProcess) - mockCmd.On("Wait").After(10 * time.Millisecond).Return(fmt.Errorf("super bad")) + mockCmd.On("Wait").After(10 * time.Millisecond).Return(errors.New("super bad")) p := newWithMock(mockCmd) err := p.WaitOrKill(context.Background(), 10*time.Millisecond) @@ -74,7 +74,7 @@ func TestWaitOrKillWaitSignalCompleted(t *testing.T) { defer mock.AssertExpectationsForObjects(t, mockCmd, mockProcess) mockCmd.On("OsProcess").Return(mockProcess) mockCmd.On("Wait").After(10 * time.Millisecond).Return(nil) - mockProcess.On("Signal", stopSignal()).Return(fmt.Errorf("os: process already finished")) + mockProcess.On("Signal", stopSignal()).Return(errors.New("os: process already finished")) p := newWithMock(mockCmd) ctx, cancel := context.WithCancel(context.Background()) @@ -89,7 +89,7 @@ func TestWaitOrKillWaitKilled(t *testing.T) { mockProcess := &mockOsProcess{} defer mock.AssertExpectationsForObjects(t, mockCmd, mockProcess) mockCmd.On("OsProcess").Return(mockProcess) - mockCmd.On("Wait").After(10 * time.Millisecond).Return(fmt.Errorf("killed")) + mockCmd.On("Wait").After(10 * time.Millisecond).Return(errors.New("killed")) mockProcess.On("Signal", stopSignal()).Return(nil) mockProcess.On("Kill").Return(nil) diff --git a/orbit/pkg/update/update.go b/orbit/pkg/update/update.go index 1e635a9861..dbb1de08aa 100644 --- a/orbit/pkg/update/update.go +++ b/orbit/pkg/update/update.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "net/http" "os" "os/exec" "path" @@ -14,6 +13,7 @@ import ( "github.com/fleetdm/fleet/v4/orbit/pkg/constant" "github.com/fleetdm/fleet/v4/orbit/pkg/platform" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/pkg/secure" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/rs/zerolog/log" @@ -69,11 +69,9 @@ func New(opt Options) (*Updater, error) { opt.Platform = constant.PlatformName } - transport := http.DefaultTransport.(*http.Transport).Clone() - transport.TLSClientConfig = &tls.Config{ + httpClient := fleethttp.NewClient(fleethttp.WithTLSClientConfig(&tls.Config{ InsecureSkipVerify: opt.InsecureTransport, - } - httpClient := &http.Client{Transport: transport} + })) remoteStore, err := client.HTTPRemoteStore(opt.ServerURL, nil, httpClient) if err != nil { diff --git a/pkg/fleethttp/fleethttp.go b/pkg/fleethttp/fleethttp.go new file mode 100644 index 0000000000..62458e1fde --- /dev/null +++ b/pkg/fleethttp/fleethttp.go @@ -0,0 +1,98 @@ +// Package fleethttp provides uniform creation and configuration of HTTP +// related types. +package fleethttp + +import ( + "crypto/tls" + "net/http" + "time" +) + +type clientOpts struct { + timeout time.Duration + tlsConf *tls.Config + noFollow bool +} + +// ClientOpt is the type for the client-specific options. +type ClientOpt func(o *clientOpts) + +// WithTimeout sets the timeout to use for the HTTP client. +func WithTimeout(t time.Duration) ClientOpt { + return func(o *clientOpts) { + o.timeout = t + } +} + +// WithTLSClientConfig provides the TLS configuration to use for the HTTP +// client's transport. +func WithTLSClientConfig(conf *tls.Config) ClientOpt { + return func(o *clientOpts) { + o.tlsConf = conf.Clone() + } +} + +// WithFollowRedir configures the HTTP client to follow redirections or not, +// based on the follow value. +func WithFollowRedir(follow bool) ClientOpt { + return func(o *clientOpts) { + o.noFollow = !follow + } +} + +// NewClient returns an HTTP client configured according to the provided +// options. +func NewClient(opts ...ClientOpt) *http.Client { + var co clientOpts + for _, opt := range opts { + opt(&co) + } + + //nolint:gocritic + cli := &http.Client{ + Timeout: co.timeout, + } + if co.noFollow { + cli.CheckRedirect = noFollowRedirect + } + if co.tlsConf != nil { + cli.Transport = NewTransport(WithTLSConfig(co.tlsConf)) + } + return cli +} + +type transportOpts struct { + tlsConf *tls.Config +} + +// TransportOpt is the type for transport-specific options. +type TransportOpt func(o *transportOpts) + +// WithTLSConfig sets the TLS configuration of the transport. +func WithTLSConfig(conf *tls.Config) TransportOpt { + return func(o *transportOpts) { + o.tlsConf = conf.Clone() + } +} + +// NewTransport creates an http transport (a type that implements +// http.RoundTripper) with the provided optional options. The transport is +// derived from Go's http.DefaultTransport and only overrides the specific +// parts it needs to, so that it keeps its sane defaults for the rest. +func NewTransport(opts ...TransportOpt) *http.Transport { + var to transportOpts + for _, opt := range opts { + opt(&to) + } + + // make sure to start from DefaultTransport to inherit its sane defaults + tr := http.DefaultTransport.(*http.Transport).Clone() + if to.tlsConf != nil { + tr.TLSClientConfig = to.tlsConf + } + return tr +} + +func noFollowRedirect(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse +} diff --git a/pkg/fleethttp/fleethttp_test.go b/pkg/fleethttp/fleethttp_test.go new file mode 100644 index 0000000000..1e28d4ab8d --- /dev/null +++ b/pkg/fleethttp/fleethttp_test.go @@ -0,0 +1,71 @@ +package fleethttp + +import ( + "crypto/tls" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestClient(t *testing.T) { + cases := []struct { + name string + opts []ClientOpt + nilTransport bool + nilRedirect bool + timeout time.Duration + }{ + {"default", nil, true, true, 0}, + {"timeout", []ClientOpt{WithTimeout(time.Second)}, true, true, time.Second}, + {"nofollow", []ClientOpt{WithFollowRedir(false)}, true, false, 0}, + {"tlsconfig", []ClientOpt{WithTLSClientConfig(&tls.Config{})}, false, true, 0}, + {"combined", []ClientOpt{ + WithTLSClientConfig(&tls.Config{}), + WithTimeout(time.Second), + WithFollowRedir(false), + }, false, false, time.Second}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cli := NewClient(c.opts...) + if c.nilTransport { + assert.Nil(t, cli.Transport) + } else { + assert.NotNil(t, cli.Transport) + } + if c.nilRedirect { + assert.Nil(t, cli.CheckRedirect) + } else { + assert.NotNil(t, cli.CheckRedirect) + } + assert.Equal(t, c.timeout, cli.Timeout) + }) + } +} + +func TestTransport(t *testing.T) { + defaultTLSConf := http.DefaultTransport.(*http.Transport).TLSClientConfig + + cases := []struct { + name string + opts []TransportOpt + defaultTLS bool + }{ + {"default", nil, true}, + {"tlsconf", []TransportOpt{WithTLSConfig(&tls.Config{})}, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + tr := NewTransport(c.opts...) + if c.defaultTLS { + assert.Equal(t, defaultTLSConf, tr.TLSClientConfig) + } else { + assert.NotEqual(t, defaultTLSConf, tr.TLSClientConfig) + } + assert.NotNil(t, tr.Proxy) + assert.NotNil(t, tr.DialContext) + }) + } +} diff --git a/server/fleet/queries.go b/server/fleet/queries.go index 6d57c0bf62..807a9e1fd1 100644 --- a/server/fleet/queries.go +++ b/server/fleet/queries.go @@ -1,6 +1,7 @@ package fleet import ( + "errors" "fmt" "regexp" "strings" @@ -51,7 +52,7 @@ var ( // actually determine whether the query is well formed. func (q Query) ValidateSQL() error { if validateSQLRegexp.MatchString(q.Query) { - return fmt.Errorf("ATTACH not allowed in queries") + return errors.New("ATTACH not allowed in queries") } return nil } diff --git a/server/logging/kafkarest.go b/server/logging/kafkarest.go index 151865bd81..36cfffd6f3 100644 --- a/server/logging/kafkarest.go +++ b/server/logging/kafkarest.go @@ -9,6 +9,7 @@ import ( "net/http" "time" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" ) @@ -44,9 +45,7 @@ func NewKafkaRESTWriter(p *KafkaRESTParams) (*kafkaRESTProducer, error) { producer := &kafkaRESTProducer{ URL: fmt.Sprintf(krPublishTopicURL, p.KafkaProxyHost, p.KafkaTopic), CheckURL: fmt.Sprintf(krCheckTopicURL, p.KafkaProxyHost, p.KafkaTopic), - client: &http.Client{ - Timeout: time.Duration(p.KafkaTimeout) * time.Second, - }, + client: fleethttp.NewClient(fleethttp.WithTimeout(time.Duration(p.KafkaTimeout) * time.Second)), } return producer, producer.checkTopic() diff --git a/server/mail/mail.go b/server/mail/mail.go index 3f833f4772..06e9ae9595 100644 --- a/server/mail/mail.go +++ b/server/mail/mail.go @@ -65,7 +65,7 @@ func getMessageBody(e fleet.Email) ([]byte, error) { func (m mailService) SendEmail(e fleet.Email) error { if !e.Config.SMTPSettings.SMTPConfigured { - return fmt.Errorf("email not configured") + return errors.New("email not configured") } msg, err := getMessageBody(e) if err != nil { diff --git a/server/service/client.go b/server/service/client.go index 15994db6a7..2a8988e203 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -16,6 +16,7 @@ import ( "strings" "time" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/fleet" ) @@ -70,14 +71,10 @@ func NewClient(addr string, insecureSkipVerify bool, rootCA, urlPrefix string, o } } - httpClient := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: insecureSkipVerify, - RootCAs: rootCAPool, - }, - }, - } + httpClient := fleethttp.NewClient(fleethttp.WithTLSClientConfig(&tls.Config{ + InsecureSkipVerify: insecureSkipVerify, + RootCAs: rootCAPool, + })) client := &Client{ addr: addr, diff --git a/server/service/client_live_query_test.go b/server/service/client_live_query_test.go index dfe9205635..e48448d2c9 100644 --- a/server/service/client_live_query_test.go +++ b/server/service/client_live_query_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/gorilla/websocket" "github.com/stretchr/testify/assert" @@ -90,7 +91,7 @@ func TestLiveQueryWithContext(t *testing.T) { baseURL: baseURL, urlPrefix: "", token: "1234", - http: &http.Client{}, + http: fleethttp.NewClient(), insecureSkipVerify: false, writer: nil, } diff --git a/server/service/http_auth_test.go b/server/service/http_auth_test.go index eea04c6164..6dc4276f88 100644 --- a/server/service/http_auth_test.go +++ b/server/service/http_auth_test.go @@ -12,6 +12,7 @@ import ( "strconv" "testing" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mock" @@ -91,7 +92,7 @@ func TestLogin(t *testing.T) { // test logout req, _ := http.NewRequest("POST", server.URL+"/api/v1/fleet/logout", nil) req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", jsn.Token)) - client := &http.Client{} + client := fleethttp.NewClient() resp, err = client.Do(req) require.Nil(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode, strconv.Itoa(tt.status)) @@ -192,7 +193,7 @@ func TestNoHeaderErrorsDifferently(t *testing.T) { _, _, server := setupAuthTest(t) req, _ := http.NewRequest("GET", server.URL+"/api/v1/fleet/users", nil) - client := &http.Client{} + client := fleethttp.NewClient() resp, err := client.Do(req) require.Nil(t, err) defer resp.Body.Close() diff --git a/server/service/integration_logger_test.go b/server/service/integration_logger_test.go index ac94306b15..16ffac4058 100644 --- a/server/service/integration_logger_test.go +++ b/server/service/integration_logger_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/go-kit/kit/log" @@ -115,7 +116,7 @@ func (s *integrationLoggerTestSuite) TestOsqueryEndpointsLogErrors() { requestBody := io.NopCloser(bytes.NewBuffer([]byte(`{"node_key":"1234","log_type":"status","data":[}`))) req, _ := http.NewRequest("POST", s.server.URL+"/api/v1/osquery/log", requestBody) - client := &http.Client{} + client := fleethttp.NewClient() _, err = client.Do(req) require.Nil(t, err) diff --git a/server/service/service_carves.go b/server/service/service_carves.go index 522f01d978..52ad1e245d 100644 --- a/server/service/service_carves.go +++ b/server/service/service_carves.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "time" @@ -81,7 +82,7 @@ func (svc *Service) CarveBlock(ctx context.Context, payload fleet.CarveBlockPayl } if payload.RequestId != carve.RequestId { - return fmt.Errorf("request_id does not match") + return errors.New("request_id does not match") } // Request is now authenticated @@ -132,7 +133,7 @@ func (svc *Service) GetBlock(ctx context.Context, carveId, blockId int64) ([]byt } if metadata.Expired { - return nil, fmt.Errorf("cannot get block for expired carve") + return nil, errors.New("cannot get block for expired carve") } if blockId > metadata.MaxBlock { diff --git a/server/service/service_carves_test.go b/server/service/service_carves_test.go index 62249d7ee1..d884d4918f 100644 --- a/server/service/service_carves_test.go +++ b/server/service/service_carves_test.go @@ -2,7 +2,7 @@ package service import ( "context" - "fmt" + "errors" "testing" "time" @@ -61,7 +61,7 @@ func TestCarveBeginNewCarveError(t *testing.T) { ms := new(mock.Store) svc := &Service{carveStore: ms} ms.NewCarveFunc = func(ctx context.Context, metadata *fleet.CarveMetadata) (*fleet.CarveMetadata, error) { - return nil, fmt.Errorf("ouch!") + return nil, errors.New("ouch!") } ctx := hostctx.NewContext(context.Background(), host) @@ -155,7 +155,7 @@ func TestCarveCarveBlockGetCarveError(t *testing.T) { ms := new(mock.Store) svc := &Service{carveStore: ms} ms.CarveBySessionIdFunc = func(ctx context.Context, sessionId string) (*fleet.CarveMetadata, error) { - return nil, fmt.Errorf("ouch!") + return nil, errors.New("ouch!") } payload := fleet.CarveBlockPayload{ @@ -308,7 +308,7 @@ func TestCarveCarveBlockNewBlockError(t *testing.T) { return metadata, nil } ms.NewBlockFunc = func(ctx context.Context, carve *fleet.CarveMetadata, blockId int64, data []byte) error { - return fmt.Errorf("kaboom!") + return errors.New("kaboom!") } payload := fleet.CarveBlockPayload{ @@ -434,7 +434,7 @@ func TestCarveGetBlockGetBlockError(t *testing.T) { ms.GetBlockFunc = func(ctx context.Context, carve *fleet.CarveMetadata, blockId int64) ([]byte, error) { assert.Equal(t, metadata.ID, carve.ID) assert.Equal(t, int64(3), blockId) - return nil, fmt.Errorf("yow!!") + return nil, errors.New("yow!!") } // Block requested is greater than max block diff --git a/server/service/service_osquery_test.go b/server/service/service_osquery_test.go index e08c3b9d18..5bd4f4a703 100644 --- a/server/service/service_osquery_test.go +++ b/server/service/service_osquery_test.go @@ -1194,7 +1194,7 @@ func TestIngestDistributedQueryOrphanedCampaignLoadError(t *testing.T) { } ds.DistributedQueryCampaignFunc = func(ctx context.Context, id uint) (*fleet.DistributedQueryCampaign, error) { - return nil, fmt.Errorf("missing campaign") + return nil, errors.New("missing campaign") } lq.On("StopQuery", "42").Return(nil) @@ -1265,7 +1265,7 @@ func TestIngestDistributedQueryOrphanedCloseError(t *testing.T) { return campaign, nil } ds.SaveDistributedQueryCampaignFunc = func(ctx context.Context, campaign *fleet.DistributedQueryCampaign) error { - return fmt.Errorf("failed save") + return errors.New("failed save") } host := fleet.Host{ID: 1} @@ -1303,7 +1303,7 @@ func TestIngestDistributedQueryOrphanedStopError(t *testing.T) { ds.SaveDistributedQueryCampaignFunc = func(ctx context.Context, campaign *fleet.DistributedQueryCampaign) error { return nil } - lq.On("StopQuery", strconv.Itoa(int(campaign.ID))).Return(fmt.Errorf("failed")) + lq.On("StopQuery", strconv.Itoa(int(campaign.ID))).Return(errors.New("failed")) host := fleet.Host{ID: 1} @@ -1366,7 +1366,7 @@ func TestIngestDistributedQueryRecordCompletionError(t *testing.T) { campaign := &fleet.DistributedQueryCampaign{ID: 42} host := fleet.Host{ID: 1} - lq.On("QueryCompletedByHost", strconv.Itoa(int(campaign.ID)), host.ID).Return(fmt.Errorf("fail")) + lq.On("QueryCompletedByHost", strconv.Itoa(int(campaign.ID)), host.ID).Return(errors.New("fail")) go func() { ch, err := rs.ReadChannel(context.Background(), *campaign) diff --git a/server/service/service_users_test.go b/server/service/service_users_test.go index 05f2f6ce37..9c7f430eac 100644 --- a/server/service/service_users_test.go +++ b/server/service/service_users_test.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "errors" - "fmt" "testing" "time" @@ -498,7 +497,7 @@ func TestUserAuth(t *testing.T) { return nil } ds.InviteByEmailFunc = func(ctx context.Context, email string) (*fleet.Invite, error) { - return nil, fmt.Errorf("AA") + return nil, errors.New("AA") } ds.UserByIDFunc = func(ctx context.Context, id uint) (*fleet.User, error) { if id == 999 { diff --git a/server/service/testing_client.go b/server/service/testing_client.go index 0832c2c95e..b01df706f9 100644 --- a/server/service/testing_client.go +++ b/server/service/testing_client.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/datastore/mysql" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/test" @@ -77,7 +78,7 @@ func (ts *withServer) DoRawWithHeaders( for key, val := range headers { req.Header.Add(key, val) } - client := &http.Client{} + client := fleethttp.NewClient() if len(queryParams)%2 != 0 { require.Fail(t, "need even number of params: key value") diff --git a/server/sso/settings.go b/server/sso/settings.go index f93ab9e9b6..98dcdca29e 100644 --- a/server/sso/settings.go +++ b/server/sso/settings.go @@ -7,6 +7,7 @@ import ( "net/http" "time" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" dsigtypes "github.com/russellhaering/goxmldsig/types" ) @@ -70,9 +71,7 @@ func ParseMetadata(metadata string) (*Metadata, error) { // and timeout defines how long to wait to get a response form the metadata // server. func GetMetadata(metadataURL string) (*Metadata, error) { - client := &http.Client{ - Timeout: 5 * time.Second, - } + client := fleethttp.NewClient(fleethttp.WithTimeout(5 * time.Second)) request, err := http.NewRequest(http.MethodGet, metadataURL, nil) if err != nil { return nil, err diff --git a/server/utils.go b/server/utils.go index f33834e95e..5e16b45457 100644 --- a/server/utils.go +++ b/server/utils.go @@ -10,6 +10,8 @@ import ( "io/ioutil" "net/http" "time" + + "github.com/fleetdm/fleet/v4/pkg/fleethttp" ) // GenerateRandomText return a string generated by filling in keySize bytes with @@ -29,7 +31,7 @@ func PostJSONWithTimeout(ctx context.Context, url string, v interface{}) error { return err } - client := &http.Client{Timeout: 30 * time.Second} + client := fleethttp.NewClient(fleethttp.WithTimeout(30 * time.Second)) req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewBuffer(jsonBytes)) if err != nil { return err diff --git a/server/vulnerabilities/cpe.go b/server/vulnerabilities/cpe.go index afb534ecfa..1ba0fd6955 100644 --- a/server/vulnerabilities/cpe.go +++ b/server/vulnerabilities/cpe.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/config" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/fleet" @@ -226,7 +227,7 @@ func TranslateSoftwareToCPE( ) error { dbPath := path.Join(vulnPath, "cpe.sqlite") - client := &http.Client{} + client := fleethttp.NewClient() if err := SyncCPEDatabase(client, dbPath, config); err != nil { return ctxerr.Wrap(ctx, err, "sync cpe db") } diff --git a/server/vulnerabilities/cpe_test.go b/server/vulnerabilities/cpe_test.go index ed004902bf..712028333c 100644 --- a/server/vulnerabilities/cpe_test.go +++ b/server/vulnerabilities/cpe_test.go @@ -15,6 +15,7 @@ import ( "github.com/dnaeon/go-vcr/v2/recorder" "github.com/facebookincubator/nvdtools/cpedict" + "github.com/fleetdm/fleet/v4/pkg/fleethttp" "github.com/fleetdm/fleet/v4/server/config" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mock" @@ -57,14 +58,13 @@ func TestSyncCPEDatabase(t *testing.T) { t.Skip("set environment variable NETWORK_TEST=1 to run") } + client := fleethttp.NewClient() // Disabling vcr because the resulting file exceeds the 100mb limit for github - r, err := recorder.NewAsMode("fixtures/nvd-cpe-release", recorder.ModeDisabled, http.DefaultTransport) + r, err := recorder.NewAsMode("fixtures/nvd-cpe-release", recorder.ModeDisabled, client.Transport) require.NoError(t, err) defer r.Stop() - client := &http.Client{ - Transport: r, - } + client.Transport = r tempDir := os.TempDir() dbPath := path.Join(tempDir, "cpe.sqlite") @@ -213,7 +213,7 @@ func TestSyncsCPEFromURL(t *testing.T) { })) defer ts.Close() - client := &http.Client{} + client := fleethttp.NewClient() tempDir := t.TempDir() dbPath := path.Join(tempDir, "cpe.sqlite") @@ -227,7 +227,7 @@ func TestSyncsCPEFromURL(t *testing.T) { } func TestSyncsCPESkipsIfDisableSync(t *testing.T) { - client := &http.Client{} + client := fleethttp.NewClient() tempDir := t.TempDir() dbPath := path.Join(tempDir, "cpe.sqlite") diff --git a/tools/ci/rules.go b/tools/ci/rules.go new file mode 100644 index 0000000000..39dc86a516 --- /dev/null +++ b/tools/ci/rules.go @@ -0,0 +1,23 @@ +//go:build ignore +// +build ignore + +package gorules + +import ( + "github.com/quasilyte/go-ruleguard/dsl" +) + +func fmtErrorfWithoutArgs(m dsl.Matcher) { + m.Match(`fmt.Errorf($msg)`). + Report(`fmt.Errorf: change for errors.New($msg)`). + Suggest(`errors.New($msg)`) +} + +func createHttpClient(m dsl.Matcher) { + m.Match( + `http.Client{$*_}`, + `new(http.Client)`, + `http.Transport{$*_}`, + `new(http.Transport)`, + ).Report(`http.Client: use fleethttp.NewClient instead`) +}