diff mbox series

[ovs-dev,v2] python: replace pyOpenSSL with ssl

Message ID 2bcb122be6169cda9551c0077d3d78c2835f343e.1633534682.git.tredaelli@redhat.com
State Superseded
Headers show
Series [ovs-dev,v2] python: replace pyOpenSSL with ssl | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Timothy Redaelli Oct. 6, 2021, 6:49 p.m. UTC
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@redhat.com>
Reported-at: https://bugzilla.redhat.com/1988429
Signed-off-by: Timothy Redaelli <tredaelli@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(-)

Comments

Terry Wilson Oct. 13, 2021, 10:15 p.m. UTC | #1
On Wed, Oct 6, 2021 at 1:50 PM Timothy Redaelli <tredaelli@redhat.com> 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@redhat.com>
> Reported-at: https://bugzilla.redhat.com/1988429
> Signed-off-by: Timothy Redaelli <tredaelli@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
>
>          # 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
>
>      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)
>
>      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])
> --
> 2.31.1
>

I tested with the neutron ml2/ovn tests that run over SSL and
everything behaved the same. Nice work--what a pain!

Acked-by: Terry Wilson <twilson@redhat.com>
Tested-by: Terry Wilson <twilson@redhat.com>
Ilya Maximets Oct. 21, 2021, 11:46 p.m. UTC | #2
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@redhat.com>
> Reported-at: https://bugzilla.redhat.com/1988429
> Signed-off-by: Timothy Redaelli <tredaelli@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])
>
diff mbox series

Patch

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
 
         # 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
 
     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)
 
     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])