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

[Ilya Maximets]

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 at 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
>