Don't restart sshd when only sshd.sandbox_dir changes (#1948)

ssh.go

@@ -14,6 +14,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"runtime/pprof"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -65,7 +66,7 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) {
 			// 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") {
+			if ssh.Running() && !slices.ContainsFunc(sshdServerKeys, c.HasChanged) {
 				key, err := loadSSHHostKey(c)
 				if err == nil {
 					err = ssh.SetHostKey(key)
@@ -92,6 +93,10 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) {
 	})
 }
 
+// sshdServerKeys are what configSSH reads. sshd.sandbox_dir isn't one, it's read once at startup and restarting the
+// server doesn't apply it
+var sshdServerKeys = []string{"sshd.enabled", "sshd.listen", "sshd.host_key", "sshd.trusted_cas", "sshd.authorized_users"}
+
 // 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", "")

sshd_reload_test.go

@@ -26,13 +26,15 @@ type sshdReloadHarness struct {
 	server  *sshd.SSHServer
 	keyFile string
 	signer  ssh.Signer
+	listen  string
+	users   []string
 	extra   string
 }
 
 func newSSHDReloadHarness(t *testing.T) *sshdReloadHarness {
 	t.Helper()
 	l := test.NewLogger()
-	h := &sshdReloadHarness{keyFile: filepath.Join(t.TempDir(), "host_key")}
+	h := &sshdReloadHarness{keyFile: filepath.Join(t.TempDir(), "host_key"), listen: "127.0.0.1:0", users: []string{"test"}}
 	h.writeHostKey(t, newEd25519Key(t))
 
 	_, userKey, err := ed25519.GenerateKey(rand.Reader)
@@ -71,8 +73,12 @@ func (h *sshdReloadHarness) writeHostKey(t *testing.T, key crypto.Signer) ssh.Pu
 }
 
 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)
+	users := ""
+	for _, u := range h.users {
+		users += fmt.Sprintf("    - user: %s\n      keys: [%q]\n", u, string(ssh.MarshalAuthorizedKey(h.signer.PublicKey())))
+	}
+	return fmt.Sprintf("sshd:\n  enabled: true\n  listen: %s\n  host_key: %s\n  authorized_users:\n%s%s",
+		h.listen, h.keyFile, users, h.extra)
 }
 
 // dial connects to wherever the server listens now, a restart on :0 moves it. hostKeyAlgorithms limits which host keys
@@ -134,15 +140,41 @@ func TestSSHDReload_UnchangedKeepsSessions(t *testing.T) {
 	assert.False(t, dropped(client, 500*time.Millisecond), "an unchanged sshd was restarted")
 }
 
+// Every key the server reads restarts it when it changes
 func TestSSHDReload_ChangedConfigRestarts(t *testing.T) {
+	cases := map[string]func(t *testing.T, h *sshdReloadHarness){
+		"listen":           func(t *testing.T, h *sshdReloadHarness) { h.listen = "localhost:0" },
+		"authorized_users": func(t *testing.T, h *sshdReloadHarness) { h.users = append(h.users, "other") },
+		"trusted_cas":      func(t *testing.T, h *sshdReloadHarness) { h.extra = "  trusted_cas: []\n" },
+		"host_key": func(t *testing.T, h *sshdReloadHarness) {
+			h.keyFile = filepath.Join(t.TempDir(), "other_host_key")
+			h.writeHostKey(t, newEd25519Key(t))
+		},
+	}
+	for name, change := range cases {
+		t.Run(name, func(t *testing.T) {
+			h := newSSHDReloadHarness(t)
+			client, _ := h.connect(t)
+
+			change(t, h)
+			require.NoError(t, h.c.ReloadConfigString(h.config()).Err)
+			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)
+		})
+	}
+}
+
+// sandbox_dir is only read at startup, restarting sshd for it would drop every session and still not apply it
+func TestSSHDReload_SandboxDirKeepsSessions(t *testing.T) {
 	h := newSSHDReloadHarness(t)
 	client, _ := h.connect(t)
 
-	h.extra = "  trusted_cas: []\n"
-	require.NoError(t, h.c.ReloadConfigString(h.config()).Err)
-	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)
+	h.extra = "  sandbox_dir: " + t.TempDir() + "\n"
+	res := h.c.ReloadConfigString(h.config())
+	require.NoError(t, res.Err)
+	assert.Equal(t, []string{"sshd.sandbox_dir"}, res.RestartKeys)
+	assert.False(t, dropped(client, 500*time.Millisecond), "sshd was restarted")
 }
 
 // The config can stay the same while the host key file it names changes. New connections get the new key, open