Merge pull request #211 from flashcatcloud/fix/broker-fd-preflight

fix(cli): fail fast when the broker credential fd is not inherited

Author: ysyneu

internal/cli/broker_dial_other.go

@@ -7,7 +7,7 @@ import (
 	"net/http"
 )
 
-func newBrokerHTTPClient(int) *http.Client { return nil }
+func newBrokerHTTPClient(int) (*http.Client, error) { return nil, errBrokerUnsupported }
 
 var errBrokerUnsupported = errors.New("flashduty: broker mode is not supported on this platform")
 

internal/cli/broker_dial_unix.go

@@ -14,11 +14,6 @@ import (
 	"time"
 )
 
-// errBrokerUnsupported is returned when broker mode is requested on a build that
-// cannot provide it. On unix this is effectively unreachable (newBrokerHTTPClient
-// never returns nil), but defaultNewClient references it on every platform.
-var errBrokerUnsupported = errors.New("flashduty: broker mode is not supported on this platform")
-
 // errBrokerClosed is returned (wrapped) when the runner-side broker control
 // channel is gone: the runner exited, or reclaimed the channel once the
 // command that started this process finished, so fduty calls from a
@@ -110,7 +105,21 @@ func (d *brokerDialer) dial(_ context.Context, _, _ string) (net.Conn, error) {
 // every connection over the inherited control fd. Timeout matches the SDK's
 // historical default (30s) so behavior is unchanged for non-streaming calls;
 // streaming export relies on request context like before.
-func newBrokerHTTPClient(credFD int) *http.Client {
+//
+// It first checks that credFD is an open socket in this process. The runner
+// hands the control end to bash, and only processes that inherit fd credFD
+// reach fduty with it intact: Python's subprocess (close_fds=True by default),
+// Node's child_process and sudo all close it. Without the check that surfaces
+// as a handshake EBADF/ENOTSOCK at the first request, after the SDK's URL
+// prefix, with nothing saying how to fix it.
+func newBrokerHTTPClient(credFD int) (*http.Client, error) {
+	if _, err := syscall.GetsockoptInt(credFD, syscall.SOL_SOCKET, syscall.SO_TYPE); err != nil {
+		return nil, fmt.Errorf("FLASHDUTY_CRED_FD=%d is not an open socket in this process (%v): "+
+			"the program that started fduty did not pass the credential channel down. "+
+			"Run fduty from the shell, or keep fd %d open when spawning it: "+
+			"Python subprocess.run(cmd, pass_fds=(%d,)); Node: set entry %d of spawn's stdio array to %d",
+			credFD, err, credFD, credFD, credFD, credFD)
+	}
 	d := &brokerDialer{credFD: credFD}
 	return &http.Client{
 		Timeout: 30 * time.Second,
@@ -125,5 +134,5 @@ func newBrokerHTTPClient(credFD int) *http.Client {
 			IdleConnTimeout:       90 * time.Second,
 			ResponseHeaderTimeout: 0,
 		},
-	}
+	}, nil
 }

internal/cli/broker_dial_unix_test.go

@@ -106,9 +106,9 @@ func TestBrokerHTTPClient_DialAndRewrite(t *testing.T) {
 	defer func() { _ = syscall.Close(childFD) }()
 	defer stop()
 
-	client := newBrokerHTTPClient(childFD)
-	if client == nil {
-		t.Fatal("newBrokerHTTPClient returned nil")
+	client, err := newBrokerHTTPClient(childFD)
+	if err != nil {
+		t.Fatalf("newBrokerHTTPClient: %v", err)
 	}
 	defer client.CloseIdleConnections() // release dispatched keep-alive conns
 	// The CLI's base URL is an http placeholder; broker rewrites host.
@@ -208,6 +208,41 @@ func TestDefaultNewClient_RejectsStdioFD(t *testing.T) {
 	}
 }
 
+// TestDefaultNewClient_CredFDNotInherited covers a caller that drops the
+// inherited control fd before exec (Python's subprocess closes fds >= 3 by
+// default): the fd number is then closed, or reused by an unrelated file.
+// Either way defaultNewClient must fail up front with an error that names the
+// fd and the fix, instead of a handshake errno at the first request.
+func TestDefaultNewClient_CredFDNotInherited(t *testing.T) {
+	t.Setenv("HOME", t.TempDir())
+	t.Setenv("FLASHDUTY_APP_KEY", "")
+
+	f, err := os.CreateTemp(t.TempDir(), "not-a-socket")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = f.Close() }()
+	pair, err := syscall.Socketpair(syscall.AF_UNIX, controlSockType, 0)
+	if err != nil {
+		t.Fatalf("socketpair: %v", err)
+	}
+	closedFD := pair[0]
+	_ = syscall.Close(pair[0])
+	_ = syscall.Close(pair[1])
+
+	for name, fd := range map[string]int{"closed fd": closedFD, "regular file": int(f.Fd())} {
+		t.Setenv("FLASHDUTY_CRED_FD", strconv.Itoa(fd))
+		_, err := defaultNewClient()
+		if err == nil {
+			t.Fatalf("%s: defaultNewClient must fail", name)
+		}
+		want := "FLASHDUTY_CRED_FD=" + strconv.Itoa(fd) + " is not an open socket"
+		if msg := err.Error(); !strings.HasPrefix(msg, want) || !strings.Contains(msg, "pass_fds=("+strconv.Itoa(fd)+",)") {
+			t.Fatalf("%s: error must start with %q and name pass_fds, got: %v", name, want, msg)
+		}
+	}
+}
+
 // TestBrokerHTTPClient_RefusedReturnsError verifies the dialer surfaces the
 // broker's 0xFF refusal (e.g. the runner failed to mint a connection) as a real
 // error instead of hanging or wrapping a nil conn.
@@ -232,7 +267,10 @@ func TestBrokerHTTPClient_RefusedReturnsError(t *testing.T) {
 		}
 	}()
 
-	client := newBrokerHTTPClient(childFD)
+	client, err := newBrokerHTTPClient(childFD)
+	if err != nil {
+		t.Fatalf("newBrokerHTTPClient: %v", err)
+	}
 	req, _ := http.NewRequestWithContext(context.Background(), "GET",
 		"http://flashduty.broker.local/x?app_key=SENTINEL", nil)
 	if _, err := client.Do(req); err == nil {

internal/cli/root.go

@@ -234,9 +234,9 @@ func defaultNewClient() (*flashduty.Client, error) {
 		if perr != nil || fd < 3 {
 			return nil, fmt.Errorf("invalid FLASHDUTY_CRED_FD=%q", fdStr)
 		}
-		hc := newBrokerHTTPClient(fd)
-		if hc == nil {
-			return nil, errBrokerUnsupported
+		hc, err := newBrokerHTTPClient(fd)
+		if err != nil {
+			return nil, err
 		}
 		opts = append(opts, flashduty.WithHTTPClient(hc))
 		appKey = "broker-sentinel" // non-empty: go-flashduty rejects ""; broker overwrites it