From 580597b4b59f6791840705ee9d05d191800d16e7 Mon Sep 17 00:00:00 2001 From: Chris Thomas Date: Wed, 26 Aug 2026 16:21:12 +0200 Subject: [PATCH] Resume the control session on the data connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes secsy/goftp#49. 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 a data connection that has not resumed is refused: 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 the advice in the issue — set ClientSessionCache — supplies only the first. That is why several people tried it and reported no change. The second is that the cache key has to match. With ServerName empty, crypto/tls keys the cache by *address*, and the address includes the port. The data connection is on a different port, so it never finds the control connection's session; resumption simply does not happen, and nothing says so. Setting ServerName to the host makes both channels agree on one key. So: one cache per client, shared by every connection it opens, and a ServerName defaulted to the host when the caller has not set one. The caller's tls.Config is cloned rather than modified — it is theirs and may be shared with connections this package knows nothing about — and a cache they supplied is used rather than replaced. Both are tested. InsecureSkipVerify is carried across explicitly, so naming the server for the cache key does not start verifying a certificate the caller did not ask to have verified. test-servers/proftpd.conf carried TLSOptions NoSessionReuseRequired, which is exactly the workaround the reporter found. With it, the suite was passing against a configuration almost nobody runs, and this bug was invisible. It is gone. Removing it turned TestExplicitTLS and TestStoreEmptyFileOverTLS red with the reported error, which is how this was reproduced; both pass now. Full suite green three consecutive runs against the stricter server. --- client.go | 16 ++++++ persistent_connection.go | 62 +++++++++++++++++++++-- tls_session_reuse_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 tls_session_reuse_test.go diff --git a/client.go b/client.go index 779cd5e..0e6c192 100644 --- a/client.go +++ b/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, diff --git a/persistent_connection.go b/persistent_connection.go index 15f5d74..2b0db7a 100644 --- a/persistent_connection.go +++ b/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 { diff --git a/tls_session_reuse_test.go b/tls_session_reuse_test.go new file mode 100644 index 0000000..ab68a5e --- /dev/null +++ b/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) + } +}