Merge branch 'fix-lint-sshd-server_config' into 'main'

Fix lint issues for sshd server_config

Closes #723

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



Merged-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Co-authored-by: gaurav.marwal <gauravmarwal@gmail.com>

internal/sshd/server_config.go

+// Package sshd implements functionality related to SSH server configuration and handling
 package sshd
 
 import (
@@ -5,6 +6,7 @@ import (
 	"encoding/base64"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -50,7 +52,7 @@ func parseHostKeys(keyFiles []string) []ssh.Signer {
 	var hostKeys []ssh.Signer
 
 	for _, filename := range keyFiles {
-		keyRaw, err := os.ReadFile(filename)
+		keyRaw, err := os.ReadFile(filepath.Clean(filename))
 		if err != nil {
 			log.WithError(err).WithFields(log.Fields{"filename": filename}).Error("Failed to read host key")
 			continue
@@ -76,7 +78,7 @@ func parseHostCerts(hostKeys []ssh.Signer, certFiles []string) map[string]*ssh.C
 	}
 
 	for _, filename := range certFiles {
-		keyRaw, err := os.ReadFile(filename)
+		keyRaw, err := os.ReadFile(filepath.Clean(filename))
 		if err != nil {
 			log.WithError(err).WithFields(log.Fields{"filename": filename}).Error("failed to read host certificate")
 			continue
@@ -126,7 +128,7 @@ func newServerConfig(cfg *config.Config) (*serverConfig, error) {
 
 	hostKeys := parseHostKeys(cfg.Server.HostKeyFiles)
 	if len(hostKeys) == 0 {
-		return nil, fmt.Errorf("No host keys could be loaded, aborting")
+		return nil, fmt.Errorf("no host keys could be loaded, aborting")
 	}
 
 	hostKeyToCertMap := parseHostCerts(hostKeys, cfg.Server.HostCertFiles)
@@ -208,12 +210,12 @@ func (s *serverConfig) handleUserCertificate(ctx context.Context, user string, c
 	}, nil
 }
 
-func (s *serverConfig) get(ctx context.Context) *ssh.ServerConfig {
+func (s *serverConfig) get(parentCtx context.Context) *ssh.ServerConfig {
 	var gssapiWithMICConfig *ssh.GSSAPIWithMICConfig
 	if s.cfg.Server.GSSAPI.Enabled {
-		gssApiServer, _ := NewGSSAPIServer(&s.cfg.Server.GSSAPI)
+		gssAPIServer, _ := NewGSSAPIServer(&s.cfg.Server.GSSAPI)
 
-		if gssApiServer != nil {
+		if gssAPIServer != nil {
 			gssapiWithMICConfig = &ssh.GSSAPIWithMICConfig{
 				AllowLogin: func(conn ssh.ConnMetadata, srcName string) (*ssh.Permissions, error) {
 					if conn.User() != s.cfg.User {
@@ -227,14 +229,14 @@ func (s *serverConfig) get(ctx context.Context) *ssh.ServerConfig {
 						},
 					}, nil
 				},
-				Server: gssApiServer,
+				Server: gssAPIServer,
 			}
 		}
 	}
 
 	sshCfg := &ssh.ServerConfig{
 		PublicKeyCallback: func(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) {
-			ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
+			ctx, cancel := context.WithTimeout(parentCtx, 10*time.Second)
 			defer cancel()
 
 			log.WithContextFields(ctx, log.Fields{"ssh_key_type": key.Type()}).Info("public key authentication")
@@ -250,29 +252,42 @@ func (s *serverConfig) get(ctx context.Context) *ssh.ServerConfig {
 		ServerVersion:       "SSH-2.0-GitLab-SSHD",
 	}
 
-	if len(s.cfg.Server.MACs) > 0 {
-		sshCfg.MACs = s.cfg.Server.MACs
-	} else {
-		sshCfg.MACs = supportedMACs
+	s.configureMACs(sshCfg)
+	s.configureKeyExchanges(sshCfg)
+	s.configureCiphers(sshCfg)
+	s.configurePublicKeyAlgorithms(sshCfg)
+
+	for _, key := range s.hostKeys {
+		sshCfg.AddHostKey(key)
 	}
 
-	if len(s.cfg.Server.KexAlgorithms) > 0 {
-		sshCfg.KeyExchanges = s.cfg.Server.KexAlgorithms
-	} else {
-		sshCfg.KeyExchanges = supportedKeyExchanges
+	return sshCfg
+}
+
+func (s *serverConfig) configurePublicKeyAlgorithms(sshCfg *ssh.ServerConfig) {
+	if len(s.cfg.Server.PublicKeyAlgorithms) > 0 {
+		sshCfg.PublicKeyAuthAlgorithms = s.cfg.Server.PublicKeyAlgorithms
 	}
+}
 
+func (s *serverConfig) configureCiphers(sshCfg *ssh.ServerConfig) {
 	if len(s.cfg.Server.Ciphers) > 0 {
 		sshCfg.Ciphers = s.cfg.Server.Ciphers
 	}
+}
 
-	if len(s.cfg.Server.PublicKeyAlgorithms) > 0 {
-		sshCfg.PublicKeyAuthAlgorithms = s.cfg.Server.PublicKeyAlgorithms
+func (s *serverConfig) configureKeyExchanges(sshCfg *ssh.ServerConfig) {
+	if len(s.cfg.Server.KexAlgorithms) > 0 {
+		sshCfg.KeyExchanges = s.cfg.Server.KexAlgorithms
+	} else {
+		sshCfg.KeyExchanges = supportedKeyExchanges
 	}
+}
 
-	for _, key := range s.hostKeys {
-		sshCfg.AddHostKey(key)
+func (s *serverConfig) configureMACs(sshCfg *ssh.ServerConfig) {
+	if len(s.cfg.Server.MACs) > 0 {
+		sshCfg.MACs = s.cfg.Server.MACs
+	} else {
+		sshCfg.MACs = supportedMACs
 	}
-
-	return sshCfg
 }

internal/sshd/server_config_test.go

@@ -27,7 +27,7 @@ func TestNewServerConfigWithoutHosts(t *testing.T) {
 	_, err := newServerConfig(&config.Config{GitlabUrl: "http://localhost"})
 
 	require.Error(t, err)
-	require.Equal(t, "No host keys could be loaded, aborting", err.Error())
+	require.Equal(t, "no host keys could be loaded, aborting", err.Error())
 }
 
 func TestHostKeyAndCerts(t *testing.T) {
@@ -317,14 +317,14 @@ func TestGSSAPIWithMIC(t *testing.T) {
 	require.NotNil(t, sshServerConfig.GSSAPIWithMICConfig)
 	require.NotNil(t, sshServerConfig.GSSAPIWithMICConfig.AllowLogin)
 	require.NotNil(t, server)
-	require.Equal(t, server.ServicePrincipalName, "host/test@TEST.TEST")
+	require.Equal(t, "host/test@TEST.TEST", server.ServicePrincipalName)
 
 	sshServerConfig.SetDefaults()
 
 	require.NotNil(t, sshServerConfig.GSSAPIWithMICConfig)
 	require.NotNil(t, sshServerConfig.GSSAPIWithMICConfig.AllowLogin)
 	require.NotNil(t, server)
-	require.Equal(t, server.ServicePrincipalName, "host/test@TEST.TEST")
+	require.Equal(t, "host/test@TEST.TEST", server.ServicePrincipalName)
 }
 
 func TestGSSAPIWithMICDisabled(t *testing.T) {