Merge branch 'id-revert-ssh-connection-timeouts' into 'main'

Revert "Abort long-running unauthenticated SSH connections"

See merge request gitlab-org/gitlab-shell!605

config.yml.example

@@ -84,8 +84,6 @@ sshd:
   readiness_probe: "/start"
   # The endpoint that returns 200 OK if the server is alive. Defaults to "/health".
   liveness_probe: "/health"
-  # The server disconnects after this time (in seconds) if the user has not successfully logged in. Defaults to 60.
-  login_grace_time: 60
   # SSH host key files.
   host_key_files:
     - /run/secrets/ssh-hostkeys/ssh_host_rsa_key

internal/config/config.go

@@ -26,7 +26,6 @@ type ServerConfig struct {
 	ProxyProtocol           bool     `yaml:"proxy_protocol,omitempty"`
 	WebListen               string   `yaml:"web_listen,omitempty"`
 	ConcurrentSessionsLimit int64    `yaml:"concurrent_sessions_limit,omitempty"`
-	LoginGraceTimeSeconds   uint64   `yaml:"login_grace_time"`
 	GracePeriodSeconds      uint64   `yaml:"grace_period"`
 	ReadinessProbe          string   `yaml:"readiness_probe"`
 	LivenessProbe           string   `yaml:"liveness_probe"`
@@ -79,7 +78,6 @@ var (
 		Listen:                  "[::]:22",
 		WebListen:               "localhost:9122",
 		ConcurrentSessionsLimit: 10,
-		LoginGraceTimeSeconds:   60,
 		GracePeriodSeconds:      10,
 		ReadinessProbe:          "/start",
 		LivenessProbe:           "/health",
@@ -91,10 +89,6 @@ var (
 	}
 )
 
-func (sc *ServerConfig) LoginGraceTime() time.Duration {
-	return time.Duration(sc.LoginGraceTimeSeconds) * time.Second
-}
-
 func (sc *ServerConfig) GracePeriod() time.Duration {
 	return time.Duration(sc.GracePeriodSeconds) * time.Second
 }

internal/sshd/sshd.go

@@ -179,12 +179,11 @@ func (s *Server) handleConn(ctx context.Context, nconn net.Conn) {
 
 	ctxlog.Info("server: handleConn: start")
 
-	sconn, chans, reqs, err := s.initSSHConnection(ctx, nconn)
+	sconn, chans, reqs, err := ssh.NewServerConn(nconn, s.serverConfig.get(ctx))
 	if err != nil {
 		ctxlog.WithError(err).Error("server: handleConn: failed to initialize SSH connection")
 		return
 	}
-
 	go ssh.DiscardRequests(reqs)
 
 	var establishSessionDuration float64
@@ -211,20 +210,6 @@ func (s *Server) handleConn(ctx context.Context, nconn net.Conn) {
 	}).Info("server: handleConn: done")
 }
 
-func (s *Server) initSSHConnection(ctx context.Context, nconn net.Conn) (sconn *ssh.ServerConn, chans <-chan ssh.NewChannel, reqs <-chan *ssh.Request, err error) {
-	timer := time.AfterFunc(s.Config.Server.LoginGraceTime(), func() {
-		nconn.Close()
-	})
-	defer func() {
-		// If time.Stop() equals false, that means that AfterFunc has been executed
-		if !timer.Stop() {
-			err = fmt.Errorf("initSSHConnection: ssh handshake timeout of %v", s.Config.Server.LoginGraceTime())
-		}
-	}()
-
-	return ssh.NewServerConn(nconn, s.serverConfig.get(ctx))
-}
-
 func unconditionalRequirePolicy(_ net.Addr) (proxyproto.Policy, error) {
 	return proxyproto.REQUIRE, nil
 }

internal/sshd/sshd_test.go

@@ -3,7 +3,6 @@ package sshd
 import (
 	"context"
 	"fmt"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -135,33 +134,6 @@ func TestInvalidServerConfig(t *testing.T) {
 	require.Nil(t, s.Shutdown())
 }
 
-func TestLoginGraceTime(t *testing.T) {
-	s := setupServer(t)
-
-	unauthenticatedRequestStatus := make(chan string)
-	completed := make(chan bool)
-
-	clientCfg := clientConfig(t)
-	clientCfg.HostKeyCallback = func(_ string, _ net.Addr, _ ssh.PublicKey) error {
-		unauthenticatedRequestStatus <- "authentication-started"
-		<-completed // Wait infinitely
-
-		return nil
-	}
-
-	go func() {
-		// Start an SSH connection that never ends
-		ssh.Dial("tcp", serverUrl, clientCfg)
-	}()
-
-	require.Equal(t, "authentication-started", <-unauthenticatedRequestStatus)
-
-	// Verify that the server shutdowns instantly without waiting for any connection to execute
-	// That means that the server doesn't have any connections to wait for
-	require.NoError(t, s.Shutdown())
-	verifyStatus(t, s, StatusClosed)
-}
-
 func setupServer(t *testing.T) *Server {
 	t.Helper()
 
@@ -211,7 +183,6 @@ func setupServerWithConfig(t *testing.T, cfg *config.Config) *Server {
 	cfg.User = user
 	cfg.Server.Listen = serverUrl
 	cfg.Server.ConcurrentSessionsLimit = 1
-	cfg.Server.LoginGraceTimeSeconds = 1
 	cfg.Server.HostKeyFiles = []string{path.Join(testhelper.TestRoot, "certs/valid/server.key")}
 
 	s, err := NewServer(cfg)