diff --git a/.golangci.yml b/.golangci.yml index c31589ecdde9476afeb5853810592b15bfa88fa3..47c8474851b379333009f15e5efc458e97d3144d 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -132,6 +132,7 @@ linters-settings: allow: - $gostd - github.com/stretchr/testify + - github.com/stretchr/testify/require - gitlab.com/gitlab-org/gitlab-shell - gitlab.com/gitlab-org/labkit - gitlab.com/gitlab-org/gitaly @@ -142,8 +143,8 @@ linters-settings: - github.com/golang-jwt/jwt - github.com/mikesmitty/edkey - github.com/sirupsen/logrus - - github.com/grpc-ecosystem/go-grpc-prometheus - github.com/mattn/go-shellwords + - github.com/grpc-ecosystem/go-grpc-prometheus # list-type: blacklist # include-go-root: false diff --git a/client/client_test.go b/client/client_test.go index 9bc7f1c0279ff05f7dd6a59d512f73eba24ae435..dde0c425e107a14bf9d0ee5e0270e47c357d327a 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -22,7 +22,7 @@ import ( var ( secret = "sssh, it's a secret" - defaultHttpOpts = []HTTPClientOpt{WithHTTPRetryOpts(time.Millisecond, time.Millisecond, 2)} + defaultHTTPOpts = []HTTPClientOpt{WithHTTPRetryOpts(time.Millisecond, time.Millisecond, 2)} ) func TestClients(t *testing.T) { @@ -84,7 +84,7 @@ func TestClients(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { url := tc.server(t, buildRequests(t, tc.relativeURLRoot)) - httpClient, err := NewHTTPClientWithOpts(url, tc.relativeURLRoot, tc.caFile, "", 1, defaultHttpOpts) + httpClient, err := NewHTTPClientWithOpts(url, tc.relativeURLRoot, tc.caFile, "", 1, defaultHTTPOpts) require.NoError(t, err) client, err := NewGitlabNetClient("", "", tc.secret, httpClient) @@ -112,7 +112,7 @@ func testSuccessfulGet(t *testing.T, client *GitlabNetClient) { responseBody, err := io.ReadAll(response.Body) require.NoError(t, err) - require.Equal(t, string(responseBody), "Hello") + require.Equal(t, "Hello", string(responseBody)) }) } @@ -136,12 +136,14 @@ func testMissing(t *testing.T, client *GitlabNetClient) { t.Run("Missing error for GET", func(t *testing.T) { response, err := client.Get(context.Background(), "/missing") require.EqualError(t, err, "Internal API error (404)") + defer response.Body.Close() require.Nil(t, response) }) t.Run("Missing error for POST", func(t *testing.T) { response, err := client.Post(context.Background(), "/missing", map[string]string{}) require.EqualError(t, err, "Internal API error (404)") + defer response.Body.Close() require.Nil(t, response) }) } @@ -150,12 +152,14 @@ func testErrorMessage(t *testing.T, client *GitlabNetClient) { t.Run("Error with message for GET", func(t *testing.T) { response, err := client.Get(context.Background(), "/error") require.EqualError(t, err, "Don't do that") + defer response.Body.Close() require.Nil(t, response) }) t.Run("Error with message for POST", func(t *testing.T) { response, err := client.Post(context.Background(), "/error", map[string]string{}) require.EqualError(t, err, "Don't do that") + defer response.Body.Close() require.Nil(t, response) }) } @@ -164,12 +168,14 @@ func testBrokenRequest(t *testing.T, client *GitlabNetClient) { t.Run("Broken request for GET", func(t *testing.T) { response, err := client.Get(context.Background(), "/broken") require.EqualError(t, err, "Internal API unreachable") + defer response.Body.Close() require.Nil(t, response) }) t.Run("Broken request for POST", func(t *testing.T) { response, err := client.Post(context.Background(), "/broken", map[string]string{}) require.EqualError(t, err, "Internal API unreachable") + defer response.Body.Close() require.Nil(t, response) }) } @@ -180,7 +186,7 @@ func testJWTAuthenticationHeader(t *testing.T, client *GitlabNetClient) { require.NoError(t, err) claims := &jwt.RegisteredClaims{} - token, err := jwt.ParseWithClaims(string(responseBody), claims, func(token *jwt.Token) (interface{}, error) { + token, err := jwt.ParseWithClaims(string(responseBody), claims, func(_ *jwt.Token) (interface{}, error) { return []byte(secret), nil }) require.NoError(t, err) @@ -273,7 +279,7 @@ func buildRequests(t *testing.T, relativeURLRoot string) []testserver.TestReques }, { Path: "/api/v4/internal/error", - Handler: func(w http.ResponseWriter, r *http.Request) { + Handler: func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusBadRequest) body := map[string]string{ @@ -284,7 +290,7 @@ func buildRequests(t *testing.T, relativeURLRoot string) []testserver.TestReques }, { Path: "/api/v4/internal/broken", - Handler: func(w http.ResponseWriter, r *http.Request) { + Handler: func(_ http.ResponseWriter, _ *http.Request) { panic("Broken") }, }, @@ -302,19 +308,20 @@ func buildRequests(t *testing.T, relativeURLRoot string) []testserver.TestReques func TestRetryOnFailure(t *testing.T) { reqAttempts := 0 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { reqAttempts++ w.WriteHeader(500) })) defer srv.Close() - httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHttpOpts) + httpClient, err := NewHTTPClientWithOpts(srv.URL, "/", "", "", 1, defaultHTTPOpts) require.NoError(t, err) require.NotNil(t, httpClient.RetryableHTTP) client, err := NewGitlabNetClient("", "", "", httpClient) require.NoError(t, err) - _, err = client.Get(context.Background(), "/") + resp, err := client.Get(context.Background(), "/") require.EqualError(t, err, "Internal API unreachable") + defer resp.Body.Close() require.Equal(t, 3, reqAttempts) } diff --git a/client/httpclient.go b/client/httpclient.go index f91f4d6e628ff66fff70902115813b0041c1749f..2068c0ed79561e4d874d65abda11fce330d8d1b5 100644 --- a/client/httpclient.go +++ b/client/httpclient.go @@ -84,7 +84,7 @@ func validateCaFile(filename string) error { } // NewHTTPClientWithOpts builds an HTTP client using the provided options -func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, readTimeoutSeconds uint64, opts []HTTPClientOpt) (*HTTPClient, error) { +func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, readTimeoutSeconds int64, opts []HTTPClientOpt) (*HTTPClient, error) { hcc := &httpClientCfg{ caFile: caFile, caPath: caPath, @@ -201,7 +201,7 @@ func buildHTTPTransport(gitlabURL string) (*http.Transport, string) { return &http.Transport{}, gitlabURL } -func readTimeout(timeoutSeconds uint64) time.Duration { +func readTimeout(timeoutSeconds int64) time.Duration { if timeoutSeconds == 0 { timeoutSeconds = defaultReadTimeoutSeconds } diff --git a/client/httpclient_test.go b/client/httpclient_test.go index e831b99053465ef1b4f4806942a63bb754d56940..77926d199d472fc2638bedbd27fddef756bbcfcc 100644 --- a/client/httpclient_test.go +++ b/client/httpclient_test.go @@ -16,7 +16,7 @@ import ( ) func TestReadTimeout(t *testing.T) { - expectedSeconds := uint64(300) + expectedSeconds := int64(300) client, err := NewHTTPClientWithOpts("http://localhost:3000", "", "", "", expectedSeconds, nil) require.NoError(t, err) diff --git a/client/httpsclient_test.go b/client/httpsclient_test.go index dd30ec56160cf4e88ebcf13b18e1d1fd473b6ea4..ca1ffd8141c6fe1eead0dee54a6c3d970abafee9 100644 --- a/client/httpsclient_test.go +++ b/client/httpsclient_test.go @@ -60,7 +60,7 @@ func TestSuccessfulRequests(t *testing.T) { responseBody, err := io.ReadAll(response.Body) require.NoError(t, err) - require.Equal(t, string(responseBody), "Hello") + require.Equal(t, "Hello", string(responseBody)) }) } } @@ -103,10 +103,12 @@ func TestFailedRequests(t *testing.T) { require.Error(t, err) require.ErrorIs(t, err, ErrCafileNotFound) } else { - _, err = client.Get(context.Background(), "/hello") + resp, err := client.Get(context.Background(), "/hello") require.Error(t, err) - require.Equal(t, err.Error(), tc.expectedError) + defer resp.Body.Close() + + require.Equal(t, tc.expectedError, err.Error()) } }) } @@ -126,7 +128,7 @@ func setupWithRequests(t *testing.T, caFile, caPath, clientCAPath, clientCertPat url := testserver.StartHTTPSServer(t, requests, clientCAPath) - opts := defaultHttpOpts + opts := defaultHTTPOpts if clientCertPath != "" && clientKeyPath != "" { opts = append(opts, WithClientCert(clientCertPath, clientKeyPath)) } diff --git a/client/testserver/gitalyserver.go b/client/testserver/gitalyserver.go index fdb60db7961a62a01fcd669d09371d86027f9688..fec41ad2c5a046484bf8d422554732224631c20b 100644 --- a/client/testserver/gitalyserver.go +++ b/client/testserver/gitalyserver.go @@ -1,3 +1,4 @@ +// Package testserver - package testserver import ( diff --git a/cmd/gitlab-shell-authorized-keys-check/main.go b/cmd/gitlab-shell-authorized-keys-check/main.go index 012736b607ecf243df4bab65443926d134a3f420..27eaff29517be94a7e447e9a3e00686030a767ad 100644 --- a/cmd/gitlab-shell-authorized-keys-check/main.go +++ b/cmd/gitlab-shell-authorized-keys-check/main.go @@ -40,7 +40,7 @@ func main() { fmt.Fprintf(readWriter.ErrOut, "%v\n", err) } - os.Exit(int(code)) + os.Exit(code) } func execute(readWriter *readwriter.ReadWriter) (int, error) { diff --git a/cmd/gitlab-shell/main.go b/cmd/gitlab-shell/main.go index 0e9e77c2b65ad4d0b45ab5a6927a86fdf7883552..3486cadeed62facd386676b8ac0abf036fc00017 100644 --- a/cmd/gitlab-shell/main.go +++ b/cmd/gitlab-shell/main.go @@ -39,12 +39,14 @@ func main() { executable, err := executable.New(executable.GitlabShell) if err != nil { + //nolint fmt.Fprintln(readWriter.ErrOut, "Failed to determine executable, exiting") os.Exit(1) } config, err := config.NewFromDirExternal(executable.RootDir) if err != nil { + //nolint fmt.Fprintln(readWriter.ErrOut, "Failed to read config, exiting:", err) os.Exit(1) } @@ -57,6 +59,7 @@ func main() { if err != nil { // For now this could happen if `SSH_CONNECTION` is not set on // the environment + //nolint fmt.Fprintf(readWriter.ErrOut, "%v\n", err) os.Exit(1) } diff --git a/internal/command/lfstransfer/lfstransfer_test.go b/internal/command/lfstransfer/lfstransfer_test.go index 5bdf015c20add6e40193b11f9a5f403c3ab66249..33b7b0afdd80a90e322c4bb560b650042ddbefb4 100644 --- a/internal/command/lfstransfer/lfstransfer_test.go +++ b/internal/command/lfstransfer/lfstransfer_test.go @@ -310,7 +310,7 @@ func TestLfsTransferBatchDownload(t *testing.T) { case strings.HasPrefix(arg, "token="): tokenArg = arg default: - require.Fail(t, "Unexpected batch item argument: %v", arg) + require.Failf(t, "Unexpected batch item argument: %v", arg) } } @@ -378,7 +378,7 @@ func TestLfsTransferBatchUpload(t *testing.T) { case strings.HasPrefix(arg, "token="): tokenArg = arg default: - require.Fail(t, "Unexpected batch item argument: %v", arg) + require.Failf(t, "Unexpected batch item argument: %v", arg) } } diff --git a/internal/command/receivepack/gitalycall_test.go b/internal/command/receivepack/gitalycall_test.go index 40e854bced43003009b4d88e2d35f311242c08a1..e01392e21970339cc2c9634b36f698741c8ea36a 100644 --- a/internal/command/receivepack/gitalycall_test.go +++ b/internal/command/receivepack/gitalycall_test.go @@ -95,7 +95,7 @@ func TestReceivePack(t *testing.T) { require.Equal(t, v, actual[0]) } require.Empty(t, testServer.ReceivedMD["some-other-ff"]) - require.Equal(t, testServer.ReceivedMD["x-gitlab-correlation-id"][0], "a-correlation-id") + require.Equal(t, "a-correlation-id", testServer.ReceivedMD["x-gitlab-correlation-id"][0]) } }) } diff --git a/internal/config/config.go b/internal/config/config.go index e842e90b8be48010f80db5b14ca1a1a108fa1669..f5e2d1177248c264d278f177652d1c962602c7e4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -56,7 +56,7 @@ type ServerConfig struct { type HTTPSettingsConfig struct { User string `yaml:"user"` Password string `yaml:"password"` - ReadTimeoutSeconds uint64 `yaml:"read_timeout"` + ReadTimeoutSeconds int64 `yaml:"read_timeout"` CaFile string `yaml:"ca_file"` CaPath string `yaml:"ca_path"` } diff --git a/internal/sshd/sshd.go b/internal/sshd/sshd.go index e83b78ef9e421858cf6e062a6c8ae5e088860da1..478c63b9de2c89f5c29081467c6788ca5875fd9c 100644 --- a/internal/sshd/sshd.go +++ b/internal/sshd/sshd.go @@ -265,20 +265,6 @@ func (s *Server) proxyPolicy() (proxyproto.PolicyFunc, error) { } } -func extractDataFromContext(ctx context.Context) command.LogData { - logData := command.LogData{} - - if ctx == nil { - return logData - } - - if ctx.Value(command.LogDataKey) != nil { - logData = ctx.Value(command.LogDataKey).(command.LogData) - } - - return logData -} - func extractLogDataFromContext(ctx context.Context) command.LogData { logData := command.LogData{} diff --git a/internal/sshenv/sshenv_test.go b/internal/sshenv/sshenv_test.go index 09c1ff1a196efd56d74e2b9d637abdf1e924c5b0..159cfc19f0ccb56fc956fa6e3b58d7d85e841e6e 100644 --- a/internal/sshenv/sshenv_test.go +++ b/internal/sshenv/sshenv_test.go @@ -46,5 +46,5 @@ func TestRemoteAddrFromEnv(t *testing.T) { } func TestEmptyRemoteAddrFromEnv(t *testing.T) { - require.Equal(t, "", remoteAddrFromEnv()) + require.Empty(t, remoteAddrFromEnv()) } diff --git a/support/lint_last_known_acceptable.txt b/support/lint_last_known_acceptable.txt index 1f68e46ddbccac914c016752fee69e456ed0eedb..f61ab2d6a4ee43b9b02fb6d0f385cb999631231d 100644 --- a/support/lint_last_known_acceptable.txt +++ b/support/lint_last_known_acceptable.txt @@ -1,41 +1,19 @@ -client/client_test.go:25:2: var-naming: var defaultHttpOpts should be defaultHTTPOpts (revive) -client/client_test.go:115:3: expected-actual: need to reverse actual and expected values (testifylint) -client/client_test.go:137:30: response body must be closed (bodyclose) -client/client_test.go:143:31: response body must be closed (bodyclose) -client/client_test.go:151:30: response body must be closed (bodyclose) -client/client_test.go:157:31: response body must be closed (bodyclose) -client/client_test.go:165:30: response body must be closed (bodyclose) -client/client_test.go:171:31: response body must be closed (bodyclose) -client/client_test.go:183:72: unused-parameter: parameter 'token' seems to be unused, consider removing or renaming it as _ (revive) -client/client_test.go:276:41: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) -client/client_test.go:287:18: unused-parameter: parameter 'w' seems to be unused, consider removing or renaming it as _ (revive) -client/client_test.go:305:73: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) -client/client_test.go:317:21: response body must be closed (bodyclose) -client/httpclient.go:209:22: G115: integer overflow conversion uint64 -> int64 (gosec) -client/httpsclient_test.go:63:4: expected-actual: need to reverse actual and expected values (testifylint) -client/httpsclient_test.go:106:24: response body must be closed (bodyclose) -client/httpsclient_test.go:109:5: expected-actual: need to reverse actual and expected values (testifylint) -client/testserver/gitalyserver.go:1:1: package-comments: should have a package comment (revive) -client/testserver/gitalyserver.go:21:6: exported: exported type TestGitalyServer should have comment or be unexported (revive) -client/testserver/gitalyserver.go:26:1: exported: exported method TestGitalyServer.SSHReceivePack should have comment or be unexported (revive) -client/testserver/gitalyserver.go:38:1: exported: exported method TestGitalyServer.SSHUploadPackWithSidechannel should have comment or be unexported (revive) -client/testserver/gitalyserver.go:43:18: Error return value of `conn.Close` is not checked (errcheck) -client/testserver/gitalyserver.go:58:1: exported: exported method TestGitalyServer.SSHUploadArchive should have comment or be unexported (revive) -client/testserver/gitalyserver.go:70:1: exported: exported function StartGitalyServer should have comment or be unexported (revive) -client/testserver/gitalyserver.go:119:3: go-require: require must only be used in the goroutine running the test function (testifylint) +client/testserver/gitalyserver.go:22:6: exported: exported type TestGitalyServer should have comment or be unexported (revive) +client/testserver/gitalyserver.go:27:1: exported: exported method TestGitalyServer.SSHReceivePack should have comment or be unexported (revive) +client/testserver/gitalyserver.go:39:1: exported: exported method TestGitalyServer.SSHUploadPackWithSidechannel should have comment or be unexported (revive) +client/testserver/gitalyserver.go:44:18: Error return value of `conn.Close` is not checked (errcheck) +client/testserver/gitalyserver.go:59:1: exported: exported method TestGitalyServer.SSHUploadArchive should have comment or be unexported (revive) +client/testserver/gitalyserver.go:71:1: exported: exported function StartGitalyServer should have comment or be unexported (revive) +client/testserver/gitalyserver.go:120:3: go-require: require must only be used in the goroutine running the test function (testifylint) client/testserver/testserver.go:20:6: exported: exported type TestRequestHandler should have comment or be unexported (revive) client/testserver/testserver.go:46:12: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec) client/testserver/testserver.go:52:17: Error return value of `server.Serve` is not checked (errcheck) client/testserver/testserver.go:117:18: G304: Potential file inclusion via variable (gosec) cmd/gitlab-shell-authorized-keys-check/main.go:40:14: Error return value of `fmt.Fprintf` is not checked (errcheck) -cmd/gitlab-shell-authorized-keys-check/main.go:43:13: unnecessary conversion (unconvert) cmd/gitlab-shell/command/command.go:74:3: cmd/gitlab-shell/command/command.go:74: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "FIXME: When 1.21+ only Golang is support..." (godox) cmd/gitlab-shell/main.go:1:1: package-comments: should have a package comment (revive) -cmd/gitlab-shell/main.go:42:15: Error return value of `fmt.Fprintln` is not checked (errcheck) -cmd/gitlab-shell/main.go:48:15: Error return value of `fmt.Fprintln` is not checked (errcheck) -cmd/gitlab-shell/main.go:53:23: Error return value of `logCloser.Close` is not checked (errcheck) -cmd/gitlab-shell/main.go:60:14: Error return value of `fmt.Fprintf` is not checked (errcheck) -cmd/gitlab-shell/main.go:61:3: exitAfterDefer: os.Exit will exit, and `defer logCloser.Close()` will not run (gocritic) +cmd/gitlab-shell/main.go:55:23: Error return value of `logCloser.Close` is not checked (errcheck) +cmd/gitlab-shell/main.go:64:3: exitAfterDefer: os.Exit will exit, and `defer logCloser.Close()` will not run (gocritic) internal/command/authorizedkeys/authorized_keys.go:29:4: internal/command/authorizedkeys/authorized_keys.go:29: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: Log this event once we have a cons..." (godox) internal/command/command.go:1:1: package-comments: should have a package comment (revive) internal/command/command.go:15:6: exported: exported type Command should have comment or be unexported (revive) @@ -65,7 +43,6 @@ internal/command/lfstransfer/lfstransfer_test.go:1114:1: cognitive complexity 33 internal/command/readwriter/readwriter.go:1:1: package-comments: should have a package comment (revive) internal/command/readwriter/readwriter.go:7:6: exported: exported type ReadWriter should have comment or be unexported (revive) internal/command/receivepack/gitalycall_test.go:31:5: var-naming: struct field keyId should be keyID (revive) -internal/command/receivepack/gitalycall_test.go:98:5: expected-actual: need to reverse actual and expected values (testifylint) internal/config/config.go:1:1: package-comments: should have a package comment (revive) internal/config/config.go:21:2: G101: Potential hardcoded credentials (gosec) internal/config/config.go:24:6: exported: exported type YamlDuration should have comment or be unexported (revive) @@ -90,4 +67,3 @@ internal/gitlabnet/healthcheck/client_test.go:19:41: unused-parameter: parameter internal/gitlabnet/lfstransfer/client.go:137:3: internal/gitlabnet/lfstransfer/client.go:137: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "FIXME: This causes tests to fail" (godox) internal/sshd/server_config.go:130:19: SA1019: ssh.KeyAlgoDSA is deprecated: DSA is only supported at insecure key sizes, and was removed from major implementations. (staticcheck) internal/sshd/server_config_test.go:5:2: SA1019: "crypto/dsa" has been deprecated since Go 1.16 because it shouldn't be used: DSA is a legacy algorithm, and modern alternatives such as Ed25519 (implemented by package crypto/ed25519) should be used instead. Keys with 1024-bit moduli (L1024N160 parameters) are cryptographically weak, while bigger keys are not widely supported. Note that FIPS 186-5 no longer approves DSA for signature generation. (staticcheck) -internal/sshd/sshd.go:268:6: func `extractDataFromContext` is unused (unused)