From a5c4b5515f5af878923d67752d282f8cc8f072e7 Mon Sep 17 00:00:00 2001
From: Antoine Pitrou <solipsis@pitrou.net>
Date: Thu, 22 Apr 2010 23:33:02 +0000
Subject: [PATCH] Issue #8108: Fix the unwrap() method of SSL objects when the
socket has a non-infinite timeout. Also make that method friendlier with
applications wanting to continue using the socket in clear-text mode, by
disabling OpenSSL's internal readahead. Thanks to Darryl Miles for guidance.
Issue #8108: test_ftplib's non-blocking SSL server now has proper handling
of SSL shutdowns.
---
Lib/test/test_ftplib.py | 47 +++++++++++++++++++++-------
Misc/NEWS | 11 +++++++
Modules/_ssl.c | 69 +++++++++++++++++++++++++++++++++++++----
3 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/Lib/test/test_ftplib.py b/Lib/test/test_ftplib.py
index 182d5a7c743..4f6d1c12b2e 100644
--- a/Lib/test/test_ftplib.py
+++ b/Lib/test/test_ftplib.py
@@ -29,6 +29,7 @@ NLST_DATA = 'foo\r\nbar\r\n'
class DummyDTPHandler(asynchat.async_chat):
+ dtp_conn_closed = False
def __init__(self, conn, baseclass):
asynchat.async_chat.__init__(self, conn)
@@ -39,8 +40,13 @@ class DummyDTPHandler(asynchat.async_chat):
self.baseclass.last_received_data += self.recv(1024)
def handle_close(self):
- self.baseclass.push('226 transfer complete')
- self.close()
+ # XXX: this method can be called many times in a row for a single
+ # connection, including in clear-text (non-TLS) mode.
+ # (behaviour witnessed with test_data_connection)
+ if not self.dtp_conn_closed:
+ self.baseclass.push('226 transfer complete')
+ self.close()
+ self.dtp_conn_closed = True
class DummyFTPHandler(asynchat.async_chat):
@@ -253,6 +259,7 @@ if ssl is not None:
"""An asyncore.dispatcher subclass supporting TLS/SSL."""
_ssl_accepting = False
+ _ssl_closing = False
def secure_connection(self):
self.socket = ssl.wrap_socket(self.socket, suppress_ragged_eofs=False,
@@ -277,15 +284,36 @@ if ssl is not None:
else:
self._ssl_accepting = False
+ def _do_ssl_shutdown(self):
+ self._ssl_closing = True
+ try:
+ self.socket = self.socket.unwrap()
+ except ssl.SSLError, err:
+ if err.args[0] in (ssl.SSL_ERROR_WANT_READ,
+ ssl.SSL_ERROR_WANT_WRITE):
+ return
+ except socket.error, err:
+ # Any "socket error" corresponds to a SSL_ERROR_SYSCALL return
+ # from OpenSSL's SSL_shutdown(), corresponding to a
+ # closed socket condition. See also:
+ # http://www.mail-archive.com/openssl-users@openssl.org/msg60710.html
+ pass
+ self._ssl_closing = False
+ super(SSLConnection, self).close()
+
def handle_read_event(self):
if self._ssl_accepting:
self._do_ssl_handshake()
+ elif self._ssl_closing:
+ self._do_ssl_shutdown()
else:
super(SSLConnection, self).handle_read_event()
def handle_write_event(self):
if self._ssl_accepting:
self._do_ssl_handshake()
+ elif self._ssl_closing:
+ self._do_ssl_shutdown()
else:
super(SSLConnection, self).handle_write_event()
@@ -315,12 +343,9 @@ if ssl is not None:
raise
def close(self):
- try:
- if isinstance(self.socket, ssl.SSLSocket):
- if self.socket._sslobj is not None:
- self.socket.unwrap()
- finally:
- super(SSLConnection, self).close()
+ if (isinstance(self.socket, ssl.SSLSocket) and
+ self.socket._sslobj is not None):
+ self._do_ssl_shutdown()
class DummyTLS_DTPHandler(SSLConnection, DummyDTPHandler):
@@ -597,21 +622,21 @@ class TestTLS_FTPClass(TestCase):
sock = self.client.transfercmd('list')
self.assertNotIsInstance(sock, ssl.SSLSocket)
sock.close()
- self.client.voidresp()
+ self.assertEqual(self.client.voidresp(), "226 transfer complete")
# secured, after PROT P
self.client.prot_p()
sock = self.client.transfercmd('list')
self.assertIsInstance(sock, ssl.SSLSocket)
sock.close()
- self.client.voidresp()
+ self.assertEqual(self.client.voidresp(), "226 transfer complete")
# PROT C is issued, the connection must be in cleartext again
self.client.prot_c()
sock = self.client.transfercmd('list')
self.assertNotIsInstance(sock, ssl.SSLSocket)
sock.close()
- self.client.voidresp()
+ self.assertEqual(self.client.voidresp(), "226 transfer complete")
def test_login(self):
# login() is supposed to implicitly secure the control connection
diff --git a/Misc/NEWS b/Misc/NEWS
index 329b053fe6b..dda4164927e 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -25,6 +25,11 @@ Core and Builtins
Library
-------
+- Issue #8108: Fix the unwrap() method of SSL objects when the socket has
+ a non-infinite timeout. Also make that method friendlier with applications
+ wanting to continue using the socket in clear-text mode, by disabling
+ OpenSSL's internal readahead. Thanks to Darryl Miles for guidance.
+
- Issue #8484: Load all ciphers and digest algorithms when initializing
the _ssl extension, such that verification of some SSL certificates
doesn't fail because of an "unknown algorithm".
@@ -68,6 +73,12 @@ Extension Modules
- Build the ossaudio extension on GNU/kFreeBSD.
+Tests
+-----
+
+- Issue #8108: test_ftplib's non-blocking SSL server now has proper handling
+ of SSL shutdowns.
+
What's New in Python 2.7 beta 1?
================================
diff --git a/Modules/_ssl.c b/Modules/_ssl.c
index 4702ecbfe1d..d19bf2d488f 100644
--- a/Modules/_ssl.c
+++ b/Modules/_ssl.c
@@ -9,6 +9,9 @@
directly.
XXX should partial writes be enabled, SSL_MODE_ENABLE_PARTIAL_WRITE?
+
+ XXX integrate several "shutdown modes" as suggested in
+ http://bugs.python.org/issue8108#msg102867 ?
*/
#include "Python.h"
@@ -115,6 +118,7 @@ typedef struct {
X509* peer_cert;
char server[X509_NAME_MAXLEN];
char issuer[X509_NAME_MAXLEN];
+ int shutdown_seen_zero;
} PySSLObject;
@@ -1357,7 +1361,8 @@ Read up to len bytes from the SSL socket.");
static PyObject *PySSL_SSLshutdown(PySSLObject *self)
{
- int err;
+ int err, ssl_err, sockstate, nonblocking;
+ int zeros = 0;
/* Guard against closed socket */
if (self->Socket->sock_fd < 0) {
@@ -1366,13 +1371,65 @@ static PyObject *PySSL_SSLshutdown(PySSLObject *self)
return NULL;
}
- PySSL_BEGIN_ALLOW_THREADS
- err = SSL_shutdown(self->ssl);
- if (err == 0) {
- /* we need to call it again to finish the shutdown */
+ /* Just in case the blocking state of the socket has been changed */
+ nonblocking = (self->Socket->sock_timeout >= 0.0);
+ BIO_set_nbio(SSL_get_rbio(self->ssl), nonblocking);
+ BIO_set_nbio(SSL_get_wbio(self->ssl), nonblocking);
+
+ while (1) {
+ PySSL_BEGIN_ALLOW_THREADS
+ /* Disable read-ahead so that unwrap can work correctly.
+ * Otherwise OpenSSL might read in too much data,
+ * eating clear text data that happens to be
+ * transmitted after the SSL shutdown.
+ * Should be safe to call repeatedly everytime this
+ * function is used and the shutdown_seen_zero != 0
+ * condition is met.
+ */
+ if (self->shutdown_seen_zero)
+ SSL_set_read_ahead(self->ssl, 0);
err = SSL_shutdown(self->ssl);
+ PySSL_END_ALLOW_THREADS
+ /* If err == 1, a secure shutdown with SSL_shutdown() is complete */
+ if (err > 0)
+ break;
+ if (err == 0) {
+ /* Don't loop endlessly; instead preserve legacy
+ behaviour of trying SSL_shutdown() only twice.
+ This looks necessary for OpenSSL < 0.9.8m */
+ if (++zeros > 1)
+ break;
+ /* Shutdown was sent, now try receiving */
+ self->shutdown_seen_zero = 1;
+ continue;
+ }
+
+ /* Possibly retry shutdown until timeout or failure */
+ ssl_err = SSL_get_error(self->ssl, err);
+ if (ssl_err == SSL_ERROR_WANT_READ)
+ sockstate = check_socket_and_wait_for_timeout(self->Socket, 0);
+ else if (ssl_err == SSL_ERROR_WANT_WRITE)
+ sockstate = check_socket_and_wait_for_timeout(self->Socket, 1);
+ else
+ break;
+ if (sockstate == SOCKET_HAS_TIMED_OUT) {
+ if (ssl_err == SSL_ERROR_WANT_READ)
+ PyErr_SetString(PySSLErrorObject,
+ "The read operation timed out");
+ else
+ PyErr_SetString(PySSLErrorObject,
+ "The write operation timed out");
+ return NULL;
+ }
+ else if (sockstate == SOCKET_TOO_LARGE_FOR_SELECT) {
+ PyErr_SetString(PySSLErrorObject,
+ "Underlying socket too large for select().");
+ return NULL;
+ }
+ else if (sockstate != SOCKET_OPERATION_OK)
+ /* Retain the SSL error code */
+ break;
}
- PySSL_END_ALLOW_THREADS
if (err < 0)
return PySSL_SetError(self, err, __FILE__, __LINE__);
--
2.30.9