[ovs-dev] Don't raise an Exception on failure to connect via SSL

With other socket types, trying to connect and failing will return
an error code, but if an SSL Stream is used, then when
check_connection_completion(sock) is called, SSL will raise an
exception that doesn't derive from socket.error which is handled.

This adds handling for SSL.error, which get_exception_errno
will simply return errno.EPROTO for.

Signed-off-by: Terry Wilson <twilson@redhat.com>
---
 python/ovs/socket_util.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

On 9/15/20 7:17 PM, Terry Wilson wrote:
> With other socket types, trying to connect and failing will return
> an error code, but if an SSL Stream is used, then when
> check_connection_completion(sock) is called, SSL will raise an
> exception that doesn't derive from socket.error which is handled.
> 
> This adds handling for SSL.error, which get_exception_errno
> will simply return errno.EPROTO for.
> 
> Signed-off-by: Terry Wilson <twilson@redhat.com>
> ---

Hi, Terry.  Thanks for working on this!

Some comments inline.

Best regards, Ilya Maximets.

>  python/ovs/socket_util.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/python/ovs/socket_util.py b/python/ovs/socket_util.py
> index 3faa64e9d..ffcdd28cb 100644
> --- a/python/ovs/socket_util.py
> +++ b/python/ovs/socket_util.py
> @@ -19,6 +19,11 @@ import random
>  import socket
>  import sys
>  
> +try:
> +    from OpenSSL import SSL
> +except ImportError:
> +    SSL = None
> +
>  import ovs.fatal_signal
>  import ovs.poller
>  import ovs.vlog
> @@ -184,7 +189,7 @@ def check_connection_completion(sock):
>                  # XXX rate-limit
>                  vlog.err("poll return POLLERR but send succeeded")
>                  return errno.EPROTO
> -            except socket.error as e:
> +            except (socket.error, SSL.Error) if SSL else socket.error  as e:

This doesn't look right because this breaks the stream abstraction.
I think, SSL.Error should not go outside of SSLStream class in the first place.

Looking at methods of SSLStream class, I see that it catches only few of SSL
errors and translates them to errno, but it must catch all possible SSL errors.

See interpret_ssl_error() of the C implementation in lib/stream-ssl.c.
I think, we need to implement similar handling in python code to unify behavior.

What do you think?

>                  return get_exception_errno(e)
>          else:
>              return 0
> @@ -259,7 +264,7 @@ def get_exception_errno(e):
>      exception is documented as having two completely different forms of
>      arguments: either a string or a (errno, string) tuple.  We only want the
>      errno."""
> -    if isinstance(e.args, tuple):
> +    if isinstance(e, socket.error) and isinstance(e.args, tuple):
>          return e.args[0]
>      else:
>          return errno.EPROTO
>

On Tue, Sep 15, 2020 at 2:55 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 9/15/20 7:17 PM, Terry Wilson wrote:
> > With other socket types, trying to connect and failing will return
> > an error code, but if an SSL Stream is used, then when
> > check_connection_completion(sock) is called, SSL will raise an
> > exception that doesn't derive from socket.error which is handled.
> >
> > This adds handling for SSL.error, which get_exception_errno
> > will simply return errno.EPROTO for.
> >
> > Signed-off-by: Terry Wilson <twilson@redhat.com>
> > ---
>
> Hi, Terry.  Thanks for working on this!
>

Thanks for the review!

> > -            except socket.error as e:
> > +            except (socket.error, SSL.Error) if SSL else socket.error
> as e:
>
> This doesn't look right because this breaks the stream abstraction.
> I think, SSL.Error should not go outside of SSLStream class in the first
> place.
>

If that's true, doesn't having the staticmethod Stream.open() call
check_connection_completion(sock) break the abstraction by operating
directly on the sock object and not the Stream object?


> Looking at methods of SSLStream class, I see that it catches only few of
> SSL
> errors and translates them to errno, but it must catch all possible SSL
> errors.
>

Right, it already catches the errors we need in the SSLStream.send(), it's
just that check_connection_completion() is in socket_util.py which doesn't
operate on streams, so calls socket.send(). Stream.open() is a
staticmethod, so it's not going to be class-aware, so also kludgy to add
the SSL-specific stuff there. But basically it seems like either wrap
everywhere that calls check_connection_completion(), come up with some kind
of wrapper for check_connection_completion in stream.py classes that is at
least a classmethod that , or just handle it in socket_util. I'm not sure
there is an easy solution that I'd find 'pretty', but I'm willing to do
whatever is least ugly to everyone. ;)

See interpret_ssl_error() of the C implementation in lib/stream-ssl.c.
> I think, we need to implement similar handling in python code to unify
> behavior.
>
> What do you think?
>

I think most of that already exists with the SSLStream class. I remember
using it for 5fe179987d to do the same thing stream-ssl.c did for closing
an SSL connection. If Stream.open() wasn't called as a staticmethod/factory
function, then it'd be a lot easier to make a cleaner implementation.The
cleanest thing I can think of is to define a staticmethod
Stream.check_connection_completion(sock) that returns the sock_util
version, then override with a staticmethod in SSLStream that try's calling
Stream.check_connection_completion but catches SSL.Error and returns the
errno. And replace calls to the sock_util.check_connection_completion with
the class-based ones. I'll send a patch shortly with that implementation to
see if looks better. Thanks again for the review!

Terry

Just checking in again to see if I could get a committer's eye on this one.
Got a couple of Acks, just trying to make it the last lap. Thanks! ;)

On Tue, Sep 15, 2020 at 12:17 PM Terry Wilson <twilson@redhat.com> wrote:

> With other socket types, trying to connect and failing will return
> an error code, but if an SSL Stream is used, then when
> check_connection_completion(sock) is called, SSL will raise an
> exception that doesn't derive from socket.error which is handled.
>
> This adds handling for SSL.error, which get_exception_errno
> will simply return errno.EPROTO for.
>
> Signed-off-by: Terry Wilson <twilson@redhat.com>
> ---
>  python/ovs/socket_util.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/python/ovs/socket_util.py b/python/ovs/socket_util.py
> index 3faa64e9d..ffcdd28cb 100644
> --- a/python/ovs/socket_util.py
> +++ b/python/ovs/socket_util.py
> @@ -19,6 +19,11 @@ import random
>  import socket
>  import sys
>
> +try:
> +    from OpenSSL import SSL
> +except ImportError:
> +    SSL = None
> +
>  import ovs.fatal_signal
>  import ovs.poller
>  import ovs.vlog
> @@ -184,7 +189,7 @@ def check_connection_completion(sock):
>                  # XXX rate-limit
>                  vlog.err("poll return POLLERR but send succeeded")
>                  return errno.EPROTO
> -            except socket.error as e:
> +            except (socket.error, SSL.Error) if SSL else socket.error  as
> e:
>                  return get_exception_errno(e)
>          else:
>              return 0
> @@ -259,7 +264,7 @@ def get_exception_errno(e):
>      exception is documented as having two completely different forms of
>      arguments: either a string or a (errno, string) tuple.  We only want
> the
>      errno."""
> -    if isinstance(e.args, tuple):
> +    if isinstance(e, socket.error) and isinstance(e.args, tuple):
>          return e.args[0]
>      else:
>          return errno.EPROTO
> --
> 2.26.2
>
>

On 11/12/20 6:17 PM, Terry Wilson wrote:
> Just checking in again to see if I could get a committer's eye on this one. Got a couple of Acks, just trying to make it the last lap. Thanks! ;)

Hi, Terry.  Sorry for it taking so long. 
This patch is on my list to merge on this week.

Best regards, Ilya Maximets.