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