[ovs-dev] [PATCH v2] python: replace pyOpenSSL with ssl
Ilya Maximets
i.maximets at ovn.org
Thu Oct 21 23:46:53 UTC 2021
On 10/6/21 20:49, Timothy Redaelli wrote:
> Currently, pyOpenSSL is half-deprecated upstream and so it's removed on
> some distributions (for example on CentOS Stream 9,
> https://issues.redhat.com/browse/CS-336), but since OVS only
> supports Python 3 it's possible to replace pyOpenSSL with "import ssl"
> included in base Python 3.
>
> Stream recv and send had to be splitted as _recv and _send, since SSLError
> is a subclass of socket.error and so it was not possible to except for
> SSLWantReadError and SSLWantWriteError in recv and send of SSLStream.
>
> Reported-by: Timothy Redaelli <tredaelli at redhat.com>
> Reported-at: https://bugzilla.redhat.com/1988429
> Signed-off-by: Timothy Redaelli <tredaelli at redhat.com>
> ---
> v1 -> v2:
> - asyncronous connect with ssl is different than with pyOpenSSL, so a
> different approach is needed or it fails when connect is not ready
> when ctx.wrap_socket is executed.
> Moreover it's not possible to use connect, but it's necessary to use
> connect_ex (probably due to a python bug or limitation).
> So to do not behave diffently than TCPSocket, it's necessary to raise
> the errno exception like connect do.
> ---
> .ci/linux-prepare.sh | 2 +-
> .cirrus.yml | 2 +-
> .travis.yml | 1 -
> python/ovs/poller.py | 6 +--
> python/ovs/stream.py | 118 +++++++++++++++++++++++++++++--------------
> tests/ovsdb-idl.at | 2 +-
> 6 files changed, 86 insertions(+), 45 deletions(-)
>
> diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
> index c55125cf7..b9b499bad 100755
> --- a/.ci/linux-prepare.sh
> +++ b/.ci/linux-prepare.sh
> @@ -21,7 +21,7 @@ make -j4 HAVE_LLVM= HAVE_SQLITE= install
> cd ..
>
> pip3 install --disable-pip-version-check --user \
> - flake8 hacking sphinx pyOpenSSL wheel setuptools
> + flake8 hacking sphinx wheel setuptools
> pip3 install --user --upgrade docutils
> pip3 install --user 'meson==0.47.1'
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 358f2ba25..bb206f35f 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -9,7 +9,7 @@ freebsd_build_task:
>
> env:
> DEPENDENCIES: automake libtool gmake gcc wget openssl python3
> - PY_DEPS: sphinx|openssl
> + PY_DEPS: sphinx
> matrix:
> COMPILER: gcc
> COMPILER: clang
> diff --git a/.travis.yml b/.travis.yml
> index 51d051108..c7aeede06 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -17,7 +17,6 @@ addons:
> - libjemalloc-dev
> - libnuma-dev
> - libpcap-dev
> - - python3-openssl
> - python3-pip
> - python3-sphinx
> - libelf-dev
> diff --git a/python/ovs/poller.py b/python/ovs/poller.py
> index 3624ec865..157719c3a 100644
> --- a/python/ovs/poller.py
> +++ b/python/ovs/poller.py
> @@ -26,9 +26,9 @@ if sys.platform == "win32":
> import ovs.winutils as winutils
>
> try:
> - from OpenSSL import SSL
> + import ssl
> except ImportError:
> - SSL = None
> + ssl = None
>
> try:
> from eventlet import patcher as eventlet_patcher
> @@ -73,7 +73,7 @@ class _SelectSelect(object):
> def register(self, fd, events):
> if isinstance(fd, socket.socket):
> fd = fd.fileno()
> - if SSL and isinstance(fd, SSL.Connection):
> + if ssl and isinstance(fd, ssl.SSLSocket):
> fd = fd.fileno()
>
> if sys.platform != 'win32':
> diff --git a/python/ovs/stream.py b/python/ovs/stream.py
> index f5a520862..205e74888 100644
> --- a/python/ovs/stream.py
> +++ b/python/ovs/stream.py
> @@ -22,9 +22,9 @@ import ovs.socket_util
> import ovs.vlog
>
> try:
> - from OpenSSL import SSL
> + import ssl
> except ImportError:
> - SSL = None
> + ssl = None
>
> if sys.platform == 'win32':
> import ovs.winutils as winutils
> @@ -322,6 +322,12 @@ class Stream(object):
> The recv function will not block waiting for data to arrive. If no
> data have been received, it returns (errno.EAGAIN, "") immediately."""
>
> + try:
> + return self._recv(n)
> + except socket.error as e:
> + return (ovs.socket_util.get_exception_errno(e), "")
> +
> + def _recv(self, n):
> retval = self.connect()
> if retval != 0:
> return (retval, "")
> @@ -331,10 +337,7 @@ class Stream(object):
> if sys.platform == 'win32' and self.socket is None:
> return self.__recv_windows(n)
>
> - try:
> - return (0, self.socket.recv(n))
> - except socket.error as e:
> - return (ovs.socket_util.get_exception_errno(e), "")
> + return (0, self.socket.recv(n))
>
> def __recv_windows(self, n):
> if self._read_pending:
> @@ -396,6 +399,12 @@ class Stream(object):
> Will not block. If no bytes can be immediately accepted for
> transmission, returns -errno.EAGAIN immediately."""
>
> + try:
> + return self._send(buf)
> + except socket.error as e:
> + return -ovs.socket_util.get_exception_errno(e)
> +
> + def _send(self, buf):
> retval = self.connect()
> if retval != 0:
> return -retval
> @@ -409,10 +418,7 @@ class Stream(object):
> if sys.platform == 'win32' and self.socket is None:
> return self.__send_windows(buf)
>
> - try:
> - return self.socket.send(buf)
> - except socket.error as e:
> - return -ovs.socket_util.get_exception_errno(e)
> + return self.socket.send(buf)
>
> def __send_windows(self, buf):
> if self._write_pending:
> @@ -769,36 +775,68 @@ class SSLStream(Stream):
> def check_connection_completion(sock):
> try:
> return Stream.check_connection_completion(sock)
> - except SSL.SysCallError as e:
> + except ssl.SSLSyscallError as e:
> return ovs.socket_util.get_exception_errno(e)
>
> @staticmethod
> def needs_probes():
> return True
>
> - @staticmethod
> - def verify_cb(conn, cert, errnum, depth, ok):
> - return ok
> -
> @staticmethod
> def _open(suffix, dscp):
> - error, sock = TCPStream._open(suffix, dscp)
> - if error:
> - return error, None
> + address = ovs.socket_util.inet_parse_active(suffix, 0)
> + try:
> + is_addr_inet = ovs.socket_util.is_valid_ipv4_address(address[0])
> + if is_addr_inet:
> + family = socket.AF_INET
> + else:
> + family = socket.AF_INET6
> + sock = socket.socket(family, socket.SOCK_STREAM, 0)
> + except socket.error as e:
> + return ovs.socket_util.get_exception_errno(e), None
Instead of duplicating this piece of code. It's probably better to
create a function that returns a socket and a family, and use this
function here and inside the inet_open_active().
Socmething like 'inet_create_socket_active'. What do you think?
>
> # Create an SSL context
> - ctx = SSL.Context(SSL.SSLv23_METHOD)
> - ctx.set_verify(SSL.VERIFY_PEER, SSLStream.verify_cb)
> - ctx.set_options(SSL.OP_NO_SSLv2 | SSL.OP_NO_SSLv3)
> + ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
> + ctx.verify_mode = ssl.CERT_REQUIRED
> + ctx.options |= ssl.OP_NO_SSLv2
> + ctx.options |= ssl.OP_NO_SSLv3
> # If the client has not set the SSL configuration files
> # exception would be raised.
> - ctx.use_privatekey_file(Stream._SSL_private_key_file)
> - ctx.use_certificate_file(Stream._SSL_certificate_file)
> ctx.load_verify_locations(Stream._SSL_ca_cert_file)
> + ctx.load_cert_chain(Stream._SSL_certificate_file,
> + Stream._SSL_private_key_file)
> + ssl_sock = ctx.wrap_socket(sock, do_handshake_on_connect=False)
>
> - ssl_sock = SSL.Connection(ctx, sock)
> - ssl_sock.set_connect_state()
> - return error, ssl_sock
> + # Connect
> + try:
> + ovs.socket_util.set_nonblocking(ssl_sock)
> + ovs.socket_util.set_dscp(ssl_sock, family, dscp)
> + ssl_sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
> + error = ssl_sock.connect_ex(address)
> + if error:
> + errcls = None
> + if error == errno.EPIPE or error == errno.ESHUTDOWN:
> + errcls = BrokenPipeError
> + elif error == errno.ECONNABORTED:
> + errcls = ConnectionAbortedError
> + elif error == errno.ECONNREFUSED:
> + errcls = ConnectionRefusedError
> + elif error == errno.ECONNRESET:
> + errcls = ConnectionResetError
> + elif error == errno.EINTR:
> + errcls = InterruptedError
> + elif error == errno.EACCES or error == errno.EPERM:
> + errcls = PermissionError
> + elif error == errno.ETIMEDOUT:
> + errcls = TimeoutError
> + elif error != errno.EINPROGRESS and error != errno.EWOULDBLOCK:
> + errcls = socket.error
> + if errcls:
> + raise errcls(error, os.strerror(error))
> + return 0, ssl_sock
> + except socket.error as e:
> + ssl_sock.close()
> + return ovs.socket_util.get_exception_errno(e), None
I'm a bit confused by this part of a code. Looks like we're converting
errno to the exception just to catch it and convert back. Isn't this
equal to something like that (didn't test):
try:
ovs.socket_util.set_nonblocking(ssl_sock)
ovs.socket_util.set_dscp(ssl_sock, family, dscp)
ssl_sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
error = ssl_sock.connect_ex(address)
if error and error not in [errno.EINPROGRESS, errno.EWOULDBLOCK]:
ssl_sock.close()
return error, None
return 0, ssl_sock
except socket.error as e:
ssl_sock.close()
?
In general, I think, this function needs more comments that will describe
why we can't use TCPStream._open() here and why we need to use connect_ex().
>
> def connect(self):
> retval = super(SSLStream, self).connect()
> @@ -809,40 +847,44 @@ class SSLStream(Stream):
> # TCP Connection is successful. Now do the SSL handshake
> try:
> self.socket.do_handshake()
> - except SSL.WantReadError:
> + except ssl.SSLWantReadError:
> return errno.EAGAIN
> - except SSL.SysCallError as e:
> + except ssl.SSLSyscallError as e:
> return ovs.socket_util.get_exception_errno(e)
>
> return 0
>
> def recv(self, n):
> try:
> - return super(SSLStream, self).recv(n)
> - except SSL.WantReadError:
> + return super(SSLStream, self)._recv(n)
> + except ssl.SSLWantReadError:
> return (errno.EAGAIN, "")
> - except SSL.SysCallError as e:
> + except ssl.SSLSyscallError as e:
> return (ovs.socket_util.get_exception_errno(e), "")
> - except SSL.ZeroReturnError:
> + except ssl.SSLZeroReturnError:
> return (0, "")
> + except socket.error as e:
> + return (ovs.socket_util.get_exception_errno(e), "")
>
> def send(self, buf):
> try:
> - return super(SSLStream, self).send(buf)
> - except SSL.WantWriteError:
> + return super(SSLStream, self)._send(buf)
> + except ssl.SSLWantWriteError:
> return -errno.EAGAIN
> - except SSL.SysCallError as e:
> + except ssl.SSLSyscallError as e:
> + return -ovs.socket_util.get_exception_errno(e)
> + except socket.error as e:
> return -ovs.socket_util.get_exception_errno(e)
Can we collapse above exception types into one 'except' statement?
>
> def close(self):
> if self.socket:
> try:
> - self.socket.shutdown()
> - except SSL.Error:
> + self.socket.shutdown(socket.SHUT_RDWR)
> + except (socket.error, OSError, ValueError):
> pass
> return super(SSLStream, self).close()
>
>
> -if SSL:
> +if ssl:
> # Register SSL only if the OpenSSL module is available
> Stream.register_method("ssl", SSLStream)
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 501c13b81..0f229b2f9 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -225,7 +225,7 @@ m4_define([OVSDB_CHECK_IDL_TCP6_MULTIPLE_REMOTES_PY],
> m4_define([OVSDB_CHECK_IDL_SSL_PY],
> [AT_SETUP([$1 - Python3 - SSL])
> AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
> - $PYTHON3 -c "import OpenSSL.SSL"
> + $PYTHON3 -c "import ssl"
> SSL_PRESENT=$?
> AT_SKIP_IF([test $SSL_PRESENT != 0])
> AT_KEYWORDS([ovsdb server idl positive Python with ssl socket $5])
>
More information about the dev
mailing list