fix(cli): take turns with the security key across commands

docs/src/content/docs/user-guide/cli/mfa-verification.mdx

@@ -51,6 +51,10 @@ The CLI supports multiple MFA methods:
     you don't miss one while the terminal is out of sight. On macOS, allow notifications for Script Editor if they
     don't appear. To turn them off, set `"touch_notifications": false` in `~/.config/rack-gateway/config.json`.
 
+    Commands take turns with the key. When two run at once (say, two AI agents each running a command), the second
+    prints `Waiting for another rack-gateway command to finish with the security key...` and asks for its PIN and
+    touch only once the first has its touch, so each prompt is for one command.
+
     Only security keys marked **CLI Compatible** in the web UI can be used from the CLI.
 
     <Aside type="note">

internal/cli/config_lock.go

@@ -1,8 +1,9 @@
 package cli
 
 import (
-	"os"
 	"path/filepath"
+
+	"github.com/DocSpring/rack-gateway/internal/cli/filelock"
 )
 
 // updateConfig applies change to the latest config and saves it, holding the config lock throughout, so two
@@ -36,16 +37,5 @@ func lockConfig() (func(), error) {
 	if err := ensurePrivateDir(ConfigPath); err != nil {
 		return nil, err
 	}
-	file, err := os.OpenFile(filepath.Join(filepath.Clean(ConfigPath), "config.lock"), os.O_CREATE|os.O_RDWR, 0o600)
-	if err != nil {
-		return nil, err
-	}
-	if err := lockFile(file); err != nil {
-		_ = file.Close()
-		return nil, err
-	}
-	return func() {
-		_ = unlockFile(file)
-		_ = file.Close()
-	}, nil
+	return filelock.Lock(filepath.Join(filepath.Clean(ConfigPath), "config.lock"), nil)
 }

internal/cli/filelock/filelock.go

@@ -0,0 +1,37 @@
+// Package filelock takes exclusive locks that several rack-gateway commands share: a lock file that one process
+// at a time holds. The operating system releases a lock when its process exits, so a command that crashes or is
+// killed never leaves one behind.
+package filelock
+
+import (
+	"os"
+	"path/filepath"
+)
+
+// Lock takes the exclusive lock on the file at path, creating it (and its directory, private to the user) when it
+// doesn't exist. When another process holds the lock, it calls waiting, then waits until it can take it. The
+// returned func releases the lock.
+func Lock(path string, waiting func()) (func(), error) {
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		return nil, err
+	}
+	file, err := os.OpenFile(filepath.Clean(path), os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return nil, err
+	}
+	locked, err := tryLockFile(file)
+	if err == nil && !locked {
+		if waiting != nil {
+			waiting()
+		}
+		err = lockFile(file)
+	}
+	if err != nil {
+		_ = file.Close()
+		return nil, err
+	}
+	return func() {
+		_ = unlockFile(file)
+		_ = file.Close()
+	}, nil
+}

internal/cli/filelock/filelock_test.go

@@ -0,0 +1,48 @@
+package filelock
+
+import (
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// A second Lock on the same file waits, saying so, until the first is released. (Each Lock opens the file again,
+// so two in one process conflict as two processes do.)
+func TestLockWaitsForTheHolder(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "locks", "key.lock")
+	unlockFirst, err := Lock(path, func() { t.Error("nothing holds the lock yet") })
+	require.NoError(t, err)
+
+	type result struct {
+		unlock func()
+		err    error
+	}
+	waiting := make(chan struct{})
+	acquired := make(chan result, 1)
+	go func() {
+		unlock, err := Lock(path, func() { close(waiting) })
+		acquired <- result{unlock, err}
+	}()
+
+	select {
+	case second := <-acquired:
+		require.NoError(t, second.err)
+		t.Fatal("the second Lock took the lock while the first held it")
+	case <-waiting:
+	}
+	select {
+	case <-acquired:
+		t.Fatal("the second Lock took the lock while the first held it")
+	case <-time.After(200 * time.Millisecond):
+	}
+	unlockFirst()
+	second := <-acquired
+	require.NoError(t, second.err)
+	second.unlock()
+
+	unlockThird, err := Lock(path, func() { t.Error("the lock was released") })
+	require.NoError(t, err)
+	unlockThird()
+}

internal/cli/filelock/filelock_unix.go

@@ -0,0 +1,27 @@
+//go:build !windows
+
+package filelock
+
+import (
+	"errors"
+	"os"
+
+	"golang.org/x/sys/unix"
+)
+
+func lockFile(file *os.File) error {
+	return unix.Flock(int(file.Fd()), unix.LOCK_EX)
+}
+
+// tryLockFile takes the lock if no other process holds it, and reports whether it did.
+func tryLockFile(file *os.File) (bool, error) {
+	err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB)
+	if errors.Is(err, unix.EWOULDBLOCK) {
+		return false, nil
+	}
+	return err == nil, err
+}
+
+func unlockFile(file *os.File) error {
+	return unix.Flock(int(file.Fd()), unix.LOCK_UN)
+}

internal/cli/filelock/filelock_windows.go

@@ -0,0 +1,33 @@
+//go:build windows
+
+package filelock
+
+import (
+	"errors"
+	"math"
+	"os"
+
+	"golang.org/x/sys/windows"
+)
+
+func lockFile(file *os.File) error {
+	return lockFileEx(file, windows.LOCKFILE_EXCLUSIVE_LOCK)
+}
+
+// tryLockFile takes the lock if no other process holds it, and reports whether it did.
+func tryLockFile(file *os.File) (bool, error) {
+	err := lockFileEx(file, windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY)
+	if errors.Is(err, windows.ERROR_LOCK_VIOLATION) {
+		return false, nil
+	}
+	return err == nil, err
+}
+
+func lockFileEx(file *os.File, flags uint32) error {
+	return windows.LockFileEx(windows.Handle(file.Fd()), flags, 0, math.MaxUint32, math.MaxUint32,
+		&windows.Overlapped{})
+}
+
+func unlockFile(file *os.File) error {
+	return windows.UnlockFileEx(windows.Handle(file.Fd()), 0, math.MaxUint32, math.MaxUint32, &windows.Overlapped{})
+}

internal/cli/webauthn/key_lock.go

@@ -0,0 +1,42 @@
+//go:build !nofido
+
+package webauthn
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+
+	"github.com/DocSpring/rack-gateway/internal/cli/filelock"
+)
+
+// keyLockPath is the lock every rack-gateway command holds while it uses a security key. It's in the user's cache
+// directory, which every command shares. A temporary directory isn't: each agent session can have a TMPDIR of its
+// own, so commands there would use the key at once.
+var keyLockPath = func() (string, error) {
+	dir, err := os.UserCacheDir()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(dir, "rack-gateway", "security-key.lock"), nil
+}
+
+// withKeyLock runs use while holding the security key lock, waiting for any other rack-gateway command that holds
+// it. A key answers one command at a time: two commands asking for a PIN and a touch at once (two agents running
+// commands, say) both fail, with libfido2's "rx error" or "unsupported option", and the person can't tell which
+// touch is for which. A command holds the lock from its PIN dialog to its touch, so the next one asks only then.
+func withKeyLock[T any](use func() (T, error)) (T, error) {
+	var zero T
+	path, err := keyLockPath()
+	if err != nil {
+		return zero, fmt.Errorf("failed to find where to lock the security key: %w", err)
+	}
+	unlock, err := filelock.Lock(path, func() {
+		fmt.Fprintln(os.Stderr, "Waiting for another rack-gateway command to finish with the security key...")
+	})
+	if err != nil {
+		return zero, fmt.Errorf("failed to lock the security key for this command: %w", err)
+	}
+	defer unlock()
+	return use()
+}

internal/cli/webauthn/key_lock_test.go

@@ -0,0 +1,52 @@
+//go:build !nofido
+
+package webauthn
+
+import (
+	"errors"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/keys-pub/go-libfido2"
+	"github.com/stretchr/testify/require"
+
+	"github.com/DocSpring/rack-gateway/internal/cli/filelock"
+)
+
+// Each way a command uses the security key holds the lock, so a command waits for another that's using the key
+// rather than both failing. (No key is opened: the test replaces opening one.)
+func TestSecurityKeyUsersWaitForTheLock(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "security-key.lock")
+	lockPath, open := keyLockPath, openDevices
+	t.Cleanup(func() { keyLockPath, openDevices = lockPath, open })
+	keyLockPath = func() (string, error) { return path, nil }
+	errNoKey := errors.New("no security key in tests")
+	openDevices = func() ([]*libfido2.Device, error) { return nil, errNoKey }
+
+	for name, use := range map[string]func() error{
+		"assertion": func() error {
+			_, _, err := GetAssertionWithCachedPIN(AssertionOptions{}, "1234")
+			return err
+		},
+		"PIN check": func() error { return CheckPIN("1234") },
+		"PIN query": func() error {
+			if SingleKeyHasPIN() {
+				return errors.New("SingleKeyHasPIN found a PIN without a key")
+			}
+			return errNoKey
+		},
+	} {
+		unlock, err := filelock.Lock(path, nil)
+		require.NoError(t, err)
+		done := make(chan error, 1)
+		go func() { done <- use() }()
+		select {
+		case <-done:
+			t.Fatalf("the %s used the key while another command held the lock", name)
+		case <-time.After(200 * time.Millisecond):
+		}
+		unlock()
+		require.ErrorIs(t, <-done, errNoKey, name)
+	}
+}

internal/cli/webauthn/pin_check.go

@@ -22,24 +22,32 @@ type pinKey interface {
 
 // SingleKeyHasPIN reports whether exactly one security key is connected and it has a PIN set. Its PIN can then be
 // asked for before the gateway sends a challenge: it's the key that will sign it. With several keys connected, the
-// key to use is only known from the challenge's credentials.
+// key to use is only known from the challenge's credentials. It holds the security key lock (see withKeyLock).
 func SingleKeyHasPIN() bool {
-	keys, err := openPINKeys()
-	return err == nil && onlyKeyHasPIN(keys)
+	hasPIN, err := withKeyLock(func() (bool, error) {
+		keys, err := openPINKeys()
+		return err == nil && onlyKeyHasPIN(keys), nil
+	})
+	return err == nil && hasPIN
 }
 
 // CheckPIN checks pin on the one connected security key, without a touch. A PIN the key refuses returns an error
 // wrapping ErrWrongPIN, which says how many tries are left. A key that can't check a PIN this way (it has no
-// credential management) returns nil, and the assertion checks the PIN.
+// credential management) returns nil, and the assertion checks the PIN. It holds the security key lock (see
+// withKeyLock).
 func CheckPIN(pin string) error {
-	keys, err := openPINKeys()
-	if err != nil {
-		return err
-	}
-	if len(keys) != 1 {
-		return fmt.Errorf("%d security keys are connected: connect only the one you use with rack-gateway", len(keys))
-	}
-	return verifyPIN(keys[0], pin)
+	_, err := withKeyLock(func() (struct{}, error) {
+		keys, err := openPINKeys()
+		if err != nil {
+			return struct{}{}, err
+		}
+		if len(keys) != 1 {
+			return struct{}{}, fmt.Errorf("%d security keys are connected: connect only the one you use with "+
+				"rack-gateway", len(keys))
+		}
+		return struct{}{}, verifyPIN(keys[0], pin)
+	})
+	return err
 }
 
 func openPINKeys() ([]pinKey, error) {

internal/cli/webauthn/webauthn.go

@@ -25,7 +25,20 @@ func GetAssertion(options AssertionOptions) (*AssertionResponse, error) {
 // GetAssertionWithCachedPIN authenticates with FIDO2 device, optionally using a cached PIN.
 // Returns the assertion response and the PIN used (for caching in subsequent calls).
 // If cachedPIN is empty, asks for the PIN when the key has one. Otherwise uses the cached PIN.
+// It holds the security key lock throughout (see withKeyLock).
 func GetAssertionWithCachedPIN(options AssertionOptions, cachedPIN string) (*AssertionResponse, string, error) {
+	var pin string
+	resp, err := withKeyLock(func() (*AssertionResponse, error) {
+		var resp *AssertionResponse
+		var err error
+		resp, pin, err = requestAssertion(options, cachedPIN)
+		return resp, err
+	})
+	return resp, pin, err
+}
+
+// requestAssertion is GetAssertionWithCachedPIN, without the lock.
+func requestAssertion(options AssertionOptions, cachedPIN string) (*AssertionResponse, string, error) {
 	allowList, err := decodeCredentialIDs(options.AllowCredentials)
 	if err != nil {
 		return nil, "", err
@@ -67,8 +80,8 @@ func openFIDODevices() ([]securityKey, error) {
 	return keys, nil
 }
 
-// openDevices opens every connected FIDO2 device that can be opened.
-func openDevices() ([]*libfido2.Device, error) {
+// openDevices opens every connected FIDO2 device that can be opened. Tests replace it.
+var openDevices = func() ([]*libfido2.Device, error) {
 	locs, err := libfido2.DeviceLocations()
 	if err != nil {
 		return nil, fmt.Errorf("failed to list FIDO2 devices: %w", err)