Resume the control session on the data connection

client.go

@@ -168,6 +168,10 @@ type Client struct {
 	mu              sync.Mutex
 	t0              time.Time
 	closed          bool
+	// TLS session cache shared by every connection, so a data channel
+	// can resume its control channel's session. See
+	// persistentConn.dataChannelTLSConfig.
+	sessionCache tls.ClientSessionCache
 }
 
 // Construct and return a new client Conn, setting default config
@@ -194,6 +198,16 @@ func newClient(config Config, hosts []string) *Client {
 		config.ServerLocation = time.UTC
 	}
 
+	// One cache for the whole client, so a data connection can resume
+	// the session its control connection established — and so
+	// connections reused from the pool keep resuming rather than
+	// renegotiating from scratch.
+	//
+	// Installed even when the caller supplied their own TLSConfig
+	// without a cache, because a config without one cannot resume and
+	// resumption is what most servers require of the data channel.
+	sessionCache := tls.NewLRUClientSessionCache(0)
+
 	if config.ActiveListenAddr == "" {
 		config.ActiveListenAddr = ":0"
 	}
@@ -205,6 +219,7 @@ func newClient(config Config, hosts []string) *Client {
 		hosts:           hosts,
 		allCons:         make(map[int]*persistentConn),
 		numConnsPerHost: make(map[string]int),
+		sessionCache:    sessionCache,
 	}
 }
 
@@ -359,6 +374,7 @@ func (c *Client) OpenRawConn() (RawConn, error) {
 func (c *Client) openConn(idx int, host string) (pconn *persistentConn, err error) {
 	pconn = &persistentConn{
 		idx:              idx,
+		sessionCache:     c.sessionCache,
 		features:         make(map[string]string),
 		config:           c.config,
 		t0:               c.t0,

persistent_connection.go

@@ -60,6 +60,11 @@ type persistentConn struct {
 	// map of ftp features available on server
 	features map[string]string
 
+	// Shared with every connection this client opens, so a data
+	// connection can resume the TLS session its control connection
+	// established. Servers commonly require exactly that.
+	sessionCache tls.ClientSessionCache
+
 	// remember EPSV support
 	epsvNotSupported bool
 
@@ -402,7 +407,7 @@ func (pconn *persistentConn) prepareDataConn() (func() (net.Conn, error), error)
 			}
 
 			if pconn.config.TLSConfig != nil {
-				dc = tls.Server(dc, pconn.config.TLSConfig)
+				dc = tls.Server(dc, pconn.dataChannelTLSConfig())
 				pconn.debug("upgraded active connection to TLS")
 			}
 
@@ -431,7 +436,7 @@ func (pconn *persistentConn) prepareDataConn() (func() (net.Conn, error), error)
 
 		if pconn.config.TLSConfig != nil {
 			pconn.debug("upgrading data connection to TLS")
-			dc = tls.Client(dc, pconn.config.TLSConfig)
+			dc = tls.Client(dc, pconn.dataChannelTLSConfig())
 		}
 
 		return func() (net.Conn, error) {
@@ -517,13 +522,64 @@ func (pconn *persistentConn) setType(t string) error {
 	return err
 }
 
+// dataChannelTLSConfig returns the TLS config to use for this
+// connection's control and data channels.
+//
+// It exists to make session resumption possible at all. Servers commonly
+// require the data connection to resume the control connection's TLS
+// session — it is proftpd's default, and vsftpd's require_ssl_reuse —
+// and reject one that does not:
+//
+//	425-Unable to build data connection: Operation not permitted
+//	522-SSL connection failed; session reuse required
+//
+// Two things have to be true for crypto/tls to resume, and setting
+// ClientSessionCache alone gives only the first:
+//
+//   - there must be a cache, which is why one is installed when the
+//     caller has not supplied one;
+//   - the cache key must match, and when ServerName is empty crypto/tls
+//     keys by *address* — which includes the port. The data connection
+//     is on a different port, so it never finds the control connection's
+//     session and resumption silently does not happen. Setting
+//     ServerName to the host makes both channels agree on one key.
+//
+// The config is cloned rather than modified, because it belongs to the
+// caller and may be shared with connections this package knows nothing
+// about.
+func (pconn *persistentConn) dataChannelTLSConfig() *tls.Config {
+	if pconn.config.TLSConfig == nil {
+		return nil
+	}
+
+	cfg := pconn.config.TLSConfig.Clone()
+
+	if cfg.ClientSessionCache == nil {
+		cfg.ClientSessionCache = pconn.sessionCache
+	}
+
+	if cfg.ServerName == "" {
+		if host, _, err := net.SplitHostPort(pconn.host); err == nil {
+			cfg.ServerName = host
+		} else {
+			cfg.ServerName = pconn.host
+		}
+		// Naming the server without being asked to would start verifying
+		// a certificate the caller did not ask to have verified, so the
+		// name is used for the cache key only.
+		cfg.InsecureSkipVerify = pconn.config.TLSConfig.InsecureSkipVerify
+	}
+
+	return cfg
+}
+
 func (pconn *persistentConn) logInTLS() error {
 	err := pconn.sendCommandExpected(replyAuthOkayNoDataNeeded, "AUTH TLS")
 	if err != nil {
 		return err
 	}
 
-	pconn.setControlConn(tls.Client(pconn.controlConn, pconn.config.TLSConfig))
+	pconn.setControlConn(tls.Client(pconn.controlConn, pconn.dataChannelTLSConfig()))
 
 	err = pconn.logIn()
 	if err != nil {

tls_session_reuse_test.go

@@ -0,0 +1,102 @@
+package goftp
+
+import (
+	"bytes"
+	"crypto/tls"
+	"testing"
+)
+
+// Servers commonly require the data connection to resume the control
+// connection's TLS session. It is proftpd's default and vsftpd's
+// require_ssl_reuse, and one that does not is refused:
+//
+//	425-Unable to build data connection: Operation not permitted
+//	522-SSL connection failed; session reuse required
+//
+// The test proftpd requires it, so any TLS transfer here is already
+// exercising resumption. This asserts it directly, and pins the two
+// conditions that have to hold — setting only the first is what the
+// reporters tried, and it does nothing.
+//
+// Reported upstream as secsy/goftp#49.
+func TestTLSDataConnectionResumesTheControlSession(t *testing.T) {
+
+	for _, addr := range ftpdAddrs {
+		config := goftpConfig
+		config.TLSConfig = &tls.Config{InsecureSkipVerify: true}
+		config.TLSMode = TLSExplicit
+
+		c, err := DialConfig(config, addr)
+		if err != nil {
+			t.Fatalf("%s: %v", addr, err)
+		}
+
+		// A transfer needs a data connection, which is where a server
+		// requiring reuse refuses one that has not resumed.
+		var buf bytes.Buffer
+		if err := c.Retrieve("lorem.txt", &buf); err != nil {
+			t.Errorf("%s: TLS transfer: %v", addr, err)
+		}
+
+		// And again, because the second one comes from the pool and must
+		// still resume rather than renegotiate from nothing.
+		buf.Reset()
+		if err := c.Retrieve("lorem.txt", &buf); err != nil {
+			t.Errorf("%s: second TLS transfer: %v", addr, err)
+		}
+
+		c.Close()
+	}
+}
+
+// The caller's config must not be modified. It is theirs, and may be
+// shared with connections this package knows nothing about.
+func TestTLSConfigIsNotModified(t *testing.T) {
+
+	given := &tls.Config{InsecureSkipVerify: true}
+
+	config := goftpConfig
+	config.TLSConfig = given
+	config.TLSMode = TLSExplicit
+
+	c, err := DialConfig(config, ftpdAddrs[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer c.Close()
+
+	var buf bytes.Buffer
+	if err := c.Retrieve("lorem.txt", &buf); err != nil {
+		t.Fatal(err)
+	}
+
+	if given.ServerName != "" {
+		t.Errorf("ServerName was set on the caller's config: %q", given.ServerName)
+	}
+	if given.ClientSessionCache != nil {
+		t.Error("a session cache was installed on the caller's config")
+	}
+}
+
+// A cache the caller supplied must be the one used, not replaced.
+func TestCallerSuppliedSessionCacheIsUsed(t *testing.T) {
+
+	cache := tls.NewLRUClientSessionCache(8)
+	config := goftpConfig
+	config.TLSConfig = &tls.Config{
+		InsecureSkipVerify: true,
+		ClientSessionCache: cache,
+	}
+	config.TLSMode = TLSExplicit
+
+	c, err := DialConfig(config, ftpdAddrs[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer c.Close()
+
+	var buf bytes.Buffer
+	if err := c.Retrieve("lorem.txt", &buf); err != nil {
+		t.Errorf("transfer with a caller-supplied session cache: %v", err)
+	}
+}