Commit `d299dba2d3`

d299dba2d3e319a9ac4488f03ca00818e55aaf54

parent: 6821a6f760

Verified · cmc ci/build: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-03 23:43 UTC

sshd: close idle connections on shutdown, and do not count a store failure as a bad key

A CLI keeps a shared connection open between commands, and Shutdown
waited for it like a session in flight: a deploy's restart held the
drain for its full 30 s on one idle connection. Each connection now
counts its running sessions; Shutdown closes the idle ones at once and
waits only for sessions mid-command.

authenticate treated any error from the key lookup as an unknown key:
a failure against the limiter and a denial. A busy database during a
restart could lock every client out for a minute. A store error other
than not-found is now a temporary refusal that counts nothing.

The CLI says, on ssh's exit 255, that a burst of connections may have
tripped the instance's auth rate limit, since ssh's own message cannot
tell a bad key from a throttle.

TestShutdownClosesIdleConnections opens a control master with no
session and expects the daemon to exit within seconds of SIGTERM.

Closes #141

Layout: unified · split

cmd/gitbay/ssh.go +2

 	if ee, ok := err.(*exec.ExitError); ok {
 		code := ee.ExitCode()
 		if code == 255 { // ssh-level failure (connection, auth, host key)
+			fmt.Fprintln(os.Stderr, "gitbay: ssh could not connect or authenticate; if this worked a moment ago,"+
+				" the instance may be rate-limiting authentication after a burst of connections: wait a minute and retry")
 			return protocol.ExitProtocol
 		}
 		return code

e2e/shutdown_test.go +49

	@@ -1,7 +1,11 @@
1	package e2e	1	package e2e
2		2
3	import (	3	import (
		4	"fmt"
4	"net/http"	5	"net/http"
		6	"os/exec"
		7	"path/filepath"
		8	"strings"
5	"syscall"	9	"syscall"
6	"testing"	10	"testing"
7	"time"	11	"time"
	@@ -32,3 +36,48 @@ func TestServeStopsOnSIGTERM(t *testing.T) {
32	t.Fatal("http listener still answering after shutdown")	36	t.Fatal("http listener still answering after shutdown")
33	}	37	}
34	}	38	}
		39
		40	// A CLI keeps a shared connection open between commands. On shutdown that
		41	// idle connection is closed at once rather than holding the drain for its
		42	// full 30 s; only a session mid-command is waited for (#141).
		43	func TestShutdownClosesIdleConnections(t *testing.T) {
		44	inst := startInstance(t)
		45	aliceKey := inst.newKey(t, "alice")
		46	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
		47
		48	// A control master with no session: -N, backgrounded, persistent.
		49	// A unix socket path has a short limit; t.TempDir is far past it here.
		50	sock := fmt.Sprintf("/tmp/gitbay-e2e-cm-%d", inst.port)
		51	base := []string{"-p", fmt.Sprint(inst.port), "-i", aliceKey, "-o", "IdentitiesOnly=yes",
		52	"-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=" + filepath.Join(inst.sshDir, "known_hosts"),
		53	"-o", "BatchMode=yes", "-o", "ControlPath=" + sock}
		54	master := exec.Command("ssh", append(append([]string{}, base...), "-o", "ControlMaster=yes", "-o", "ControlPersist=yes", "-N", "-f", "git@127.0.0.1")...)
		55	if out, err := master.CombinedOutput(); err != nil {
		56	t.Fatalf("control master: %v\n%s", err, out)
		57	}
		58	t.Cleanup(func() {
		59	exec.Command("ssh", append(append([]string{}, base...), "-O", "exit", "git@127.0.0.1")...).Run()
		60	})
		61	// The shared connection works.
		62	if out, err := exec.Command("ssh", append(append([]string{}, base...), "git@127.0.0.1", "whoami")...).Output(); err != nil \|\| strings.TrimSpace(string(out)) != "alice" {
		63	t.Fatalf("multiplexed whoami: %v %q", err, out)
		64	}
		65
		66	start := time.Now()
		67	if err := inst.proc.Process.Signal(syscall.SIGTERM); err != nil {
		68	t.Fatal(err)
		69	}
		70	done := make(chan error, 1)
		71	go func() { done <- inst.proc.Wait() }()
		72	select {
		73	case err := <-done:
		74	if err != nil {
		75	t.Fatalf("daemon did not exit cleanly: %v", err)
		76	}
		77	case <-time.After(20 * time.Second):
		78	t.Fatal("daemon still running 20s after SIGTERM with only an idle connection open")
		79	}
		80	if took := time.Since(start); took > 5*time.Second {
		81	t.Fatalf("shutdown took %s with only an idle connection open", took)
		82	}
		83	}

internal/sshd/sshd.go +49 −10

 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 	"golang.org/x/crypto/ssh"
 	sshCfg      *ssh.ServerConfig
 	authLimiter *rateLimiter
 	sessions    sync.WaitGroup // accepted connections still being served
+	mu          sync.Mutex
+	conns       map[*conn]struct{}
+}
+// conn is one accepted connection and how many sessions it is running.
+// A CLI's shared connection sits idle between commands; on shutdown an
+// idle connection is closed at once and only a session mid-command is
+// waited for (#141).
+type conn struct {
+	net    net.Conn
+	active atomic.Int32
 }
 func New(cfg config.Config, st *store.Store) (*Server, error) {
-	s := &Server{cfg: cfg, st: st, authLimiter: newRateLimiter(cfg.Limits.SSHAuthRate, time.Minute)}
+	s := &Server{cfg: cfg, st: st, authLimiter: newRateLimiter(cfg.Limits.SSHAuthRate, time.Minute), conns: map[*conn]struct{}{}}
 	sc := &ssh.ServerConfig{
 		PublicKeyCallback: s.authenticate,
 	}
 	fp := ssh.FingerprintSHA256(pub)
 	key, err := s.st.SSHKeyByFingerprint(fp)
+	if err != nil && !errors.Is(err, store.ErrNotFound) {
+		// The store, not the key, failed. Neither a failure against the
+		// limiter nor "unknown key": a busy database during a restart
+		// would otherwise lock every client out for a minute.
+		slog.Error("ssh auth: key lookup", "err", err)
+		return nil, fmt.Errorf("authentication temporarily unavailable")
+	}
 	if err != nil {
 		if s.cfg.Registration.Mode != "closed" {
 			return &ssh.Permissions{Extensions: map[string]string{
 // Serve accepts connections on ln until it is closed.
 func (s *Server) Serve(ln net.Listener) error {
 	for {
-		conn, err := ln.Accept()
+		nc, err := ln.Accept()
 		if err != nil {
 			return err
 		}
+		c := &conn{net: nc}
+		s.mu.Lock()
+		s.conns[c] = struct{}{}
+		s.mu.Unlock()
 		s.sessions.Add(1)
 		go func() {
 			defer s.sessions.Done()
-			s.handleConn(conn)
+			defer func() {
+				s.mu.Lock()
+				delete(s.conns, c)
+				s.mu.Unlock()
+			}()
+			s.handleConn(c)
 		}()
 	}
 }
-// Shutdown waits for every accepted connection to finish, or for ctx. The
+// Shutdown closes every idle connection, then waits for the ones with a
-// caller closes the listener first; a push in flight completes rather
+// session running, or for ctx. The caller closes the listener first; a
-// than being cut mid-pack.
+// push in flight completes rather than being cut mid-pack.
 func (s *Server) Shutdown(ctx context.Context) error {
+	s.mu.Lock()
+	for c := range s.conns {
+		if c.active.Load() == 0 {
+			c.net.Close()
+		}
+	}
+	s.mu.Unlock()
 	done := make(chan struct{})
 	go func() {
 		s.sessions.Wait()
 	}
 }
-func (s *Server) handleConn(conn net.Conn) {
+func (s *Server) handleConn(c *conn) {
-	defer conn.Close()
+	defer c.net.Close()
-	sconn, chans, reqs, err := ssh.NewServerConn(conn, s.sshCfg)
+	sconn, chans, reqs, err := ssh.NewServerConn(c.net, s.sshCfg)
 	if err != nil {
 		return
 	}
 		if err != nil {
 			continue
 		}
-		go s.handleSession(sconn, ch, chReqs)
+		c.active.Add(1)
+		go func() {
+			defer c.active.Add(-1)
+			s.handleSession(sconn, ch, chReqs)
+		}()
 	}
 }

Commit d299dba2d3

Commit `d299dba2d3`