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

Terry Wilson twilson at redhat.com
Tue Sep 15 21:00:25 UTC 2020


On Tue, Sep 15, 2020 at 2:55 PM Ilya Maximets <i.maximets at 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 at 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


More information about the dev mailing list