From 5ba7aff4b3996179e9f9d7cd8187ecb9940bfb0b Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Mon, 12 Oct 2026 17:56:51 -0400 Subject: [PATCH] ssh: Only restart sshd on reload when its config or host key changed (#1923) --- ssh.go | 46 ++++++++--- sshd/server.go | 61 +++++++++++--- sshd_reload_test.go | 196 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 277 insertions(+), 26 deletions(-) create mode 100644 sshd_reload_test.go diff --git a/ssh.go b/ssh.go index 2a440d98..34b5c3a7 100644 --- a/ssh.go +++ b/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) diff --git a/sshd/server.go b/sshd/server.go index e0ee9364..b5e1aa8f 100644 --- a/sshd/server.go +++ b/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) } } diff --git a/sshd_reload_test.go b/sshd_reload_test.go new file mode 100644 index 00000000..8226fd09 --- /dev/null +++ b/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) +}