Commit `c0969481b0`

c0969481b067872df263c8f577ac712cefc7b682

parent: bf64c30c08

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-28 06:36 UTC

sshd: re-read the key per exec; revocation cuts its connections

Ref #256

Layout: unified · split

cmd/gitbayd/system.go +1 −1

	@@ -94,7 +94,7 @@ func shellCmd() *cobra.Command {
94	fmt.Fprintf(os.Stderr, "gitbay control plane: interactive shells are not available.\nTry: ssh <host> help\n")	94	fmt.Fprintf(os.Stderr, "gitbay control plane: interactive shells are not available.\nTry: ssh <host> help\n")
95	os.Exit(protocol.ExitUsage)	95	os.Exit(protocol.ExitUsage)
96	}	96	}
97	code := sshd.Exec(cfg, st, user, key.Scope, key.Fingerprint, control.ParseTerm(os.Getenv("GITBAY_TERM")), cmdline, os.Stdin, os.Stdout, os.Stderr, nil, nil)	97	code := sshd.Exec(cfg, st, user, key, control.ParseTerm(os.Getenv("GITBAY_TERM")), cmdline, os.Stdin, os.Stdout, os.Stderr, nil, nil, nil)
98	st.Close()	98	st.Close()
99	os.Exit(code)	99	os.Exit(code)
100	return nil	100	return nil

internal/sshd/revoke_test.go added +112

@@ -0,0 +1,112 @@
	1	package sshd
	2
	3	import (
	4	"bytes"
	5	"errors"
	6	"strings"
	7	"testing"
	8	"time"
	9
	10	"golang.org/x/crypto/ssh"
	11	)
	12
	13	// execStatus runs cmd on a new session and returns its exit status and
	14	// stderr; -1 when the session could not run.
	15	func execStatus(client *ssh.Client, cmd string) (int, string) {
	16	sess, err := client.NewSession()
	17	if err != nil {
	18	return -1, err.Error()
	19	}
	20	defer sess.Close()
	21	var stderr bytes.Buffer
	22	sess.Stderr = &stderr
	23	err = sess.Run(cmd)
	24	var exit *ssh.ExitError
	25	switch {
	26	case err == nil:
	27	return 0, stderr.String()
	28	case errors.As(err, &exit):
	29	return exit.ExitStatus(), stderr.String()
	30	}
	31	return -1, err.Error()
	32	}
	33
	34	// waitClosed fails unless the server closes the client's connection
	35	// within five seconds.
	36	func waitClosed(t testing.T, client ssh.Client) {
	37	t.Helper()
	38	done := make(chan struct{})
	39	go func() { client.Wait(); close(done) }()
	40	select {
	41	case <-done:
	42	case <-time.After(5 * time.Second):
	43	t.Fatal("the connection stayed open")
	44	}
	45	}
	46
	47	// Each exec reads the key again. The rows change behind the store's
	48	// back here, so no revocation is announced and the connection stays up:
	49	// what refuses the command is the per-exec check alone.
	50	func TestExecRevalidatesKey(t *testing.T) {
	51	ts := newTestServer(t)
	52	if code, errOut := execStatus(ts.client, "whoami"); code != 0 {
	53	t.Fatalf("whoami: %d %s", code, errOut)
	54	}
	55	if _, err := ts.st.DB.Exec("UPDATE ssh_keys SET scope = 'git' WHERE id = ?", ts.keyID); err != nil {
	56	t.Fatal(err)
	57	}
	58	if code, errOut := execStatus(ts.client, "whoami"); code != 4 \|\| !strings.Contains(errOut, "does not allow control commands") {
	59	t.Fatalf("whoami after re-scope: %d %q", code, errOut)
	60	}
	61	if _, err := ts.st.DB.Exec("DELETE FROM ssh_keys WHERE id = ?", ts.keyID); err != nil {
	62	t.Fatal(err)
	63	}
	64	if code, errOut := execStatus(ts.client, "whoami"); code != 4 \|\| !strings.Contains(errOut, "no longer registered") {
	65	t.Fatalf("whoami after delete: %d %q", code, errOut)
	66	}
	67	}
	68
	69	// Removing the key cuts the connection, ending a command running on it.
	70	func TestRemoveKeyCutsConnection(t *testing.T) {
	71	ts := newTestServer(t)
	72	withBuild(t, ts)
	73	var stderr bytes.Buffer
	74	sess := startFollow(t, ts.client, &stderr)
	75	if err := ts.st.RemoveSSHKey(ts.uid, ts.fp); err != nil {
	76	t.Fatal(err)
	77	}
	78	waited := make(chan error, 1)
	79	go func() { waited <- sess.Wait() }()
	80	select {
	81	case err := <-waited:
	82	if err == nil {
	83	t.Fatal("the follow exited cleanly after its key was removed")
	84	}
	85	case <-time.After(5 * time.Second):
	86	t.Fatal("the follow outlived its key")
	87	}
	88	waitClosed(t, ts.client)
	89	}
	90
	91	func TestDisableCutsConnection(t *testing.T) {
	92	ts := newTestServer(t)
	93	if err := ts.st.SetUserDisabled(ts.uid, true); err != nil {
	94	t.Fatal(err)
	95	}
	96	waitClosed(t, ts.client)
	97	}
	98
	99	// A revocation made by another process is not announced here; the
	100	// sweep finds it. A live key survives the sweep.
	101	func TestSweepCutsOutOfProcessRevocation(t *testing.T) {
	102	ts := newTestServer(t)
	103	ts.srv.sweepOnce()
	104	if code, errOut := execStatus(ts.client, "whoami"); code != 0 {
	105	t.Fatalf("the sweep cut a live key: %d %s", code, errOut)
	106	}
	107	if _, err := ts.st.DB.Exec("UPDATE users SET disabled = 1 WHERE id = ?", ts.uid); err != nil {
	108	t.Fatal(err)
	109	}
	110	ts.srv.sweepOnce()
	111	waitClosed(t, ts.client)
	112	}

internal/sshd/sshd.go +121 −17

 	"fmt"
 	"io"
 	"log/slog"
+	"maps"
 	"net"
 	"os"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"sync"
 	"sync/atomic"
 type conn struct {
 	net    net.Conn
 	active atomic.Int32
+	// keyID and userID are the key that authenticated the connection and
+	// its account: 0 before the handshake and for an unregistered key.
+	// Guarded by Server.mu.
+	keyID, userID int64
+	revoked       chan struct{} // closed by cut
+	cutOnce       sync.Once
+}
+// cut ends the connection because its key was revoked: a git transport
+// on it is killed, and every other command loses its channel.
+func (c *conn) cut() {
+	c.cutOnce.Do(func() { close(c.revoked) })
+	c.net.Close()
 }
 func New(cfg config.Config, st *store.Store) (*Server, error) {
 		sc.AddHostKey(sg)
 	}
 	s.sshCfg = sc
+	st.OnRevoke(s.revoke)
 	return s, nil
 }
 	return &ssh.Permissions{Extensions: map[string]string{
 		"user-id": strconv.FormatInt(key.UserID, 10),
 		"key-id":  strconv.FormatInt(key.ID, 10),
-		"key-fp":  fp,
-		"scope":   key.Scope,
 	}}, nil
 }
 // Serve accepts connections on ln until it is closed.
 func (s *Server) Serve(ln net.Listener) error {
+	served := make(chan struct{})
+	defer close(served)
+	go s.sweep(served)
 	for {
 		nc, err := ln.Accept()
 		if err != nil {
 			return err
 		}
-		c := &conn{net: nc}
+		c := &conn{net: nc, revoked: make(chan struct{})}
 		s.mu.Lock()
 		s.conns[c] = struct{}{}
 		s.mu.Unlock()
 	}
 }
+// revoke closes the connections opened by the keys r names.
+func (s *Server) revoke(r store.Revoked) {
+	var cut []*conn
+	s.mu.Lock()
+	for c := range s.conns {
+		if c.keyID == 0 {
+			continue
+		}
+		if (r.UserID != 0 && c.userID == r.UserID) || slices.Contains(r.KeyIDs, c.keyID) {
+			cut = append(cut, c)
+		}
+	}
+	s.mu.Unlock()
+	for _, c := range cut {
+		c.cut()
+	}
+}
+// sweepInterval bounds how long a revocation this process was not told
+// about (gitbayd admin on the host) leaves a connection open.
+const sweepInterval = 15 * time.Second
+func (s *Server) sweep(served <-chan struct{}) {
+	t := time.NewTicker(sweepInterval)
+	defer t.Stop()
+	for {
+		select {
+		case <-t.C:
+			s.sweepOnce()
+		case <-served:
+			return
+		case <-s.stopping:
+			return
+		}
+	}
+}
+// sweepOnce cuts every connection whose key is no longer live. Only
+// connections whose key was asked about are judged: one that
+// authenticated while the query ran waits for the next sweep.
+func (s *Server) sweepOnce() {
+	asked := map[int64]bool{}
+	s.mu.Lock()
+	for c := range s.conns {
+		if c.keyID != 0 {
+			asked[c.keyID] = true
+		}
+	}
+	s.mu.Unlock()
+	if len(asked) == 0 {
+		return
+	}
+	live, err := s.st.LiveSSHKeys(slices.Collect(maps.Keys(asked)))
+	if err != nil {
+		slog.Error("ssh sweep: key lookup", "err", err)
+		return
+	}
+	var cut []*conn
+	s.mu.Lock()
+	for c := range s.conns {
+		if asked[c.keyID] && !live[c.keyID] {
+			cut = append(cut, c)
+		}
+	}
+	s.mu.Unlock()
+	for _, c := range cut {
+		c.cut()
+	}
+}
 // Stop ends the commands that run until something happens (build log
 // --follow), so a shutdown drain waits only for work that finishes. It
 // does not close connections; Shutdown does.
 		return
 	}
 	defer sconn.Close()
+	ext := sconn.Permissions.Extensions
+	s.mu.Lock()
+	c.keyID, _ = strconv.ParseInt(ext["key-id"], 10, 64)
+	c.userID, _ = strconv.ParseInt(ext["user-id"], 10, 64)
+	s.mu.Unlock()
 	go ssh.DiscardRequests(reqs)
 	for newCh := range chans {
 		c.active.Add(1)
 		go func() {
 			defer c.active.Add(-1)
-			s.handleSession(sconn, ch, chReqs)
+			s.handleSession(c, sconn, ch, chReqs)
 		}()
 	}
 }
-func (s *Server) handleSession(sconn *ssh.ServerConn, ch ssh.Channel, reqs <-chan *ssh.Request) {
+func (s *Server) handleSession(c *conn, sconn *ssh.ServerConn, ch ssh.Channel, reqs <-chan *ssh.Request) {
 	defer ch.Close()
 	var term control.Term
 	for req := range reqs {
 				}
 				close(done)
 			}()
-			code := s.runExec(sconn, ch, term, payload.Command, done)
+			code := s.runExec(c, sconn, ch, term, payload.Command, done)
 			sendExit(ch, code)
 			return
 		case "shell":
 	ch.SendRequest("exit-status", false, ssh.Marshal(&msg))
 }
-func (s *Server) runExec(sconn *ssh.ServerConn, ch ssh.Channel, term control.Term, cmdline string, done <-chan struct{}) int {
+func (s *Server) runExec(c *conn, sconn *ssh.ServerConn, ch ssh.Channel, term control.Term, cmdline string, done <-chan struct{}) int {
 	ext := sconn.Permissions.Extensions
 	if blob := ext["anon-key"]; blob != "" {
 		return s.runAnonymous(ch, blob, cmdline)
 	}
 	userID, _ := strconv.ParseInt(ext["user-id"], 10, 64)
 	keyID, _ := strconv.ParseInt(ext["key-id"], 10, 64)
+	// A connection outlives its commands, so the key is read again for
+	// each one: what it may do is what it may do now (#256).
+	key, err := s.st.SSHKeyByID(keyID)
+	if errors.Is(err, store.ErrNotFound) || (err == nil && key.UserID != userID) {
+		fmt.Fprintln(ch.Stderr(), "this key is no longer registered")
+		return protocol.ExitDenied
+	}
+	if err != nil {
+		slog.Error("ssh exec: key lookup", "err", err)
+		fmt.Fprintln(ch.Stderr(), "authentication temporarily unavailable")
+		return protocol.ExitFailure
+	}
 	user, err := s.st.UserByID(userID)
 	if err != nil {
 		fmt.Fprintln(ch.Stderr(), "account no longer exists")
 		return protocol.ExitDenied
 	}
 	_ = s.st.TouchSSHKey(keyID)
-	return Exec(s.cfg, s.st, user, ext["scope"], ext["key-fp"], term, cmdline, ch, ch, ch.Stderr(), done, s.stopping)
+	return Exec(s.cfg, s.st, user, key, term, cmdline, ch, ch, ch.Stderr(), done, s.stopping, c.revoked)
 }
 // runAnonymous handles a session from an unregistered key: the register
 // Exec runs one SSH exec command line for an authenticated key. It is the
 // single dispatch path shared by the embedded listener and the system-sshd
-// forced command (gitbayd shell).
+// forced command (gitbayd shell). Closing revoked kills a git transport.
-func Exec(cfg config.Config, st *store.Store, user store.User, scope, source string, term control.Term, cmdline string,
+func Exec(cfg config.Config, st *store.Store, user store.User, key store.SSHKey, term control.Term, cmdline string,
-	stdin io.Reader, stdout, stderr io.Writer, done, stopping <-chan struct{}) int {
+	stdin io.Reader, stdout, stderr io.Writer, done, stopping, revoked <-chan struct{}) int {
 	if user.Disabled {
 		fmt.Fprintln(stderr, "this account is disabled; contact the instance admin")
 		return protocol.ExitDenied
 				fmt.Fprintln(stderr, "your account is not active yet: verify your email first")
 				return protocol.ExitDenied
 			}
-			return runGit(cfg, st, user, scope, argv, stdin, stdout, stderr)
+			return runGit(cfg, st, user, key.Scope, argv, stdin, stdout, stderr, revoked)
 		case "git-lfs-authenticate":
 			// Part of the git transport, not the control plane: usable by
 			// git-scoped and deploy keys, with the transports' access rules.
 				fmt.Fprintln(stderr, "your account is not active yet: verify your email first")
 				return protocol.ExitDenied
 			}
-			return runLFSAuthenticate(cfg, st, user, scope, argv, stdout, stderr)
+			return runLFSAuthenticate(cfg, st, user, key.Scope, argv, stdout, stderr)
 		}
 	}
 	ctx := &control.Ctx{
 		User:     user,
-		Scope:    scope,
+		Scope:    key.Scope,
-		Source:   source,
+		Source:   key.Fingerprint,
 		Term:     term,
 		Store:    st,
 		Cfg:      cfg,
 // runGit streams a git transport service after access checks.
 func runGit(cfg config.Config, st *store.Store, user store.User, scope string, argv []string,
-	stdin io.Reader, stdout, stderr io.Writer) int {
+	stdin io.Reader, stdout, stderr io.Writer, revoked <-chan struct{}) int {
 	service := argv[0]
 	if len(argv) != 2 {
 		fmt.Fprintf(stderr, "usage: %s <path>\n", service)
 			}
 		}
 	}
-	if err := gitutil.Transport(service, dir, stdin, stdout, stderr, env, maxPack, nil); err != nil {
+	if err := gitutil.Transport(service, dir, stdin, stdout, stderr, env, maxPack, revoked); err != nil {
 		return protocol.ExitFailure
 	}
 	return protocol.ExitOK

internal/sshd/sshd_test.go +46 −17

 	"gitbay.org/gitbay/internal/store"
 )
-// followServer starts an embedded server holding alice, her public repo
+// testServer is an embedded server over a fresh store holding alice
-// alice/app and a queued build 1 whose log has one line, and returns it
+// with one full-scope key, and a client connected with that key.
-// with a client connected as alice.
+type testServer struct {
-func followServer(t *testing.T) (*Server, *ssh.Client) {
+	srv    *Server
+	st     *store.Store
+	client *ssh.Client
+	uid    int64
+	keyID  int64
+	fp     string
+}
+func newTestServer(t *testing.T) testServer {
 	t.Helper()
 	root := t.TempDir()
 	st, err := store.Open(filepath.Join(root, "gitbay.db"))
 	if err != nil {
 		t.Fatal(err)
 	}
-	repoID, err := st.CreateRepo("user", uid, "app", "public")
-	if err != nil {
-		t.Fatal(err)
-	}
-	id, err := st.CreateBuild(repoID, "unit", "abc", "main", `["true"]`, "", "", true)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if err := st.AppendBuildLog(id, []byte("queued\n")); err != nil {
-		t.Fatal(err)
-	}
 	_, priv, err := ed25519.GenerateKey(rand.Reader)
 	if err != nil {
 		t.Fatal(err)
 		t.Fatal(err)
 	}
 	pub := signer.PublicKey()
-	if err := st.AddSSHKey(uid, ssh.FingerprintSHA256(pub), pub.Type(), pub.Marshal(), "full", "test"); err != nil {
+	fp := ssh.FingerprintSHA256(pub)
+	if err := st.AddSSHKey(uid, fp, pub.Type(), pub.Marshal(), "full", "test"); err != nil {
+		t.Fatal(err)
+	}
+	key, err := st.SSHKeyByFingerprint(fp)
+	if err != nil {
 		t.Fatal(err)
 	}
 		t.Fatal(err)
 	}
 	t.Cleanup(func() { client.Close() })
-	return srv, client
+	return testServer{srv: srv, st: st, client: client, uid: uid, keyID: key.ID, fp: fp}
+}
+// withBuild gives alice the public repo alice/app and a queued build 1
+// whose log has one line.
+func withBuild(t *testing.T, ts testServer) {
+	t.Helper()
+	repoID, err := ts.st.CreateRepo("user", ts.uid, "app", "public")
+	if err != nil {
+		t.Fatal(err)
+	}
+	id, err := ts.st.CreateBuild(repoID, "unit", "abc", "main", `["true"]`, "", "", true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := ts.st.AppendBuildLog(id, []byte("queued\n")); err != nil {
+		t.Fatal(err)
+	}
+}
+// followServer starts an embedded server holding alice, her public repo
+// alice/app and a queued build 1 whose log has one line, and returns it
+// with a client connected as alice.
+func followServer(t *testing.T) (*Server, *ssh.Client) {
+	t.Helper()
+	ts := newTestServer(t)
+	withBuild(t, ts)
+	return ts.srv, ts.client
 }
 // startFollow runs build log --follow on a new session and returns once

Commit c0969481b0

Commit `c0969481b0`