diff --git a/Doc/library/ftplib.rst b/Doc/library/ftplib.rst index e1baeff3f373bf1..e605afb39d61c14 100644 --- a/Doc/library/ftplib.rst +++ b/Doc/library/ftplib.rst @@ -549,6 +549,22 @@ FTP_TLS objects Set up clear text data connection. + .. note:: + The TLS shutdown of the data connection is best-effort. Once a + transfer has finished exchanging its payload, + :meth:`~FTP.retrbinary`, :meth:`~FTP.retrlines`, + :meth:`~FTP.storbinary` and :meth:`~FTP.storlines` attempt to + complete a TLS shutdown handshake on the data socket, but limit it + to a few seconds and ignore the errors it may raise. Servers are + not required to perform a TLS shutdown on a connection which is + created for a single transfer and closed immediately afterwards, + and some of them never answer it at all. + + .. versionchanged:: 3.16 + A failed or unresponsive TLS shutdown of the data connection no + longer raises an exception or blocks indefinitely once the transfer + itself has completed. + Module variables ^^^^^^^^^^^^^^^^ diff --git a/Lib/ftplib.py b/Lib/ftplib.py index 2f092d50f31782b..cdf191455e2397b 100644 --- a/Lib/ftplib.py +++ b/Lib/ftplib.py @@ -436,9 +436,8 @@ def retrbinary(self, cmd, callback, blocksize=8192, rest=None): with self.transfercmd(cmd, rest) as conn: while data := conn.recv(blocksize): callback(data) - # shutdown ssl layer if _SSLSocket is not None and isinstance(conn, _SSLSocket): - conn.unwrap() + _shutdown_data_channel(conn) return self.voidresp() def retrlines(self, cmd, callback = None): @@ -471,9 +470,8 @@ def retrlines(self, cmd, callback = None): elif line[-1:] == '\n': line = line[:-1] callback(line) - # shutdown ssl layer if _SSLSocket is not None and isinstance(conn, _SSLSocket): - conn.unwrap() + _shutdown_data_channel(conn) return self.voidresp() def storbinary(self, cmd, fp, blocksize=8192, callback=None, rest=None): @@ -497,9 +495,8 @@ def storbinary(self, cmd, fp, blocksize=8192, callback=None, rest=None): conn.sendall(buf) if callback: callback(buf) - # shutdown ssl layer if _SSLSocket is not None and isinstance(conn, _SSLSocket): - conn.unwrap() + _shutdown_data_channel(conn) return self.voidresp() def storlines(self, cmd, fp, callback=None): @@ -528,9 +525,8 @@ def storlines(self, cmd, fp, callback=None): conn.sendall(buf) if callback: callback(buf) - # shutdown ssl layer if _SSLSocket is not None and isinstance(conn, _SSLSocket): - conn.unwrap() + _shutdown_data_channel(conn) return self.voidresp() def acct(self, password): @@ -674,6 +670,33 @@ def close(self): else: _SSLSocket = ssl.SSLSocket + _DATA_SHUTDOWN_TIMEOUT = 5.0 + + def _shutdown_data_channel(conn, timeout=None): + """Perform a best-effort TLS shutdown of a data connection. + + The data connection of an FTP transfer is ephemeral: it is created + by transfercmd() for one transfer and closed right afterwards. + Servers are not required to complete a TLS shutdown handshake on it + and many do not send close_notify at all (gh-77303, gh-78738, + gh-103443, gh-124850), which makes an unconditional unwrap() either + raise or block forever *after* the whole payload has already been + sent, turning a completed transfer into an apparent failure. The + handshake is still attempted (well-behaved peers get a clean + close_notify), but it is time-limited and never propagates an + error. + """ + if timeout is None: + timeout = _DATA_SHUTDOWN_TIMEOUT + try: + conn.settimeout(timeout) + conn.unwrap() + except (OSError, ValueError): + # ConnectionResetError, SSLEOFError, TimeoutError and + # ssl.SSLError are all OSError subclasses; ValueError covers + # "No SSL wrapper around ...". + pass + class FTP_TLS(FTP): '''A FTP subclass which adds TLS support to FTP as described in RFC-4217. diff --git a/Lib/test/test_ftplib.py b/Lib/test/test_ftplib.py index 983a8b92cf6384d..dfa5cd62c2046ac 100644 --- a/Lib/test/test_ftplib.py +++ b/Lib/test/test_ftplib.py @@ -972,6 +972,107 @@ def test_data_connection(self): LIST_DATA.encode(self.client.encoding)) self.assertEqual(self.client.voidresp(), "226 transfer complete") + def _unreliable_data_connection(self, exc): + """Make unwrap() on the *client* data connection raise *exc*. + + transfercmd() is wrapped so that unwrap() on the socket it returns + raises *exc* instead of performing the TLS shutdown. Only the data + connection of the client is touched: the control connection and the + server side keep working normally, which is exactly the situation + reported in the bugs (the peer aborts or half-closes the ephemeral + data connection once the transfer is over). + """ + original = self.client.transfercmd + + def transfercmd(*args, **kwargs): + sock = original(*args, **kwargs) + real_unwrap = sock.unwrap + + def unwrap(): + # Shut the TLS layer down before reporting the error. A + # peer that fails the shutdown has still received (or sent) + # the whole payload, but closing the data connection without + # the close_notify handshake lets the dummy server below + # mistake a partial TLS read for end-of-stream and drop the + # tail of the transfer. + try: + real_unwrap() + except OSError: + pass + raise exc + + sock.unwrap = unwrap + return sock + + return mock.patch.object(self.client, 'transfercmd', transfercmd) + + def test_data_shutdown_error_is_ignored(self): + # gh-77303, gh-103443, gh-124850: the payload has been fully sent + # or received by the time the data connection is shut down, so a + # failure there must not turn a completed transfer into an error. + self.client.auth() + self.client.prot_p() + payload = RETR_DATA.encode(self.client.encoding) + errors = ( + ConnectionResetError('connection reset by peer'), + ssl.SSLEOFError('EOF occurred in violation of protocol'), + ssl.SSLError('shutdown while in init'), + OSError(errno.ECONNRESET, 'Connection reset by peer'), + ValueError('No SSL wrapper around socket'), + ) + for exc in errors: + with self.subTest(exc=exc): + with self._unreliable_data_connection(exc): + received = [] + self.client.retrbinary('retr', received.append) + self.assertEqual(b''.join(received), payload) + + self.client.storbinary('stor', io.BytesIO(payload)) + self.assertEqual( + bytes(self.server.handler_instance.last_received_data), + RETR_DATA.encode(self.server.encoding)) + + def test_data_shutdown_timeout_is_ignored(self): + # gh-78738: a peer which never answers close_notify makes an + # unconditional unwrap() block forever on a socket whose whole + # purpose is over. The shutdown has to be time-limited, and the + # resulting TimeoutError has to be swallowed. + self.client.auth() + self.client.prot_p() + payload = RETR_DATA.encode(self.client.encoding) + + original = self.client.transfercmd + + def transfercmd(*args, **kwargs): + sock = original(*args, **kwargs) + original_timeout = sock.gettimeout() + + def unwrap(): + shutdown_timeout = sock.gettimeout() + if shutdown_timeout == original_timeout: + raise AssertionError( + 'data connection shutdown is unbounded: unwrap() is ' + 'called without lowering the socket timeout first') + # Emulate what _ssl does when the peer stays silent. + time.sleep(shutdown_timeout) + raise TimeoutError('the handshake operation timed out') + + sock.unwrap = unwrap + return sock + + with mock.patch.object(ftplib, '_DATA_SHUTDOWN_TIMEOUT', 0.1, + create=True): + with mock.patch.object(self.client, 'transfercmd', transfercmd): + start = time.monotonic() + received = [] + self.client.retrbinary('retr', received.append) + self.assertEqual(b''.join(received), payload) + + self.client.storbinary('stor', io.BytesIO(payload)) + self.assertEqual( + bytes(self.server.handler_instance.last_received_data), + RETR_DATA.encode(self.server.encoding)) + self.assertLess(time.monotonic() - start, TIMEOUT) def test_login(self): # login() is supposed to implicitly secure the control connection self.assertNotIsInstance(self.client.sock, ssl.SSLSocket) diff --git a/Misc/NEWS.d/next/Library/2026-10-07-11-33-20.gh-issue-103443.hU.rst b/Misc/NEWS.d/next/Library/2026-10-07-11-33-20.gh-issue-103443.hU.rst new file mode 100644 index 000000000000000..f975055feff8c3f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-10-07-11-33-20.gh-issue-103443.hU.rst @@ -0,0 +1,8 @@ +:func:`ftplib.FTP.retrbinary`, :func:`ftplib.FTP.retrlines`, +:func:`ftplib.FTP.storbinary` and :func:`ftplib.FTP.storlines` no longer let +the TLS shutdown of the data connection turn an already completed transfer +into an exception or into an indefinite hang. The shutdown handshake is +still attempted, but it now runs with a short timeout and the errors a peer +can cause there -- such as :exc:`ConnectionResetError` or +:class:`ssl.SSLEOFError` when it does not complete the handshake on a data +connection that only exists for a single transfer -- are ignored.