Merge branch '781-sshd-lint' into 'main'

Lint fixes for sshd packages

Closes #781

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/1138



Merged-by: Igor Drozdov <idrozdov@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Igor Drozdov <idrozdov@gitlab.com>
Co-authored-by: Archish <archishthakkar@gmail.com>

cmd/gitlab-sshd/main.go

+// Package main implements the GitLab SSH daemon.
 package main
 
 import (
@@ -27,8 +28,8 @@ var (
 )
 
 func overrideConfigFromEnvironment(cfg *config.Config) {
-	if gitlabUrl := os.Getenv("GITLAB_URL"); gitlabUrl != "" {
-		cfg.GitlabUrl = gitlabUrl
+	if gitlabURL := os.Getenv("GITLAB_URL"); gitlabURL != "" {
+		cfg.GitlabUrl = gitlabURL
 	}
 	if gitlabTracing := os.Getenv("GITLAB_TRACING"); gitlabTracing != "" {
 		cfg.GitlabTracing = gitlabTracing
@@ -67,8 +68,11 @@ func main() {
 	cfg.ApplyGlobalState()
 
 	logCloser := logger.ConfigureStandalone(cfg)
-	defer logCloser.Close()
-
+	defer func() {
+		if err := logCloser.Close(); err != nil {
+			log.WithError(err).Fatal("Error closing logCloser")
+		}
+	}()
 	ctx, finished := command.Setup("gitlab-sshd", cfg)
 	defer finished()
 
@@ -81,15 +85,7 @@ func main() {
 
 	// Startup monitoring endpoint.
 	if cfg.Server.WebListen != "" {
-		go func() {
-			err := monitoring.Start(
-				monitoring.WithListenerAddress(cfg.Server.WebListen),
-				monitoring.WithBuildInformation(Version, BuildTime),
-				monitoring.WithServeMux(server.MonitoringServeMux()),
-			)
-
-			log.WithError(err).Fatal("monitoring service raised an error")
-		}()
+		startupMonitoringEndpoint(cfg, server)
 	}
 
 	ctx, cancel := context.WithCancel(ctx)
@@ -98,6 +94,14 @@ func main() {
 	done := make(chan os.Signal, 1)
 	signal.Notify(done, syscall.SIGINT, syscall.SIGTERM)
 
+	gracefulShutdown(ctx, done, cfg, server, cancel)
+
+	if err := server.ListenAndServe(ctx); err != nil {
+		log.WithError(err).Fatal("GitLab built-in sshd failed to listen for new connections")
+	}
+}
+
+func gracefulShutdown(ctx context.Context, done chan os.Signal, cfg *config.Config, server *sshd.Server, cancel context.CancelFunc) {
 	go func() {
 		sig := <-done
 		signal.Reset(syscall.SIGINT, syscall.SIGTERM)
@@ -105,14 +109,24 @@ func main() {
 		gracePeriod := time.Duration(cfg.Server.GracePeriod)
 		log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": gracePeriod.Seconds(), "signal": sig.String()}).Info("Shutdown initiated")
 
-		server.Shutdown()
+		if err := server.Shutdown(); err != nil {
+			log.WithError(err).Fatal("Error shutting down the server")
+		}
 
 		<-time.After(gracePeriod)
 
 		cancel()
 	}()
+}
 
-	if err := server.ListenAndServe(ctx); err != nil {
-		log.WithError(err).Fatal("GitLab built-in sshd failed to listen for new connections")
-	}
+func startupMonitoringEndpoint(cfg *config.Config, server *sshd.Server) {
+	go func() {
+		err := monitoring.Start(
+			monitoring.WithListenerAddress(cfg.Server.WebListen),
+			monitoring.WithBuildInformation(Version, BuildTime),
+			monitoring.WithServeMux(server.MonitoringServeMux()),
+		)
+
+		log.WithError(err).Fatal("monitoring service raised an error")
+	}()
 }

internal/sshd/gssapi_unsupported.go

@@ -8,6 +8,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
 )
 
+// NewGSSAPIServer initializes and returns a new OSGSSAPIServer.
 func NewGSSAPIServer(c *config.GSSAPIConfig) (*OSGSSAPIServer, error) {
 	s := &OSGSSAPIServer{
 		ServicePrincipalName: c.ServicePrincipalName,
@@ -16,18 +17,22 @@ func NewGSSAPIServer(c *config.GSSAPIConfig) (*OSGSSAPIServer, error) {
 	return s, nil
 }
 
+// OSGSSAPIServer represents a server that handles GSSAPI requests.
 type OSGSSAPIServer struct {
 	ServicePrincipalName string
 }
 
+// AcceptSecContext returns an error indicating that GSSAPI is unsupported.
 func (*OSGSSAPIServer) AcceptSecContext([]byte) ([]byte, string, bool, error) {
 	return []byte{}, "", false, errors.New("gssapi is unsupported")
 }
 
+// VerifyMIC returns an error indicating that GSSAPI is unsupported.
 func (*OSGSSAPIServer) VerifyMIC([]byte, []byte) error {
 	return errors.New("gssapi is unsupported")
 }
 
+// DeleteSecContext returns an error indicating that GSSAPI is unsupported.
 func (*OSGSSAPIServer) DeleteSecContext() error {
 	return errors.New("gssapi is unsupported")
 }

internal/sshd/server_config_test.go

@@ -60,8 +60,9 @@ func TestHostKeyAndCerts(t *testing.T) {
 	data, err := os.ReadFile(path.Join(testRoot, "certs/valid/server.pub"))
 	require.NoError(t, err)
 
-	publicKey, _, _, _, err := ssh.ParseAuthorizedKey(data)
+	publicKey, comment, _, _, err := ssh.ParseAuthorizedKey(data)
 	require.NoError(t, err)
+	require.NotNil(t, comment)
 	require.NotNil(t, publicKey)
 	cert, ok := cfg.hostKeyToCertMap[string(publicKey.Marshal())]
 	require.True(t, ok)

internal/sshd/sshd_test.go

@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/pires/go-proxyproto"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ssh"
 
@@ -409,16 +410,16 @@ func setupServerWithContext(ctx context.Context, t *testing.T, cfg *config.Confi
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				correlationID = r.Header.Get("X-Request-Id")
 
-				require.NotEmpty(t, correlationID)
-				require.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
+				assert.NotEmpty(t, correlationID)
+				assert.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
 
 				fmt.Fprint(w, `{"id": 1000, "key": "key"}`)
 			},
 		}, {
 			Path: "/api/v4/internal/discover",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, correlationID, r.Header.Get("X-Request-Id"))
-				require.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
+				assert.Equal(t, correlationID, r.Header.Get("X-Request-Id"))
+				assert.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
 
 				fmt.Fprint(w, `{"id": 1000, "name": "Test User", "username": "test-user"}`)
 			},

support/lint_last_known_acceptable.txt

@@ -53,11 +53,6 @@ cmd/gitlab-sshd/acceptance_test.go:132:5: go-require: do not use require in http
 cmd/gitlab-sshd/acceptance_test.go:135:5: go-require: do not use require in http handlers (testifylint)
 cmd/gitlab-sshd/acceptance_test.go:188:4: go-require: do not use require in http handlers (testifylint)
 cmd/gitlab-sshd/acceptance_test.go:498:4: go-require: do not use require in http handlers (testifylint)
-cmd/gitlab-sshd/main.go:1:1: package-comments: should have a package comment (revive)
-cmd/gitlab-sshd/main.go:30:5: var-naming: var gitlabUrl should be gitlabURL (revive)
-cmd/gitlab-sshd/main.go:44: Function 'main' is too long (73 > 60) (funlen)
-cmd/gitlab-sshd/main.go:70:23: Error return value of `logCloser.Close` is not checked (errcheck)
-cmd/gitlab-sshd/main.go:108:18: Error return value of `server.Shutdown` is not checked (errcheck)
 internal/command/authorizedkeys/authorized_keys.go:29: 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)
@@ -249,18 +244,8 @@ internal/gitlabnet/client.go:35:1: exported: exported function ParseIP should ha
 internal/gitlabnet/healthcheck/client_test.go:19:41: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive)
 internal/gitlabnet/lfstransfer/client.go:137: internal/gitlabnet/lfstransfer/client.go:137: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "FIXME: This causes tests to fail" (godox)
 internal/gitlabnet/personalaccesstoken/client_test.go:30:5: go-require: do not use require in http handlers (testifylint)
-internal/sshd/gssapi_unsupported.go:11:1: exported: exported function NewGSSAPIServer should have comment or be unexported (revive)
-internal/sshd/gssapi_unsupported.go:19:6: exported: exported type OSGSSAPIServer should have comment or be unexported (revive)
-internal/sshd/gssapi_unsupported.go:23:1: exported: exported method OSGSSAPIServer.AcceptSecContext should have comment or be unexported (revive)
-internal/sshd/gssapi_unsupported.go:27:1: exported: exported method OSGSSAPIServer.VerifyMIC should have comment or be unexported (revive)
-internal/sshd/gssapi_unsupported.go:31:1: exported: exported method OSGSSAPIServer.DeleteSecContext should have comment or be unexported (revive)
 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/server_config_test.go:63:2: declaration has 3 blank identifiers (dogsled)
 internal/sshd/sshd.go:268:6: func `extractDataFromContext` is unused (unused)
-internal/sshd/sshd_test.go:412:5: go-require: do not use require in http handlers (testifylint)
-internal/sshd/sshd_test.go:413:5: go-require: do not use require in http handlers (testifylint)
-internal/sshd/sshd_test.go:420:5: go-require: do not use require in http handlers (testifylint)
-internal/sshd/sshd_test.go:421:5: go-require: do not use require in http handlers (testifylint)
 internal/testhelper/requesthandlers/requesthandlers.go:25:5: go-require: do not use require in http handlers (testifylint)
 internal/testhelper/requesthandlers/requesthandlers.go:63:5: go-require: do not use require in http handlers (testifylint)
 internal/testhelper/requesthandlers/requesthandlers.go:90:5: go-require: do not use require in http handlers (testifylint)