Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/cli/broker_dial_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
23 changes: 16 additions & 7 deletions internal/cli/broker_dial_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -125,5 +134,5 @@ func newBrokerHTTPClient(credFD int) *http.Client {
IdleConnTimeout: 90 * time.Second,
ResponseHeaderTimeout: 0,
},
}
}, nil
}
46 changes: 42 additions & 4 deletions internal/cli/broker_dial_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions internal/cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading