From 818a879f6bc56f4f3376f5b9015a36bda247f86b Mon Sep 17 00:00:00 2001 From: Chris Thomas Date: Wed, 26 Aug 2026 12:53:16 +0200 Subject: [PATCH] Let callers supply their own dialer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Config.DialFunc. When set, it opens every TCP connection the client makes — control connections and data connections alike — in place of the package's own dialer. It exists for callers who need the connection itself rather than only what travels over it: counting bytes for transfer statistics, routing through a proxy or a tunnel, or returning something that is not really a socket. Today there is no way to reach the connection, so those callers cannot use this package at all. Both dial sites now funnel through one Config.dial helper, which is what makes the data connections reachable too. That is the part worth having: a caller who supplied a dialer to count bytes and only saw the control connection would miss every transfer, which is nearly all the bytes there are, and would have no way to tell. Two things become the caller's responsibility, documented on the field because neither is guessable: - Config.Timeout no longer applies to the dial. It still governs reads and writes on the returned connection. - In TLSImplicit mode the returned connection is wrapped in TLS by this package, so DialFunc should return a plain connection. The handshake happens at dial time rather than on first read, matching what tls.DialWithDialer does. Leaving DialFunc nil keeps the previous behaviour exactly, including the timeout. ## Running the tests without the test servers The new tests need no server: both call sites go through Config.dial, so exercising that covers them by construction. But the package's TestMain stops with log.Fatal when ./build_test_server.sh has not been run, so on a machine that cannot build pure-ftpd and proftpd no test runs at all — including the ones that do not care. GOFTP_SKIP_SERVERS now skips the four tests that need a live server and runs the rest. Skipping is on the tests themselves rather than the runner, so they report as skipped with the reason rather than failing on a connection refused, which reads like a bug in the package instead of a missing prerequisite. --- client.go | 48 ++++++++++++++++++-- client_test.go | 2 + dial_func_test.go | 96 ++++++++++++++++++++++++++++++++++++++++ file_system_test.go | 2 + goftp.go | 11 +++++ main_test.go | 22 +++++++++ persistent_connection.go | 2 +- 7 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 dial_func_test.go diff --git a/client.go b/client.go index 779cd5e..a9c5c0a 100644 --- a/client.go +++ b/client.go @@ -149,6 +149,29 @@ type Config struct { // hung connections. DisableEPSV bool + // DialFunc, when set, is used to establish every TCP connection this + // client makes - control connections and data connections alike - + // in place of the package's own dialer. + // + // It exists for callers who need to see the connection itself rather + // than only what travels over it: counting bytes for transfer + // statistics, routing through a proxy or a tunnel, or handing back a + // connection that is not really a socket at all. + // + // Two things become the caller's responsibility when it is set, + // because the package can no longer do them: + // + // - Timeout is not applied to the dial. Config.Timeout still + // governs reads and writes on the returned connection; the dial + // itself is timed however DialFunc chooses. + // - For TLSImplicit mode the returned connection is wrapped in TLS + // by this package, so DialFunc should return a plain connection. + // A DialFunc that performs its own handshake and returns the TLS + // connection will find it wrapped a second time. + // + // Leaving it nil keeps the previous behaviour exactly. + DialFunc func(network, address string) (net.Conn, error) + // For testing convenience. stubResponses map[string]stubResponse } @@ -371,13 +394,30 @@ func (c *Client) openConn(idx int, host string) (pconn *persistentConn, err erro if c.config.TLSConfig != nil && c.config.TLSMode == TLSImplicit { pconn.debug("opening TLS control connection to %s", host) - dialer := &net.Dialer{ - Timeout: c.config.Timeout, + if c.config.DialFunc != nil { + // Handshake here rather than leaving it to the first read: + // tls.DialWithDialer does the same, and a caller reading + // the returned conn expects the negotiation to have either + // succeeded or failed by now. + var raw net.Conn + raw, err = c.config.DialFunc("tcp", host) + if err == nil { + tlsConn := tls.Client(raw, pconn.config.TLSConfig) + if err = tlsConn.Handshake(); err != nil { + raw.Close() + } else { + conn = tlsConn + } + } + } else { + dialer := &net.Dialer{ + Timeout: c.config.Timeout, + } + conn, err = tls.DialWithDialer(dialer, "tcp", host, pconn.config.TLSConfig) } - conn, err = tls.DialWithDialer(dialer, "tcp", host, pconn.config.TLSConfig) } else { pconn.debug("opening control connection to %s", host) - conn, err = net.DialTimeout("tcp", host, c.config.Timeout) + conn, err = c.config.dial("tcp", host) } var ( diff --git a/client_test.go b/client_test.go index ff14c14..f4c50d0 100644 --- a/client_test.go +++ b/client_test.go @@ -71,6 +71,7 @@ func TestExplicitTLS(t *testing.T) { } func TestImplicitTLS(t *testing.T) { + requireServers(t) closer, err := startPureFTPD(implicitTLSAddrs, "ftpd/pure-ftpd-implicittls") if err != nil { t.Fatal(err) @@ -108,6 +109,7 @@ func TestImplicitTLS(t *testing.T) { } func TestPooling(t *testing.T) { + requireServers(t) config := Config{ ConnectionsPerHost: 2, User: "goftp", diff --git a/dial_func_test.go b/dial_func_test.go new file mode 100644 index 0000000..269ffad --- /dev/null +++ b/dial_func_test.go @@ -0,0 +1,96 @@ +package goftp + +import ( + "errors" + "net" + "strings" + "testing" + "time" +) + +// A connection that is not a socket, to prove the package uses whatever +// DialFunc hands back rather than dialling for itself. +type fakeConn struct{ net.Conn } + +var errRefused = errors.New("dial refused by the test") + +// Every connection the package opens goes through Config.dial, so a +// DialFunc reaching it here reaches the data connections too. That is +// the property worth pinning: a caller supplying one to count bytes +// would otherwise silently miss every transfer, which is most of the +// bytes there are. +func TestConfigDialUsesDialFunc(t *testing.T) { + var got []string + cfg := Config{ + DialFunc: func(network, address string) (net.Conn, error) { + got = append(got, network+" "+address) + return fakeConn{}, nil + }, + } + + conn, err := cfg.dial("tcp", "example.invalid:21") + if err != nil { + t.Fatalf("dial returned %v, want nil", err) + } + if _, ok := conn.(fakeConn); !ok { + t.Errorf("dial returned %T, want the connection DialFunc supplied", conn) + } + if len(got) != 1 || got[0] != "tcp example.invalid:21" { + t.Errorf("DialFunc saw %v, want one call for tcp example.invalid:21", got) + } +} + +// An error from DialFunc is the caller's to see, unchanged. Wrapping it +// would hide the reason a proxy or tunnel refused. +func TestConfigDialPropagatesDialFuncError(t *testing.T) { + cfg := Config{ + DialFunc: func(string, string) (net.Conn, error) { return nil, errRefused }, + } + if _, err := cfg.dial("tcp", "example.invalid:21"); !errors.Is(err, errRefused) { + t.Errorf("dial returned %v, want %v", err, errRefused) + } +} + +// Without one, nothing changes: the package dials for itself and applies +// Config.Timeout, which DialFunc callers take over. +func TestConfigDialWithoutDialFuncStillDials(t *testing.T) { + // Port 0 on the loopback address is never listening, so this + // exercises the built-in path and fails fast rather than hanging. + cfg := Config{Timeout: 100 * time.Millisecond} + if _, err := cfg.dial("tcp", "127.0.0.1:0"); err == nil { + t.Error("dialling a port nothing listens on should fail") + } +} + +// The client must reach the hook rather than dialling around it. +func TestClientControlConnectionUsesDialFunc(t *testing.T) { + called := 0 + client, err := DialConfig(Config{ + DialFunc: func(string, string) (net.Conn, error) { + called++ + return nil, errRefused + }, + }, "127.0.0.1:21") + if err != nil { + t.Fatalf("DialConfig returned %v", err) + } + defer client.Close() + + // Any operation opens the control connection. + // + // The package wraps the failure in its own error type, which does + // not implement Unwrap, so the assertion is on the message it + // carries rather than on errors.Is. That the reason survives at all + // is what matters here: a caller whose proxy refused needs to be + // told why. + _, err = client.Getwd() + if err == nil { + t.Fatal("Getwd succeeded against a DialFunc that refuses") + } + if !strings.Contains(err.Error(), errRefused.Error()) { + t.Errorf("Getwd returned %q, which does not carry %q", err, errRefused) + } + if called == 0 { + t.Error("the client dialled without going through DialFunc") + } +} diff --git a/file_system_test.go b/file_system_test.go index 6346b3e..9feae18 100644 --- a/file_system_test.go +++ b/file_system_test.go @@ -278,6 +278,7 @@ func TestReadDir(t *testing.T) { } func TestReadDirNoMLSD(t *testing.T) { + requireServers(t) // pureFTPD seems to have some issues with timestamps in LIST output for _, addr := range proAddrs { config := goftpConfig @@ -394,6 +395,7 @@ func TestStat(t *testing.T) { } func TestStatNoMLST(t *testing.T) { + requireServers(t) // pureFTPD seems to have some issues with timestamps in LIST output for _, addr := range proAddrs { config := goftpConfig diff --git a/goftp.go b/goftp.go index 1e93ab1..f6d1e2b 100644 --- a/goftp.go +++ b/goftp.go @@ -35,6 +35,17 @@ func DialConfig(config Config, hosts ...string) (*Client, error) { return newClient(config, expandedHosts), nil } +// dial opens a connection with the caller's DialFunc if one was given, +// and with a plain timed dial otherwise. Every connection this package +// makes goes through here, so a DialFunc sees the data connections as +// well as the control ones - which is the point of supplying it. +func (c Config) dial(network, address string) (net.Conn, error) { + if c.DialFunc != nil { + return c.DialFunc(network, address) + } + return net.DialTimeout(network, address, c.Timeout) +} + var hasPort = regexp.MustCompile(`^[^:]+:\d+$|\]:\d+$`) func lookupHosts(hosts []string, ipv6Lookup bool) ([]string, error) { diff --git a/main_test.go b/main_test.go index 22a1254..aa87774 100644 --- a/main_test.go +++ b/main_test.go @@ -31,7 +31,29 @@ var ( proAddrs = []string{"127.0.0.1:2124"} ) +// Set by GOFTP_SKIP_SERVERS. Tests that need a live server call +// requireServers, which skips them when it is set. +var skipServers bool + +// requireServers skips the calling test when no FTP test server was +// started. Without it such a test fails on a connection refused, which +// looks like a bug in the package rather than a missing prerequisite. +func requireServers(t *testing.T) { + t.Helper() + if skipServers { + t.Skip("GOFTP_SKIP_SERVERS is set; this test needs ./build_test_server.sh") + } +} + func TestMain(m *testing.M) { + // The tests that talk to a real server need one built by + // ./build_test_server.sh, which not every machine can run. Setting + // this skips them, so the package's unit tests stay reachable + // without that infrastructure. + if os.Getenv("GOFTP_SKIP_SERVERS") != "" { + skipServers = true + os.Exit(m.Run()) + } pureCloser, err := startPureFTPD(pureAddrs, "ftpd/pure-ftpd") ftpdAddrs = append(ftpdAddrs, pureAddrs...) diff --git a/persistent_connection.go b/persistent_connection.go index 15f5d74..c040f43 100644 --- a/persistent_connection.go +++ b/persistent_connection.go @@ -419,7 +419,7 @@ func (pconn *persistentConn) prepareDataConn() (func() (net.Conn, error), error) } pconn.debug("opening data connection to %s", host) - dc, netErr := net.DialTimeout("tcp", host, pconn.config.Timeout) + dc, netErr := pconn.config.dial("tcp", host) if netErr != nil { var isTemporary bool