ssh: Only restart sshd on reload when its config or host key changed (#1923)

ssh.go

@@ -60,6 +60,20 @@ type sshDeviceInfoFlags struct {
 func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) {
 	c.RegisterReloadCallback(func(c *config.C) {
 		if c.GetBool("sshd.enabled", false) {
+			// Restarting the server drops every open session, the one that asked for this reload included. Only restart
+			// when it isn't running or its config changed. The host key file can change under the same config, new
+			// handshakes use the new key without a restart
+			if ssh.Running() && !c.HasChanged("sshd") {
+				key, err := loadSSHHostKey(c)
+				if err == nil {
+					err = ssh.SetHostKey(key)
+				}
+				if err != nil {
+					l.Error("Failed to reconfigure the sshd", "error", err)
+					ssh.Stop()
+				}
+				return
+			}
 			sshRun, err := configSSH(l, ssh, c)
 			if err != nil {
 				l.Error("Failed to reconfigure the sshd", "error", err)
@@ -74,6 +88,22 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) {
 	})
 }
 
+// loadSSHHostKey returns sshd.host_key, read from its file when it names one.
+func loadSSHHostKey(c *config.C) ([]byte, error) {
+	hostKeyPathOrKey := c.GetString("sshd.host_key", "")
+	if hostKeyPathOrKey == "" {
+		return nil, fmt.Errorf("sshd.host_key must be provided")
+	}
+	if strings.Contains(hostKeyPathOrKey, "-----BEGIN") {
+		return []byte(hostKeyPathOrKey), nil
+	}
+	b, err := os.ReadFile(hostKeyPathOrKey)
+	if err != nil {
+		return nil, fmt.Errorf("error while loading sshd.host_key file: %s", err)
+	}
+	return b, nil
+}
+
 // configSSH reads the ssh info out of the passed-in Config and
 // updates the passed-in SSHServer. On success, it returns a function
 // that callers may invoke to run the configured ssh server. On
@@ -92,19 +122,9 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error)
 		return nil, fmt.Errorf("sshd.listen can not use port 22")
 	}
 
-	hostKeyPathOrKey := c.GetString("sshd.host_key", "")
-	if hostKeyPathOrKey == "" {
-		return nil, fmt.Errorf("sshd.host_key must be provided")
-	}
-
-	var hostKeyBytes []byte
-	if strings.Contains(hostKeyPathOrKey, "-----BEGIN") {
-		hostKeyBytes = []byte(hostKeyPathOrKey)
-	} else {
-		hostKeyBytes, err = os.ReadFile(hostKeyPathOrKey)
-		if err != nil {
-			return nil, fmt.Errorf("error while loading sshd.host_key file: %s", err)
-		}
+	hostKeyBytes, err := loadSSHHostKey(c)
+	if err != nil {
+		return nil, err
 	}
 
 	err = ssh.SetHostKey(hostKeyBytes)

sshd/server.go

@@ -8,14 +8,18 @@ import (
 	"log/slog"
 	"net"
 	"sync"
+	"sync/atomic"
 
 	"github.com/armon/go-radix"
 	"golang.org/x/crypto/ssh"
 )
 
 type SSHServer struct {
-	config *ssh.ServerConfig
-	l      *slog.Logger
+	// config is replaced whole when the host key changes, ssh.ServerConfig.AddHostKey only replaces a key of the same
+	// algorithm and appends any other
+	config       atomic.Pointer[ssh.ServerConfig]
+	authenticate func(ssh.ConnMetadata, ssh.PublicKey) (*ssh.Permissions, error)
+	l            *slog.Logger
 
 	certChecker *ssh.CertChecker
 
@@ -28,7 +32,12 @@ type SSHServer struct {
 	// List of available commands
 	helpCommand *Command
 	commands    *radix.Tree
-	listener    net.Listener
+	listener    atomic.Pointer[net.Listener]
+
+	// runs counts the Runs that are listening. A reload's Run can start before the one it replaced has returned
+	runs atomic.Int32
+	// addr is where the newest Run listens, nil when none is
+	addr atomic.Pointer[net.Addr]
 
 	// ctx parents per-Run contexts. Cancelling it (e.g. via Control.Stop) tears the server down even
 	// across reloads, since each Run derives a fresh child rather than reusing this one directly.
@@ -85,10 +94,8 @@ func NewSSHServer(ctx context.Context, l *slog.Logger) (*SSHServer, error) {
 		},
 	}
 
-	s.config = &ssh.ServerConfig{
-		PublicKeyCallback: cc.Authenticate,
-		ServerVersion:     fmt.Sprintf("SSH-2.0-Nebula???"),
-	}
+	s.authenticate = cc.Authenticate
+	s.config.Store(s.newConfig())
 
 	s.RegisterCommand(&Command{
 		Name:             "help",
@@ -107,10 +114,32 @@ func (s *SSHServer) SetHostKey(hostPrivateKey []byte) error {
 		return fmt.Errorf("failed to parse private key: %s", err)
 	}
 
-	s.config.AddHostKey(private)
+	config := s.newConfig()
+	config.AddHostKey(private)
+	s.config.Store(config)
 	return nil
 }
 
+func (s *SSHServer) newConfig() *ssh.ServerConfig {
+	return &ssh.ServerConfig{
+		PublicKeyCallback: s.authenticate,
+		ServerVersion:     fmt.Sprintf("SSH-2.0-Nebula???"),
+	}
+}
+
+// Addr is the address the newest Run listens on, nil when none is.
+func (s *SSHServer) Addr() net.Addr {
+	if a := s.addr.Load(); a != nil {
+		return *a
+	}
+	return nil
+}
+
+// Running is true while a Run is listening.
+func (s *SSHServer) Running() bool {
+	return s.runs.Load() > 0
+}
+
 func (s *SSHServer) ClearTrustedCAs() {
 	s.authLock.Lock()
 	s.trustedCAs = []ssh.PublicKey{}
@@ -181,10 +210,16 @@ func (s *SSHServer) Run(addr string) error {
 	// this run owns. They start equal but a fast reload may overwrite s.listener with the next run's
 	// listener before this run's watcher fires, so each run must close its own listener via the local
 	// reference.
-	s.listener = listener
+	s.listener.Store(&listener)
+	s.runs.Add(1)
+	bound := listener.Addr()
+	s.addr.Store(&bound)
 
 	runCtx, cancel := context.WithCancel(s.ctx)
 	defer cancel()
+	// Deferred after cancel so they run first, a run on its way out no longer counts as listening
+	defer s.runs.Add(-1)
+	defer s.addr.CompareAndSwap(&bound, nil)
 
 	// Close the listener when this run's context is cancelled. That can come from the parent
 	// (Control.Stop), from Run returning normally (defer cancel above), or transitively when a sibling
@@ -197,7 +232,7 @@ func (s *SSHServer) Run(addr string) error {
 		}
 	}()
 
-	s.l.Info("SSH server is listening", "sshListener", addr)
+	s.l.Info("SSH server is listening", "sshListener", bound)
 
 	// Run loops until there is an error
 	s.run(runCtx, listener)
@@ -226,7 +261,7 @@ func (s *SSHServer) run(ctx context.Context, listener net.Listener) {
 				<-sessionContext.Done()
 				c.Close()
 			}()
-			conn, chans, reqs, err := ssh.NewServerConn(c, s.config)
+			conn, chans, reqs, err := ssh.NewServerConn(c, s.config.Load())
 			fp := ""
 			if conn != nil {
 				fp = conn.Permissions.Extensions["fp"]
@@ -264,8 +299,8 @@ func (s *SSHServer) run(ctx context.Context, listener net.Listener) {
 }
 
 func (s *SSHServer) Stop() {
-	if s.listener != nil {
-		if err := s.listener.Close(); err != nil {
+	if l := s.listener.Load(); l != nil {
+		if err := (*l).Close(); err != nil && !errors.Is(err, net.ErrClosed) {
 			s.l.Warn("Failed to close the sshd listener", "error", err)
 		}
 	}

sshd_reload_test.go

@@ -0,0 +1,196 @@
+package nebula
+
+import (
+	"crypto"
+	"crypto/ed25519"
+	"crypto/rand"
+	"crypto/rsa"
+	"encoding/pem"
+	"fmt"
+	"net"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/sshd"
+	"github.com/slackhq/nebula/test"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/crypto/ssh"
+)
+
+type sshdReloadHarness struct {
+	c       *config.C
+	server  *sshd.SSHServer
+	keyFile string
+	signer  ssh.Signer
+	extra   string
+}
+
+func newSSHDReloadHarness(t *testing.T) *sshdReloadHarness {
+	t.Helper()
+	l := test.NewLogger()
+	h := &sshdReloadHarness{keyFile: filepath.Join(t.TempDir(), "host_key")}
+	h.writeHostKey(t, newEd25519Key(t))
+
+	_, userKey, err := ed25519.GenerateKey(rand.Reader)
+	require.NoError(t, err)
+	h.signer, err = ssh.NewSignerFromKey(userKey)
+	require.NoError(t, err)
+
+	h.c = config.NewC(l)
+	require.NoError(t, h.c.LoadString(h.config()))
+	h.server, err = sshd.NewSSHServer(t.Context(), l)
+	require.NoError(t, err)
+	wireSSHReload(l, h.server, h.c)
+	run, err := configSSH(l, h.server, h.c)
+	require.NoError(t, err)
+	go run()
+	t.Cleanup(h.server.Stop)
+	require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond)
+	return h
+}
+
+func newEd25519Key(t *testing.T) crypto.Signer {
+	_, key, err := ed25519.GenerateKey(rand.Reader)
+	require.NoError(t, err)
+	return key
+}
+
+// writeHostKey replaces the host key file and returns the public key a client should now see
+func (h *sshdReloadHarness) writeHostKey(t *testing.T, key crypto.Signer) ssh.PublicKey {
+	t.Helper()
+	block, err := ssh.MarshalPrivateKey(key, "")
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(h.keyFile, pem.EncodeToMemory(block), 0o600))
+	pub, err := ssh.NewPublicKey(key.Public())
+	require.NoError(t, err)
+	return pub
+}
+
+func (h *sshdReloadHarness) config() string {
+	return fmt.Sprintf("sshd:\n  enabled: true\n  listen: 127.0.0.1:0\n  host_key: %s\n  authorized_users:\n    - user: test\n      keys: [%q]\n%s",
+		h.keyFile, string(ssh.MarshalAuthorizedKey(h.signer.PublicKey())), h.extra)
+}
+
+// dial connects to wherever the server listens now, a restart on :0 moves it. hostKeyAlgorithms limits which host keys
+// the client accepts, none means any
+func (h *sshdReloadHarness) dial(hostKeyAlgorithms ...string) (*ssh.Client, ssh.PublicKey, error) {
+	addr := h.server.Addr()
+	if addr == nil {
+		return nil, nil, fmt.Errorf("not listening")
+	}
+	var hostKey ssh.PublicKey
+	c, err := ssh.Dial("tcp", addr.String(), &ssh.ClientConfig{
+		User: "test",
+		Auth: []ssh.AuthMethod{ssh.PublicKeys(h.signer)},
+		HostKeyCallback: func(_ string, _ net.Addr, key ssh.PublicKey) error {
+			hostKey = key
+			return nil
+		},
+		HostKeyAlgorithms: hostKeyAlgorithms,
+		Timeout:           time.Second,
+	})
+	return c, hostKey, err
+}
+
+func (h *sshdReloadHarness) connect(t *testing.T) (*ssh.Client, ssh.PublicKey) {
+	t.Helper()
+	var client *ssh.Client
+	var hostKey ssh.PublicKey
+	require.Eventually(t, func() bool {
+		var err error
+		client, hostKey, err = h.dial()
+		return err == nil
+	}, 5*time.Second, 20*time.Millisecond)
+	t.Cleanup(func() { _ = client.Close() })
+	return client, hostKey
+}
+
+// dropped is true when the server closed the client's connection within wait
+func dropped(client *ssh.Client, wait time.Duration) bool {
+	done := make(chan struct{})
+	go func() {
+		_ = client.Wait()
+		close(done)
+	}()
+	select {
+	case <-done:
+		return true
+	case <-time.After(wait):
+		return false
+	}
+}
+
+// Restarting sshd drops every open session, the one that asked for the reload included. A reload that touches nothing
+// sshd reads leaves it alone
+func TestSSHDReload_UnchangedKeepsSessions(t *testing.T) {
+	h := newSSHDReloadHarness(t)
+	client, _ := h.connect(t)
+
+	require.NoError(t, h.c.ReloadConfigString(h.config()+"punchy:\n  punch: true\n"))
+	assert.False(t, dropped(client, 500*time.Millisecond), "an unchanged sshd was restarted")
+}
+
+func TestSSHDReload_ChangedConfigRestarts(t *testing.T) {
+	h := newSSHDReloadHarness(t)
+	client, _ := h.connect(t)
+
+	h.extra = "  trusted_cas: []\n"
+	require.NoError(t, h.c.ReloadConfigString(h.config()))
+	assert.True(t, dropped(client, 5*time.Second), "a changed sshd config was not applied")
+	require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond)
+	h.connect(t)
+}
+
+// The config can stay the same while the host key file it names changes. New connections get the new key, open
+// sessions are left alone
+func TestSSHDReload_ChangedHostKeyFile(t *testing.T) {
+	h := newSSHDReloadHarness(t)
+	client, _ := h.connect(t)
+
+	want := h.writeHostKey(t, newEd25519Key(t))
+	require.NoError(t, h.c.ReloadConfigString(h.config()))
+	_, got := h.connect(t)
+	assert.Equal(t, want.Marshal(), got.Marshal(), "a new host key was not applied")
+	assert.False(t, dropped(client, 500*time.Millisecond), "a new host key restarted sshd")
+}
+
+// A host key of another algorithm replaces the old one rather than being offered next to it
+func TestSSHDReload_HostKeyAlgorithmChange(t *testing.T) {
+	h := newSSHDReloadHarness(t)
+	h.connect(t)
+
+	rsaKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	require.NoError(t, err)
+	want := h.writeHostKey(t, rsaKey)
+	require.NoError(t, h.c.ReloadConfigString(h.config()))
+
+	_, got := h.connect(t)
+	assert.Equal(t, want.Marshal(), got.Marshal())
+	_, _, err = h.dial(ssh.KeyAlgoED25519)
+	assert.Error(t, err, "the old ed25519 host key is still offered")
+}
+
+// A server that isn't running, say its port was busy, is tried again on every reload as before
+func TestSSHDReload_NotRunningRestarts(t *testing.T) {
+	h := newSSHDReloadHarness(t)
+	h.server.Stop()
+	require.Eventually(t, func() bool { return !h.server.Running() }, 5*time.Second, 10*time.Millisecond)
+
+	require.NoError(t, h.c.ReloadConfigString(h.config()))
+	require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond)
+	h.connect(t)
+}
+
+// A host key that no longer loads stops sshd, the same as a config that doesn't
+func TestSSHDReload_BadHostKeyFileStops(t *testing.T) {
+	h := newSSHDReloadHarness(t)
+	h.connect(t)
+
+	require.NoError(t, os.WriteFile(h.keyFile, []byte("not a key"), 0o600))
+	require.NoError(t, h.c.ReloadConfigString(h.config()))
+	require.Eventually(t, func() bool { return !h.server.Running() }, 5*time.Second, 10*time.Millisecond)
+}